Skip to content

Commit 02f8ec0

Browse files
edmorleyedmorley
authored
Enable remaining optional Shellcheck rules (heroku#1654)
In heroku#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.
1 parent 2b04d80 commit 02f8ec0

22 files changed

+74
-32
lines changed

‎.shellcheckrc

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,2 @@
11
# Enable all checks, including the optional ones that are off by default.
22
enable=all
3-
4-
# TODO: Triage and potentially enable more of these optional checks.
5-
6-
# SC2154 (warning): var is referenced but not assigned.
7-
disable=SC2154
8-
# SC2250 (style): Prefer putting braces around variable references even when not strictly required.
9-
disable=SC2250
10-
# SC2310 (info): This function is invoked in an 'if' condition so set -e will be disabled.
11-
disable=SC2310
12-
# SC2311 (info): Bash implicitly disabled set -e for this function invocation because it's inside a command substitution.
13-
disable=SC2311
14-
# SC2312 (info): Consider invoking this command separately to avoid masking its return value
15-
disable=SC2312

‎bin/compile

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
#!/usr/bin/env bash
22
# Usage: bin/compile <build-dir> <cache-dir> <env-dir>
33
# See: https://devcenter.heroku.com/articles/buildpack-api
4+
# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
45

56
set -euo pipefail
67

@@ -131,6 +132,7 @@ fi
131132
if [[ -f "$CACHE_DIR/.heroku/python-stack" ]]; then
132133
CACHED_PYTHON_STACK=$(cat "$CACHE_DIR/.heroku/python-stack")
133134
else
135+
# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
134136
CACHED_PYTHON_STACK=$STACK
135137
fi
136138

@@ -170,6 +172,7 @@ mkdir -p /app/.heroku/src
170172
# symlinks to emulate that we are operating in `/app` during the build process.
171173
# This is (hopefully obviously) because apps end up running from `/app` in production.
172174
# Realpath is used to support use-cases where one of the locations is a symlink to the other.
175+
# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
173176
if [[ "$(realpath "${BUILD_DIR}")" != "$(realpath /app)" ]]; then
174177
# python expects to reside in /app, so set up symlinks
175178
# 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}"
231234
# Support for editable installations.
232235
# In CI, $BUILD_DIR is /app.
233236
# Realpath is used to support use-cases where one of the locations is a symlink to the other.
237+
# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
234238
if [[ "$(realpath "${BUILD_DIR}")" != "$(realpath /app)" ]]; then
235239
rm -rf "$BUILD_DIR/.heroku/src"
236240
deep-cp /app/.heroku/src "$BUILD_DIR/.heroku/src"

‎bin/detect

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ source code.
5454
5555
Currently the root directory of your app contains:
5656
57-
$(ls -1 --indicator-style=slash "${BUILD_DIR}")
57+
$(ls -1 --indicator-style=slash "${BUILD_DIR}" || true)
5858
5959
If your app already has a package manager file, check that it:
6060

‎bin/release

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ source "${BUILDPACK_DIR}/export"
1717

1818
source "${BUILDPACK_DIR}/bin/utils"
1919

20+
# shellcheck disable=SC2310 # TODO: This function is invoked in an '&&' condition so set -e will be disabled.
2021
if [[ -f "${BUILD_DIR}/manage.py" ]] && is_module_available 'django' && is_module_available 'psycopg2'; then
2122
cat <<-'EOF'
2223
---

‎bin/report

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,12 @@ ALL_OTHER_FIELDS=(
9191
)
9292

9393
for field in "${STRING_FIELDS[@]}"; do
94+
# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
9495
kv_pair_string "${field}" "$(meta_get "${field}")"
9596
done
9697

9798
for field in "${ALL_OTHER_FIELDS[@]}"; do
99+
# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
98100
kv_pair "${field}" "$(meta_get "${field}")"
99101
done
100102

‎bin/steps/collectstatic

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#!/usr/bin/env bash
2+
# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
23

34
# Django Collectstatic runner. If you have Django installed, collectstatic will
45
# automatically be executed as part of the build process. If collectstatic
@@ -18,8 +19,7 @@ source "${BUILDPACK_DIR}/bin/utils"
1819

1920
# Required for `meta_set`.
2021
source "${BUILDPACK_DIR}/lib/metadata.sh"
21-
# shellcheck disable=SC2153
22-
meta_init "${CACHE_DIR}" "python"
22+
meta_init "${CACHE_DIR:?}" "python"
2323

2424
if [[ -f .heroku/collectstatic_disabled ]]; then
2525
puts-step "Skipping Django collectstatic since the file '.heroku/collectstatic_disabled' exists."
@@ -35,6 +35,7 @@ if [[ "${DISABLE_COLLECTSTATIC:-0}" != "0" ]]; then
3535
fi
3636

3737
# Ensure that Django is actually installed.
38+
# shellcheck disable=SC2310 # TODO: This function is invoked in an 'if' condition so set -e will be disabled.
3839
if ! is_module_available 'django'; then
3940
exit 0
4041
fi

‎bin/steps/nltk

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#!/usr/bin/env bash
2+
# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
23

34
# This script is run in a subshell via sub_env so doesn't inherit the options/vars/utils from `bin/compile`.
45
# TODO: Stop running this script in a subshell.
@@ -8,14 +9,14 @@ source "${BUILDPACK_DIR}/bin/utils"
89

910
# Required for `meta_set`.
1011
source "${BUILDPACK_DIR}/lib/metadata.sh"
11-
# shellcheck disable=SC2153
12-
meta_init "${CACHE_DIR}" "python"
12+
meta_init "${CACHE_DIR:?}" "python"
1313

1414
# These are required by `set_env`.
15-
PROFILE_PATH="${BUILD_DIR}/.profile.d/python.sh"
15+
PROFILE_PATH="${BUILD_DIR:?}/.profile.d/python.sh"
1616
EXPORT_PATH="${BUILDPACK_DIR}/export"
1717

1818
# Check that nltk was installed by pip, otherwise obviously not needed
19+
# shellcheck disable=SC2310 # TODO: This function is invoked in an 'if' condition so set -e will be disabled.
1920
if is_module_available 'nltk'; then
2021
puts-step "Downloading NLTK corpora..."
2122

‎bin/steps/pipenv-python-version

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
#!/usr/bin/env bash
2+
# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
3+
# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
24

35
# TODO: Move this to lib/ as part of the refactoring for .python-version support.
46

‎bin/steps/python

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
11
#!/usr/bin/env bash
2+
# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
3+
# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
4+
5+
set -euo pipefail
26

37
PYTHON_VERSION=$(cat runtime.txt)
48
# Remove leading and trailing whitespace. Note: This implementation relies upon
@@ -113,6 +117,7 @@ if [[ "$STACK" != "$CACHED_PYTHON_STACK" ]]; then
113117
fi
114118

115119
if [[ -f .heroku/python-version ]]; then
120+
# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
116121
if [[ ! "$(cat .heroku/python-version)" == "$PYTHON_VERSION" ]]; then
117122
puts-step "Python version has changed from $(cat .heroku/python-version) to ${PYTHON_VERSION}, clearing cache"
118123
rm -rf .heroku/python

‎bin/steps/sqlite3

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#!/usr/bin/env bash
2+
# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
23

34
# TODO: Remove this entirely since the Python stdlib now includes modern sqlite support,
45
# and the APT buildpack should be used if an app needs the sqlite CLI/headers.
@@ -68,6 +69,8 @@ buildpack_sqlite3_install() {
6869
# the conditional disables `set -e` inside the called function:
6970
# https://stackoverflow.com/q/19789102
7071
# ...plus whoever wrote this forgot the `exit 1` in the `else` anyway.
72+
# shellcheck disable=SC2310 # TODO: This function is invoked in an 'if' condition so set -e will be disabled.
73+
# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
7174
if sqlite3_install "$BUILD_DIR/.heroku/python"; then
7275
# mcount "success.python.sqlite3"
7376
:
@@ -76,5 +79,6 @@ buildpack_sqlite3_install() {
7679
# mcount "failure.python.sqlite3"
7780
fi
7881

82+
# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
7983
mkdir -p "$CACHE_DIR/.heroku/"
8084
}

‎bin/utils

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,14 @@
11
#!/usr/bin/env bash
2+
# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
23

34
# Be careful about moving these to bin/compile, since this utils script is sourced
45
# directly by other scripts run via subshells, and not only from bin/compile.
56
shopt -s extglob
67
shopt -s nullglob
78

8-
source "${BUILDPACK_DIR}/vendor/buildpack-stdlib_v8.sh"
9+
source "${BUILDPACK_DIR:?}/vendor/buildpack-stdlib_v8.sh"
910

10-
if [[ "$(uname)" == "Darwin" ]]; then
11-
sed() { command sed -l "$@"; }
12-
else
13-
sed() { command sed -u "$@"; }
14-
fi
11+
sed() { command sed -u "$@"; }
1512

1613
# Syntax sugar.
1714
indent() {

‎bin/warnings

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
#!/usr/bin/env bash
22

33
gdal-missing() {
4-
if grep -qi 'Could not find gdal-config' "$WARNINGS_LOG"; then
4+
# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
5+
if grep -qi 'Could not find gdal-config' "${WARNINGS_LOG}"; then
56
echo
67
puts-warn "Hello! Package installation failed since the GDAL library was not found."
78
puts-warn "For GDAL, GEOS and PROJ support, use the Geo buildpack alongside the Python buildpack:"

‎builds/build_python_runtime.sh

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ function abort() {
1919
exit 1
2020
}
2121

22-
case "${STACK}" in
22+
case "${STACK:?}" in
2323
heroku-24)
2424
SUPPORTED_PYTHON_VERSIONS=(
2525
"3.10"
@@ -170,7 +170,8 @@ else
170170
LDFLAGS="$(dpkg-buildflags --get LDFLAGS) -Wl,--strip-all"
171171
fi
172172

173-
make -j "$(nproc)" "EXTRA_CFLAGS=${EXTRA_CFLAGS}" "LDFLAGS=${LDFLAGS}"
173+
CPU_COUNT="$(nproc)"
174+
make -j "${CPU_COUNT}" "EXTRA_CFLAGS=${EXTRA_CFLAGS}" "LDFLAGS=${LDFLAGS}"
174175
make install
175176

176177
if [[ "${PYTHON_MAJOR_VERSION}" == 3.[8-9] ]]; then

‎etc/publish.sh

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#!/usr/bin/env bash
2+
# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
23

34
set -euo pipefail
45

‎lib/kvstore.sh

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#!/usr/bin/env bash
22

3+
# This is technically redundant, since all consumers of this lib will have enabled these,
4+
# however, it helps Shellcheck realise the options under which these functions will run.
5+
set -euo pipefail
6+
37
# TODO: Switch this file to using namespaced functions like `kvstore::<fn_name>`.
48

59
# Taken from: https://github.com/heroku/heroku-buildpack-nodejs/blob/main/lib/kvstore.sh
@@ -71,6 +75,7 @@ kv_list() {
7175

7276
kv_keys "${f}" | tr ' ' '\n' | while read -r key; do
7377
if [[ -n "${key}" ]]; then
78+
# shellcheck disable=SC2312 # TODO: Invoke this command separately to avoid masking its return value.
7479
echo "${key}=$(kv_get_escaped "${f}" "${key}")"
7580
fi
7681
done

‎lib/metadata.sh

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
#!/usr/bin/env bash
22

3+
# This is technically redundant, since all consumers of this lib will have enabled these,
4+
# however, it helps Shellcheck realise the options under which these functions will run.
5+
set -euo pipefail
6+
37
# TODO: Switch this file to using namespaced functions like `metadata::<fn_name>`.
48

59
# Based on: https://github.com/heroku/heroku-buildpack-nodejs/blob/main/lib/metadata.sh
610

7-
source "${BUILDPACK_DIR}/lib/kvstore.sh"
11+
source "${BUILDPACK_DIR:?}/lib/kvstore.sh"
812

913
# Variables shared by this whole module
1014
BUILD_DATA_FILE=""
@@ -69,6 +73,6 @@ log_meta_data() {
6973
# print all values on one line in logfmt format
7074
# https://brandur.org/logfmt
7175
# the echo call ensures that all values are printed on a single line
72-
# shellcheck disable=SC2005 disable=SC2046
76+
# shellcheck disable=SC2005,SC2046,SC2312
7377
echo $(kv_list "${BUILD_DATA_FILE}")
7478
}

‎lib/output.sh

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#!/usr/bin/env bash
22

3+
# This is technically redundant, since all consumers of this lib will have enabled these,
4+
# however, it helps Shellcheck realise the options under which these functions will run.
5+
set -euo pipefail
6+
37
# TODO: Switch this file to using namespaced functions like `output::<fn_name>`.
48

59
ANSI_RED='\033[1;31m'

‎lib/package_manager.sh

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#!/usr/bin/env bash
22

3+
# This is technically redundant, since all consumers of this lib will have enabled these,
4+
# however, it helps Shellcheck realise the options under which these functions will run.
5+
set -euo pipefail
6+
37
function package_manager::determine_package_manager() {
48
local build_dir="${1}"
59
local package_managers_found=()

‎lib/pip.sh

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#!/usr/bin/env bash
22

3+
# This is technically redundant, since all consumers of this lib will have enabled these,
4+
# however, it helps Shellcheck realise the options under which these functions will run.
5+
set -euo pipefail
6+
37
function pip::install_pip_setuptools_wheel() {
48
# We use the pip wheel bundled within Python's standard library to install our chosen
59
# pip version, since it's faster than `ensurepip` followed by an upgrade in place.
@@ -27,6 +31,7 @@ function pip::install_dependencies() {
2731
# to allow for the env var interpolation feature of requirements files to work.
2832
#
2933
# PIP_EXTRA_INDEX_URL allows for an alternate pypi URL to be used.
34+
# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
3035
if [[ -r "${ENV_DIR}/PIP_EXTRA_INDEX_URL" ]]; then
3136
PIP_EXTRA_INDEX_URL="$(cat "${ENV_DIR}/PIP_EXTRA_INDEX_URL")"
3237
export PIP_EXTRA_INDEX_URL
@@ -40,7 +45,8 @@ function pip::install_dependencies() {
4045
fi
4146

4247
set +e
43-
/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
48+
# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
49+
/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
4450
local PIP_STATUS="${PIPESTATUS[0]}"
4551
set -e
4652

‎lib/pipenv.sh

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#!/usr/bin/env bash
22

3+
# This is technically redundant, since all consumers of this lib will have enabled these,
4+
# however, it helps Shellcheck realise the options under which these functions will run.
5+
set -euo pipefail
6+
37
# export CLINT_FORCE_COLOR=1
48
# export PIPENV_FORCE_COLOR=1
59

@@ -25,6 +29,7 @@ function pipenv::install_dependencies() {
2529
# TODO: Expose all config vars (after suitable checks are added for unsafe env vars).
2630
#
2731
# PIP_EXTRA_INDEX_URL allows for an alternate pypi URL to be used.
32+
# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
2833
if [[ -r "${ENV_DIR}/PIP_EXTRA_INDEX_URL" ]]; then
2934
PIP_EXTRA_INDEX_URL="$(cat "${ENV_DIR}/PIP_EXTRA_INDEX_URL")"
3035
export PIP_EXTRA_INDEX_URL

‎lib/utils.sh

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#!/usr/bin/env bash
22

3+
# This is technically redundant, since all consumers of this lib will have enabled these,
4+
# however, it helps Shellcheck realise the options under which these functions will run.
5+
set -euo pipefail
6+
37
# Python bundles pip within its standard library, which we can use to install our chosen
48
# pip version from PyPI, saving us from having to download the usual pip bootstrap script.
59
function utils::bundled_pip_module_path() {

‎vendor/buildpack-stdlib_v8.sh

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
11
#!/usr/bin/env bash
2+
# shellcheck disable=SC2154 # TODO: Env var is referenced but not assigned.
3+
# shellcheck disable=SC2250 # TODO: Use braces around variable references even when not strictly required.
4+
5+
set -euo pipefail
26

37
# Based on:
48
# https://raw.githubusercontent.com/heroku/buildpack-stdlib/v8/stdlib.sh
@@ -64,7 +68,6 @@ export_env() {
6468
sub_env() {
6569
(
6670
# TODO: Fix https://github.com/heroku/buildpack-stdlib/issues/37
67-
# shellcheck disable=SC2153
6871
export_env "$ENV_DIR" "${WHITELIST:-}" "${BLACKLIST:-}"
6972

7073
"$@"

0 commit comments

Comments
 (0)