-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Tools: Install setuptools, packaging and wheel (and others) through apt #28056
base: master
Are you sure you want to change the base?
Conversation
0fd7a99
to
53596b8
Compare
if [ ${RELEASE_CODENAME} == 'focal' ]; then | ||
SETUPTOOLS=setuptools==70.3.0 | ||
fi | ||
$PIP install $PIP_USER_ARGUMENT -U pip packaging $SETUPTOOLS wheel |
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.
This changes our setup for everything, even if it isn't using venv.
Doesn't this mean we're no longer updating wheel
and packaging
outside of those things doing venv?
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.
Correct. I bundled them together because if you update wheel without updating setuptools, then that is another way to hose the environment. We want to ensure they all stay ABI compatible. Either they all go in as APT and are kept compatible by the OS, or they all stay at latest from pip.
Debian buster has an issue: https://github.com/ArduPilot/ardupilot/actions/runs/10786348034/job/29913069056?pr=28056#step:5:2878 I don't have a fix in mind yet. docker build . -t ardupilot --build-arg USER_UID=$(id -u) --build-arg USER_GID=$(id -g) --build-arg BASE_IMAGE=debian --build-arg TAG=buster
docker run --rm -it -v "$(pwd):/ardupilot" -u "$(id -u):$(id -g)" ardupilot:latest bash That said, buster is EOL. Perhaps if that's a blocker, we kill it from setup? Edit: Using lxml through apt solves this one too. |
17206a9
to
59c4bf4
Compare
I had to move
|
So it sounds like we need to test every distro we support in there or explicitly decide to remove support.
|
As part of this PR, or another? I'd really like to fix the breaking of ROS environments ASAP. |
Anything which can be affected by this PR needs to be tested. This is why I commented on your change to affect things which don't use venv - it seemed to require a lot more testing than the alternative. Seems like you worked out the problems with buster. |
Yep, this workflow of this combination of apt deps is working in an isolated Ubuntu 22 environment with ROS 2. Is there a better alternative you had in mind that avoids installing the latest version through pip outside a venv?
Yep! |
Have you tested a Bionic install? Yes, it's EOL. But we don't want to break things without knowing we have. Which is why I was trying to suggest structuring this change such that it is clearly only affecting e.g. Noble, and nothing else. That makes it easy to test. And we avoid the "ah, crap, sorry" moments when someone needs to install on something which we accidentally broke. The older code should naturally age-out - and that's why I asked about taking buster out. |
I'm putting this up for dev call because
More test results:
If we want to make conditional logic to only do this on |
Make a separate PR to remove bionic. For tests, check the vagrant file for the lists of manual tests. It's extensive. We'll do a best effort to test this before merging. |
* Installing these on host OS outside of a VENV can break many other tools * If in VENV, we upgrade them to the latest just in case * Remove focal workaround now that upstream fixed it Signed-off-by: Ryan Friedman <[email protected]>
* The python version is not compatible with setuptools/wheel/packaging from apt Signed-off-by: Ryan Friedman <[email protected]>
* https://www.pygame.org/wiki/GettingStarted#Debian/Ubuntu/Mint * They recommend apt if you want stability Signed-off-by: Ryan Friedman <[email protected]>
59c4bf4
to
38860b8
Compare
Bionic is removed. Needs testing on the debian builds better. |
Scope
Details
Reverts parts of #27689 because upstream pypa/setuptools#4511 is now fixed, and this change is the root cause of #27925
Instead of a plain revert, I think it's safer and more stable to use the OS supplied version by default.
The maintainers approved this approach in #28035