Add shellcheck.py and apply first round of fixes (#7698)

[Shellcheck](https://www.shellcheck.net) is a phenomenal linter for bash scripts. We have a non-trivial amount of bash code, so any additional help we can get to ensure quality is useful.

This PR only takes shellcheck's suggestions when obviously correct. More complex changes are left for followup PRs, after which we can start to require shellcheck in CI.

build-support/bin/BUILD

@@ -33,3 +33,12 @@ python_binary(
   ],
   compatibility = ['CPython>=3.6'],
 )
+
+python_binary(
+  name = 'shellcheck',
+  sources = 'shellcheck.py',
+  dependencies = [
+    ':common',
+  ],
+  compatibility = ['CPython>=3.6'],
+)

build-support/bin/check_clippy.sh

@@ -3,6 +3,6 @@
 set -e
 
 REPO_ROOT="$(git rev-parse --show-toplevel)"
-cd ${REPO_ROOT}
+cd "${REPO_ROOT}"
 
 ./build-support/bin/native/cargo clippy --manifest-path="${REPO_ROOT}/src/rust/engine/Cargo.toml" --all

build-support/bin/check_packages.sh

@@ -4,19 +4,19 @@ set -euo pipefail
 IFS=$'\n\t'
 
 REPO_ROOT="$(git rev-parse --show-toplevel)"
-cd ${REPO_ROOT}
+cd "${REPO_ROOT}"
 
 DIRS_TO_CHECK=("$@")
 
-non_empty_files=$(find ${DIRS_TO_CHECK[@]} -type f -name "__init__.py" -not -empty)
+non_empty_files=$(find "${DIRS_TO_CHECK[@]}" -type f -name "__init__.py" -not -empty)
 
 bad_files=()
 for package_file in ${non_empty_files}
 do
-  if [[ "$(sed -E -e 's/^[[:space:]]+//' -e 's/[[:space:]]+$//' ${package_file})" != \
+  if [[ "$(sed -E -e 's/^[[:space:]]+//' -e 's/[[:space:]]+$//' "${package_file}")" != \
         "__import__('pkg_resources').declare_namespace(__name__)" ]]
   then
-    bad_files+=(${package_file})
+    bad_files+=("${package_file}")
   fi
 done
 

build-support/bin/check_rust_formatting.sh

@@ -36,25 +36,25 @@ done
 NATIVE_ROOT="${REPO_ROOT}/src/rust/engine"
 
 cmd=(
-  ${REPO_ROOT}/build-support/bin/native/cargo fmt --all --
+  "${REPO_ROOT}/build-support/bin/native/cargo" fmt --all --
 )
 if [[ "${check}" == "true" ]]; then
   cmd=("${cmd[@]}" "--check")
 fi
 
 bad_files=(
   $(
-    cd "${NATIVE_ROOT}"
+    cd "${NATIVE_ROOT}" || exit "${PIPESTATUS[0]}"
 
     # Ensure generated code is present since `cargo fmt` needs to do enough parsing to follow use's
     # and these will land in generated code.
     echo >&2 "Ensuring generated code is present for downstream formatting checks..."
-    ${REPO_ROOT}/build-support/bin/native/cargo check -p bazel_protos
+    "${REPO_ROOT}/build-support/bin/native/cargo" check -p bazel_protos
 
-    ${cmd[@]} | \
+    "${cmd[@]}" | \
       awk '$0 ~ /^Diff in/ {print $3}' | \
       sort -u
-    exit ${PIPESTATUS[0]}
+    exit "${PIPESTATUS[0]}"
   )
 )
 exit_code=$?
@@ -63,15 +63,15 @@ if [[ ${exit_code} -ne 0 ]]; then
   if [[ "${check}" == "true" ]]; then
     echo >&2 "The following rust files were incorrectly formatted, run \`$0 -f\` to reformat them:"
     for bad_file in ${bad_files[*]}; do
-      echo >&2 ${bad_file}
+      echo >&2 "${bad_file}"
     done
   else
     cat << EOF >&2
 An error occurred while checking the formatting of rust files.
 Try running \`(cd "${NATIVE_ROOT}" && ${cmd[@]})\` to investigate.
 Its error is:
 EOF
-    cd "${NATIVE_ROOT}" && ${cmd[@]} >/dev/null
+    cd "${NATIVE_ROOT}" && "${cmd[@]}" >/dev/null
   fi
   exit 1
 fi

build-support/bin/check_rust_target_headers.sh

@@ -6,8 +6,10 @@ cargo="${REPO_ROOT}/build-support/bin/native/cargo"
 
 "${cargo}" ensure-installed --package cargo-ensure-prefix --version 0.1.3
 
-out="$("${cargo}" ensure-prefix --manifest-path="${REPO_ROOT}/src/rust/engine/Cargo.toml" --prefix-path="${REPO_ROOT}/build-support/rust-target-prefix.txt" --all --exclude=bazel_protos)"
-if [[ $? -ne 0 ]]; then
+if ! out="$("${cargo}" ensure-prefix \
+  --manifest-path="${REPO_ROOT}/src/rust/engine/Cargo.toml" \
+  --prefix-path="${REPO_ROOT}/build-support/rust-target-prefix.txt" \
+  --all --exclude=bazel_protos)"; then
   echo >&2 "Rust targets didn't have correct prefix:"
   echo >&2 "${out}"
   exit 1

build-support/bin/check_shell.sh

@@ -1,5 +1,9 @@
 #!/bin/bash -eu
-bad_output="$(find * -name '*.sh' -print0 | xargs -0 grep '^ *\(readonly\|declare\) .*\(\$(\|`\)' || :)"
+
+# shellcheck disable=SC2016
+bad_output="$(find ./* -name '*.sh' -print0 \
+  | xargs -0 grep '^ *\(readonly\|declare\) .*\(\$(\|`\)' \
+  || :)"
 
 if [[ -n "${bad_output}" ]]; then
   echo >&2 "Found bash files with readonly variables defined by invoking subprocesses."

build-support/bin/ci.sh

@@ -4,8 +4,8 @@
 # fails the build.
 set -o pipefail
 
-REPO_ROOT=$(cd $(dirname "${BASH_SOURCE[0]}") && cd "$(git rev-parse --show-toplevel)" && pwd)
-cd ${REPO_ROOT}
+REPO_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")" && cd "$(git rev-parse --show-toplevel)" && pwd)
+cd "${REPO_ROOT}" || exit 1
 
 source build-support/common.sh
 
@@ -85,11 +85,11 @@ while getopts "h27fxbmrjlpeasu:ny:ci:tz" opt; do
     *) usage "Invalid option: -${OPTARG}" ;;
   esac
 done
