-
Couldn't load subscription status.
- Fork 24
docs: say that all versions in https://apt.llvm.org are available #162
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
Conversation
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.
-
We don't test with versions older than 9.
-
It is preferable that only the major version number is specified. For instance, if
clang-format-12 --versionshows using v12.0.1, then the major version number,12, is enough. We don't have exhaustive parsing of semantic version numbers builtin (see src here). If the version specified is not an integer (major version number), then the version input is interpreted as a path to the binary executables.This point actually proves that the clang-tools-pip and cpp-linter packages are not in complete alignment about dealing with the input
versionstring.
- We don't test with versions older than 9.
Should I remove the older ones, or state that they are available but untested?
- It is preferable that only the major version number is specified. For instance, if
clang-format-12 --versionshows using v12.0.1, then the major version number,12, is enough. We don't have exhaustive parsing of semantic version numbers builtin (see src here). If the version specified is not an integer (major version number), then the version input is interpreted as a path to the binary executables.
This point actually proves that the clang-tools-pip and cpp-linter packages are not in complete alignment about dealing with the inputversionstring.
I'll add the behavior to the docs shortly.
Should I remove the older ones, or state that they are available but untested?
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.
It is preferable that only the major version number is specified. For instance, if
clang-format-12 --versionshows using v12.0.1, then the major version number,12, is enough. We don't have exhaustive parsing of semantic version numbers builtin (see src here). If the version specified is not an integer (major version number), then the version input is interpreted as a path to the binary executables.This point actually proves that the clang-tools-pip and cpp-linter packages are not in complete alignment about dealing with the input
versionstring.
No need to document this. cpp-linter/cpp-linter#46 aims to resolve this in code.
No description provided.