diff --git a/CHANGELOG.md b/CHANGELOG.md index be0f589cd..2cb5c1599 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## [Unreleased] +- Added a warning if the Python buildpack has been run multiple times in the same build. In January 2025 this warning will be made an error. ([#1724](https://github.com/heroku/heroku-buildpack-python/pull/1724)) +- Added a warning if an existing `.heroku/python/` directory is found in the app source. In January 2025 this warning will be made an error. ([#1724](https://github.com/heroku/heroku-buildpack-python/pull/1724)) +- Improved the error message shown if the buildpack is used on an unsupported stack. ([#1724](https://github.com/heroku/heroku-buildpack-python/pull/1724)) - Fixed Dev Center links to reflect recent article URL changes. ([#1723](https://github.com/heroku/heroku-buildpack-python/pull/1723)) - Added metrics for the existence of a uv lockfile. ([#1725](https://github.com/heroku/heroku-buildpack-python/pull/1725)) diff --git a/bin/compile b/bin/compile index 495d8c20d..7edf8a44c 100755 --- a/bin/compile +++ b/bin/compile @@ -21,6 +21,7 @@ BUILDPACK_DIR=$(cd "$(dirname "$(dirname "${BASH_SOURCE[0]}")")" && pwd) source "${BUILDPACK_DIR}/bin/utils" source "${BUILDPACK_DIR}/lib/utils.sh" source "${BUILDPACK_DIR}/lib/cache.sh" +source "${BUILDPACK_DIR}/lib/checks.sh" source "${BUILDPACK_DIR}/lib/hooks.sh" source "${BUILDPACK_DIR}/lib/metadata.sh" source "${BUILDPACK_DIR}/lib/output.sh" @@ -32,10 +33,15 @@ source "${BUILDPACK_DIR}/lib/poetry.sh" compile_start_time=$(nowms) -# Initialise metadata store. +# Initialise the buildpack metadata store. +# This is used to track state across builds (for cache invalidation and messaging when build +# configuration changes) and also so that `bin/report` can generate the build report. meta_init "${CACHE_DIR}" "python" meta_setup +checks::ensure_supported_stack "${STACK:?Required env var STACK is not set}" +checks::warn_if_duplicate_python_buildpack "${BUILD_DIR}" + # Prepend proper path for old-school virtualenv hackery. # This may not be necessary. export PATH=:/usr/local/bin:$PATH @@ -103,6 +109,10 @@ cd "$BUILD_DIR" # Runs a `bin/pre_compile` script if found in the app source, allowing build customisation. hooks::run_hook "pre_compile" +# This check must be after the pre_compile hook, so that we can check not only the original +# app source, but also that the hook hasn't written to '.heroku/python/' either. +checks::warn_if_existing_python_dir_present "${BUILD_DIR}" + package_manager="$(package_manager::determine_package_manager "${BUILD_DIR}")" meta_set "package_manager" "${package_manager}" @@ -135,7 +145,7 @@ python_major_version="${python_full_version%.*}" meta_set "python_version" "${python_full_version}" meta_set "python_version_major" "${python_major_version}" -cache::restore "${BUILD_DIR}" "${CACHE_DIR}" "${STACK:?}" "${cached_python_full_version}" "${python_full_version}" "${package_manager}" +cache::restore "${BUILD_DIR}" "${CACHE_DIR}" "${STACK}" "${cached_python_full_version}" "${python_full_version}" "${package_manager}" # The directory for the .profile.d scripts. mkdir -p "$(dirname "$PROFILE_PATH")" diff --git a/bin/report b/bin/report index eb354bdbc..54325611d 100755 --- a/bin/report +++ b/bin/report @@ -82,6 +82,8 @@ ALL_OTHER_FIELDS=( cache_save_duration dependencies_install_duration django_collectstatic_duration + duplicate_python_buildpack + existing_python_dir nltk_downloader_duration package_manager_install_duration pipenv_has_lockfile diff --git a/lib/cache.sh b/lib/cache.sh index 9d9415ba3..ef62d582f 100644 --- a/lib/cache.sh +++ b/lib/cache.sh @@ -138,7 +138,7 @@ function cache::restore() { mkdir -p "${build_dir}/.heroku" # NB: For now this has to handle files already existing in build_dir since some apps accidentally - # run the Python buildpack twice. TODO: Add an explicit check/error for duplicate buildpacks. + # run the Python buildpack twice. TODO: Refactor this once duplicate buildpacks become an error. # TODO: Investigate why errors are ignored and ideally stop doing so. # TODO: Compare the performance of moving the directory vs copying files. cp -R "${cache_dir}/.heroku/python" "${build_dir}/.heroku/" &>/dev/null || true diff --git a/lib/checks.sh b/lib/checks.sh new file mode 100644 index 000000000..9129244e7 --- /dev/null +++ b/lib/checks.sh @@ -0,0 +1,116 @@ +#!/usr/bin/env bash + +function checks::ensure_supported_stack() { + local stack="${1}" + + case "${stack}" in + heroku-20 | heroku-22 | heroku-24) + return 0 + ;; + cedar* | heroku-16 | heroku-18) + # This error will only ever be seen on non-Heroku environments, since the + # Heroku build system rejects builds using EOL stacks. + output::error <<-EOF + Error: The '${stack}' stack is no longer supported. + + This buildpack no longer supports the '${stack}' stack since it has + reached its end-of-life: + https://devcenter.heroku.com/articles/stack#stack-support-details-for-apps-using-classic-buildpacks + + Upgrade to a newer stack to continue using this buildpack. + EOF + meta_set "failure_reason" "stack::eol" + exit 1 + ;; + *) + output::error <<-EOF + Error: The '${stack}' stack is not recognised. + + This buildpack does not recognise or support the '${stack}' stack. + + If '${stack}' is a valid stack, make sure that you are using the latest + version of this buildpack and have not pinned to an older release: + https://devcenter.heroku.com/articles/managing-buildpacks#view-your-buildpacks + https://devcenter.heroku.com/articles/managing-buildpacks#classic-buildpacks-references + EOF + meta_set "failure_reason" "stack::unknown" + exit 1 + ;; + esac +} + +# TODO: Turn this into an error in January 2025. +function checks::warn_if_duplicate_python_buildpack() { + local build_dir="${1}" + + # The check for the `PYTHONHOME` env var prevents this warning triggering in the case + # where the Python install was committed to the Git repo (which will be handled later). + # (The env var can only have come from the `export` file of an earlier buildpack, + # since app provided config vars haven't been exported to the environment here.) + if [[ -f "${build_dir}/.heroku/python/bin/python" && -v PYTHONHOME ]]; then + output::warning <<-EOF + Warning: The Python buildpack has already been run this build. + + An existing Python installation was found in the build directory + from a buildpack run earlier in the build. + + This normally means there are duplicate Python buildpacks set + on your app, which is not supported, can cause errors and + slow down builds. + + Check the buildpacks set on your app and remove any duplicate + Python buildpack entries: + https://devcenter.heroku.com/articles/managing-buildpacks#view-your-buildpacks + https://devcenter.heroku.com/articles/managing-buildpacks#remove-classic-buildpacks + + If you have a use-case that requires duplicate buildpacks, + please comment on: + https://github.com/heroku/heroku-buildpack-python/issues/1704 + + In January 2025 this warning will be made an error. + EOF + meta_set "duplicate_python_buildpack" "true" + # shellcheck disable=SC2034 # This is used below until we make this check an error. + DUPLICATE_PYTHON_BUILDPACK=1 + fi +} + +# TODO: Turn this into an error in January 2025. +function checks::warn_if_existing_python_dir_present() { + local build_dir="${1}" + + # Avoid warning twice in the case of duplicate buildpacks. + # TODO: Remove this once `warn_if_duplicate_python_buildpack` becomes an error. + if [[ -v DUPLICATE_PYTHON_BUILDPACK ]]; then + return 0 + fi + + # We use `-e` here to catch the case where `python` is a file rather than a directory. + if [[ -e "${build_dir}/.heroku/python" ]]; then + output::warning <<-EOF + Warning: Existing '.heroku/python/' directory found. + + Your app's source code contains an existing directory named + '.heroku/python/', which is where the Python buildpack needs + to install its files. This existing directory contains: + + $(find .heroku/python/ -maxdepth 2 || true) + + Writing to internal locations used by the Python buildpack + is not supported and can cause unexpected errors. + + If you have committed a '.heroku/python/' directory to your + Git repo, you must delete it or use a different location. + + Otherwise, check that an earlier buildpack or 'bin/pre_compile' + hook has not created this directory. + + If you have a use-case that requires writing to this location, + please comment on: + https://github.com/heroku/heroku-buildpack-python/issues/1704 + + In January 2025 this warning will be made an error. + EOF + meta_set "existing_python_dir" "true" + fi +} diff --git a/spec/hatchet/checks_spec.rb b/spec/hatchet/checks_spec.rb index d29554821..ed55fc139 100644 --- a/spec/hatchet/checks_spec.rb +++ b/spec/hatchet/checks_spec.rb @@ -3,6 +3,43 @@ require_relative '../spec_helper' RSpec.describe 'Buildpack validation checks' do + context 'when there are duplicate Python buildpacks set on the app' do + let(:buildpacks) { %i[default default] } + let(:app) { Hatchet::Runner.new("spec/fixtures/python_#{DEFAULT_PYTHON_MAJOR_VERSION}", buildpacks:) } + + it 'fails detection' do + app.deploy do |app| + expect(clean_output(app.output)).to include(<<~OUTPUT) + remote: -----> Python app detected + remote: + remote: ! Warning: The Python buildpack has already been run this build. + remote: ! + remote: ! An existing Python installation was found in the build directory + remote: ! from a buildpack run earlier in the build. + remote: ! + remote: ! This normally means there are duplicate Python buildpacks set + remote: ! on your app, which is not supported, can cause errors and + remote: ! slow down builds. + remote: ! + remote: ! Check the buildpacks set on your app and remove any duplicate + remote: ! Python buildpack entries: + remote: ! https://devcenter.heroku.com/articles/managing-buildpacks#view-your-buildpacks + remote: ! https://devcenter.heroku.com/articles/managing-buildpacks#remove-classic-buildpacks + remote: ! + remote: ! If you have a use-case that requires duplicate buildpacks, + remote: ! please comment on: + remote: ! https://github.com/heroku/heroku-buildpack-python/issues/1704 + remote: ! + remote: ! In January 2025 this warning will be made an error. + remote: + remote: -----> Using Python #{DEFAULT_PYTHON_MAJOR_VERSION} specified in .python-version + remote: -----> Restoring cache + remote: -----> Using cached install of Python #{DEFAULT_PYTHON_FULL_VERSION} + OUTPUT + end + end + end + context 'when the app source contains a broken Python install' do let(:app) { Hatchet::Runner.new('spec/fixtures/python_in_app_source', allow_failure: true) } @@ -10,6 +47,32 @@ app.deploy do |app| expect(clean_output(app.output)).to include(<<~OUTPUT) remote: -----> Python app detected + remote: + remote: ! Warning: Existing '.heroku/python/' directory found. + remote: ! + remote: ! Your app's source code contains an existing directory named + remote: ! '.heroku/python/', which is where the Python buildpack needs + remote: ! to install its files. This existing directory contains: + remote: ! + remote: ! .heroku/python/ + remote: ! .heroku/python/bin + remote: ! .heroku/python/bin/python + remote: ! + remote: ! Writing to internal locations used by the Python buildpack + remote: ! is not supported and can cause unexpected errors. + remote: ! + remote: ! If you have committed a '.heroku/python/' directory to your + remote: ! Git repo, you must delete it or use a different location. + remote: ! + remote: ! Otherwise, check that an earlier buildpack or 'bin/pre_compile' + remote: ! hook has not created this directory. + remote: ! + remote: ! If you have a use-case that requires writing to this location, + remote: ! please comment on: + remote: ! https://github.com/heroku/heroku-buildpack-python/issues/1704 + remote: ! + remote: ! In January 2025 this warning will be made an error. + remote: remote: -----> Using Python #{DEFAULT_PYTHON_MAJOR_VERSION} specified in .python-version remote: -----> Using cached install of Python #{DEFAULT_PYTHON_FULL_VERSION} remote: