Skip to content
Permalink
Browse files

Add `shellcheck.py` and apply first round of fixes (pantsbuild#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.
  • Loading branch information...
Eric-Arellano committed May 14, 2019
1 parent 27c7e6a commit 948bc8bc12b63e03dd7689d480bd7249d8287571
@@ -33,3 +33,12 @@ python_binary(
],
compatibility = ['CPython>=3.6'],
)

python_binary(
name = 'shellcheck',
sources = 'shellcheck.py',
dependencies = [
':common',
],
compatibility = ['CPython>=3.6'],
)
@@ -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
@@ -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

@@ -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
@@ -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
@@ -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."
@@ -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
@@ -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() {
@@ -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" "$@"
@@ -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)

@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)
@@ -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"

0 comments on commit 948bc8b

Please sign in to comment.
You can’t perform that action at this time.