Improve shellcheck coverage (#1596)

* Switches to using `git ls-files` instead of a hardcoded list of scripts to
  lint (shellcheck doesn't support recursively scanning for scripts itself).
  This means a few files that were not previously linted now are, so need
  fixes.
* Adds a `.shellcheckrc` which is used to enable optional shellcheck
  checks via `enable=all`. Most of these checks either already passed
  or have been fixed in this PR - however, a few others need a closer
  look so have been disabled for now to reduce the size of this PR.
* As part of doing this I discovered an NLTK bug with setting `NLTK_DATA`
  after #1595, which has now been fixed. Since that change was not yet
  released, I've not mentioned the fix here in the changelog.

.gitignore

@@ -1,5 +1,8 @@
 __pycache__/
 .hatchet/repos/
 .venv/
+# The setup-ruby GitHub Action creates this directory when caching is enabled, so we ignore
+# it here so it does not show up in the output of `git ls-files` for `make lint-scripts`.
+vendor/bundle/
 .DS_Store
 .rspec_status

.shellcheckrc

@@ -0,0 +1,15 @@
+# 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

Makefile

@@ -9,15 +9,8 @@ STACK_IMAGE_TAG := heroku/$(subst -,:,$(STACK))-build
 
 lint: lint-scripts lint-ruby
 
-# TODO: Enable scanning for files that are currently missed and/or restructure repo
-# layout to make it more viable to use wildcards here, given:
-# https://github.com/koalaman/shellcheck/issues/962
 lint-scripts:
-	@shellcheck -x bin/compile bin/detect bin/release bin/test-compile bin/utils bin/warnings bin/default_pythons
-	@shellcheck -x bin/steps/collectstatic bin/steps/nltk bin/steps/pip-install bin/steps/pipenv bin/steps/pipenv-python-version bin/steps/python
-	@shellcheck -x bin/steps/hooks/*
-	@shellcheck -x vendor/WEB_CONCURRENCY.sh
-	@shellcheck -x builds/*.sh
+	@git ls-files -z --cached --others --exclude-standard 'bin/*' '*/bin/*' '*.sh' | xargs -0 shellcheck --check-sourced --color=always
 
 lint-ruby:
 	@bundle exec rubocop

bin/compile

@@ -8,7 +8,7 @@ set -eo pipefail
 export BPLOG_PREFIX="buildpack.python"
 export BUILDPACK_LOG_FILE=${BUILDPACK_LOG_FILE:-/dev/null}
 
-[ "$BUILDPACK_XTRACE" ] && set -o xtrace
+[[ -n "$BUILDPACK_XTRACE" ]] && set -o xtrace
 
 BUILD_DIR="${1}"
 CACHE_DIR="${2}"
@@ -59,7 +59,7 @@ source "${BUILDPACK_DIR}/bin/warnings"
 mkdir -p /app/.heroku
 
 PROFILE_PATH="$BUILD_DIR/.profile.d/python.sh"
-EXPORT_PATH="${BUILDPACK_DIR}/bin/../export"
+EXPORT_PATH="${BUILDPACK_DIR}/export"
 GUNICORN_PROFILE_PATH="$BUILD_DIR/.profile.d/python.gunicorn.sh"
 WEB_CONCURRENCY_PROFILE_PATH="$BUILD_DIR/.profile.d/WEB_CONCURRENCY.sh"
 
@@ -120,12 +120,12 @@ source "${BUILDPACK_DIR}/bin/steps/hooks/pre_compile"
 
 # Sticky runtimes. If there was a previous build, and it used a given version of Python,
 # continue to use that version of Python in perpetuity.
-if [ -f "$CACHE_DIR/.heroku/python-version" ]; then
+if [[ -f "$CACHE_DIR/.heroku/python-version" ]]; then
   CACHED_PYTHON_VERSION=$(cat "$CACHE_DIR/.heroku/python-version")
 fi
 
 # We didn't always record the stack version. This code is in place because of that.
-if [ -f "$CACHE_DIR/.heroku/python-stack" ]; then
+if [[ -f "$CACHE_DIR/.heroku/python-stack" ]]; then
   CACHED_PYTHON_STACK=$(cat "$CACHE_DIR/.heroku/python-stack")
 else
   CACHED_PYTHON_STACK=$STACK
@@ -184,7 +184,7 @@ source "${BUILDPACK_DIR}/bin/steps/pipenv"
 # If no requirements.txt file given, assume `setup.py develop` is intended.
 # This allows for people to ship a setup.py application to Heroku
 
-if [ ! -f requirements.txt ] && [ ! -f Pipfile ]; then
+if [[ ! -f requirements.txt ]] && [[ ! -f Pipfile ]]; then
   echo "-e ." > requirements.txt
 fi
 

bin/detect

@@ -2,10 +2,12 @@
 # Usage: bin/compile <build-dir>
 # See: https://devcenter.heroku.com/articles/buildpack-api
 
-BUILD_DIR=$1
+set -euo pipefail
+
+BUILD_DIR="${1}"
 
 # Exit early if app is clearly not Python.
-if [ ! -f "$BUILD_DIR/requirements.txt" ] && [ ! -f "$BUILD_DIR/setup.py" ] && [ ! -f "$BUILD_DIR/Pipfile" ]; then
+if [[ ! -f "$BUILD_DIR/requirements.txt" ]] && [[ ! -f "$BUILD_DIR/setup.py" ]] && [[ ! -f "$BUILD_DIR/Pipfile" ]]; then
   exit 1
 fi
 

bin/steps/collectstatic

@@ -10,7 +10,9 @@
 #   - $DISABLE_COLLECTSTATIC: disables this functionality.
 #   - $DEBUG_COLLECTSTATIC: upon failure, print out environment variables.
 
-# This script is run in a subshell via sub_env so doesn't inherit the vars/utils from `bin/compile`.
+# 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.
+set -eo pipefail
 BUILDPACK_DIR=$(cd "$(dirname "$(dirname "$(dirname "${BASH_SOURCE[0]}")")")" && pwd)
 source "${BUILDPACK_DIR}/bin/utils"
 
@@ -86,7 +88,7 @@ echo
 echo "       https://devcenter.heroku.com/articles/django-assets"
 
 # Additionally, dump out the environment, if debug mode is on.
-if [ "$DEBUG_COLLECTSTATIC" ]; then
+if [[ -n "$DEBUG_COLLECTSTATIC" ]]; then
   echo
   echo "****** Collectstatic environment variables:"
   echo

bin/steps/hooks/post_compile

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-if [ -f bin/post_compile ]; then
+if [[ -f bin/post_compile ]]; then
     echo "-----> Running post-compile hook"
     chmod +x bin/post_compile
     sub_env bin/post_compile

bin/steps/hooks/pre_compile

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-if [ -f bin/pre_compile ]; then
+if [[ -f bin/pre_compile ]]; then
     echo "-----> Running pre-compile hook"
     chmod +x bin/pre_compile
     sub_env bin/pre_compile

bin/steps/nltk

@@ -1,25 +1,22 @@
 #!/usr/bin/env bash
 
-# This script serves as the NLTK build step of the
-# [**Python Buildpack**](https://github.com/heroku/heroku-buildpack-python)
-# compiler.
-#
-# A [buildpack](https://devcenter.heroku.com/articles/buildpacks) is an
-# adapter between a Python application and Heroku's runtime.
-#
-# This script is invoked by [`bin/compile`](/).
-
-# This script is run in a subshell via sub_env so doesn't inherit the vars/utils from `bin/compile`.
+# 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.
+set -eo pipefail
 BUILDPACK_DIR=$(cd "$(dirname "$(dirname "$(dirname "${BASH_SOURCE[0]}")")")" && pwd)
 source "${BUILDPACK_DIR}/bin/utils"
 
+# These are required by `set_env`.
+PROFILE_PATH="${BUILD_DIR}/.profile.d/python.sh"
+EXPORT_PATH="${BUILDPACK_DIR}/export"
+
 # Check that nltk was installed by pip, otherwise obviously not needed
 if is_module_available 'nltk'; then
     puts-step "Downloading NLTK corpora..."
 
     nltk_packages_definition="$BUILD_DIR/nltk.txt"
 
-    if [ -f "$nltk_packages_definition" ]; then
+    if [[ -f "$nltk_packages_definition" ]]; then
 
         readarray -t nltk_packages < "$nltk_packages_definition"
         puts-step "Downloading NLTK packages: ${nltk_packages[*]}"

bin/steps/pip-install

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-if [ ! "$SKIP_PIP_INSTALL" ]; then
+if [[ -z "$SKIP_PIP_INSTALL" ]]; then
 
     puts-step "Installing requirements with pip"
 
@@ -20,7 +20,7 @@ if [ ! "$SKIP_PIP_INSTALL" ]; then
     # Measure that we're using pip.
     mcount "tool.pip"
 
-    if [ ! -f "$BUILD_DIR/.heroku/python/bin/pip" ]; then
+    if [[ ! -f "$BUILD_DIR/.heroku/python/bin/pip" ]]; then
         exit 1
     fi
 
@@ -39,7 +39,7 @@ if [ ! "$SKIP_PIP_INSTALL" ]; then
     /app/.heroku/python/bin/pip freeze --disable-pip-version-check > .heroku/python/requirements-installed.txt
 
     # Install test dependencies, for CI.
-    if [ "$INSTALL_TEST" ]; then
+    if [[ -n "$INSTALL_TEST" ]]; then
         if [[ -f requirements-test.txt ]]; then
             puts-step "Installing test dependencies..."
             /app/.heroku/python/bin/pip install -r requirements-test.txt --exists-action=w --src='/app/.heroku/src' --disable-pip-version-check --no-cache-dir 2>&1 | cleanup | indent

bin/steps/pipenv

@@ -33,7 +33,7 @@ if [[ -f Pipfile ]]; then
     /app/.heroku/python/bin/pip install --quiet --disable-pip-version-check --no-cache-dir "pipenv==${PIPENV_VERSION}"
 
     # Install the test dependencies, for CI.
-    if [ "$INSTALL_TEST" ]; then
+    if [[ -n "$INSTALL_TEST" ]]; then
         puts-step "Installing test dependencies"
         /app/.heroku/python/bin/pipenv install --dev --system --deploy --extra-pip-args='--src=/app/.heroku/src' 2>&1 | cleanup | indent
 

bin/steps/pipenv-python-version

@@ -52,6 +52,8 @@ if [[ -f $BUILD_DIR/Pipfile ]]; then
                     3.12)
                         echo "${LATEST_312}" > "${BUILD_DIR}/runtime.txt"
                         ;;
+                    # TODO: Make this case an error
+                    *) ;;
                 esac
             fi
 

bin/steps/python

@@ -33,6 +33,7 @@ case "${PYTHON_VERSION}" in
   python-3.6.+([0-9]))
     eol_python_version_error "3.6" "December 23rd, 2021"
     ;;
+  *) ;;
 esac
 
 # The Python runtime archive filename is of form: 'python-X.Y.Z-ubuntu-22.04-amd64.tar.zst'
@@ -93,6 +94,8 @@ case "${PYTHON_VERSION}" in
     puts-warn
     warn_if_patch_update_available "${PYTHON_VERSION}" "${LATEST_38}"
     ;;
+  # TODO: Make this case an error, since it should be unreachable.
+  *) ;;
 esac
 
 mcount "version.python.${PYTHON_VERSION}"
@@ -102,8 +105,8 @@ if [[ "$STACK" != "$CACHED_PYTHON_STACK" ]]; then
     rm -rf .heroku/python-stack .heroku/python-version .heroku/python .heroku/vendor .heroku/python .heroku/python-sqlite3-version
 fi
 
-if [ -f .heroku/python-version ]; then
-  if [ ! "$(cat .heroku/python-version)" = "$PYTHON_VERSION" ]; then
+if [[ -f .heroku/python-version ]]; then
+  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
   else

bin/steps/sqlite3

@@ -15,16 +15,18 @@ sqlite3_install() {
   mkdir -p "$APT_CACHE_DIR/archives/partial"
   mkdir -p "$APT_STATE_DIR/lists/partial"
 
-  APT_OPTIONS="-o debug::nolocking=true"
-  APT_OPTIONS="$APT_OPTIONS -o dir::cache=$APT_CACHE_DIR"
-  APT_OPTIONS="$APT_OPTIONS -o dir::state=$APT_STATE_DIR"
-  APT_OPTIONS="$APT_OPTIONS -o dir::etc::sourcelist=/etc/apt/sources.list"
-
-  apt-get $APT_OPTIONS update > /dev/null 2>&1
-  if [ -z "$HEADERS_ONLY" ]; then
-    apt-get $APT_OPTIONS -y -d --reinstall install libsqlite3-dev sqlite3 > /dev/null 2>&1
+  APT_OPTIONS=(
+    "--option=debug::nolocking=true"
+    "--option=dir::cache=${APT_CACHE_DIR}"
+    "--option=dir::state=${APT_STATE_DIR}"
+    "--option=dir::etc::sourcelist=/etc/apt/sources.list"
+  )
+
+  apt-get "${APT_OPTIONS[@]}" update > /dev/null 2>&1
+  if [[ -z "$HEADERS_ONLY" ]]; then
+    apt-get "${APT_OPTIONS[@]}" -y -d --reinstall install libsqlite3-dev sqlite3 > /dev/null 2>&1
   else
-    apt-get $APT_OPTIONS -y -d --reinstall install libsqlite3-dev
+    apt-get "${APT_OPTIONS[@]}" -y -d --reinstall install libsqlite3-dev
   fi
 
   find "$APT_CACHE_DIR/archives/" -name "*.deb" -exec dpkg -x {} "$HEROKU_PYTHON_DIR/sqlite3/" \;
@@ -50,7 +52,7 @@ sqlite3_install() {
   # need to point the libsqlite3.so to the stack image library for /usr/bin/ld -lsqlite3
   SQLITE3_LIBFILE="/usr/lib/${GNU_ARCH}-linux-gnu/$(readlink -n "$HEROKU_PYTHON_DIR/sqlite3/usr/lib/${GNU_ARCH}-linux-gnu/libsqlite3.so")"
   ln -s "$SQLITE3_LIBFILE" "$HEROKU_PYTHON_DIR/lib/libsqlite3.so"
-  if [ -z "$HEADERS_ONLY" ]; then
+  if [[ -z "$HEADERS_ONLY" ]]; then
     mv "$HEROKU_PYTHON_DIR/sqlite3/usr/bin"/* "$HEROKU_PYTHON_DIR/bin/"
   fi
 
@@ -60,13 +62,6 @@ sqlite3_install() {
 }
 
 buildpack_sqlite3_install() {
-  HEROKU_PYTHON_DIR="$BUILD_DIR/.heroku/python"
-
-  SQLITE3_VERSION_FILE="$BUILD_DIR/.heroku/python-sqlite3-version"
-  if [ -f "$SQLITE3_VERSION_FILE" ]; then
-    INSTALLED_SQLITE3_VERSION=$(cat "$SQLITE3_VERSION_FILE")
-  fi
-
   puts-step "Installing SQLite3"
 
   if sqlite3_install "$BUILD_DIR/.heroku/python" ; then

bin/utils

@@ -7,7 +7,7 @@ shopt -s nullglob
 
 source "${BUILDPACK_DIR}/vendor/buildpack-stdlib_v8.sh"
 
-if [ "$(uname)" == Darwin ]; then
+if [[ "$(uname)" == "Darwin" ]]; then
     sed() { command sed -l "$@"; }
 else
     sed() { command sed -u "$@"; }
@@ -52,7 +52,7 @@ deep-cp() {
 
 # Measure the size of the Python installation.
 measure-size() {
-  echo "$(du -s .heroku/python 2>/dev/null || echo 0) | awk '{print $1}')"
+  { du -s .heroku/python 2>/dev/null || echo 0; } | awk '{print $1}'
 }
 
 # Returns 0 if the specified module exists, otherwise returns 1.

etc/publish.sh

@@ -7,7 +7,7 @@ BP_NAME=${1:-"heroku/python"}
 curVersion=$(heroku buildpacks:versions "$BP_NAME" | awk 'FNR == 3 { print $1 }')
 newVersion="v$((curVersion + 1))"
 
-read -p "Deploy as version: $newVersion [y/n]? " choice
+read -r -p "Deploy as version: $newVersion [y/n]? " choice
 case "$choice" in
   y|Y ) echo "";;
   n|N ) exit 0;;
@@ -18,7 +18,7 @@ git fetch origin
 originMain=$(git rev-parse origin/main)
 echo "Tagging commit $originMain with $newVersion... "
 git tag "$newVersion" "${originMain:?}"
-git push origin refs/tags/$newVersion
+git push origin "refs/tags/${newVersion}"
 
 echo -e "\nPublishing to the buildpack registry..."
 heroku buildpacks:publish "$BP_NAME" "$newVersion"

spec/fixtures/hooks/bin/post_compile

@@ -1,3 +1,5 @@
+#!/usr/bin/env bash
+
 set -euo pipefail
 
 echo 'post_compile ran with env vars:'

spec/fixtures/hooks/bin/pre_compile

@@ -1,3 +1,5 @@
+#!/usr/bin/env bash
+
 set -euo pipefail
 
 echo 'pre_compile ran with env vars:'

spec/fixtures/pipenv_editable/bin/test-entrypoints

@@ -5,7 +5,7 @@ set -euo pipefail
 cd .heroku/python/lib/python*/site-packages/
 
 # List any path like strings in .egg-link, .pth, and finder files in site-packages.
-grep --extended-regexp --only-matching '/\S+' *.egg-link *.pth __editable___*_finder.py | sort
+grep --extended-regexp --only-matching -- '/\S+' *.egg-link *.pth __editable___*_finder.py | sort
 echo
 
 echo -n "Running entrypoint for the pyproject.toml-based local package: "

spec/fixtures/requirements_editable/bin/test-entrypoints

@@ -5,7 +5,7 @@ set -euo pipefail
 cd .heroku/python/lib/python*/site-packages/
 
 # List any path like strings in .egg-link, .pth, and finder files in site-packages.
-grep --extended-regexp --only-matching '/\S+' *.egg-link *.pth __editable___*_finder.py | sort
+grep --extended-regexp --only-matching -- '/\S+' *.egg-link *.pth __editable___*_finder.py | sort
 echo
 
 echo -n "Running entrypoint for the pyproject.toml-based local package: "

spec/hatchet/nltk_spec.rb

@@ -19,6 +19,8 @@
           remote: \\[nltk_data\\]     /tmp/build_.*/.heroku/python/nltk_data...
           remote: \\[nltk_data\\]   Unzipping corpora/stopwords.zip.
         REGEX
+
+        # TODO: Add a test that the downloaded corpora can be found at runtime.
       end
     end
   end

vendor/buildpack-stdlib_v8.sh

@@ -92,7 +92,7 @@ un_set_env() {
 # Outputs a regex of default blacklist env vars.
 _env_blacklist() {
   local regex=${1:-''}
-  if [ -n "$regex" ]; then
+  if [[ -n "$regex" ]]; then
     regex="|$regex"
   fi
   echo "^(PATH|CPATH|CPPATH|LD_PRELOAD|LIBRARY_PATH|LD_LIBRARY_PATH|PYTHONHOME$regex)$"
@@ -105,7 +105,7 @@ export_env() {
   local whitelist=${2:-''}
   local blacklist
   blacklist="$(_env_blacklist "$3")"
-  if [ -d "$env_dir" ]; then
+  if [[ -d "$env_dir" ]]; then
     # Environment variable names won't contain characters affected by:
     # shellcheck disable=SC2045
     for e in $(ls "$env_dir"); do

vendor/python.gunicorn.sh

@@ -1,3 +1,8 @@
+#!/usr/bin/env bash
+
+# Note: Since this is a .profile.d/ script it will be sourced, meaning that we cannot enable
+# exit on error, have to use return not exit, and returning non-zero doesn't have an effect.
+
 # Automatic configuration for Gunicorn's ForwardedAllowIPS setting.
 export FORWARDED_ALLOW_IPS='*'