cmake format target: add whitespace correction #12696
Conversation
cmcfarlen
left a comment
cmcfarlen
left a comment
There was a problem hiding this comment.
This should not be necessary. clang-format does remove trailing whitespace. It should also change CR+LF to just LF unless most of the file is CR+LF. If we want to ensure LF, we could make this change:
diff --git a/.clang-format b/.clang-format
index 606199c54..0c46d40b9 100644
--- a/.clang-format
+++ b/.clang-format
@@ -151,7 +151,7 @@ JavaScriptWrapImports: true
KeepEmptyLinesAtTheStartOfBlocks: false
KeepEmptyLinesAtEOF: false
LambdaBodyIndentation: Signature
-LineEnding: DeriveLF
+LineEnding: LF
MacroBlockBegin: ''
MacroBlockEnd: ''
MaxEmptyLinesToKeep: 1
If other files are problematic, I say let CI flag it and the author fix their editor to do the right thing.
|
clang-format removes trailing white space, but people leave trailing white space in non c++ files. That causes issues with PRs as our CI system enforces no trailing white spaces anywhere (save in excepted files, which whitespace-format.sh replicates.) We might as well take care of this for devs. Why not do this? It runs in under half a second. It saves some annoyance. |
cmcfarlen
left a comment
cmcfarlen
left a comment
There was a problem hiding this comment.
since this doesn't check for issues in the pre-commit hook, it would only reliably help if users ran the format target before each PR. I'm also not convinced this would always be safe to run for C++ files, so maybe only have this script work on non-code files and let yapf, cmake and clangd handle their respective files?
If this script made a mistake it would be quite frustrating, and most of the time it will be redundant work with the other formatters we run. Devs should really setup their editor to do what this script does.
| while IFS= read -r file; do | ||
| if [ -f "$file" ]; then | ||
| echo " Fixing trailing whitespace in: $file" | ||
| "${SED_INPLACE[@]}" 's/[[:space:]]*$//' "$file" |
There was a problem hiding this comment.
I wonder if this always applicable? One case I can think of where it might not be correct is if there was some C++ raw string that needed trailing whitespace. I know clangd has special code to handle some corner cases.
There was a problem hiding this comment.
Maybe in theory. Keep in mind this will fail CI today if that ever is the case because that check is done in CI now. My intention with this PR is to pull the CI check to a local change so that things fail locally rather than later in CI.
For reference, here's the CI check that this file borrows logic from:
https://github.com/apache/trafficserver-ci/blob/2286af7d567d1f36a78aeb138a97cf9f3dc70b45/jenkins/github/format.pipeline#L62-L74
Even better: with this change, I can take these special white space checks out of CI and just use this format target alone. Then if such a string is added, this script can be updated for it along with the PR that introduces such problems.
| add_custom_target( | ||
| format | ||
| DEPENDS clang-format yapf cmake-format | ||
| DEPENDS clang-format yapf cmake-format whitespace-format |
There was a problem hiding this comment.
I rarely run this target. Often yapf doesn't work, so I just run the cmake-format when that check complains.
There was a problem hiding this comment.
This is interesting, albeit not something this patch will address. I never have issues yapf, but I always develop in a container from CI that has the tools (namely, python, pip, pipenv) that makes this work reliably.
I assume you have python issues? I don't know of a way to address python formatting more reliably. It would be nice if we had a compiled binary we can ship like we do with clang-format, but I think that's not available.
| while IFS= read -r file; do | ||
| if [ -f "$file" ]; then | ||
| echo " Removing DOS carriage returns from: $file" | ||
| "${SED_INPLACE[@]}" $'s/\r$//' "$file" |
There was a problem hiding this comment.
I think there can be some traps here as well. If we end up with binary files in the repo, this might destroy them.
There was a problem hiding this comment.
The -I in grep ignores binary files.
bneradt
force-pushed
the
whitespace-format
branch
from
72d01c5 to
2fbaec6
Compare
Our format target does formatting of C++ and Python files via clang-format and yapf. Annoyingly, it hasn't done some extra whitespace checks that CI does. So a PR can be made that passes `format` locally but then CI will reject it for trailing whitespace, or DOS line endinds. This adds a new tools/whitespace-format.sh script that automatically fixes trailing whitespace and DOS carriage returns, matching the checks performed by CI. This script is then integrated into the `format` cmake target so it gets run for that. I verified that the script works on both Linux and Mac.
bneradt
force-pushed
the
whitespace-format
branch
from
2fbaec6 to
4d18f0f
Compare
Good point. I updated the pre-commit hook. |
With the following PR, we no longer need to roll our own whitespace check in ci because the format target itself will cover it as it already does for cmake, clang, and python formatting. apache/trafficserver#12696
With the following PR, we no longer need to roll our own whitespace check in ci because the format target itself will cover it as it already does for cmake, clang, and python formatting. apache/trafficserver#12696
This reverts commit a399830.
|
Removing from 10.1 project as there are conflicts and we shouldn't be adding anything but cherry-picks to that branch anyway. |
2 participants
Our format target does formatting of C++ and Python files via clang-format and yapf. Annoyingly, it hasn't done some extra whitespace checks that CI does. So a PR can be made that passes
formatlocally but then CI will reject it for trailing whitespace, or DOS line endinds. This adds a new tools/whitespace-format.sh script that automatically fixes trailing whitespace and DOS carriage returns, matching the checks performed by CI. This script is then integrated into theformatcmake target so it gets run for that.I verified that the script works on both Linux and Mac.