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

Prevent action downloading files after GH checkout action #6

Closed
2bndy5 opened this issue Aug 18, 2021 · 8 comments · Fixed by #9
Closed

Prevent action downloading files after GH checkout action #6

2bndy5 opened this issue Aug 18, 2021 · 8 comments · Fixed by #9
Labels
enhancement New feature or request

Comments

@2bndy5
Copy link
Collaborator

2bndy5 commented Aug 18, 2021

If you require the user's repos uses GH's actions/checkout prior to the cpp-lint action, then all files are already there in a folder designated by the environment variable $GITHUB_WORKSPACE.

The JSON that the bash script is getting the list of download URLs can instead be used to get the relative path of files. This way, you'd only have to prefix the filenames with $GITHUB_WORKSPACE when passing them to clang-* tools.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 18, 2021

I'm also exploring the possibility of using GH CLI (which is available to all GH runners) instead of the json option. That would mean less dependencies to install in this action's Dockerfile. Maybe this will be a seperate issue when I understand it better.

As for installing clang-* in this action's Dockerfile, you could switch to a docker container that already has them installed... again that would be yet another issue.

@shenxianpeng
Copy link
Collaborator

In my daily work, I used Bitubkcet, I want to bring this linter action to Bitbucket, using shell script may be easier to integrate with my internal environment as I don't need to install GH CLI.

But here in GitHub, using the environment variable $GITHUB_WORKSPACE and GH CLI is a great idea.

Btw, there is a docker image clang-tool I created several days ago include clang-format and clang-tidy maybe useful.

@shenxianpeng shenxianpeng added the enhancement New feature or request label Aug 19, 2021
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 19, 2021

In my daily work, I used Bitubkcet, I want to bring this linter action to Bitbucket, using shell script may be easier to integrate with my internal environment as I don't need to install GH CLI

Interesting! I initially wanted to get involved as a way to learn bash scripting better. I'm now learning more about the git's REST API (which is what the JSON comes from). Maybe we can check for the file's existence before downloading it. I have another idea of letting clang-tidy look for a custom defined .clang-tidy file instead of hard-coded checks. However, the entire repo needs to be present for clang-tidy to find a clang-tidy config file (and a compile_commands.json referred to as "compilation database" in clang-tidy's output).

Btw, there is a docker image clang-tool I created several days ago include clang-format and clang-tidy maybe useful.

Cool! Could it be tailored to have all available versions of clang-*? That way we can let users specify what version (via another new input arg) to use; this is a common input option for most other clang-tidy actions.

brendan@B-DESKTOP:/mnt/c/Users/ytreh/Documents/GitHub/cpp-linter-action$ apt list clang-tidy-*
Listing... Done
clang-tidy-10/focal,now 1:10.0.0-4ubuntu1 amd64 [installed,automatic]
clang-tidy-11/focal-updates 1:11.0.0-2~ubuntu20.04.1 amd64
clang-tidy-12/focal-updates 1:12.0.0-3ubuntu1~20.04.3 amd64
clang-tidy-6.0/focal 1:6.0.1-14 amd64
clang-tidy-7/focal 1:7.0.1-12 amd64
clang-tidy-8/focal 1:8.0.1-9 amd64
clang-tidy-9/focal 1:9.0.1-12 amd64
brendan@B-DESKTOP:/mnt/c/Users/ytreh/Documents/GitHub/cpp-linter-action$ apt list clang-format-*
Listing... Done
clang-format-10/focal 1:10.0.0-4ubuntu1 amd64
clang-format-11/focal-updates 1:11.0.0-2~ubuntu20.04.1 amd64
clang-format-12/focal-updates 1:12.0.0-3ubuntu1~20.04.3 amd64
clang-format-6.0/focal 1:6.0.1-14 amd64
clang-format-7/focal 1:7.0.1-12 amd64
clang-format-8/focal 1:8.0.1-9 amd64
clang-format-9/focal 1:9.0.1-12 amd64

as you can see, the versions available are aligned.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 19, 2021

@shenxianpeng I switched to your docker image and runtime has decreased by 30+ seconds!!!

@shenxianpeng
Copy link
Collaborator

shenxianpeng commented Aug 19, 2021

@shenxianpeng I switched to your docker image and runtime has decreased by 30+ seconds!!!

Wow, this is great! I think it can have all available versions of clang-*. I'm just not so familiar with clang, from your experience, will users care about the version?

Support custom defines .clang-tidy file makes a lot of sense to users.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 19, 2021

Judging from the differences in the clang docs, compatibility between versions of clang are sketchy. From my experience, its better to have the action run with the same version of clang that you're using locally. Linux apt will default to installing v10, but latest stable release is v12 on GH (which is where I get the clang installer for Windows). Yes, I think people will care.

@shenxianpeng
Copy link
Collaborator

Because clang-tool image includes clang-format and clang-tidy, so I guess rename the docker image from xianpengshen/clang-tool to xianpengshen/clang-tools may be more suitable. I'll do this change later.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 19, 2021

While looking into solving #8 , I've also added this feature in the process. I'll wait till #10 is merged to rebase then open another PR...

Note I haven't started using GH CLI at all; I'm purely using GH's REST API only (hopefully that's still somewhat compatible with a REST API from Bitbucket).

@2bndy5 2bndy5 mentioned this issue Aug 22, 2021
This was linked to pull requests Aug 22, 2021
@2bndy5 2bndy5 removed a link to a pull request Aug 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants