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

[build] pin setuptools to avoid installation failures #771

Closed
wants to merge 2 commits into from

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Sep 5, 2024

Since python:3.12.5-slim, setuptools is not pre-installed anymore, causing our builds to fail when using this (or later) images.

This PR follows the second option outlined in this comment to allow pinning setuptools using the --allow-unsafe flag, as suggested as a fix by pip-tools:

# WARNING: The following packages were not pinned, but pip requires them to be
# pinned when the requirements file includes hashes and the requirement is not
# satisfied by a package already installed. Consider using the --allow-unsafe flag.
# setuptools

This is an alternative to #779 to fix #768

@Shastick Shastick marked this pull request as ready for review September 6, 2024 10:58
Copy link
Contributor

@barroco barroco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a comment to justify the usage of --allow-unsafe / a reference to the issue ?

@Shastick
Copy link
Contributor Author

Could you please add a comment to justify the usage of --allow-unsafe / a reference to the issue ?

Updated the comment in the Makefile with a reference to the issue

@Shastick Shastick marked this pull request as draft September 10, 2024 08:55
@Shastick Shastick marked this pull request as ready for review September 10, 2024 08:58
@Shastick
Copy link
Contributor Author

Added comments/references in two more places

Copy link
Contributor

@barroco barroco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the justification is solid enough to move forward. We need to understand the problem reported in #771 because, as discussed during the Contributors Weekly call, this has an impact on the baseline of tests which may have a significant impact for current real world deployments.

@@ -9,7 +9,7 @@
#
# This image is intended to be built from the repository root context/folder.

FROM python:3.12.4-slim
FROM python:3.12-slim
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should stay with more pinning rather than less since sometimes our users will need a particular commit to work on an ongoing basis, and more pinning makes that more likely.

# pinned when the requirements file includes hashes and the requirement is not
# satisfied by a package already installed. Consider using the --allow-unsafe flag.
# setuptools
# The following packages are considered to be unsafe in a requirements file:
Copy link
Member

@BenjaminPelletier BenjaminPelletier Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reference as to why it's unsafe? Is it because setuptools is used to install packages, so we're basically trying to upgrade the thing that upgrades things? If so, shouldn't we explicitly update setuptools to a specific pinned version individually before then (separately) using it to install/upgrade all other dependencies? If that were the case, it seems like that strategy would achieve both pinning and safety.

@Shastick
Copy link
Contributor Author

Shastick commented Sep 10, 2024

Options to move forward:

  1. If possible, Install pip via a separate step in the Dockerfile, continue not using --allow-unsafe as we currently do on master. Pin the dependency of setuptools to a well-defined version to help with [ci] increase build reproducibility  #778.
  2. Add setuptools to requirements.in and add the --allow-unsafe flag when building requirements.txt (what this PR currently proposes).

About --allow-unsafe :

Here is an issue with some guesses as to why some packages are defined as unsafe.

The TL/DR:

There are only three packages that are deemed 'unsafe': UNSAFE_PACKAGES = {"setuptools", "distribute", "pip"}. This list has not changed since 2017.

The only effect of --allow-unsafe seems to be to allow pinning any of these three packages.

The rationale behind avoiding the unsafe packages is that these are often installed in environments using a package manager and not via pip, sometimes causing problems when they are both present in a requirements file and already installed with a conflicting version.

Forward

Given that the interuss/monitoring project has full control of the build process, both options above are equally suitable: assuming both work, we may easily switch between any of the two if it should become necessary.

I we can get it to work, I'd suggest going with option 1. above (install and manage setuptools outside of the requirements.in file) to stay close to what the pip-tools authors recommend and avoid frightening users with a poorly documented --allow-unsafe option.

Failing that, option 2. has no drawbacks that I am aware off and can be used as well.

This PR applies the recommendation from the generated requirements file to use the `--allow-unsafe`:

```
```

The PR also removes the pinned minor version of the base image.

Closes interuss#768
@Shastick
Copy link
Contributor Author

Closed in favor of #779

@Shastick Shastick closed this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[build] pip install fails when building the image
3 participants