Enable remaining optional Shellcheck rules (#1654)

edmorley · web-flow · commit 02f8ec0f5827 · 2024-10-03T14:44:05.000Z
In #1596 a number of optional Shellcheck rules were enabled. However, there were some which were deferred due to the number of fixes required, and so were left disabled via the `.shellcheckrc` file. In order that we can get the benefit of these rules for new code, I've removed the global disabling in favour of per file or per line `disable` directives (and in some cases, fixing outright). These can then be fixed piecemeal as refactorings occur later. Of note, one of these optional Shellcheck rules (SC2311) would have saved me a fair amount of debugging time earlier today in the new Python version handling implementation. GUS-W-16898648.
diff --git a/.shellcheckrc b/.shellcheckrc
@@ -1,15 +1,2 @@
 # Enable all checks, including the optional ones that are off by default.
 enable=all
-
-# TODO: Triage and potentially enable more of these optional checks.
-
-# SC2154 (warning): var is referenced but not assigned.
-disable=SC2154
-# SC2250 (style): Prefer putting braces around variable references even when not strictly required.
-disable=SC2250
-# SC2310 (info): This function is invoked in an 'if' condition so set -e will be disabled.
-disable=SC2310
-# SC2311 (info): Bash implicitly disabled set -e for this function invocation because it's inside a command substitution.
-disable=SC2311
-# SC2312 (info): Consider invoking this command separately to avoid masking its return value
-disable=SC2312
diff --git a/bin/compile b/bin/compile
@@ -1,6 +1,7 @@
 #!/usr/bin/env bash
 # Usage: bin/compile <build-dir> <cache-dir> <env-dir>
 # See: https://devcenter.heroku.com/articles/buildpack-api
+# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
 
 set -euo pipefail
 
@@ -131,6 +132,7 @@ fi
 if [[ -f "$CACHE_DIR/.heroku/python-stack" ]]; then
 	CACHED_PYTHON_STACK=$(cat "$CACHE_DIR/.heroku/python-stack")
 else
+	# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
 	CACHED_PYTHON_STACK=$STACK
 fi
 
@@ -170,6 +172,7 @@ mkdir -p /app/.heroku/src
 # symlinks to emulate that we are operating in `/app` during the build process.
 # This is (hopefully obviously) because apps end up running from `/app` in production.
 # Realpath is used to support use-cases where one of the locations is a symlink to the other.
+# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
 if [[ "$(realpath "${BUILD_DIR}")" != "$(realpath /app)" ]]; then
 	# python expects to reside in /app, so set up symlinks
 	# we will not remove these later so subsequent buildpacks can still invoke it
@@ -231,6 +234,7 @@ meta_time "nltk_downloader_duration" "${nltk_downloader_start_time}"
 # Support for editable installations.
 # In CI, $BUILD_DIR is /app.
 # Realpath is used to support use-cases where one of the locations is a symlink to the other.
+# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
 if [[ "$(realpath "${BUILD_DIR}")" != "$(realpath /app)" ]]; then
 	rm -rf "$BUILD_DIR/.heroku/src"
 	deep-cp /app/.heroku/src "$BUILD_DIR/.heroku/src"
diff --git a/bin/detect b/bin/detect
@@ -54,7 +54,7 @@ source code.
 
 Currently the root directory of your app contains:
 
-$(ls -1 --indicator-style=slash "${BUILD_DIR}")
+$(ls -1 --indicator-style=slash "${BUILD_DIR}" || true)
 
 If your app already has a package manager file, check that it:
 
diff --git a/bin/release b/bin/release
@@ -17,6 +17,7 @@ source "${BUILDPACK_DIR}/export"
 
 source "${BUILDPACK_DIR}/bin/utils"
 
+# shellcheck disable=SC2310 # TODO: This function is invoked in an '&&' condition so set -e will be disabled.
 if [[ -f "${BUILD_DIR}/manage.py" ]] && is_module_available 'django' && is_module_available 'psycopg2'; then
 	cat <<-'EOF'
 		---
diff --git a/bin/report b/bin/report
@@ -91,10 +91,12 @@ ALL_OTHER_FIELDS=(
 )
 
 for field in "${STRING_FIELDS[@]}"; do
+	# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
 	kv_pair_string "${field}" "$(meta_get "${field}")"
 done
 
 for field in "${ALL_OTHER_FIELDS[@]}"; do
+	# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
 	kv_pair "${field}" "$(meta_get "${field}")"
 done
 
diff --git a/bin/steps/collectstatic b/bin/steps/collectstatic
@@ -1,4 +1,5 @@
 #!/usr/bin/env bash
+# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
 
 # Django Collectstatic runner. If you have Django installed, collectstatic will
 # automatically be executed as part of the build process. If collectstatic
@@ -18,8 +19,7 @@ source "${BUILDPACK_DIR}/bin/utils"
 
 # Required for `meta_set`.
 source "${BUILDPACK_DIR}/lib/metadata.sh"
-# shellcheck disable=SC2153
-meta_init "${CACHE_DIR}" "python"
+meta_init "${CACHE_DIR:?}" "python"
 
 if [[ -f .heroku/collectstatic_disabled ]]; then
 	puts-step "Skipping Django collectstatic since the file '.heroku/collectstatic_disabled' exists."
@@ -35,6 +35,7 @@ if [[ "${DISABLE_COLLECTSTATIC:-0}" != "0" ]]; then
 fi
 
 # Ensure that Django is actually installed.
+# shellcheck disable=SC2310 # TODO: This function is invoked in an 'if' condition so set -e will be disabled.
 if ! is_module_available 'django'; then
 	exit 0
 fi
diff --git a/bin/steps/nltk b/bin/steps/nltk
@@ -1,4 +1,5 @@
 #!/usr/bin/env bash
+# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
 
 # This script is run in a subshell via sub_env so doesn't inherit the options/vars/utils from `bin/compile`.
 # TODO: Stop running this script in a subshell.
@@ -8,14 +9,14 @@ source "${BUILDPACK_DIR}/bin/utils"
 
 # Required for `meta_set`.
 source "${BUILDPACK_DIR}/lib/metadata.sh"
-# shellcheck disable=SC2153
-meta_init "${CACHE_DIR}" "python"
+meta_init "${CACHE_DIR:?}" "python"
 
 # These are required by `set_env`.
-PROFILE_PATH="${BUILD_DIR}/.profile.d/python.sh"
+PROFILE_PATH="${BUILD_DIR:?}/.profile.d/python.sh"
 EXPORT_PATH="${BUILDPACK_DIR}/export"
 
 # Check that nltk was installed by pip, otherwise obviously not needed
+# shellcheck disable=SC2310 # TODO: This function is invoked in an 'if' condition so set -e will be disabled.
 if is_module_available 'nltk'; then
 	puts-step "Downloading NLTK corpora..."
 
diff --git a/bin/steps/pipenv-python-version b/bin/steps/pipenv-python-version
@@ -1,4 +1,6 @@
 #!/usr/bin/env bash
+# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
+# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
 
 # TODO: Move this to lib/ as part of the refactoring for .python-version support.
 
diff --git a/bin/steps/python b/bin/steps/python
@@ -1,4 +1,8 @@
 #!/usr/bin/env bash
+# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
+# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
+
+set -euo pipefail
 
 PYTHON_VERSION=$(cat runtime.txt)
 # Remove leading and trailing whitespace. Note: This implementation relies upon
@@ -113,6 +117,7 @@ if [[ "$STACK" != "$CACHED_PYTHON_STACK" ]]; then
 fi
 
 if [[ -f .heroku/python-version ]]; then
+	# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
 	if [[ ! "$(cat .heroku/python-version)" == "$PYTHON_VERSION" ]]; then
 		puts-step "Python version has changed from $(cat .heroku/python-version) to ${PYTHON_VERSION}, clearing cache"
 		rm -rf .heroku/python
diff --git a/bin/steps/sqlite3 b/bin/steps/sqlite3
@@ -1,4 +1,5 @@
 #!/usr/bin/env bash
+# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
 
 # TODO: Remove this entirely since the Python stdlib now includes modern sqlite support,
 # and the APT buildpack should be used if an app needs the sqlite CLI/headers.
@@ -68,6 +69,8 @@ buildpack_sqlite3_install() {
 	# the conditional disables `set -e` inside the called function:
 	# https://stackoverflow.com/q/19789102
 	# ...plus whoever wrote this forgot the `exit 1` in the `else` anyway.
+	# shellcheck disable=SC2310 # TODO: This function is invoked in an 'if' condition so set -e will be disabled.
+	# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
 	if sqlite3_install "$BUILD_DIR/.heroku/python"; then
 		# mcount "success.python.sqlite3"
 		:
@@ -76,5 +79,6 @@ buildpack_sqlite3_install() {
 		# mcount "failure.python.sqlite3"
 	fi
 
+	# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
 	mkdir -p "$CACHE_DIR/.heroku/"
 }
diff --git a/bin/utils b/bin/utils
@@ -1,17 +1,14 @@
 #!/usr/bin/env bash
+# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
 
 # Be careful about moving these to bin/compile, since this utils script is sourced
 # directly by other scripts run via subshells, and not only from bin/compile.
 shopt -s extglob
 shopt -s nullglob
 
-source "${BUILDPACK_DIR}/vendor/buildpack-stdlib_v8.sh"
+source "${BUILDPACK_DIR:?}/vendor/buildpack-stdlib_v8.sh"
 
-if [[ "$(uname)" == "Darwin" ]]; then
-	sed() { command sed -l "$@"; }
-else
-	sed() { command sed -u "$@"; }
-fi
+sed() { command sed -u "$@"; }
 
 # Syntax sugar.
 indent() {
diff --git a/bin/warnings b/bin/warnings
@@ -1,7 +1,8 @@
 #!/usr/bin/env bash
 
 gdal-missing() {
-	if grep -qi 'Could not find gdal-config' "$WARNINGS_LOG"; then
+	# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
+	if grep -qi 'Could not find gdal-config' "${WARNINGS_LOG}"; then
 		echo
 		puts-warn "Hello! Package installation failed since the GDAL library was not found."
 		puts-warn "For GDAL, GEOS and PROJ support, use the Geo buildpack alongside the Python buildpack:"
diff --git a/builds/build_python_runtime.sh b/builds/build_python_runtime.sh
@@ -19,7 +19,7 @@ function abort() {
 	exit 1
 }
 
-case "${STACK}" in
+case "${STACK:?}" in
 	heroku-24)
 		SUPPORTED_PYTHON_VERSIONS=(
 			"3.10"
@@ -170,7 +170,8 @@ else
 	LDFLAGS="$(dpkg-buildflags --get LDFLAGS) -Wl,--strip-all"
 fi
 
-make -j "$(nproc)" "EXTRA_CFLAGS=${EXTRA_CFLAGS}" "LDFLAGS=${LDFLAGS}"
+CPU_COUNT="$(nproc)"
+make -j "${CPU_COUNT}" "EXTRA_CFLAGS=${EXTRA_CFLAGS}" "LDFLAGS=${LDFLAGS}"
 make install
 
 if [[ "${PYTHON_MAJOR_VERSION}" == 3.[8-9] ]]; then
diff --git a/etc/publish.sh b/etc/publish.sh
@@ -1,4 +1,5 @@
 #!/usr/bin/env bash
+# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
 
 set -euo pipefail
 
diff --git a/lib/kvstore.sh b/lib/kvstore.sh
@@ -1,5 +1,9 @@
 #!/usr/bin/env bash
 
+# This is technically redundant, since all consumers of this lib will have enabled these,
+# however, it helps Shellcheck realise the options under which these functions will run.
+set -euo pipefail
+
 # TODO: Switch this file to using namespaced functions like `kvstore::<fn_name>`.
 
 # Taken from: https://github.com/heroku/heroku-buildpack-nodejs/blob/main/lib/kvstore.sh
@@ -71,6 +75,7 @@ kv_list() {
 
 	kv_keys "${f}" | tr ' ' '\n' | while read -r key; do
 		if [[ -n "${key}" ]]; then
+			# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
 			echo "${key}=$(kv_get_escaped "${f}" "${key}")"
 		fi
 	done
diff --git a/lib/metadata.sh b/lib/metadata.sh
@@ -1,10 +1,14 @@
 #!/usr/bin/env bash
 
+# This is technically redundant, since all consumers of this lib will have enabled these,
+# however, it helps Shellcheck realise the options under which these functions will run.
+set -euo pipefail
+
 # TODO: Switch this file to using namespaced functions like `metadata::<fn_name>`.
 
 # Based on: https://github.com/heroku/heroku-buildpack-nodejs/blob/main/lib/metadata.sh
 
-source "${BUILDPACK_DIR}/lib/kvstore.sh"
+source "${BUILDPACK_DIR:?}/lib/kvstore.sh"
 
 # Variables shared by this whole module
 BUILD_DATA_FILE=""
@@ -69,6 +73,6 @@ log_meta_data() {
 	# print all values on one line in logfmt format
 	# https://brandur.org/logfmt
 	# the echo call ensures that all values are printed on a single line
-	# shellcheck disable=SC2005 disable=SC2046
+	# shellcheck disable=SC2005,SC2046,SC2312
 	echo $(kv_list "${BUILD_DATA_FILE}")
 }
diff --git a/lib/output.sh b/lib/output.sh
@@ -1,5 +1,9 @@
 #!/usr/bin/env bash
 
+# This is technically redundant, since all consumers of this lib will have enabled these,
+# however, it helps Shellcheck realise the options under which these functions will run.
+set -euo pipefail
+
 # TODO: Switch this file to using namespaced functions like `output::<fn_name>`.
 
 ANSI_RED='\033[1;31m'
diff --git a/lib/package_manager.sh b/lib/package_manager.sh
@@ -1,5 +1,9 @@
 #!/usr/bin/env bash
 
+# This is technically redundant, since all consumers of this lib will have enabled these,
+# however, it helps Shellcheck realise the options under which these functions will run.
+set -euo pipefail
+
 function package_manager::determine_package_manager() {
 	local build_dir="${1}"
 	local package_managers_found=()
diff --git a/lib/pip.sh b/lib/pip.sh
@@ -1,5 +1,9 @@
 #!/usr/bin/env bash
 
+# This is technically redundant, since all consumers of this lib will have enabled these,
+# however, it helps Shellcheck realise the options under which these functions will run.
+set -euo pipefail
+
 function pip::install_pip_setuptools_wheel() {
 	# We use the pip wheel bundled within Python's standard library to install our chosen
 	# pip version, since it's faster than `ensurepip` followed by an upgrade in place.
@@ -27,6 +31,7 @@ function pip::install_dependencies() {
 	# to allow for the env var interpolation feature of requirements files to work.
 	#
 	# PIP_EXTRA_INDEX_URL allows for an alternate pypi URL to be used.
+	# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
 	if [[ -r "${ENV_DIR}/PIP_EXTRA_INDEX_URL" ]]; then
 		PIP_EXTRA_INDEX_URL="$(cat "${ENV_DIR}/PIP_EXTRA_INDEX_URL")"
 		export PIP_EXTRA_INDEX_URL
@@ -40,7 +45,8 @@ function pip::install_dependencies() {
 	fi
 
 	set +e
-	/app/.heroku/python/bin/pip install "${args[@]}" --exists-action=w --src='/app/.heroku/src' --disable-pip-version-check --no-cache-dir --progress-bar off 2>&1 | tee "$WARNINGS_LOG" | cleanup | indent
+	# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
+	/app/.heroku/python/bin/pip install "${args[@]}" --exists-action=w --src='/app/.heroku/src' --disable-pip-version-check --no-cache-dir --progress-bar off 2>&1 | tee "${WARNINGS_LOG}" | cleanup | indent
 	local PIP_STATUS="${PIPESTATUS[0]}"
 	set -e
 
diff --git a/lib/pipenv.sh b/lib/pipenv.sh
@@ -1,5 +1,9 @@
 #!/usr/bin/env bash
 
+# This is technically redundant, since all consumers of this lib will have enabled these,
+# however, it helps Shellcheck realise the options under which these functions will run.
+set -euo pipefail
+
 # export CLINT_FORCE_COLOR=1
 # export PIPENV_FORCE_COLOR=1
 
@@ -25,6 +29,7 @@ function pipenv::install_dependencies() {
 	# TODO: Expose all config vars (after suitable checks are added for unsafe env vars).
 	#
 	# PIP_EXTRA_INDEX_URL allows for an alternate pypi URL to be used.
+	# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
 	if [[ -r "${ENV_DIR}/PIP_EXTRA_INDEX_URL" ]]; then
 		PIP_EXTRA_INDEX_URL="$(cat "${ENV_DIR}/PIP_EXTRA_INDEX_URL")"
 		export PIP_EXTRA_INDEX_URL
diff --git a/lib/utils.sh b/lib/utils.sh
@@ -1,5 +1,9 @@
 #!/usr/bin/env bash
 
+# This is technically redundant, since all consumers of this lib will have enabled these,
+# however, it helps Shellcheck realise the options under which these functions will run.
+set -euo pipefail
+
 # Python bundles pip within its standard library, which we can use to install our chosen
 # pip version from PyPI, saving us from having to download the usual pip bootstrap script.
 function utils::bundled_pip_module_path() {
diff --git a/vendor/buildpack-stdlib_v8.sh b/vendor/buildpack-stdlib_v8.sh
@@ -1,4 +1,8 @@
 #!/usr/bin/env bash
+# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
+# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
+
+set -euo pipefail
 
 # Based on:
 # https://raw.githubusercontent.com/heroku/buildpack-stdlib/v8/stdlib.sh
@@ -64,7 +68,6 @@ export_env() {
 sub_env() {
 	(
 		# TODO: Fix https://github.com/heroku/buildpack-stdlib/issues/37
-		# shellcheck disable=SC2153
 		export_env "$ENV_DIR" "${WHITELIST:-}" "${BLACKLIST:-}"
 
 		"$@"
