Apply final set of Shellcheck fixes and turn on in CI

.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