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

Add check format local #490

Closed

Conversation

beomki-yeo
Copy link
Contributor

For people who don't have clang-format-10

@beomki-yeo beomki-yeo linked an issue Nov 16, 2023 that may be closed by this pull request
@beomki-yeo
Copy link
Contributor Author

beomki-yeo commented Nov 20, 2023

@paulgessinger I have question:

There is the line in check_format.sh that runs git diff:

git diff --exit-code --stat -- ${INCLUDE_DIRS[@]/#/:/}

If I run this with check_format_local.sh, the line makes an warning:

warning: Not a git repository. Use --no-index to compare two paths outside a working tree
usage: git diff --no-index [<options>] <path> <path>

I think I have seen this in Acts core as well, do you know how to bypass this? (Maybe adding an if condition which checks whether pwd is in git repository?)

@paulgessinger
Copy link
Member

@beomki-yeo I guess it depends if the mounted volume has .git in the directory hierarchy. If not it won't work I think.

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know... I see that this can help people in some situations, but...

On your personal machine you can just install clang-format, and in 99.999999% of the cases it will just work fine with the check_format.sh script. I didn't yet run into a situation where clang-format on Ubuntu 20.04, Ubuntu 22.04 or RHEL9 would not have produced the same output as clang-format in the specific Docker image that we use.

And for the very-very rare cases where this happens, I'm okay with just starting Docker myself. 🤔

If we want to make this script "a thing", it would have to be smarter than this. Right now I can just start it from a "wrong directory", and it will not work.

So... do we really want to write a script like this? Because then it would have to be made a little more robust than this...

@beomki-yeo
Copy link
Contributor Author

On your personal machine you can just install clang-format, and in 99.999999% of the cases it will just work fine with the check_format.sh script.

Have you tested with other version of clang-format?
clang-format-10 is not easy to install these days and i believe our CI will break even with clang-format-11 if I remember correctly.

@paulgessinger
Copy link
Member

@beomki-yeo @krasznaa

  1. Consider bumping the version once in a while
  2. I usually grab statically compiled binaries from here: https://github.com/muttleyxd/clang-tools-static-binaries/releases/tag/master-8f72ab3c

@krasznaa
Copy link
Member

As I wrote, on Ubuntu I only ever installed the default clang-format package, and that worked "pretty much" always the same as the version coming with the Docker image.

Admittedly I apparently spoke too quickly about RHEL9. Apparently on that I only ever used the Docker image. As I'm having a hard time finding a viable package for it on a vanilla system. 😦

But what always works for me, even on Windows: VS Code comes with a built-in code formatter. Which respects the project's .clang-format settings. So in 99% of the cases I just use the VS Code shortcut these days. And then do one big run with check_format.sh at the end of some bigger updates.

@beomki-yeo beomki-yeo marked this pull request as draft November 20, 2023 17:24
@beomki-yeo beomki-yeo closed this Sep 25, 2024
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 this pull request may close these issues.

Add a docker for clang-format
3 participants