-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversation
0dc2cf6
to
fdb0744
Compare
There was a problem hiding this 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 ?
fdb0744
to
e6f25d5
Compare
Updated the comment in the Makefile with a reference to the issue |
e6f25d5
to
982c192
Compare
Added comments/references in two more places |
982c192
to
b5d1be6
Compare
There was a problem hiding this 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.
monitoring/Dockerfile
Outdated
@@ -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 |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
Options to move forward:
About
|
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
b5d1be6
to
5806cb6
Compare
Closed in favor of #779 |
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 bypip-tools
:This is an alternative to #779 to fix #768