Add shellcheck.py and apply first round of fixes

build-support/bin/BUILD

@@ -20,3 +20,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"