Skip to content
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.

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

allow version to be a path #95

Merged
merged 8 commits into from
Jun 4, 2024
Merged

allow version to be a path #95

merged 8 commits into from
Jun 4, 2024

Conversation

2bndy5
Copy link
Contributor

@2bndy5 2bndy5 commented May 21, 2024

resolves #94

Copy link

codecov bot commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.12%. Comparing base (b93b1da) to head (9eea4eb).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #95      +/-   ##
==========================================
+ Coverage   95.92%   96.12%   +0.20%     
==========================================
  Files           7        7              
  Lines         270      284      +14     
==========================================
+ Hits          259      273      +14     
  Misses         11       11              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@2bndy5
Copy link
Contributor Author

2bndy5 commented May 21, 2024

The MacOS runners are having the same trouble stated in cpp-linter/cpp-linter-action#237

clang_tools/util.py Outdated Show resolved Hide resolved
clang_tools/main.py Outdated Show resolved Hide resolved
avoids re-parsing the specified version multiple times
@2bndy5 2bndy5 marked this pull request as ready for review May 23, 2024 05:40
@shenxianpeng shenxianpeng self-requested a review May 27, 2024 04:01
Copy link
Collaborator

@shenxianpeng shenxianpeng left a comment

Choose a reason for hiding this comment

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

I just finished my trip, sorry for the late reply.

@2bndy5
Copy link
Contributor Author

2bndy5 commented May 27, 2024

Oh, not a problem. I didn't know you were on vacation. There's no real rush anyway. Not many users are using a path as the specified version.

@shenxianpeng
Copy link
Collaborator

usage: clang-tools [-h] [-i VERSION] [-t TOOL [TOOL ...]] [-d DIR] [-f] [-b] [-u VERSION]

options:
  -h, --help            show this help message and exit
  -i VERSION, --install VERSION
                        Install clang-tools about a specific version.
  -t TOOL [TOOL ...], --tool TOOL [TOOL ...]
                        Specify which tool(s) to install.
  -d DIR, --directory DIR
                        The directory where the clang-tools are installed.
  -f, --overwrite       Force overwriting the symlink to the installed binary. This will only overwrite an existing symlink.
  -b, --no-progress-bar
                        Do not display a progress bar for downloads.
  -u VERSION, --uninstall VERSION
                        Uninstall clang-tools with specific version. This is done before any install.

Maybe we should update the following part of clang-tools help as well.

-i VERSION, --install VERSION
                        Install clang-tools about a specific version.

@2bndy5
Copy link
Contributor Author

2bndy5 commented May 28, 2024

Sure. Although, this is behavior/PR is specific to cpp-linter-action.

@2bndy5
Copy link
Contributor Author

2bndy5 commented May 29, 2024

I amended the help string as suggested.

I also took the time to update the generated doc about the CLI options. It now looks more like cpp-linter CLI doc with badges for minimum version and default value (if not an empty string). I added a badge (where applicable) to indicate the option is a switch/flag (which accepts no value).
image
image

apply sonarcloud suggestions
@2bndy5
Copy link
Contributor Author

2bndy5 commented May 29, 2024

I just noticed a regression about v12 vs v12.0.1
image
These changes currently ignore the minor and patch versions if specified. I have to think on this for a better solution.

@2bndy5
Copy link
Contributor Author

2bndy5 commented May 31, 2024

I fixed the regression by storing the parsed version tuple as a class (Version) instance attribute. Then passing the Version instance to functions that need both string and 3-tuple of integers.

also update docs' copyright year
@2bndy5
Copy link
Contributor Author

2bndy5 commented Jun 4, 2024

This is essentially blocked by an issue with the built static binaries. Any objection to merging/releasing this to get it upstream?

@shenxianpeng
Copy link
Collaborator

shenxianpeng commented Jun 4, 2024

I have no objection to merge and release. I just have a question about help.

With the help of a path that points to the directory where the binaries already exist. it seems clang-tools support installation like below, but it does not support as expected. (ref the output below)

gitpod /workspace/clang-tools-pip (handle-version-path) $ ls /usr/lib/llvm-15/bin/clang-tidy
/usr/lib/llvm-15/bin/clang-tidy
gitpod /workspace/clang-tools-pip (handle-version-path) $ ls /usr/lib/llvm-15/bin/clang-format 
/usr/lib/llvm-15/bin/clang-format
gitpod /workspace/clang-tools-pip (handle-version-path) $ clang-tools -i  /usr/lib/llvm-15
The version specified is not a semantic specification
gitpod /workspace/clang-tools-pip (handle-version-path) $ clang-tools -i  /usr/lib/llvm-15/bin
The version specified is not a semantic specification
gitpod /workspace/clang-tools-pip (handle-version-path) $ clang-tools -i  /usr/lib/llvm-15/bin/clang-format 
The version specified is not a semantic specification

clang_tools/main.py Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@2bndy5 2bndy5 merged commit 8d5f32b into main Jun 4, 2024
49 of 53 checks passed
@2bndy5 2bndy5 deleted the handle-version-path branch June 4, 2024 16:22
@shenxianpeng shenxianpeng added the bug Something isn't working label Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow version to be a direct path to binary
2 participants