Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -806,10 +806,18 @@ add_custom_target(
VERBATIM
)

add_custom_target(
whitespace-format
${CMAKE_SOURCE_DIR}/tools/whitespace-format.sh
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
COMMENT "fixing whitespace issues (trailing whitespace, DOS line endings)"
VERBATIM
)

# Add a format target that runs all the formatters.
add_custom_target(
format
DEPENDS clang-format yapf cmake-format
DEPENDS clang-format yapf cmake-format whitespace-format
Copy link
Contributor

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-format when that check complains.

Copy link
Contributor Author

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.

COMMENT "formatting all files"
)

Expand Down
26 changes: 26 additions & 0 deletions tools/git/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,32 @@ else
echo
fi

# Check for trailing whitespace in staged files.
TRAILING_WS=$(git diff-index --cached --diff-filter=ACMR --name-only HEAD | grep -vE "lib/(yamlcpp|Catch2|systemtap)" | grep -vE "\.(gold|test_input)$" | xargs git diff --cached | grep -E '^\+.*[[:space:]]$' || true)
if [ -n "$TRAILING_WS" ]; then
echo "The commit is not accepted because it contains trailing whitespace."
echo "The easiest way to fix this is to run:"
echo
echo " $ cmake --build <build_dir> --target format"
exit 1
else
echo "This commit complies with the trailing whitespace rules."
echo
fi

# Check for DOS carriage returns in staged files.
DOS_CR=$(git diff-index --cached --diff-filter=ACMR --name-only HEAD | grep -vE "lib/(yamlcpp|Catch2|systemtap)" | grep -vE "\.test_input$" | xargs git diff --cached | grep -E '^\+.*\r$' || true)
if [ -n "$DOS_CR" ]; then
echo "The commit is not accepted because it contains DOS carriage returns."
echo "The easiest way to fix this is to run:"
echo
echo " $ cmake --build <build_dir> --target format"
exit 1
else
echo "This commit complies with the DOS line ending rules."
echo
fi

# Cleanup before exit
rm -f "$clang_patch_file" "$yapf_patch_file"
exit 0
68 changes: 68 additions & 0 deletions tools/whitespace-format.sh
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"
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:
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.

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"
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The -I in grep ignores binary files.

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