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: Use apt for pexpect and allow system packages in the virtual environment #28035

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

Ryanf55
Copy link
Collaborator

@Ryanf55 Ryanf55 commented Sep 7, 2024

Purpose

If an apt package is available for a python dependency, I think we should try to use that.

Doing python3 -m pip install pexpect opens us to completely breaking as soon as the next major version comes out.
Instead, we can rely on our packaging team at canonical to ensure our packages are compatible.

Let's see how CI does.

If we like this approach, it's an alternative to virtual environments for Ubuntu jammy: #26137

Same for

  • python3-future
  • python3-pexpect
  • And I propose all the rest of the python packages that we can, we switch to apt, especially setuptools

Signed-off-by: Ryan Friedman <[email protected]>
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Sep 7, 2024

This is also recommended by another user here who struggled with empy:
#23664 (comment)

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Sep 7, 2024

Looks like the manic build failed.

For pexecpt, even though ubuntu:manic has that package, and it installs, python can't find it.
https://github.com/ArduPilot/ardupilot/actions/runs/10753345775/job/29822682675?pr=28035#step:5:1778

If we are ok, we can add the argument --system-site-pacakges to give the virtual environment access to the system packages installed through apt which would resolve that. This patch is applied in c74ed81.

@Ryanf55 Ryanf55 changed the title Tools: Use apt for pexpect Tools: Use apt for pexpect and allow system packages in the virtual environment Sep 7, 2024
@khancyr
Copy link
Contributor

khancyr commented Sep 7, 2024

The system package things is already needs for Archlinux so we can totally use it for Ubuntu too.

What we should looks after is a requirement.txt ( or newer equivalent) so we can track better the python package needs for release

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Sep 7, 2024

The ROS way of doing things is use system packages whenever possible. If we need to install custom versions not distributed with the system, that is when we we should stick them a virtual environment.

Yes, a requirements file is great, but there's so much logic in this script I don't know if you'd be off any better.

@peterbarker
Copy link
Contributor

Yes, we've had things break when python package maintainers do silly things. We fix those problems when they occur - often by pinning the packages until we can work out why and how we need to update the way we use a package.

Things generally tend not to break.

As to using the system packages - that would seem to remove one of the chief advantages of pyenv - separation of your environment from the system environment. Now you've got two places that can break things!

If we were keen enough we could freeze every package version in requirements.txt - but I don't think we're that keen.

The empy case you're referencing there was a particularly egregious case. There was no reason for upstream to break the API in the way it was - backwards compatability was possible. The author of that package was quite helpful in trying to help us resolve it, mind you.

Are you trying to solve specific problem here?

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Sep 8, 2024

Yes, we've had things break when python package maintainers do silly things. We fix those problems when they occur - often by pinning the packages until we can work out why and how we need to update the way we use a package.

Things generally tend not to break.

Until the next major version comes out. We should at least be pinning that.

As to using the system packages - that would seem to remove one of the chief advantages of pyenv - separation of your environment from the system environment. Now you've got two places that can break things!

Respectfully, I disagree - package maintainers do not allow major version bumps after a distribution is released, and they do their best to avoid breaking things. It's on the onus of us developers to write cross-platform code that works on the main LTS distros with the versions supplied. And I don't see why penv is related, it's not used in the script; only mac uses it.

If we were keen enough we could freeze every version in requirements.txt - but I don't think we're that keen.

Agree.

The empy case you're referencing there was a particularly egregious case. There was no reason for upstream to break the API in the way it was - backwards compatability was possible. The author of that package was quite helpful in trying to help us resolve it, mind you.

Are you trying to solve specific problem here?

Yes, the problem is two-fold:

  • For the past two months, the DDS interface on master now has zero functional test coverage aside from the unit tests and linters, thus it can let in regressions. Approving PR's now takes a lot more time because I have to run all the tests individually through pytest.
  • Anyone developer on Ubuntu 22 who runs the setup script will ruin their ability for setup.py-based python packages to have tests; this will affect most ROS developers who try, and sadly the problem only manifests as a warning until you notice when you run tests that pytest collects 0 tests.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

LGTM

* When possible, we can use the apt-installed python packages which are
  ABI stable
* Same for the other OS's that have VENV setup scripts

Signed-off-by: Ryan Friedman <[email protected]>
@peterbarker peterbarker merged commit d18a2b2 into ArduPilot:master Sep 10, 2024
16 checks passed
@Ryanf55 Ryanf55 deleted the use-apt-for-pexpect branch September 10, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants