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

Revert "Use sitecustomize to emulate venv during python install" #520

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Jun 29, 2022

This reverts #512.
Fixes #518.

There is a conditional in setuptools that causes some setup.cfg options to be ignored while in a virtual environment. While their use is probably incorrect, the anitpattern is too widespread to ignore, and we can't see a way to avoid the conditional in setuptools while still using the sitecustomize approach to work around the platform patches.

This will regress colcon builds on Fedora 36, homebrew, and Jammy with setuptools installed from pip once again, until we can come up with a new solution.

@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #520 (cd94cc0) into master (d02e374) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
- Coverage   81.68%   81.66%   -0.03%     
==========================================
  Files          61       60       -1     
  Lines        3570     3566       -4     
  Branches      685      685              
==========================================
- Hits         2916     2912       -4     
  Misses        602      602              
  Partials       52       52              
Impacted Files Coverage Δ
colcon_core/task/python/build.py 36.02% <50.00%> (-1.02%) ⬇️

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 d02e374...cd94cc0. Read the comment docs.

Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

It is unfortunate but this appears to be case where the cure is imperfect and more insidious than the disease, even if not strictly speaking "worse".

@cottsay
Copy link
Member Author

cottsay commented Nov 2, 2022

Partially for my own sake, this is the spot in setuptools where this change originally went wrong: https://github.com/pypa/setuptools/blob/f777a40ed9abf529906c2939f80a184a5ed035fa/setuptools/dist.py#L691

It actually exposes a general problem here that I'm not sure we will be able to solve: If you install colcon in a venv, you will always trigger the bug #518. Evidently it wasn't a wide enough use case to get our attention before I tried to use the venv-like approach here to accomplish our goals, but here we are.

The workaround of using underscores mentioned in #518 probably only works accidentally - a comment in the setuptools code indicates that ignoring the entries is an attempt to disable the customized installation directories.

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

Successfully merging this pull request may close these issues.

entry_points install path is now 'bin' instead of 'lib' for setup.py installs
2 participants