Apply final set of Shellcheck fixes and turn on in CI (#7832)

Builds off of the work from #7698 and #7719.

Several of the problems are ignored because they are very difficult to correctly fix, and the behavior is not broken when ran locally and in CI so the cost of fixing is not deemed worth it.

.travis.yml

@@ -148,6 +148,7 @@ base_linux_config: &base_linux_config
         - libssl-dev
         - jq
         - unzip
+        - shellcheck
   language: python
   before_install:
     - ./build-support/bin/install_aws_cli_for_ci.sh

build-support/bin/check_rust_formatting.sh

@@ -42,6 +42,8 @@ if [[ "${check}" == "true" ]]; then
   cmd=("${cmd[@]}" "--check")
 fi
 
+# WONTFIX: fixing the array expansion is too difficult to be worth it. See https://github.com/koalaman/shellcheck/wiki/SC2207.
+# shellcheck disable=SC2207
 bad_files=(
   $(
     cd "${NATIVE_ROOT}" || exit "${PIPESTATUS[0]}"

build-support/bin/check_shell.sh

@@ -2,6 +2,7 @@
 
 exit_code=0
 
+# NB: we intentionally don't want quote expansion here. See https://github.com/koalaman/shellcheck/wiki/SC2016.
 # shellcheck disable=SC2016
 bad_output="$(find ./* -name '*.sh' -print0 \
   | xargs -0 grep '^ *\(readonly\|declare\) .*\(\$(\|`\)' \

build-support/bin/ci.sh

@@ -120,6 +120,7 @@ else
 fi
 export PY="${PY:-python${py_major_minor}}"
 
+# NB: we intentionally don't want quote expansion here. See https://github.com/koalaman/shellcheck/wiki/SC2016.
 # shellcheck disable=SC2016
 export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS:-['CPython==${py_major_minor}.*']}"
 banner "Setting interpreter constraints to ${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS}"

build-support/bin/get_added_files.sh

@@ -6,4 +6,4 @@
 # Check for copies (-C) and moves (-M), so we don't get false positives when people do
 # refactorings. -l50 bounds the time git takes to search for these non-additions.
 # See git-diff(1) and https://stackoverflow.com/a/2299672/2518889 for discussion of these options.
-exec git diff --cached --name-only --diff-filter=A -C -M -l50
+exec git --no-pager diff --cached --name-only --diff-filter=A -C -M -l50

build-support/bin/get_failing_travis_targets_for_pr.sh

@@ -23,6 +23,8 @@ for job in ${jobs}; do
       ;;
     "failed")
       "${curl[@]}" "https://api.travis-ci.org/job/${job}/log.txt" > "logs/jobs/${job}/txt"
+      # WONTFIX: fixing the array expansion is too difficult to be worth it. See https://github.com/koalaman/shellcheck/wiki/SC2207.
+      # shellcheck disable=SC2207
       targets=("${targets[@]}" $(cat -v "logs/jobs/${job}/txt" | awk '$2 == "....." && $3 ~ /^FAILURE/ {print $1}'))
       ;;
     *)

build-support/bin/native/bootstrap_rust.sh

@@ -75,7 +75,7 @@ function bootstrap_rust() {
         fi
       done
 
-      ln -fs "$(dirname "${cargo_bin_dir}")/lib/rustlib/src/rust/src"
+      ln -fs "$(dirname "${cargo_bin_dir}")/lib/rustlib/src/rust/src" src
       ln -fs cargo.sh cargo
       ln -fs cargo.sh ".${cargo_versioned}"
     )

build-support/bin/release-changelog-helper.sh

@@ -74,6 +74,8 @@ do
   echo "* ${subject}"
 
   urls=()
+  # WONTFIX: fixing the array expansion is too difficult to be worth it. See https://github.com/koalaman/shellcheck/wiki/SC2207.
+  # shellcheck disable=SC2207
   urls=(
     "${urls[@]}"
     $(
@@ -82,6 +84,8 @@ do
         sed -Ee "s|^\(#([0-9]+)\)$|https://github.com/pantsbuild/pants/pull/\1|"
     )
   )