-shift $((${OPTIND} - 1))
+shift $((OPTIND - 1))
 
 echo
-if [[ $# > 0 ]]; then
-  banner "CI BEGINS: $@"
+if [[ $# -gt 0 ]]; then
+  banner "CI BEGINS: $*"
 else
   banner "CI BEGINS"
 fi
@@ -106,7 +106,7 @@ export PANTS_DEV=1
 
 # We only want to output failures and skips.
 # See https://docs.pytest.org/en/latest/usage.html#detailed-summary-report.
-export PYTEST_PASSTHRU_ARGS="-q -rfa"
+export PYTEST_PASSTHRU_ARGS=(-q -rfa)
 
 # Determine the Python version to use for bootstrapping pants.pex. This would usually not be
 # necessary to set when developing locally, because the `./pants` and `./pants2` scripts set
@@ -122,6 +122,7 @@ else
 fi
 export PY="${PY:-python${py_major_minor}}"
 
+# 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}"
 
@@ -196,7 +197,7 @@ if [[ "${run_internal_backends:-false}" == "true" ]]; then
   start_travis_section "BackendTests" "Running internal backend python tests"
   (
     ./pants.pex test.pytest \
-    pants-plugins/src/python:: pants-plugins/tests/python:: -- ${PYTEST_PASSTHRU_ARGS}
+    pants-plugins/src/python:: pants-plugins/tests/python:: -- "${PYTEST_PASSTHRU_ARGS[@]}"
   ) || die "Internal backend python test failure"
   end_travis_section
 fi
@@ -208,8 +209,8 @@ if [[ "${run_python:-false}" == "true" ]]; then
   start_travis_section "CoreTests" "Running core python tests${shard_desc}"
   (
     ./pants.pex --tag='-integration' test.pytest --chroot \
-      --test-pytest-test-shard=${python_unit_shard} \
-      src/python:: tests/python:: -- ${PYTEST_PASSTHRU_ARGS}
+      "--test-pytest-test-shard=${python_unit_shard}" \
+      src/python:: tests/python:: -- "${PYTEST_PASSTHRU_ARGS[@]}"
   ) || die "Core python test failure"
   end_travis_section
 fi
@@ -221,8 +222,8 @@ if [[ "${run_contrib:-false}" == "true" ]]; then
   start_travis_section "ContribTests" "Running contrib python tests${shard_desc}"
   (
     ./pants.pex --exclude-target-regexp='.*/testprojects/.*' test.pytest \
-    --test-pytest-test-shard=${python_contrib_shard} \
-    contrib:: -- ${PYTEST_PASSTHRU_ARGS}
+    "--test-pytest-test-shard=${python_contrib_shard}" \
+    contrib:: -- "${PYTEST_PASSTHRU_ARGS[@]}"
   ) || die "Contrib python test failure"
   end_travis_section
 fi
@@ -269,7 +270,7 @@ if [[ "${test_platform_specific_behavior:-false}" == 'true' ]]; then
                        "Running platform-specific testing on platform: $(uname)"
   (
     ./pants.pex --tag='+platform_specific_behavior' test \
-                src/python/:: tests/python:: -- ${PYTEST_PASSTHRU_ARGS}
+                src/python/:: tests/python:: -- "${PYTEST_PASSTHRU_ARGS[@]}"
   ) || die "Pants platform-specific test failure"
   end_travis_section
 fi
@@ -282,8 +283,8 @@ if [[ "${run_integration:-false}" == "true" ]]; then
   start_travis_section "IntegrationTests" "Running Pants Integration tests${shard_desc}"
   (
     ./pants.pex --tag='+integration' test.pytest \
-      --test-pytest-test-shard=${python_intg_shard} \
-      src/python:: tests/python:: -- ${PYTEST_PASSTHRU_ARGS}
+      "--test-pytest-test-shard=${python_intg_shard}" \
+      src/python:: tests/python:: -- "${PYTEST_PASSTHRU_ARGS[@]}"
   ) || die "Pants Integration test failure"
   end_travis_section
 fi

build-support/bin/contributors.sh

@@ -3,6 +3,7 @@
 REPO_ROOT="$(git rev-parse --show-toplevel)"
 cd "$REPO_ROOT" || exit 1
 
+# shellcheck source=build-support/common.sh
 source "${REPO_ROOT}/build-support/common.sh"
 
 function usage() {

build-support/bin/download_binary.sh

@@ -2,10 +2,11 @@
 
 # This script wraps the main() method in binary_util.py.
 
-REPO_ROOT=$(cd $(dirname "${BASH_SOURCE[0]}") && cd ../.. && pwd -P)
+REPO_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")" && cd ../.. && pwd -P)
 BINARY_HELPER_SCRIPT="${REPO_ROOT}/src/python/pants/binaries/binary_util.py"
 
+# shellcheck source=build-support/pants_venv
 source "${REPO_ROOT}/build-support/pants_venv"
 
 activate_pants_venv 1>&2
-PYTHONPATH="${REPO_ROOT}/src/python:${PYTHONPATH}" python "$BINARY_HELPER_SCRIPT" $@
+PYTHONPATH="${REPO_ROOT}/src/python:${PYTHONPATH}" python "$BINARY_HELPER_SCRIPT" "$@"

build-support/bin/get_ci_bootstrapped_pants_pex.sh

@@ -12,7 +12,7 @@ BOOTSTRAPPED_PEX_URL=s3://${BOOTSTRAPPED_PEX_BUCKET}/${BOOTSTRAPPED_PEX_KEY}
 
 # First check that there's only one version of the object on S3, to detect malicious overwrites.
 NUM_VERSIONS=$(aws --no-sign-request --region us-east-1 s3api list-object-versions \
-  --bucket ${BOOTSTRAPPED_PEX_BUCKET} --prefix ${BOOTSTRAPPED_PEX_KEY} --max-items 2 \
+  --bucket "${BOOTSTRAPPED_PEX_BUCKET}" --prefix "${BOOTSTRAPPED_PEX_KEY}" --max-items 2 \
   | jq '.Versions | length')
 [ "${NUM_VERSIONS}" == "1" ] || (echo "Error: Found ${NUM_VERSIONS} versions for ${BOOTSTRAPPED_PEX_URL}" && exit 1)
 

build-support/bin/get_failing_travis_targets_for_pr.sh

@@ -11,12 +11,12 @@ mkdir -p logs/jobs
 curl=(curl -s -S -H 'Travis-API-Version: 3')
 
 "${curl[@]}" 'https://api.travis-ci.org/repo/pantsbuild%2Fpants/builds?event_type=pull_request&limit=100&sort_by=finished_at:desc' > logs/search
-jobs="$(cat logs/search | jq "[ .builds[] | select(.pull_request_number == ${pull}) ][0] | .jobs[].id")"
+jobs="$(jq "[ .builds[] | select(.pull_request_number == ${pull}) ][0] | .jobs[].id" logs/search)"
 targets=()
 for job in ${jobs}; do
   mkdir -p "logs/jobs/${job}"
   "${curl[@]}" "https://api.travis-ci.org/job/${job}" >"logs/jobs/${job}/info"
-  state="$(cat logs/jobs/${job}/info | jq -r '.state')"
+  state="$(jq -r '.state' "logs/jobs/${job}/info")"
   case "${state}" in
     "passed")
       continue

build-support/bin/install_aws_cli_for_ci.sh

@@ -21,7 +21,7 @@ if [[ ! -x "${AWS_CLI_BIN}" ]]; then
 
   TMPDIR=$(mktemp -d)
 
-  pushd ${TMPDIR}
+  pushd "${TMPDIR}"
 
   curl "https://s3.amazonaws.com/aws-cli/awscli-bundle.zip" -o "awscli-bundle.zip"
   unzip awscli-bundle.zip

build-support/bin/install_git_hooks.sh

@@ -9,7 +9,7 @@ REPO_ROOT="$(git rev-parse --show-toplevel)"
 
 HOOK_DIR="${GIT_DIR:-${REPO_ROOT}/.git}/hooks"
 HOOK_SRC_DIR="${REPO_ROOT}/build-support/githooks"
-HOOK_NAMES="$(ls ${HOOK_SRC_DIR})"
+HOOK_NAMES="$(ls "${HOOK_SRC_DIR}")"
 
 RELPATH_PREFIX="$(cat << EOF | python
 import os
@@ -24,8 +24,8 @@ function install_hook() {
   RELPATH="${RELPATH_PREFIX}/${HOOK}"
   (
     cd "${HOOK_DIR}" && \
-    rm -f ${HOOK} && \
-    ln -s "${RELPATH}" ${HOOK} && \
+    rm -f "${HOOK}" && \
+    ln -s "${RELPATH}" "${HOOK}" && \
     echo "${HOOK} hook linked to $(pwd)/${HOOK}";
   )
 }
@@ -38,16 +38,16 @@ function ensure_hook() {
 
   if [[ ! -e "${HOOK_DST}" ]]
   then
-    install_hook ${HOOK}
+    install_hook "${HOOK}"
   else
     if cmp --quiet "${HOOK_SRC}" "${HOOK_DST}"
     then
       echo "${HOOK} hook up to date."
     else
-      read -p "A ${HOOK} hook already exists, replace with ${HOOK_SRC}? [Yn]" ok
+      read -rp "A ${HOOK} hook already exists, replace with ${HOOK_SRC}? [Yn]" ok
       if [[ "${ok:-Y}" =~ ^[yY]([eE][sS])?$ ]]
       then
-        install_hook ${HOOK}
+        install_hook "${HOOK}"
       else
         echo "${HOOK} hook not installed"
         exit 1
@@ -58,5 +58,5 @@ function ensure_hook() {
 
 
 for HOOK in ${HOOK_NAMES}; do
-  ensure_hook ${HOOK}
+  ensure_hook "${HOOK}"
 done

build-support/bin/isort.sh

@@ -1,8 +1,9 @@
 #!/usr/bin/env bash
 
 REPO_ROOT="$(git rev-parse --show-toplevel)"
-cd ${REPO_ROOT}
+cd "${REPO_ROOT}" || exit 1
 
+# shellcheck source=build-support/common.sh
 source build-support/common.sh
 
 function usage() {
@@ -34,7 +35,7 @@ do
 done
 
 # If changes were made or issues found, output with leading whitespace trimmed.
-output="$(./pants --changed-parent="$(git_merge_base)" fmt.isort -- ${isort_args[@]})"
+output="$(./pants --changed-parent="$(git_merge_base)" fmt.isort -- "${isort_args[@]}")"
 echo "${output}" | grep -Eo '(ERROR).*$' && exit 1
 echo "${output}" | grep -Eo '(Fixing).*$'
 exit 0

build-support/bin/native/bootstrap_cffi.sh

@@ -4,6 +4,7 @@ set -e
 
 REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && cd ../../.. && pwd -P)"
 
+# shellcheck source=build-support/pants_venv
 source "${REPO_ROOT}/build-support/pants_venv"
 
 if (( $# != 2 )); then

build-support/bin/native/bootstrap_code.sh

@@ -5,7 +5,9 @@ REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && cd ../../.. && pwd -P)"
 # Exposes:
 # + die: Exit in a failure state and optionally log an error message to the console.
 # + fingerprint_data: Fingerprints the data on stdin.
-source ${REPO_ROOT}/build-support/common.sh
+
+# shellcheck source=build-support/common.sh
+source "${REPO_ROOT}/build-support/common.sh"
 
 KERNEL=$(uname -s | tr '[:upper:]' '[:lower:]')
 case "${KERNEL}" in
@@ -40,7 +42,7 @@ function calculate_current_hash() {
   #
   # Assumes we're in the venv that will be used to build the native engine.
   (
-   cd ${REPO_ROOT}
+   cd "${REPO_ROOT}" || exit 1
    (echo "${MODE_FLAG}"
     echo "${RUST_TOOLCHAIN}"
     uname
@@ -69,7 +71,8 @@ function _build_native_code() {
 
 function bootstrap_native_code() {
   # Bootstraps the native code only if needed.
-  local native_engine_version="$(calculate_current_hash)"
+  local native_engine_version
+  native_engine_version="$(calculate_current_hash)"
   local engine_version_hdr="engine_version: ${native_engine_version}"
   local target_binary="${NATIVE_ENGINE_CACHE_DIR}/${native_engine_version}/${NATIVE_ENGINE_BINARY}"
   local target_binary_metadata="${target_binary}.metadata"
@@ -90,7 +93,7 @@ function bootstrap_native_code() {
     target_binary="${NATIVE_ENGINE_CACHE_DIR}/${native_engine_version}/${NATIVE_ENGINE_BINARY}"
     target_binary_metadata="${target_binary}.metadata"
 
-    mkdir -p "$(dirname ${target_binary})"
+    mkdir -p "$(dirname "${target_binary}")"
     cp "${native_binary}" "${target_binary}"
 
     local -r metadata_file=$(mktemp -t pants.native_engine.metadata.XXXXXX)

build-support/bin/native/bootstrap_rust.sh

@@ -5,6 +5,8 @@ REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && cd ../../.. && pwd -P)"
 # Exposes:
 # + log: Log a message to the console.
 # + fingerprint_data: Fingerprints the data on stdin.
+
+# shellcheck source=build-support/common.sh
 source "${REPO_ROOT}/build-support/common.sh"
 
 rust_toolchain_root="${CACHE_ROOT}/rust"