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

Fixup PEXEnvironment extras resolution. #617

Merged
merged 1 commit into from
Nov 4, 2018

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Nov 3, 2018

In #582 and #592 support for environment markers was added to the pex
runtime. In so doing, the resolver was fixed to record full requirement
strings into PEX-INFO. Since part of those full requirement strings
could now include environment markers that selected for active extras,
a bug was introduced since we did not also add the active extras to the
environment marker evaluation environment.

This change adds a failing test that is fixed by properly setting up the
environment marker environment to include active extras.

Fixes #615.

@jsirois
Copy link
Member Author

jsirois commented Nov 3, 2018

NB: This was tested locally to resolve the specific case presented in #615, but that was heavier weight than need be in a regression test; so I dogfooded pex[requests].

In pex-tool#582 and pex-tool#592 support for environment markers was added to the pex
runtime. In so doing, the resolver was fixed to record full requirement
strings into PEX-INFO. Since part of those full requirement strings
could now include environment markers that selected for active extras,
a bug was introduced since we did not also add the active extras to the
environment marker evaluation environment.

This change adds a failing test that is fixed by properly setting up the
environment marker environment to include active extras.

Fixes pex-tool#615.
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Lgtm. Some fancy itertools going on, but still very readable and appropriate.

Thanks John!

@jsirois jsirois merged commit e53fadc into pex-tool:master Nov 4, 2018
@jsirois jsirois deleted the issues/615 branch November 4, 2018 00:39
@kwlzn kwlzn mentioned this pull request Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants