Skip to content

Make sure the clang-format version from infrastructure check during the PR has the version that aligned with the version from this repo #4495

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

Closed
andreyfe1 opened this issue Sep 6, 2021 · 12 comments · Fixed by #4499

Comments

@andreyfe1
Copy link
Contributor

clang-format check differs from clang-format from the repo. Seems it should be improved on infrastructure side.

Originally posted by @capatober in #4439 (comment)

@bader
Copy link
Contributor

bader commented Sep 6, 2021

@capatober, it's by design. Currently this check runs in a few minutes. We don't use the "clang-format from the repo" because takes a lot of time to build clang-format tool from sources using GitHub Actions resources and difference you get from it is minimal.
Do you have any problems with using clang-format installed on your system?

@andreyfe1
Copy link
Contributor Author

andreyfe1 commented Sep 6, 2021

Yes. Firstly it's not mentioned in the instruction which clang-format versions should be used. I used clang-format-6.0, but it fails during on working on sources. I tried to install the newest version of clang-format on the system, but paths are not recognized by the system. The only way that works is to use clang-format that built from the repo, but result of applying to sources is different to what infrastructure check shows. It's also not specified which clang-format version is used for PRs

@bader
Copy link
Contributor

bader commented Sep 6, 2021

paths are not recognized by the system.

What do you mean by that?

@andreyfe1
Copy link
Contributor Author

It means that clang-format is not found, but it's installed

@bader
Copy link
Contributor

bader commented Sep 6, 2021

Did you install it into custom directory? Why is clang-format not found?

@andreyfe1
Copy link
Contributor Author

It's installed in the default directory. I didn't investigate the reason since I didn't know which clang-format version should have been used. Since version is not mentioned in the instruction I thought that any version can be used, but it's not true

@bader
Copy link
Contributor

bader commented Sep 6, 2021

You probably need to update default version with update-alternatives tool.

Here is GitHub actions workflow, which you can repeat to get the same results: https://github.com/intel/llvm/blob/sycl/.github/workflows/clang-format.yml.

NOTE: GitHub actions job attaches output of clang-format tool to the job artifacts, so you can just download the patch and apply if you don't want to install clang-format locally.

@andreyfe1
Copy link
Contributor Author

Am I right that the recommended way for contributors is following?

  1. try to run any clang-format version
  2. if 1) doesn't work, call update-alternatives
  3. if 2) doesn't work, try to repeat actions from .yml file mentioned above
  4. if 3) doesn't work, download output and apply patch

Could you please update the instruction for contributors with correct steps?

However, in my opinion, step 4) looks inconvenient and time-consuming if we do it per each commit.
If my understanding is correct, the process should definitely be improved.

@bader
Copy link
Contributor

bader commented Sep 7, 2021

I've updated instructions in #4499.

@bader
Copy link
Contributor

bader commented Sep 7, 2021

Am I right that the recommended way for contributors is following?

  1. try to run any clang-format version
  2. if 1) doesn't work, call update-alternatives
  3. if 2) doesn't work, try to repeat actions from .yml file mentioned above
  4. if 3) doesn't work, download output and apply patch

I would recommend install clang-format locally if you plan to do multiple contributions. One time contributors can just apply the patch prepared by GitHub actions job.
All the hints I was giving you are about installing the right version of clang-format and all these knowledge is basic Linux administration, which easy to find by searching the Internet. What is useful to know is what version of clang-format tool GitHub Actions check is using.

@andreyfe1
Copy link
Contributor Author

I see. Thanks. I think mentioning in the instruction is enough for one-time contributors.
By the way, you mentioned Linux administration. What about advises for Windows?

@bader
Copy link
Contributor

bader commented Sep 7, 2021

What about advises for Windows?

I expect clang-format to produce the same results on any platform, so the advise is the same - install clang-format 10 locally if you want to format the patch before opening a pull request.
I personally not able to provide any BKMs as my primary development platform is Linux.

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

Successfully merging a pull request may close this issue.

2 participants