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

Improve Python install path detection #496

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

roehling
Copy link
Contributor

[ This is the companion PR to colcon/colcon-core/pull/601 ]

This PR makes the customized Python path scheme logic in ament_cmake_python contingent on an install prefix other than /usr.

This makes ament more distribution-friendly and prevents the accidental overwriting of system packages in case someone decides to install their Python packages to /usr. There is no other change in behavior.

@sloretz
Copy link
Contributor

sloretz commented Dec 8, 2023

This makes ament more distribution-friendly and prevents the accidental overwriting of system packages in case someone decides to install their Python packages to /usr

Would you mind giving an example of when this is a problem? I'm not sure when a user would choose to install packages to /usr.

Colcon installs into its own install space which by default is a folder called install at the root of the workspace, and when building debian packages on the ROS buildfarm it's set to a path in /opt/ros per ROS distro (example). If they were to use cmake install the default CMAKE_INSTALL_PREFIX seems to be /usr/local.

I'm hesitant to merge this without a careful review because the logic here is working around issues we've had with patches to setuptools that differ for debian, fedora, and homebrew.

@roehling
Copy link
Contributor Author

The main motivation for the change is to make ament more distribution-friendly by not overriding the Python schema for distribution-owned locations. As this affects the /usr prefix only, everything else remains exactly as before, including /opt/ros and $HOME workspaces.

@cottsay
Copy link
Contributor

cottsay commented Feb 5, 2024

It doesn't feel very good to have a hard-coded check for /usr specifically, especially if your goal is to be more friendly to diverse distribution configurations.

What if we skip the scheme override when we're installing to the same prefix that Python suggests system packages install to? In other words, rather than checking if CMAKE_INSTALL_PREFIX is /usr, we check if CMAKE_INSTALL_PREFIX is the same as sysconfig.get_config_var('base')?

@roehling
Copy link
Contributor Author

What if we skip the scheme override when we're installing to the same prefix that Python suggests system packages install to? In other words, rather than checking if CMAKE_INSTALL_PREFIX is /usr, we check if CMAKE_INSTALL_PREFIX is the same as sysconfig.get_config_var('base')?

I like that idea, as it will also do the right thing for virtual environments. I'll update the PR(s) as soon as I find time.

[ This is the companion PR to
colcon/colcon-core#601 ]

This PR makes the customized Python path scheme logic in
ament_cmake_python contingent on an install prefix other than `/usr`.

This makes ament more distribution-friendly and prevents the accidental
overwriting of system packages in case someone decides to install their
Python packages to /usr. There is no other change in behavior.

Signed-off-by: Timo Röhling <roehling@debian.org>
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.

None yet

3 participants