Use fixed clang format #1405
Closed
Use fixed clang format #1405
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ginkgo-bot
added
reg:build
|
Note: the changes to the CI will not be reflected in the CI runs here. |
|
If we make git-cmake-format be able to handle max (or fixed version), does it also fit the same purpose? |
|
@yhmtsai Not really. First, it would need to be the exact version, not max. But what happens if the exact version is not available? The GCF can only raise an error and stop. This PR enables to automatically download the correct version. Also, it allows us to drop GCF entirely, so we have less things to maintain. |
yhmtsai
reviewed
Nov 13, 2023
Comment on lines
+1
to
+3
| repos: | ||
| - repo: https://github.com/pre-commit/mirrors-clang-format | ||
| rev: 'v14.0.0' # The default in Ubuntu 22.04, which is used in our CI |
There was a problem hiding this comment.
because it install clang-format by pip, I wonder it affects/updates the local python package without notification.
Do you know how pre-commit manages the python environment and whether we can use this clang-format in editor?
nit: use two space to be consistent with the other yaml format
There was a problem hiding this comment.
I don't really know, how the user environment is changes. But what I can tell you is that on my system, where I use pre-commit, I don't have clang-14 in my path, even though it is used by pre-commit.
There was a problem hiding this comment.
After some search, it is handled by python virtualenv.
The default folder is ~/.cache/pre-commit https://[pre-commit](https://pre-commit.com/#managing-ci-caches).com/#managing-ci-caches
there's a folder (py_env-python3.9) under a random folder, source py_env-python3.9/bin/activate can run clang-format from the environment.
I am still looking for how to get the path correctly and use it in editor
There was a problem hiding this comment.
For what do you need it in your editor? You can just run pre-commit run and it will do the formatting.
There was a problem hiding this comment.
I usually run several times clang-format during coding with the default shortcut.
pre-commit requires us to run via command and pass files (or all-files)
There was a problem hiding this comment.
No, I mean no-concatenation will introduce issue from inconsistent clang-format.
local clang-format may break some comments which are considered long with this format. but pre-commit one does not break in that format.
For example, assuming pre-commit clang-format only puts the normal indentation for arguments, but local clang-format aligns the arguments
1 is the original code
void func(...,
tttt // some text ...
2 is from local clang-format (break when it is too long)
void func(...,
tttt // some text
// more text
3 is format 1 from pre-commit clang-format, less indention so no need for breaking
void func(...,
tttt // some text ...
4 is format 2 from pre-commit clang-format, no-concatenation if it is already split
void func(...,
tttt // some text
// more text
3 and 4 are different. If we apply format directly, we should get 3.
With inconsistent clang-format, we may get 4 in the end.
There was a problem hiding this comment.
can you still provide your clang-format version? I will still look into the issue you mentioned.
There was a problem hiding this comment.
TBH, I think this might be one of those issues which just change depending on the clang format versions.
Just as an example, I used this:
LinOp* apply(ptr_param<const LinOp> alpha,
ptr_param<const LinOp> b, // tis a long comment describing the parametr b for what ever reasons
ptr_param<const LinOp> beta,
ptr_param<LinOp> x)
and the pre-commit gave me:
LinOp* apply(ptr_param<const LinOp> alpha,
ptr_param<const LinOp> b, // tis a long comment describing the
// parametr b for what ever reasons
ptr_param<const LinOp> beta, ptr_param<LinOp> x)
which is your 2. But maybe it depends more on the actual example.
There was a problem hiding this comment.
The issue happens when the different indention format clang-format, that's why the example is under the assumption.
For more realistic example, you need to look for the attribute usage.
Clang-format does not give any consistence between version if no option to control behavior such that we may have different indention. It's also why we need to fix a version for repo code not just rely on the config version requirement. There are always some undefined behavior across versions.
Otherwise, the config only requires 9+ and format_header only requires 12+.
clang-format 9, 12, 14, 17
There was a problem hiding this comment.
I'm not sure if I understand your point. The whole point of this PR is to fix a clang-format version that is used both in the CI and each local repo.
MarcelKoch
force-pushed
the
use-fixed-clang-format
branch
2 times, most recently
from
df5e820
to
b391b77
Compare
|
Here is a PR in my fork showing the updated CI: MarcelKoch#11 |
MarcelKoch
force-pushed
the
use-fixed-clang-format
branch
from
fe9f888
to
6b94de8
Compare
upsj
reviewed
Nov 13, 2023
There was a problem hiding this comment.
The general setup looks good to me. I definitely see it clashing with my clangd installation which handles the formatting, but it will be caught at commit time, so it should be acceptable. There are a few unrelated changes in the .gitlab-ci.yml we should revert. Also the github token usage leads to the comments coming from the wrong user.
upsj
reviewed
Nov 13, 2023
|
Reading through its documentation, It seems that |
|
AFAIK, pre-commit is mostly language agnostic. Which language is supported or not depends on the hook. Also, it is always possible to restrict the hooks to certain file sets, e.g. based on filename suffix. |
yhmtsai
reviewed
Nov 13, 2023
There was a problem hiding this comment.
from pre-commit/pre-commit#1468, pre-comment does not intend to export the repo usage. It's a bit sad because the inconsistence between local and pre-commit may introduce some annoying difference without notice.
| with: | ||
| name: patch | ||
| path: format.patch | ||
| abidiff: |
There was a problem hiding this comment.
abidiff is deleted accidentally
There was a problem hiding this comment.
yes, I will revert that, when I prepare this branch for merging
.github/workflows/intel.yml
Outdated
| @@ -21,8 +21,7 @@ jobs: | |||
| fail-fast: false | |||
| matrix: | |||
| config: | |||
There was a problem hiding this comment.
the following change should be rebased error
| @@ -27,10 +27,8 @@ jobs: | |||
| fail-fast: false | |||
| matrix: | |||
| config: | |||
| # Debug shared exceeds symbol limit | |||
.gitlab-ci.yml
Outdated
| @@ -297,11 +295,9 @@ test/cuda110/nompi/clang/cuda/release/static: | |||
| variables: | |||
| USE_NAME: "cuda110-nompi-clang-${CI_PIPELINE_ID}" | |||
| SLURM_PARTITION: "accelerated" | |||
| SLURM_GRES: "gpu:4" | |||
Comment on lines
+1
to
+3
| repos: | ||
| - repo: https://github.com/pre-commit/mirrors-clang-format | ||
| rev: 'v14.0.0' # The default in Ubuntu 22.04, which is used in our CI |
There was a problem hiding this comment.
It may work in almost case but failed with the comment and string because clang-format does not try to concat them back if break early.
local clang-format breaks the string/comment eariler due to different indentation or format. the pre-commit does not fix it if they are still under 80 column limit (but not correct)
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| - name: Upload code formatting patch | ||
| if: ${{ failure() && steps.pre-commit.outcome == 'failure' }} |
There was a problem hiding this comment.
because I don't want to trigger this if any other step failed, e.g. the git checkout step. But I guess it's not necessary, since the uploading would just also fail.
I will use only one failure.
There was a problem hiding this comment.
I think keeping the second one should be enough? or the second one requires failure() to properly catch
There was a problem hiding this comment.
The first one is necessary, see: https://docs.github.com/en/actions/learn-github-actions/expressions#status-check-functions. If failure() is not used, the if will implicitly use success().
There was a problem hiding this comment.
I see. Thanks for explanation. Keeping both looks good to me
|
It's not |
yhmtsai
approved these changes
Nov 13, 2023
MarcelKoch
force-pushed
the
use-fixed-clang-format
branch
from
ff21443
to
5fbecd0
Compare
MarcelKoch
force-pushed
the
use-fixed-clang-format
branch
from
5fbecd0
to
a8ce733
Compare
- fix format-rebase.sh - yml formatting Co-authored-by: Yu-Hsiang M. Tsai <yhmtsai@gmail.com>
MarcelKoch
force-pushed
the
use-fixed-clang-format
branch
from
e9131ea
to
d578455
Compare
|
Error: The following files need to be formatted: You can find a formatting patch under Artifacts here or run |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR enables us to force users to use the same version of clang-format as our CI. This is done by requiring contributors to install pre-commit (which can be done easily through pip/pipx). pre-commit allows us to fix the used version of clang-format, so that contributors will use exactly the same version as our CI.
If pre-commit can't be detected during cmake an error is thrown. Otherwise, the pre-commit hooks are installed.
Since this replaces our git-cmake-format, I have removed it.
To see it in action: here is an PR in a fork that shows the CI use cases MarcelKoch#9
Warning
For those using git-worktrees, if one working tree is updated with this PR it will overwrite the git hooks for all working trees (I don't think you can have different hooks for different working trees). The easy fix is of course to just update all used working trees accordingly.