Skip to content

Commit

Permalink
Add a warning when multiple package managers are found (#1692)
Browse files Browse the repository at this point in the history
Currently the buildpack will use whichever package manager it
finds first, if the files of multiple package managers are found.

This occasionally results in support tickets where the user believes
the build to not be installing dependencies correctly, when in fact
they are adding dependencies to the wrong package manager file.

As such, we want to make the presence of multiple package manager
files an error, but first we should add a warning so it's not a surprise.

(Plus with the recent Poetry support addition, there will be apps using
the third party Poetry buildpack that have both a `poetry.lock` and
the generated `requirements.txt` until they remove the third-party
buildpack.)

Towards #1691.
GUS-W-17167927.
  • Loading branch information
edmorley authored Nov 8, 2024
1 parent 1f6857e commit 0cd8efc
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## [Unreleased]

- Added a warning when the files for multiple package managers are found. In the future this warning will become an error. ([#1692](https://github.com/heroku/heroku-buildpack-python/pull/1692))
- Updated the build log message shown when installing dependencies to include the package manager command being run. ([#1689](https://github.com/heroku/heroku-buildpack-python/pull/1689))
- Improved the error messages and buildpack metrics for package manager related failures. ([#1689](https://github.com/heroku/heroku-buildpack-python/pull/1689))
- Changed test dependency installation on Heroku CI to now install `requirements.txt` and `requirements-test.txt` in a single `pip install` invocation rather than separately. This allows pip's resolver to resolve any version conflicts between the two files. ([#1689](https://github.com/heroku/heroku-buildpack-python/pull/1689))
Expand Down
19 changes: 19 additions & 0 deletions lib/output.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# however, it helps Shellcheck realise the options under which these functions will run.
set -euo pipefail

ANSI_BLUE='\033[1;34m'
ANSI_RED='\033[1;31m'
ANSI_YELLOW='\033[1;33m'
ANSI_RESET='\033[0m'
Expand All @@ -28,6 +29,24 @@ function output::indent() {
sed --unbuffered "s/^/ /"
}

# Output a styled multi-line notice message to stderr.
#
# Usage:
# ```
# output::notice <<-EOF
# Note: The note summary.
#
# Detailed description.
# EOF
# ```
function output::notice() {
echo >&2
while IFS= read -r line; do
echo -e "${ANSI_BLUE} ! ${line}${ANSI_RESET}" >&2
done
echo >&2
}

# Output a styled multi-line warning message to stderr.
#
# Usage:
Expand Down
32 changes: 32 additions & 0 deletions lib/package_manager.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ set -euo pipefail
function package_manager::determine_package_manager() {
local build_dir="${1}"
local package_managers_found=()
local package_managers_found_display_text=()

if [[ -f "${build_dir}/Pipfile.lock" ]]; then
package_managers_found+=(pipenv)
package_managers_found_display_text+=("Pipfile.lock (Pipenv)")
meta_set "pipenv_has_lockfile" "true"
elif [[ -f "${build_dir}/Pipfile" ]]; then
# TODO: Start requiring a Pipfile.lock and make this branch a "missing lockfile" error instead.
Expand All @@ -20,11 +22,13 @@ function package_manager::determine_package_manager() {
We recommend you commit this into your repository.
EOF
package_managers_found+=(pipenv)
package_managers_found_display_text+=("Pipfile (Pipenv)")
meta_set "pipenv_has_lockfile" "false"
fi

if [[ -f "${build_dir}/requirements.txt" ]]; then
package_managers_found+=(pip)
package_managers_found_display_text+=("requirements.txt (pip)")
fi

# This must be after the requirements.txt check, so that the requirements.txt exported by
Expand All @@ -34,12 +38,14 @@ function package_manager::determine_package_manager() {
# ordering here won't matter.
if [[ -f "${build_dir}/poetry.lock" ]]; then
package_managers_found+=(poetry)
package_managers_found_display_text+=("poetry.lock (Poetry)")
fi

# TODO: Deprecate/sunset this fallback, since using setup.py declared dependencies is
# not a best practice, and we can only guess as to which package manager to use.
if ((${#package_managers_found[@]} == 0)) && [[ -f "${build_dir}/setup.py" ]]; then
package_managers_found+=(pip)
package_managers_found_display_text+=("setup.py (pip)")
meta_set "setup_py_only" "true"
else
meta_set "setup_py_only" "false"
Expand Down Expand Up @@ -87,6 +93,32 @@ function package_manager::determine_package_manager() {
# aren't taking effect. (We'll need to wait until after Poetry support has landed,
# and people have had a chance to migrate from the Poetry buildpack mentioned above.)
echo "${package_managers_found[0]}"

output::warning <<-EOF
Warning: Multiple Python package manager files were found.
Exactly one package manager file should be present in your app's
source code, however, several were found:
$(printf -- "%s\n" "${package_managers_found_display_text[@]}")
For now, we will build your app using the first package manager
listed above, however, in the future this warning will become
an error.
Decide which package manager you want to use with your app, and
then delete the file(s) and any config from the others.
EOF

if [[ "${package_managers_found[*]}" == *"poetry"* ]]; then
output::notice <<-EOF
Note: We recently added support for the package manager Poetry.
If you are using a third-party Poetry buildpack you must remove
it, otherwise the requirements.txt file it generates will cause
the warning above.
EOF
fi

meta_set "package_manager_multiple_found" "$(
IFS=,
echo "${package_managers_found[*]}"
Expand Down
16 changes: 16 additions & 0 deletions spec/hatchet/pipenv_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,22 @@
app.deploy do |app|
expect(clean_output(app.output)).to match(Regexp.new(<<~REGEX))
remote: -----> Python app detected
remote:
remote: ! Warning: Multiple Python package manager files were found.
remote: !
remote: ! Exactly one package manager file should be present in your app's
remote: ! source code, however, several were found:
remote: !
remote: ! Pipfile.lock \\(Pipenv\\)
remote: ! requirements.txt \\(pip\\)
remote: !
remote: ! For now, we will build your app using the first package manager
remote: ! listed above, however, in the future this warning will become
remote: ! an error.
remote: !
remote: ! Decide which package manager you want to use with your app, and
remote: ! then delete the file\\(s\\) and any config from the others.
remote:
remote: -----> Using Python 3.12 specified in Pipfile.lock
remote: -----> Installing Python #{LATEST_PYTHON_3_12}
remote: -----> Installing pip #{PIP_VERSION}, setuptools #{SETUPTOOLS_VERSION} and wheel #{WHEEL_VERSION}
Expand Down
22 changes: 22 additions & 0 deletions spec/hatchet/poetry_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,28 @@
app.deploy do |app|
expect(clean_output(app.output)).to include(<<~OUTPUT)
remote: -----> Python app detected
remote:
remote: ! Warning: Multiple Python package manager files were found.
remote: !
remote: ! Exactly one package manager file should be present in your app's
remote: ! source code, however, several were found:
remote: !
remote: ! requirements.txt (pip)
remote: ! poetry.lock (Poetry)
remote: !
remote: ! For now, we will build your app using the first package manager
remote: ! listed above, however, in the future this warning will become
remote: ! an error.
remote: !
remote: ! Decide which package manager you want to use with your app, and
remote: ! then delete the file(s) and any config from the others.
remote:
remote:
remote: ! Note: We recently added support for the package manager Poetry.
remote: ! If you are using a third-party Poetry buildpack you must remove
remote: ! it, otherwise the requirements.txt file it generates will cause
remote: ! the warning above.
remote:
remote: -----> Using Python #{DEFAULT_PYTHON_MAJOR_VERSION} specified in .python-version
remote: -----> Installing Python #{LATEST_PYTHON_3_12}
remote: -----> Installing pip #{PIP_VERSION}, setuptools #{SETUPTOOLS_VERSION} and wheel #{WHEEL_VERSION}
Expand Down

0 comments on commit 0cd8efc

Please sign in to comment.