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

docs: say that all versions in https://apt.llvm.org are available #162

Closed
wants to merge 2 commits into from

Conversation

jnooree
Copy link
Contributor

@jnooree jnooree commented Nov 7, 2023

No description provided.

@@ -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>).
Copy link
Collaborator

@shenxianpeng shenxianpeng Nov 7, 2023

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.

Copy link
Collaborator

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).

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

@shenxianpeng shenxianpeng added the documentation Improvements or additions to documentation label Nov 7, 2023
@2bndy5
Copy link
Collaborator

2bndy5 commented Nov 7, 2023

  1. We don't test with versions older than 9.

  2. It is preferable that only the major version number is specified. For instance, if clang-format-12 --version shows 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 version string.

@jnooree
Copy link
Contributor Author

jnooree commented Nov 7, 2023

  1. We don't test with versions older than 9.

Should I remove the older ones, or state that they are available but untested?

  1. It is preferable that only the major version number is specified. For instance, if clang-format-12 --version shows 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 version string.

I'll add the behavior to the docs shortly.

@2bndy5
Copy link
Collaborator

2bndy5 commented Nov 7, 2023

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.

Copy link

stale bot commented Dec 7, 2023

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.

@stale stale bot added the wontfix This will not be worked on label Dec 7, 2023
@stale stale bot closed this Dec 14, 2023
@2bndy5
Copy link
Collaborator

2bndy5 commented Dec 24, 2023

  1. It is preferable that only the major version number is specified. For instance, if clang-format-12 --version shows 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 version string.

No need to document this. cpp-linter/cpp-linter#46 aims to resolve this in code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants