MueLu: Add clang-format workflow and run it on MueLu #12058
Merged
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
GrahamBenHarper
added
pkg: MueLu
AT: WIP
|
@GrahamBenHarper Is this the same format that Kokkos uses? |
|
@csiefer2 actually probably not. I stole it from a different code. I should try the Kokkos version instead to be more consistent. |
|
@GrahamBenHarper It looks like the formatter is enforcing an 72 column width. Is that something that can be relaxed? |
|
@jhux2 yep there's definitely some gore in heavily indented regions due to the 72 column width. There should be a way to relax things like that. I'll also check what Kokkos does |
GrahamBenHarper
force-pushed
the
muelu_clangd_format
branch
from
f09e897
to
af5dac5
Compare
|
I switched over to what Kokkos uses, but it still looks pretty comparable to what I saw before. I'd have to dig deeper to see how to change the allowed column width |
|
Ideally, you want to set a code style that only flags things that are currently clearly inconsistent. |
|
I value the fact, that there is a code formatter, much more than the individual formatting options. Of course, I might make sense to stick to a style, that is somehow common in the Trilinos/Kokkos universe, so Kokkos style sounds very reasonable. I agree with @cgcgcg: ultimately, one should not be allowed to check-in style-incompatible code. |
GrahamBenHarper
force-pushed
the
muelu_clangd_format
branch
from
af5dac5
to
2a4eb66
Compare
|
@cgcgcg @mayrmt I agree. I added a @trilinos/muelu I set so I think it makes more sense to ignore the column limit for now. I haven't looked at these new changes closer yet, so I still don't know if there are other options we want to play with. Let me know if you spot something else that might be possible improve. |
|
@GrahamBenHarper Why does |
GrahamBenHarper
changed the title
|
@cgcgcg oh! that's because the reference I followed did that for CI purposes and diffing actually. I should have stared at the documentation longer because it definitely works with |
|
I'm not a big fan of the single space for class access modifiers, but this screengrab does a decent job at capturing some of the different things the formatter does. Note it removes indents for the MueLu namespace, but I think that's generally fine since almost everything is guarded by the MueLu namespace. |
|
And I think this is where most of those extra 3k deletions came from |
cgcgcg
reviewed
Jul 18, 2023
|
This last commit allows up to 3 consecutive empty lines, puts access modifiers on the same line as the class, and splits constructor initializer lists again. Now it's almost exactly the same number of additions and deletions, which is a good sign that it's not horribly mangling things |
GrahamBenHarper
force-pushed
the
muelu_clangd_format
branch
3 times, most recently
from
2fd6312
to
7401645
Compare
GrahamBenHarper
removed
the
AT: WIP
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: GrahamBenHarper |
GrahamBenHarper
changed the title
GrahamBenHarper
force-pushed
the
muelu_clangd_format
branch
from
79e9779
to
27387a8
Compare
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: GrahamBenHarper |
cgcgcg
reviewed
Dec 15, 2023
|
Status Flag 'Pull Request AutoTester' - Error: Jenkins Jobs - A user has pushed a change to the PR before testing completed. NEW EVENT 'committed', ID C_kwDOAsJyMdoAKDNjN2RhNTRkOWY0MTJiYTJjZWMzYjc0ZmQwMTg2NDkwYjU4M2M1YWI... The Jenkins Jobs will be shutdown; Testing of this PR must occur again. |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
GrahamBenHarper
added
AT: RETEST
cgcgcg
removed
the
AT: AUTOMERGE
cgcgcg
approved these changes
Dec 15, 2023
|
Can you give us mere mortals a |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: GrahamBenHarper |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
trilinos-autotester
removed
the
AT: RETEST
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ cgcgcg ]! |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
mayrmt
approved these changes
Dec 15, 2023
|
Sure @jhux2, please let me know if you have any questions about it! |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
2 similar comments
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
Fine with me |
|
For future reference: To install a pre-commit hook put files: "packages/muelu/.*pp"
repos:
- repo: https://github.com/pre-commit/mirrors-clang-format
rev: v14.0.1
hooks:
- id: clang-format
types_or: [c++, c, cuda]in |
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.
@trilinos/muelu
Suggested by @csiefer2 in light of his recent PRs :)
This will almost certainly break any in-progress PRs in MueLu, so this is not necessarily up for serious consideration at the moment until we see if we really like how it formats things.
How does this work?
I ran
clang-formatversion 14.0.0 and 14.0.6 from the llvm 14.0.0 binary release and from our SEMS clang 14.0.6 module on MueLu. To do so, I placed the.clang-formatfrom Kokkos at the top directory of MueLu, with a couple documented additions.Other technical details: when
clang-formatis invoked from a directory, it searches for.clang-format, and moves up a directory every time it does not find the format file. Once it finds the format file, it uses that file. It errors out if it does not find a format file. Therefore, we placed it in the root directory of MueLu so that any invocation within MueLu will find the format file.Summary
The workflow runs
clang-formatversion 14.0.0 on thepackages/muelu/srcdirectory on only the.cppand.hppfiles. If the formatting is correct, the workflow passes. If the formatting is not correct, the workflow will report a diff for the offending lines when you click "details". Here's a screenshot of the workflow dashboard I were to purposely mess up indentation in a file. Notice it suggests the correct indentation in green.Compatibility for Users
Since it might be complicated for users to try to setup clang-format on their own, the script in
packages/muelu/utils/run_clang_format.shprovides helpful instructions if clang-format is not detected on a system. In particular, it points users to the llvm 14.0.0 binary release and instructs them to download it if they do not have clang-format installed. Additionally, the script checks to make sure the version is 14.0.0 since there are sometimes vast differences in behavior between clang-format versions. Otherwise, just like rebasing gold files, you simply need to runpackages/muelu/utils/run_clang_format.shto fix any code formatting reported by the workflow.I think a couple people from MueLu should try to follow the directions and see if it's difficult to fix code formatting or not.