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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support different versions of clang-format and clang-tidy #1

Closed
shenxianpeng opened this issue Aug 19, 2021 · 18 comments
Closed

Support different versions of clang-format and clang-tidy #1

shenxianpeng opened this issue Aug 19, 2021 · 18 comments

Comments

@shenxianpeng
Copy link
Collaborator

No description provided.

shenxianpeng added a commit that referenced this issue Aug 19, 2021
shenxianpeng added a commit that referenced this issue Aug 19, 2021
shenxianpeng added a commit that referenced this issue Aug 19, 2021
shenxianpeng added a commit that referenced this issue Aug 19, 2021
* feat: #1 support multi versions of clang tools
@2bndy5
Copy link

2bndy5 commented Aug 19, 2021

@shenxianpeng I can't dynamically use tags in the action's Dockerfile's FROM option. So, specifying a version has to come at runtime. Meaning all versions of available clang-tools need to be installed in 1 image. Then I can execute the user-specified version like

clang-tidy-"$CLANG_VERSION" ... $SOURCE_FILE

@shenxianpeng
Copy link
Collaborator Author

shenxianpeng commented Aug 20, 2021

@2bndy5 There is no way could pass the clang-tools version from test.yml action file to Dockerfile's FROM option?
If all versions of available clang-tools need to be installed in 1 image, it may be a bit redundant for the users who do not choose clang-tools version.

shenxianpeng added a commit that referenced this issue Aug 20, 2021
@shenxianpeng
Copy link
Collaborator Author

I just uploaded clang-tools latest version 12, so FROM xianpengshen/clang-tools will pull the latest version 12.

@2bndy5
Copy link

2bndy5 commented Aug 20, 2021

Good that you got v12 installing! This will come in handy for other windows users (like myself -- I use WSL ubuntu for local developing).


My problem is this: The way selecting a version that currently works is by setting the FROM option in the action's Dockerfile

# FROM xianpengshen/clang-tools <-- this fails to download the docker container during workflow runtime
FROM xianpengshen/clang-tools:10

What I'm trying to say is that if a user wants to use v12, it won't be accessible because the image is hardcoded to only have v10 installed. The important thing to know here, is that I can't use an action's input variable (like inputs.version) to change which image is downloaded when the action is setup on the workflow's run. The only way to dynamically set what version to use is have all versions of the clang tools installed, so that I can simply execute from the action's run_checks.sh

   clang-tidy-"$CLANG_VERSION" "$filename" "$CLANG_CONFIG" >> clang_tidy_report.txt
   clang-format-"$CLANG_VERSION" -style="$FMT_STYLE" --dry-run "$filename" >> clang_format_report.txt

@2bndy5
Copy link

2bndy5 commented Aug 20, 2021

I should point out that I've already come across errors from clang-format (v10) that causes a clang-format to crash if the .clang-format config file has an option that is not supported by newer version of clang-format (even though it was stated in v13 docs without any warning).

@shenxianpeng
Copy link
Collaborator Author

I found this way can to pass the version to Dockerfile FROM

ARG TAG=latest
FROM xianpengshen/clang-tools:$TAG

Under my test:

  • run docker build -t mylinter . will pull the lastest clang-tools image.
  • run docker build -t mylinter --build-arg TAG=10 . will pull the chang-tools:10 image

Does this meet your expectations?

@2bndy5
Copy link

2bndy5 commented Aug 20, 2021

yes but the ARG option is used by the action.yml to pass the input variables to the ENTRYPOINT option. I'll try and see if the ARG option can be used twice and report back...

EDIT: The args list from a action.yml is passed to the dockerfile's ENTRYPOINT option using the CMD option. I don't see a way to set the dockerfile's ARG option from the action.yml

@2bndy5
Copy link

2bndy5 commented Aug 20, 2021

BTW its best to specify 12 because latest can (& will) change to 14 in the future.

@shenxianpeng
Copy link
Collaborator Author

Okay, maybe can set 12 as the default value in the action.yml

@2bndy5
Copy link

2bndy5 commented Aug 20, 2021

Still trying to find a way to pass inputs.version into the Dockerfile. GH docs hints that we might be able to do without the Dockerfile entirely; I don't see another way.

@2bndy5
Copy link

2bndy5 commented Aug 20, 2021

referencing my results on a different thread:
https://github.com/shenxianpeng/cpp-linter-action/pull/13#issuecomment-902604673

@2bndy5
Copy link

2bndy5 commented Aug 20, 2021

@shenxianpeng Just as an example,

here is a link to a similar action that allows the user to set the clang-tools version from the action's inputs
https://github.com/ZedThree/clang-tidy-review/blob/master/Dockerfile

observe also the same action's yml file:
https://github.com/ZedThree/clang-tidy-review/blob/master/action.yml

Please note that action uses a python script as an ENTRYPOINT, and from there it uses GH CLI tool (via a python binding) to do some pretty cool stuff (like auto-PR reviews and an upcoming feature to compose a commit of clang-tools' suggestions). However, it does not support push events, and we will shortly...

@shenxianpeng
Copy link
Collaborator Author

shenxianpeng commented Aug 20, 2021

These links are very inspired, the PR suggestions look very pretty. to have such cool features may also need to use python to implement, it will easier.

and we will shortly...

馃槂 馃憤

@shenxianpeng
Copy link
Collaborator Author

referencing my results on a different thread:
shenxianpeng/cpp-linter-action#13 (comment)

so we also need to install clang-* version in an image if we want to support other versions.

@2bndy5
Copy link

2bndy5 commented Aug 20, 2021

to have such cool features may also need to use python to implement, it will easier.

Yes, but python is slow (especially when we need to install dependencies via pip install ...), and we would only need an interpreter language to parse complex string patterns... I'm looking into learning perl as it already exists in ubuntu:latest. This is more related to shenxianpeng/cpp-linter-action#11

so we also need to install clang-* version in an image if we want to support other versions.

Yes please. If it turns out to be a detrimental slow-down, then we should probably stick to what is installed by default in ubuntu (v10). My hope is that this image (with all versions installed) can be used by other clang-tidy/format related actions as well. That would make this image more beneficial to the community and not just exclusive to us.

@shenxianpeng
Copy link
Collaborator Author

I'm looking into learning perl as it already exists in ubuntu:latest

Yes, using Perl should be faster than python. but I'd prefer bash, python to Perl for two reasons.

  1. Neither of us is very familiar with Perl, we need to learn something.
  2. Perl is going downhill (refer to TIOBE Index), it ranks twentieth, but python is in second place. From the community perspective, limit users to contribute if the scripts are written in Perl than Bash/Python.

That would make this image more beneficial to the community and not just exclusive to us.

You're right. I have done another clang-tools image which includes all versions of clang-* versions(from v12 to v6). the tag is xianpengshen/clang-tools:all

@2bndy5
Copy link

2bndy5 commented Aug 22, 2021

good feedback! I have been exploring some python parsers... I would still like to keep third-party dependencies low. Seems that some update to pip in the last year made it very slow (not as much when in an activated venv).

Just to throw it out there: I'm also fluent in Lua, but I don't think that a Lua interpreter ships with ubuntu:latest. Lua also is down to #39 on that list though 馃槗

@shenxianpeng
Copy link
Collaborator Author

I've been using Python but not so much for performance that I didn't notice that :)

You know too much... I don't even know where to use Lua, from the ranking it sounds like I don't need to learn it for now, haha.

I close it as it's done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants