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

OR interpreter constraints when multiple given #678

Merged
merged 12 commits into from
Mar 10, 2019

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Mar 7, 2019

Problem

Resolves #655.

We would expect a list of constraints, such as ['CPython>=2.7,<3', 'CPython>=3.4'], to OR the separate list entries, i.e. allow an interpreter that satisfies any of these distinct requirement strings. Instead, we always AND lists of constraints.

Solution

Always OR a list of interpreter constraints. If the user wants AND, they may do so within the single requirement string via ,, e.g. 'CPython>=2.7,<3'.

This reverts commit f058d1b.

Actually it looks like we need to keep meet_all_constraints for the sake of PEX_PYTHON_PATH and PEX_PYTHON.
It looks like we need to keep in some places the behavior of ANDing a list of constraints per pex-tool#654 (comment). But in general we want to OR.

If PEX_PYTHON or PEX_PYTHON_PATH are not set, OR.
Per pex-tool#655 (comment), Chris is okay with always ORing. This is a much more consistent and simpler approach, so is ideal over the earlier conditional logic.
@Eric-Arellano Eric-Arellano changed the title WIP: OR interpreter constraints when PEX_PYTHON or PEX_PYTHON_PATH not set OR interpreter constraints when multiple given Mar 7, 2019
Copy link
Contributor

@CMLivingston CMLivingston left a comment

Choose a reason for hiding this comment

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

This change is fine to make - it will not adversely impact Twitter code. However, is there anything we should do to deprecated this @jsirois? I don't think we need to do anything special but a fair warning might be in order.

Several tests were failing that they could not find a compatibile interpreter for the empty constraints [].

We did not originally hit this error because the builtin all() will return True if the iterable is empty. So, we were calling all(matches(filt) for filt in []), which returned True. Now that we use any(), it would return False.

Instead, we should only call matched_interpreters() if there are any constraints to begin with.
@Eric-Arellano
Copy link
Contributor Author

pantsbuild/pants@4d63f8a closed this when it shouldn't have.

@Eric-Arellano Eric-Arellano reopened this Mar 8, 2019
The test was passing multiple interpreter constraints which now get ORed rather than the intended AND. Instead, use one string with a `,` in order to AND as intended.
Py2 was fixed, but Py3 was failing for same reason that we were now ORing when what we want is AND.
Copy link
Member

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

This looks good to me, but It seems like a good time to add a test that the ORing works.

To @CMLivingston's point, this is CLI-API breaking. The practical break out there will be folks using multiple flags in place of one flag and the comma operator. Since PEX has no deprecation policy yet (#584), I'm fine with just fixing the existing CLI-flag docs:
https://github.com/pantsbuild/pex/blob/dbf92a9f1359d280632bb49f2c30508c2c35369a/pex/bin/pex.py#L314-L323
An decent example is here in pants:
https://github.com/pantsbuild/pants/blob/9873b1412b7e343142353b57357dae09895386d2/src/python/pants/backend/python/subsystems/python_setup.py#L29-L35

Two more tests were using constraints to AND still, and I didn't catch them until searching "constraint" in the test file. Should be good now.

Add a new test to ensure that multiple constraints OR as expected.

Finally, stop using sets because they're non-deterministic!
While sets indeed are non-deterministic when used for things like iteration, checking the equality of two sets is always deterministic! I forgot about this.

Restore sets to keep with original code more and because they're more appropriate here than sorted lists.
@Eric-Arellano Eric-Arellano merged commit f9327f9 into pex-tool:master Mar 10, 2019
@Eric-Arellano Eric-Arellano deleted the or-constraints branch March 10, 2019 18:40
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.

3 participants