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

Tools: Install setuptools, packaging and wheel (and others) through apt #28056

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ryanf55
Copy link
Collaborator

@Ryanf55 Ryanf55 commented Sep 10, 2024

Scope

  • 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

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

if [ ${RELEASE_CODENAME} == 'focal' ]; then
SETUPTOOLS=setuptools==70.3.0
fi
$PIP install $PIP_USER_ARGUMENT -U pip packaging $SETUPTOOLS wheel
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Sep 11, 2024

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?
https://www.debian.org/releases/buster/

Edit: Using lxml through apt solves this one too.

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Sep 11, 2024

I had to move pygame too. We could manually install all the dependencies through apt, and then compile python3-pygame, but this seems more stable.

134.5 + for PACKAGE in $PYTHON_PKGS
134.5 + pip3 install --user -U pygame
135.0 Collecting pygame
138.5   Downloading https://files.pythonhosted.org/packages/72/49/bd2fcbadb6a55bb24284bad4f530189401c99ffc234d51ba54756a776eb2/pygame-2.6.0.tar.gz (15.8MB)
142.2     Complete output from command python setup.py egg_info:
142.2     Skipping Cython compilation
142.2     
142.2     
142.2     WARNING, No "Setup" File Exists, Running "buildconfig/config.py"
142.2     Using UNIX configuration...
142.2     
142.2     /bin/sh: 1: sdl2-config: not found
142.2     /bin/sh: 1: sdl2-config: not found
142.2     /bin/sh: 1: sdl2-config: not found
142.2     Package freetype2 was not found in the pkg-config search path.
142.2     Perhaps you should add the directory containing `freetype2.pc'
142.2     to the PKG_CONFIG_PATH environment variable
142.2     No package 'freetype2' found
142.2     Package freetype2 was not found in the pkg-config search path.
142.2     Perhaps you should add the directory containing `freetype2.pc'
142.2     to the PKG_CONFIG_PATH environment variable
142.2     No package 'freetype2' found
142.2     Package freetype2 was not found in the pkg-config search path.
142.2     Perhaps you should add the directory containing `freetype2.pc'
142.2     to the PKG_CONFIG_PATH environment variable
142.2     No package 'freetype2' found
142.2     /bin/sh: 1: freetype-config: not found
142.2     /bin/sh: 1: freetype-config: not found
142.2     /bin/sh: 1: freetype-config: not found
142.2     Traceback (most recent call last):
142.2       File "<string>", line 1, in <module>
142.2       File "/tmp/pip-install-ba7a5qs8/pygame/setup.py", line 426, in <module>
142.2         buildconfig.config.main(AUTO_CONFIG)
142.2       File "/tmp/pip-install-ba7a5qs8/pygame/buildconfig/config.py", line 234, in main
142.2         deps = CFG.main(**kwds, auto_config=auto)
142.2       File "/tmp/pip-install-ba7a5qs8/pygame/buildconfig/config_unix.py", line 245, in main
142.2         raise RuntimeError('Unable to run "sdl-config". Please make sure a development version of SDL is installed.')
142.2     RuntimeError: Unable to run "sdl-config". Please make sure a development version of SDL is installed.
142.2     
142.2     Hunting dependencies...
142.2     WARNING: "sdl2-config" failed!
142.2     WARNING: "pkg-config freetype2" failed!
142.2     WARNING: "freetype-config" failed!
142.2     
142.2     ---
142.2     For help with compilation see:
142.2         https://www.pygame.org/wiki/CompileDebian
142.2     To contribute to pygame development see:
142.2         https://www.pygame.org/contribute.html
142.2     ---
142.2     
142.2     
142.2     ----------------------------------------
142.2 Command "python setup.py egg_info" failed with error code 1 in /tmp/pip-install-ba7a5qs8/pygame/

@Ryanf55 Ryanf55 changed the title Tools: Install setuptools, packaging and wheel through apt Tools: Install setuptools, packaging and wheel (and others) through apt Sep 11, 2024
@peterbarker
Copy link
Contributor

So it sounds like we need to test every distro we support in there or explicitly decide to remove support.

buster can probably go; nobody should be creating a new environment on it.

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Sep 11, 2024

So it sounds like we need to test every distro we support in there or explicitly decide to remove support.

buster can probably go; nobody should be creating a new environment on it.

So it sounds like we need to test every distro we support in there or explicitly decide to remove support.

buster can probably go; nobody should be creating a new environment on it.

As part of this PR, or another? I'd really like to fix the breaking of ROS environments ASAP.

@peterbarker
Copy link
Contributor

As part of this PR, or another? I'd really like to fix the breaking of ROS environments ASAP.

So it sounds like we need to test every distro we support in there or explicitly decide to remove support.
buster can probably go; nobody should be creating a new environment on it.

So it sounds like we need to test every distro we support in there or explicitly decide to remove support.
buster can probably go; nobody should be creating a new environment on it.

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.

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Sep 12, 2024

As part of this PR, or another? I'd really like to fix the breaking of ROS environments ASAP.

So it sounds like we need to test every distro we support in there or explicitly decide to remove support.
buster can probably go; nobody should be creating a new environment on it.

So it sounds like we need to test every distro we support in there or explicitly decide to remove support.
buster can probably go; nobody should be creating a new environment on it.

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.

Yep, this workflow of this combination of apt deps is working in an isolated Ubuntu 22 environment with ROS 2.
I've tested mavproxy, sim_vehicle.py with gdb, console, map in both Ubuntu 20, Ubuntu 22, and debian buster.

Is there a better alternative you had in mind that avoids installing the latest version through pip outside a venv?

Seems like you worked out the problems with buster.

Yep!

@peterbarker
Copy link
Contributor

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.

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Sep 16, 2024

I'm putting this up for dev call because

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

More test results:

  • Bionic: python3-pygame is not available. As it stands, this would break bionic users.

If we want to make conditional logic to only do this on focal and jammy, I can do that.

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Sep 17, 2024

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.

Ryanf55 and others added 3 commits September 20, 2024 22:39
* 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 <ryanfriedman5410+github@gmail.com>
* The python version is not compatible with setuptools/wheel/packaging
  from apt

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>
* https://www.pygame.org/wiki/GettingStarted#Debian/Ubuntu/Mint
* They recommend apt if you want stability

Signed-off-by: Ryan Friedman <25047695+Ryanf55@users.noreply.github.com>
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Sep 21, 2024

Bionic is removed. Needs testing on the debian builds better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants