-
Notifications
You must be signed in to change notification settings - Fork 842
cmake format target: add whitespace correction #12696
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| #!/usr/bin/env bash | ||
| # | ||
| # Script to fix whitespace issues: trailing whitespace and DOS carriage returns. | ||
| # | ||
| # Licensed to the Apache Software Foundation (ASF) under one | ||
| # or more contributor license agreements. See the NOTICE file | ||
| # distributed with this work for additional information | ||
| # regarding copyright ownership. The ASF licenses this file | ||
| # to you under the Apache License, Version 2.0 (the | ||
| # "License"); you may not use this file except in compliance | ||
| # with the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| set -e | ||
|
|
||
| REPO_ROOT=$(cd $(dirname $0) && git rev-parse --show-toplevel) | ||
| echo "Fixing whitespace issues in ${REPO_ROOT}" | ||
|
|
||
| # macOS requires an empty string argument for in-place sed edits. | ||
| if [[ "$OSTYPE" == "darwin"* ]]; then | ||
| SED_INPLACE=(sed -i '') | ||
| else | ||
| SED_INPLACE=(sed -i) | ||
| fi | ||
|
|
||
| FILES_MODIFIED=0 | ||
|
|
||
| echo "Checking for trailing whitespace..." | ||
| TRAILING_WS_FILES=$(git grep -IE ' +$' | eval "grep -F -v 'lib/yamlcpp' | grep -F -v 'lib/Catch2' | grep -F -v 'lib/systemtap' | grep -F -v '.gold:' | grep -F -v '.test_input'" | cut -d: -f1 | sort -u || true) | ||
|
|
||
| if [ -n "$TRAILING_WS_FILES" ]; then | ||
| while IFS= read -r file; do | ||
| if [ -f "$file" ]; then | ||
| echo " Fixing trailing whitespace in: $file" | ||
| "${SED_INPLACE[@]}" 's/[[:space:]]*$//' "$file" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: 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. |
||
| FILES_MODIFIED=$((FILES_MODIFIED + 1)) | ||
| fi | ||
| done <<< "$TRAILING_WS_FILES" | ||
| fi | ||
|
|
||
| echo "Checking for DOS carriage returns..." | ||
| DOS_FILES=$(git grep -IE $'\r$' | eval "grep -F -v 'lib/yamlcpp' | grep -F -v 'lib/Catch2' | grep -F -v 'lib/systemtap' | grep -F -v '.test_input'" | cut -d: -f1 | sort -u || true) | ||
|
|
||
| if [ -n "$DOS_FILES" ]; then | ||
| while IFS= read -r file; do | ||
| if [ -f "$file" ]; then | ||
| echo " Removing DOS carriage returns from: $file" | ||
| "${SED_INPLACE[@]}" $'s/\r$//' "$file" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there can be some traps here as well. If we end up with binary files in the repo, this might destroy them.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| FILES_MODIFIED=$((FILES_MODIFIED + 1)) | ||
| fi | ||
| done <<< "$DOS_FILES" | ||
| fi | ||
|
|
||
| if [ $FILES_MODIFIED -eq 0 ]; then | ||
| echo "Success! No whitespace issues found." | ||
| else | ||
| echo "Fixed whitespace issues in $FILES_MODIFIED file(s)." | ||
| fi | ||
|
|
||
| exit 0 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rarely run this target. Often yapf doesn't work, so I just run the
cmake-formatwhen that check complains.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.