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

Pin virtualenv 16.7.9 to fix Python bin installation. #399

Merged
merged 2 commits into from
Mar 4, 2020

Conversation

nuclearsandwich
Copy link
Member

@nuclearsandwich nuclearsandwich commented Mar 4, 2020

virtualenv>=20 and venv both set sys.base_prefix to a value outside the virtual environment whereas virtualenv 16.7.9 does not.
This triggers virtualenv detection in setuptools/distutils1 which causes them to ignore our configured install-scripts options.

Since this issue is causing test failures and issues with our distributed binary archives (ros2/build_farmer#266), I think it's worth pinning a version of virtualenv that meets our needs while we explore alternatives.

This PR affects a revert of #385 but not because venv itself was unsuitable. We see the same issue using current versions of virtualenv which was totally rewritten for the 20.x series.

virtualenv>=20 and venv both set sys.base_prefix to a value outside the
virtual environment whereas virtualenv 16.7.9 does not. This triggers
virtualenv detection in setuptools/distutils[1] which causes them to
ignore our configured install-scripts options.

Since this issue is causing test failures and issues with our
distributed binary archives, I think it's worth pinning a version of
virtualenv that meets our needs while we explore alternatives.

[1]: https://github.com/pypa/setuptools/blob/a5dec2f14e3414e4ee5dd146bff9c289d573de9a/setuptools/dist.py#L573-L579

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>
@nuclearsandwich
Copy link
Member Author

nuclearsandwich commented Mar 4, 2020

Edit: Re-triggered jobs with fixed Python syntax

Shameless edit: the package is test_launch_ros not test_ros_launch.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows-container Build Status

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>
Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Grudging approval.

@nuclearsandwich
Copy link
Member Author

Grudging approval.

I know we've been chatting offline, but for the record this is grudging because the solution is a pin to an old version not because there's anything unsound with the PR itself?

@mjcarroll
Copy link
Member

I know we've been chatting offline, but for the record this is grudging because the solution is a pin to an old version not because there's anything unsound with the PR itself?

Yes, sorry, nothing unsound about the PR. I'm mainly just grumpy because the behavior changed so substantially in a near-silent update.

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

2 participants