colcon / colcon-core

Command line tool to build sets of software packages
http://colcon.readthedocs.io
Apache License 2.0
103 stars 46 forks source link

Use sitecustomize to emulate venv during python install #512

Closed cottsay closed 2 years ago

cottsay commented 2 years ago

During initialization, the site module probes for a sitecustomize module which performs site-specific initialization of various paths.

Python virtual environments modify sys.prefix in order to ensure that any attempted package installation ends up using the virtual environment instead of the system locations.

This approach should prevent various Linux distributions' attempts to inject local into the installation locations for python packages. They seem to have special cases for detecting virtual environments and omit the custom logic when detected.

This is a potentially disruptive change. It modifies some of the core installation logic for python packages. However, some popular Linux distributions (Looking at you, Debian and Fedora) have really backed us into a corner by patching sysconfig.py to inject local into various paths, and this seems to be one of the only ways I can find to avoid that behavior consistently.

This is the specific part of site.py that inspired this change: https://github.com/python/cpython/blob/a24e67697362e76dd25d6901109277458b5971b9/Lib/site.py#L529

Some things we need to cover when testing this:

  1. Many platforms, but mainly Ubuntu Jammy and Fedora 36, where the new patches are causing problems.
  2. With the system-shipped setuptools and also with bleeding-edge pip-installed setuptools.
  3. Invoking colcon from within, or referencing packages in:
    • a virtual environment
    • a user base directory (i.e. ~/.local)
  4. Python 2 ROS 1 python packages are wrapped by cmake and are unaffected by this change

Some canary CI builds:

Normal jobs on ci.ros2.org will use a python virtual environment, which triggers different code paths in the Debian patch. I augmented some test_ci_linux jobs to not use a virtualenv so that we could properly test this.

With updated setuptools:

codecov[bot] commented 2 years ago

Codecov Report

Merging #512 (6240de8) into master (e65ae4d) will increase coverage by 0.02%. The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
+ Coverage   81.66%   81.68%   +0.02%     
==========================================
  Files          60       61       +1     
  Lines        3566     3570       +4     
  Branches      685      685              
==========================================
+ Hits         2912     2916       +4     
  Misses        602      602              
  Partials       52       52              
Impacted Files Coverage Δ
colcon_core/task/python/build.py 37.03% <80.00%> (+1.01%) :arrow_up:
colcon_core/task/python/template/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e65ae4d...6240de8. Read the comment docs.

clalancette commented 2 years ago

I tested this on Fedora 35, Fedora 36, and on Windows 10. In all cases, I bootstrapped colcon using the instructions from https://colcon.readthedocs.io/en/released/developer/bootstrap.html . On Fedora 35 and Fedora 36, I also tested using the system-install versions of pip and setuptools, as well as the latest versions installed via pip (Windows only has the option for pip installed versions, so that's all I tested there).

In all cases I tested, with the latest merge from master here, I was able to successfully bootstrap colcon, and do a build of ros2 using that colcon (to make doubly sure I was using the boostrapped one, in all cases I uninstalled colcon from packages and from pip). Full disclosure, I didn't let Windows run to completion (that takes several hours), but I did get far enough into the build to convince myself that it is actually working.

So from a purely practical perspective, this seems to be doing what we want. I think this is good to go.