From 68da793945526c2b83913599ed6ac133f35fa7b5 Mon Sep 17 00:00:00 2001 From: mlevesquedion Date: Thu, 1 Feb 2024 15:40:15 -0800 Subject: [PATCH] Fix shellcheck issues (#1980) https://github.com/openxla/stablehlo/pull/1971 introduced a `shellcheck` lint. However, it also introduced some bugs due to adding quotes in a few places where word splitting was used. Thanks to gleasonk@ for spotting this: https://github.com/openxla/stablehlo/pull/1976 As described in https://www.shellcheck.net/wiki/SC2086, quoting is generally preferable when expanding a variable. However, when building a command line it is better to create an array and then expand the array. When using a regular variable, quoting can actually lead to incorrect behavior: for example, given `x="file1 file2"`, `clang-format $x` will correctly call `clang-format` on `file1` and `file2`, however `clang-format "$x"` will call `clang-format` on a non-existent file called `file1 file2`. The lint itself was actually broken because of using `-printf '%P\0'` with `find`. This produces null-delimited output, which `mapfile` does not expect and does not parse correctly (only the first file name will be extracted). (While implementing the check I thought I needed to use `printf` to remove the the `./` prefix introduced by `find`, but turns out that's not the case). While investigating this change I discovered that the whitespace lint check is also subtly broken. Indeed in `get_source_files` `xargs` is used at the end of the pipeline, which puts everything on one line, which causes subsequent uses of `xargs -L1` to behave incorrectly (the underlying command is executed only once with all files as arguments). --- .../github_actions/lint_clang_format.sh | 10 +++---- build_tools/github_actions/lint_shellcheck.sh | 2 +- .../github_actions/lint_whitespace_checks.sh | 26 +++++++++---------- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/build_tools/github_actions/lint_clang_format.sh b/build_tools/github_actions/lint_clang_format.sh index e3cc00d3de..27e36bf2dc 100755 --- a/build_tools/github_actions/lint_clang_format.sh +++ b/build_tools/github_actions/lint_clang_format.sh @@ -36,16 +36,16 @@ if [[ $# -ne 0 ]] ; then fi echo "Gathering changed files..." -CHANGED_FILES=$(git diff "$BASE_BRANCH" HEAD --name-only --diff-filter=d | grep '.*\.h\|.*\.cpp' | xargs) -if [[ -z "$CHANGED_FILES" ]]; then +mapfile -t CHANGED_FILES < <(git diff "$BASE_BRANCH" HEAD --name-only --diff-filter=d | grep '.*\.h\|.*\.cpp') +if (( ${#CHANGED_FILES[@]} == 0 )); then echo "No files to format." exit 0 fi echo "Running clang-format [mode=$FORMAT_MODE]..." -echo " Files: $CHANGED_FILES" +echo " Files: " "${CHANGED_FILES[@]}" if [[ $FORMAT_MODE == 'fix' ]]; then - clang-format --style=google -i $CHANGED_FILES + clang-format --style=google -i "${CHANGED_FILES[@]}" else - clang-format --style=google --dry-run --Werror $CHANGED_FILES + clang-format --style=google --dry-run --Werror "${CHANGED_FILES[@]}" fi diff --git a/build_tools/github_actions/lint_shellcheck.sh b/build_tools/github_actions/lint_shellcheck.sh index b71108338e..e2a8074296 100755 --- a/build_tools/github_actions/lint_shellcheck.sh +++ b/build_tools/github_actions/lint_shellcheck.sh @@ -32,7 +32,7 @@ readonly STABLEHLO_ROOT_DIR="${SCRIPT_DIR}/../.." cd "$STABLEHLO_ROOT_DIR" IFS=$'\n' -mapfile -t targets < <(find . -type f -name '*.sh' -not -path './llvm*' -printf '%P\0') +mapfile -t targets < <(find . -type f -name '*.sh' -not -path './llvm*') echo "Running shellcheck:" shellcheck --version diff --git a/build_tools/github_actions/lint_whitespace_checks.sh b/build_tools/github_actions/lint_whitespace_checks.sh index 9937ff5082..b9d78352b1 100755 --- a/build_tools/github_actions/lint_whitespace_checks.sh +++ b/build_tools/github_actions/lint_whitespace_checks.sh @@ -35,43 +35,43 @@ if [[ $# -ne 0 ]] ; then fi get_source_files() { - git diff "$BASE_BRANCH" HEAD --name-only --diff-filter=d | grep '.*\.cpp$\|.*\.h$\|.*\.md$\|.*\.mlir$\|.*\.sh$\|.*\.td$\|.*\.txt$\|.*\.yml$\|.*\.yaml$' | xargs + git diff "$BASE_BRANCH" HEAD --name-only --diff-filter=d | grep '.*\.cpp$\|.*\.h$\|.*\.md$\|.*\.mlir$\|.*\.sh$\|.*\.td$\|.*\.txt$\|.*\.yml$\|.*\.yaml$' } echo "Checking whitespace:" -echo " $(get_source_files)" +echo " $(get_source_files | xargs)" files_without_eof_newline() { # shellcheck disable=SC2016 - [[ -z $(get_source_files) ]] || get_source_files | xargs -L1 bash -c 'test "$(tail -c 1 "$0")" && echo "$0"' + get_source_files | xargs -L1 bash -c 'test "$(tail -c 1 "$0")" && echo "$0"' } files_with_trailing_whitespace() { - get_source_files | xargs grep -lP '[ \t]+$' + get_source_files | xargs -L1 grep -lP '[ \t]+$' } fix_files_without_eof_newline() { # shellcheck disable=SC2016,SC1003 - echo "$1" | xargs --no-run-if-empty sed -i -e '$a\' + echo "$@" | xargs --no-run-if-empty sed -i -e '$a\' } fix_files_with_trailing_whitespace() { - echo "$1" | xargs --no-run-if-empty sed -i 's/[ \t]*$//' + echo "$@" | xargs --no-run-if-empty sed -i 's/[ \t]*$//' } -EOF_NL=$(files_without_eof_newline) -TRAIL_WS=$(files_with_trailing_whitespace) +mapfile -t EOF_NL < <(files_without_eof_newline) +mapfile -t TRAIL_WS < <(files_with_trailing_whitespace) if [[ $FORMAT_MODE == 'fix' ]]; then echo "Fixing EOF newlines..." - fix_files_without_eof_newline "$EOF_NL" + fix_files_without_eof_newline "${EOF_NL[@]}" echo "Fixing trailing whitespaces..." - fix_files_with_trailing_whitespace "$TRAIL_WS" + fix_files_with_trailing_whitespace "${TRAIL_WS[@]}" else - if [ -n "$EOF_NL$TRAIL_WS" ]; then + if (( ${#EOF_NL[@]} + ${#TRAIL_WS[@]} )); then echo "Missing newline at EOF:" - echo "$EOF_NL" + echo "${EOF_NL[@]}" echo "Has trailing whitespace:" - echo "$TRAIL_WS" + echo "${TRAIL_WS[@]}" echo echo "Auto-fix using:" echo " $ ./build_tools/github_actions/lint_whitespace_checks.sh -f"