+  # WONTFIX: fixing the array expansion is too difficult to be worth it. See https://github.com/koalaman/shellcheck/wiki/SC2207.
+  # shellcheck disable=SC2207
   urls=(
     "${urls[@]}"
     $(

build-support/bin/release.sh

@@ -228,6 +228,8 @@ function build_pants_packages() {
   pants_version_set "${version}"
 
   start_travis_section "${NAME}" "Building packages"
+  # WONTFIX: fixing the array expansion is too difficult to be worth it. See https://github.com/koalaman/shellcheck/wiki/SC2207.
+  # shellcheck disable=SC2207
   packages=(
     $(run_packages_script build_and_print "${version}")
   ) || die "Failed to build packages at ${version}!"
@@ -315,6 +317,8 @@ function install_and_test_packages() {
   export PANTS_PLUGIN_CACHE_DIR
   trap 'rm -rf "${PANTS_PLUGIN_CACHE_DIR}"' EXIT
 
+  # WONTFIX: fixing the array expansion is too difficult to be worth it. See https://github.com/koalaman/shellcheck/wiki/SC2207.
+  # shellcheck disable=SC2207
   packages=(
     $(run_packages_script list | grep '.' | awk '{print $1}')
   ) || die "Failed to list packages!"
@@ -515,11 +519,14 @@ function fetch_and_check_prebuilt_wheels() {
   fetch_prebuilt_wheels "${check_dir}"
 
   local missing=()
+  # WONTFIX: fixing the array expansion is too difficult to be worth it. See https://github.com/koalaman/shellcheck/wiki/SC2207.
+  # shellcheck disable=SC2207
   RELEASE_PACKAGES=(
     $(run_packages_script list | grep '.' | awk '{print $1}')
   ) || die "Failed to get a list of packages to release!"
-  for PACKAGE in "${RELEASE_PACKAGES[@]}"
-  do
+  for PACKAGE in "${RELEASE_PACKAGES[@]}"; do
+    # WONTFIX: fixing the array expansion is too difficult to be worth it. See https://github.com/koalaman/shellcheck/wiki/SC2207.
+    # shellcheck disable=SC2207
     packages=($(find_pkg "${PACKAGE}" "${PANTS_UNSTABLE_VERSION}" "${check_dir}"))
     if [ ${#packages[@]} -eq 0 ]; then
       missing+=("${PACKAGE}")
@@ -528,8 +535,7 @@ function fetch_and_check_prebuilt_wheels() {
 
     # Confirm that if the package is not cross platform that we have whls for two platforms.
     local cross_platform=""
-    for package in "${packages[@]}"
-    do
+    for package in "${packages[@]}"; do
       if [[ "${package}" =~ -none-any.whl ]]
       then
         cross_platform="true"
@@ -559,7 +565,7 @@ function adjust_wheel_platform() {
   local src_plat="$1"
   local dst_plat="$2"
   local dir="$3"
-  for src_whl in $(find "${dir}" -name '*'"${src_plat}.whl"); do
+  find "$dir" -type f -name "*${src_plat}.whl" | while read -r src_whl; do
     local dst_whl=${src_whl/$src_plat/$dst_plat}
     mv -f "${src_whl}" "${dst_whl}"
   done

build-support/bin/shellcheck.py

@@ -6,7 +6,7 @@
 import subprocess
 from glob import glob
 
-from common import die, green
+from common import die
 
 
 def main() -> None:
@@ -37,8 +37,6 @@ def run_shellcheck() -> None:
     subprocess.run(command, check=True)
   except subprocess.CalledProcessError:
     die("Please fix the above errors and run again.")
-  else:
-    green("./pants passed the shellcheck!")
 
 
 if __name__ == "__main__":

build-support/common.sh

@@ -1,3 +1,4 @@
+# NB: shellcheck complains this is unused, but it's used by callers. See https://github.com/koalaman/shellcheck/wiki/SC2034.
 # shellcheck disable=SC2034
 CACHE_ROOT=${XDG_CACHE_HOME:-$HOME/.cache}/pants
 

build-support/githooks/pre-commit

@@ -19,8 +19,7 @@ DIRS_TO_CHECK=(
   pants-plugins
   examples
   contrib
-  # TODO(6071): Add `build-support/bin` once we update the check_header_helper.py script to
-  # allow Python 3 headers (i.e. without __future__ and coding=utf-8).
+  build-support/bin
 )
 
 # TODO(#7068): Fix all these checks to only act on staged files with
@@ -30,22 +29,24 @@ DIRS_TO_CHECK=(
 # integration test!
 set -e
 
-# You can use ("$()") with double quotes in zsh, I assume this splits by IFS...
+# WONTFIX: fixing the array expansion is too difficult to be worth it. See https://github.com/koalaman/shellcheck/wiki/SC2207.
+# shellcheck disable=SC2207
 ADDED_FILES=($(./build-support/bin/get_added_files.sh))
 MERGE_BASE="$(git_merge_base)"
 
 echo "* Checking packages"
-# TODO: Determine the most *hygienic* way to split an array on the command line in portable bash,
-# and stick to it.
-./build-support/bin/check_packages.sh "${DIRS_TO_CHECK[@]}"
+./build-support/bin/check_packages.sh "${DIRS_TO_CHECK[@]}" || exit 1
 
 echo "* Checking headers"
-./build-support/bin/check_header.py "${DIRS_TO_CHECK[@]}" --files-added "${ADDED_FILES[@]}"
+./build-support/bin/check_header.py "${DIRS_TO_CHECK[@]}" --files-added "${ADDED_FILES[@]}" || exit 1
 
 echo "* Checking for banned imports"
-./build-support/bin/check_banned_imports.py
+./build-support/bin/check_banned_imports.py || exit 1
 
-echo "* Checking for bad shell patterns"
+echo "* Checking shell scripts via shellcheck"
+./build-support/bin/shellcheck.py || exit 1
+
+echo "* Checking shell scripts via our custom linter"
 ./build-support/bin/check_shell.sh || exit 1
 
 # When travis builds a tag, it does so in a shallow clone without master fetched, which

build-support/travis/travis.yml.mustache

@@ -141,6 +141,7 @@ base_linux_config: &base_linux_config
         - libssl-dev
         - jq
         - unzip
+        - shellcheck
   language: python
   before_install:
     - ./build-support/bin/install_aws_cli_for_ci.sh

contrib/release_packages.sh

@@ -19,8 +19,8 @@ function pkg_scrooge_install_test() {
 function pkg_buildgen_install_test() {
   local version=$1
   shift
-  local PIP_ARGS="$@"
-  pip install ${PIP_ARGS} "pantsbuild.pants.contrib.buildgen==${version}" && \
+  local PIP_ARGS=("$@")
+  pip install "${PIP_ARGS[@]}" "pantsbuild.pants.contrib.buildgen==${version}" && \
   python -c "from pants.contrib.buildgen.build_file_manipulator import *"
 }
 

pants

@@ -67,12 +67,15 @@ export PY="${PY:-python3}"
 # Pants will try to use Python 2 for subprocesses. Without setting minor version constraints, when using Python 3
 # we could end up using a different Python minor version for subprocesses than we do for Pants.
 py_major_minor_patch=$(${PY} -c 'import sys; print(".".join(map(str, sys.version_info[0:3])))')
+# NB: We intentionally don't want quote expansion here. See https://github.com/koalaman/shellcheck/wiki/SC2016.
 # shellcheck disable=SC2016
 export PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS="${PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS:-['CPython==${py_major_minor_patch}']}"
 
 PANTS_EXE="${HERE}/src/python/pants/bin/pants_loader.py"
 
 if [[ -n "${WRAPPER_REQUIREMENTS}" ]]; then
+  # WONTFIX: fixing the array expansion is too difficult to be worth it. See https://github.com/koalaman/shellcheck/wiki/SC2207.
+  # shellcheck disable=SC2207
   REQUIREMENTS=(
     $(echo "${WRAPPER_REQUIREMENTS}" | tr : ' ')
     "${REQUIREMENTS[@]}"
@@ -83,18 +86,20 @@ PANTS_SRCPATH=(
   "${HERE}/src/python"
 )
 if [[ -n "${WRAPPER_SRCPATH}" ]]; then
+  # WONTFIX: fixing the array expansion is too difficult to be worth it. See https://github.com/koalaman/shellcheck/wiki/SC2207.
+  # shellcheck disable=SC2207
   PANTS_SRCPATH=(
     $(echo "${WRAPPER_SRCPATH}" | tr : ' ')
     "${PANTS_SRCPATH[@]}"
   )
 fi
-PANTS_SRCPATH="$(echo "${PANTS_SRCPATH[@]}" | tr ' ' :)"
+PANTS_SRCPATH_STR="$(echo "${PANTS_SRCPATH[@]}" | tr ' ' :)"
 
 function exec_pants_bare() {
   # Redirect activation and native bootstrap to ensure that they don't interfere with stdout.
   activate_pants_venv 1>&2
   bootstrap_native_code 1>&2
-  PYTHONPATH="${PANTS_SRCPATH}:${PYTHONPATH}" \
+  PYTHONPATH="${PANTS_SRCPATH_STR}:${PYTHONPATH}" \
     exec python "${PANTS_EXE}" "$@"
 }
 

src/docs/publish_via_git.sh

@@ -23,7 +23,7 @@ set -eo pipefail
 # If a file "went away", this won't remove it.
 
 root=$(
-  cd "$(dirname $0)"
+  cd "$(dirname "$0")"
   /bin/pwd
 )
 
@@ -32,7 +32,7 @@ path_within_url=$2
 out=/tmp/pantsdoc.$$
 
 # When done, clean up tmp dir:
-trap "rm -fr $out" 0 1 2
+trap 'rm -fr $out' 0 1 2
 
 mkdir -p $out
 cd $out