-
Notifications
You must be signed in to change notification settings - Fork 21
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
docs: say that all versions in https://apt.llvm.org are available #162
Conversation
@@ -82,7 +82,7 @@ jobs: | |||
|
|||
#### `version` | |||
|
|||
- **Description**: The desired version of the [clang-tools](https://github.com/cpp-linter/clang-tools-pip) to use. Accepted options are strings which can be 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4 or 3.9. | |||
- **Description**: The desired version of the [clang-tools](https://github.com/cpp-linter/clang-tools-pip) to use. Accepted options are strings which can be 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4 or 3.9, plus all versions supported by the llvm PPA archives (<https://apt.llvm.org>). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your pull request. I think for the latest version of cpp-linter-action we have already supported 16
by
using https://github.com/cpp-linter/clang-tools-pip/releases/tag/v0.8.0. 16
should be added back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm PPA archives
This is specific to Linux and may imply to some users that only ubuntu runners are supported. We actually do support more than just Linux, and installing all version for other OSs (windows and mac) does not actually employ the LLVM PPA archive.
Therefore, I don't want these changes. @jnooree In the future, please open an issue to discuss any proposed changes before opening a Pull Request (especially one without a description).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the empty description; I thought the title was clear enough. Still it is good to mention the newer versions are available (at least for the ubuntu runners) IMO.
Based on your comments, adding version information per platform would be OK? I assume that other platforms are supported by clang-tools-pip and ubuntu is supported by the PPA archive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think the README should mention that this workflow uses clang-tools-pip for fetching the binaries and point to the project for the supported versions. Updating two repository on every new release seems fragile to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think the README should mention that this workflow uses clang-tools-pip for fetching the binaries and point to the project for the supported versions.
This seems like a valid idea. 👍🏼
Updating two repository on every new release seems fragile to me.
Its actually good practice to separate different tasks to different software. We automate dependency updates for cpp-linter-action with dependabot. We often like to test cpp-linter-action before publishing a release, especially with updates to cpp-linter or clang-tools-pip.
Based on your comments, adding version information per platform would be OK?
I'm not sure it is really needed as the cpp-linter clang-tools-pip packages handle the discrepancies between platforms installed binaries.
I assume that other platforms are supported by clang-tools-pip and ubuntu is supported by the PPA archive.
Yes, for the most part. The LLVM PPA archive doesn't have binaries for releases published before they moved the project to github (only v7+ I think). So, clang-tools-pip can take care of older versions on ubuntu runners also.
|
Should I remove the older ones, or state that they are available but untested?
I'll add the behavior to the docs shortly. |
Last I checked, the CLI was compatible, so there shouldn't be any concern. In fact we haven't seen anyone use untested versions to far. Thus, this idea seems superfluous. Maybe we should change "3.9" to "3" since only the major version number is respected and specifying "3.9" might be interpreted as a path. However, I'm more inclined to add better version parsing to the cpp-linter package; so it would handle a full version spec (ie "12.0.1") and correctly only use the major version since clang-tools-pip would have already ensured the specified version is installed. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
No need to document this. cpp-linter/cpp-linter#46 aims to resolve this in code. |
No description provided.