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

Jump to bottom
Merged
merged 5 commits into from
Mar 12, 2023
Merged

Fix issue to use native binaries when available #43

merged 5 commits into from
Mar 12, 2023

Conversation

@shenxianpeng
Copy link
Collaborator

@shenxianpeng shenxianpeng commented Mar 10, 2023 •
edited
Loading

  1. we should only check the major version instead of the whole version
  2. update files based on pre-commit run check
  3. remove 12.0.1 out of readme
  4. update test workflow to pass test on Windows and Ubuntu for versions 12.0.1 and 15.

@shenxianpeng shenxianpeng added the bug Something isn't working label Mar 10, 2023
@shenxianpeng
Copy link
Collaborator Author

shenxianpeng commented Mar 10, 2023

To fixe the failure against install (12.0.1, ubuntu-latest) I would like to remove 12.0.1 from test workflow and README.md
Right now I have no idea about how to fix install (15, windows-latest)

@shenxianpeng shenxianpeng mentioned this pull request Mar 10, 2023
Closed
@2bndy5
Copy link
Contributor

2bndy5 commented Mar 10, 2023

we should only check the major version instead of the whole version

This was my original intention. I'm ok with this change.

I would like to remove 12.0.1 from test workflow and README.md

I'm ok with this too. We could add to the logic to check the full version if it is specified. but clang version numbers usually don't get minor number changes (really only some patch number changes).

@2bndy5
Copy link
Contributor

2bndy5 commented Mar 10, 2023

Right now I have no idea about how to fix install (15, windows-latest)

I see that v15.0.7 is shipped with the Windows runner. It may simply be that the binary isn't named clang-*-15, rather it is usually just named clang-<tidy/format>.exe.

@shenxianpeng shenxianpeng changed the title Fix native binary exit Fix native binary exists Mar 11, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2023 •
edited by codecov bot
Loading

Codecov Report

Merging #43 (4ba701f) into main (8c34f13) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #43   +/-   ##
=======================================
  Coverage   87.71%   87.71%           
=======================================
  Files           4        4           
  Lines         171      171           
=======================================
  Hits          150      150           
  Misses         21       21
Impacted Files Coverage Δ
clang_tools/install.py 82.69% <ø> (ø)
clang_tools/main.py 100.00% <ø> (ø)
clang_tools/util.py 94.11% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@shenxianpeng shenxianpeng force-pushed the fix-native-binary-exit branch 4 times, most recently from d3584fa to 2d5bfe3 Compare March 11, 2023 13:44
@shenxianpeng shenxianpeng force-pushed the fix-native-binary-exit branch from 2d5bfe3 to ad73703 Compare March 11, 2023 13:58
@shenxianpeng shenxianpeng force-pushed the fix-native-binary-exit branch from aefe969 to 4ba701f Compare March 11, 2023 14:18
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@shenxianpeng shenxianpeng requested a review from 2bndy5 March 11, 2023 14:25
@shenxianpeng
Copy link
Collaborator Author

shenxianpeng commented Mar 11, 2023

I did not remove 12.0.1 I made some specific adjust and finally passed test on ubuntu and windows.

@shenxianpeng shenxianpeng self-assigned this Mar 11, 2023
@2bndy5
Copy link
Contributor

2bndy5 commented Mar 11, 2023

I think we can do better with handling the full version when specified, but this will do for now.

2bndy5
2bndy5 reviewed Mar 11, 2023
View reviewed changes
clang_tools/install.py
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 :)

@2bndy5
Copy link
Contributor

2bndy5 commented Mar 11, 2023

I also installed the covcov github app integration for cpp-linter org. I don't think anything else needs to be done on our end.

2bndy5
2bndy5 approved these changes Mar 11, 2023
View reviewed changes
Copy link
Contributor

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

For the sake of getting this out into the wild, I'm ok with moving this forward.

@shenxianpeng shenxianpeng merged commit 5987669 into main Mar 12, 2023
@shenxianpeng shenxianpeng deleted the fix-native-binary-exit branch March 12, 2023 02:35
@shenxianpeng shenxianpeng changed the title Fix native binary exists Fix issue to use native binaries when available Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@2bndy5 2bndy5 2bndy5 approved these changes

Assignees

@shenxianpeng shenxianpeng

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shenxianpeng @2bndy5 @codecov-commenter