Skip to content

Fix issue to use native binaries when available #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 12, 2023
Merged

Fix issue to use native binaries when available #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e689b0c
fix: download binary when native binary exist #37
shenxianpeng Mar 10, 2023
e793f88
fix: pre-commit run --all-files
shenxianpeng Mar 10, 2023
335d5f9
fix: let specific version pass test
shenxianpeng Mar 11, 2023
ad73703
docs: remove 12.0.1 out of readme
shenxianpeng Mar 11, 2023
4ba701f
update workflow and pass Codecov
shenxianpeng Mar 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 30 additions & 16 deletions .github/workflows/python-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,6 @@
name: Test

on:
push:
# branches: [ "main" ]
paths:
- '**.py'
- '.github/workflows/python-test.yml'
pull_request:
branches: [ "main" ]
workflow_dispatch:
Expand All @@ -29,8 +24,8 @@ jobs:
run: |
python3 -m pip install --upgrade pip
python3 -m pip install . -r requirements-dev.txt

- run: pre-commit run
- name: Run pre-commit
run: pre-commit run --all-files

- name: Collect coverage
run: |
Expand Down Expand Up @@ -81,26 +76,45 @@ jobs:
- name: Install clang-tools
run: clang-tools --install ${{ matrix.version }}

- name: show path of binaries
- name: Show path of binaries
shell: bash
run: |
which clang-format-${{ matrix.version }}
which clang-tidy-${{ matrix.version }}
if [ "${{ matrix.version }}" = "15" -a "${{ matrix.os }}" = "windows-latest" ]; then
which clang-format
which clang-tidy
elif [ "${{ matrix.version }}" = "12.0.1" -a "${{ matrix.os }}" = "ubuntu-latest" ]; then
which clang-format-12
which clang-tidy-12
else
which clang-format-${{ matrix.version }}
which clang-tidy-${{ matrix.version }}
fi

- name: Check clang-tools on Windows
if: matrix.os == 'windows-latest'
shell: bash
run: |
clang-format-${{ matrix.version }}.exe --version
clang-tidy-${{ matrix.version }}.exe --version
if [ "${{ matrix.version }}" = "15" ]; then
clang-format.exe --version
clang-tidy.exe --version
else
clang-format-${{ matrix.version }}.exe --version
clang-tidy-${{ matrix.version }}.exe --version
fi

- name: Check clang-tools on Unix
if: matrix.os == 'ubuntu-latest' || matrix.os == 'macos-latest'
run: |
clang-format-${{ matrix.version }} --version
if [ "${{ matrix.version }}" = "14" -o "${{ matrix.version }}" = "15" ]; then
echo "skip version ${{ matrix.version }} because Segmentation fault on ubuntu."
if [ "${{ matrix.version }}" = "12.0.1" -a "${{ matrix.os }}" = "ubuntu-latest" ]; then
clang-format-12 --version
clang-tidy-12 --version
else
clang-tidy-${{ matrix.version }} --version
clang-format-${{ matrix.version }} --version
if [ "${{ matrix.version }}" = "14" -o "${{ matrix.version }}" = "15" ]; then
echo "skip version ${{ matrix.version }} because Segmentation fault on ubuntu."
else
clang-tidy-${{ matrix.version }} --version
fi
fi

docs:
Expand Down
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@ tests/__pycache__/
.eggs
.mypy_cache/
*env
docs/cli_args.rst
docs/cli_args.rst
16 changes: 8 additions & 8 deletions README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,22 +124,22 @@ Supported versions
clang-format
************
.. csv-table::
:header: "Version", "15", "14", "13", "12.0.1", "12", "11", "10", "9", "8", "7", "6", "5", "4", "3.9"
:header: "Version", "15", "14", "13", "12", "11", "10", "9", "8", "7", "6", "5", "4", "3.9"
:stub-columns: 1

Linux,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️
Windows,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️
macOS,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️
Linux,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️
Windows,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️
macOS,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️

clang-tidy
**********
.. csv-table::
:header: "Version", "15", "14", "13", "12.0.1", "12", "11", "10", "9", "8", "7", "6", "5", "4", "3.9"
:header: "Version", "15", "14", "13", "12", "11", "10", "9", "8", "7", "6", "5", "4", "3.9"
:stub-columns: 1

Linux,❌,❌,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️
Windows,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️
macOS,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️
Linux,❌,❌,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️
Windows,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️
macOS,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️,✔️

Know issues:

Expand Down
4 changes: 2 additions & 2 deletions clang_tools/install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ def is_installed(tool_name: str, version: str) -> Optional[Path]:
return None # failed to locate the binary
path = Path(path).resolve()
print("at", str(path))
ver_num = ver_num.groups(0)[0].decode(encoding="utf-8").split(".") # pragma: no cover
Copy link
Contributor

@2bndy5 2bndy5 Mar 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why # pragma: no cover?

Copy link
Collaborator Author

@shenxianpeng shenxianpeng Mar 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to cover this 😅

Copy link
Contributor

@2bndy5 2bndy5 Mar 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that it wasn't getting covered. From a glance, it should be covered if the binary is located.

I'm ok with keeping the line not covered as it would be a reminder that we should be updating the unit tests or writing a new one that triggers it.

Copy link
Collaborator Author

@shenxianpeng shenxianpeng Mar 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm also surprised :)

if (
ver_num is None
or ver_num.groups(0)[0].decode(encoding="utf-8").split(".") != version_tuple
ver_num is None or ver_num[0] != ver_major
):
return None # version is unknown or not the desired major release
return path
Expand Down
1 change: 1 addition & 0 deletions clang_tools/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from .install import install_clang_tools, uninstall_clang_tools
from . import RESET_COLOR, YELLOW


def get_parser() -> argparse.ArgumentParser:
"""Get and parser to interpret CLI args."""
parser = argparse.ArgumentParser()
Expand Down
1 change: 0 additions & 1 deletion clang_tools/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
from http.client import HTTPResponse



def check_install_os() -> str:
"""Identify this Operating System.

Expand Down
3 changes: 2 additions & 1 deletion tests/test_install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,10 @@ def test_install_tools(monkeypatch: pytest.MonkeyPatch, tmp_path: Path, version:
fd.name for fd in tmp_path.iterdir()
]


def test_path_warning(capsys: pytest.CaptureFixture):
"""Explicitly fail to download a set of tools to test the prompts that

1. warns users about using a dir not in env var PATH.
2. indicates a failure to download a tool
"""
Expand Down