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.

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

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f82d6c
docs: mention all versions from https://apt.llvm.org are available
jnooree Nov 7, 2023
6d37259
docs: also update README.md
jnooree Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

- Set this option to a blank string (`''`) to use the platform's default installed version.
- This value can also be a path to where the clang tools are installed (if using a custom install location).
- Default: '12'
Expand Down
2 changes: 1 addition & 1 deletion action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ inputs:
required: false
default: "."
version:
description: "The desired version of the clang tools 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. Defaults to 12."
description: "The desired version of the clang tools 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 available in the https://apt.llvm.org repository. Defaults to 12."
required: false
default: "12"
verbosity:
Expand Down
Loading