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

Projects
None yet
3 participants
@Eric-Arellano
Copy link
Contributor

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'.

Eric-Arellano added some commits Mar 7, 2019

Revert "Always OR interpreter constraints when given list"
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.
Keep default AND for PP and PPP, else OR
It looks like we need to keep in some places the behavior of ANDing a list of constraints per #654 (comment). But in general we want to OR.

If PEX_PYTHON or PEX_PYTHON_PATH are not set, OR.

Eric-Arellano added some commits Mar 7, 2019

Restore Always OR interpreter constraints when given list
Per #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

@CMLivingston
Copy link
Contributor

left a comment

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.

Restore allowing all discoverable interpreters when no constraints
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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

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

@Eric-Arellano Eric-Arellano reopened this Mar 8, 2019

Eric-Arellano added some commits Mar 8, 2019

Fix last failing test
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.
Fix one last test
Py2 was fixed, but Py3 was failing for same reason that we were now ORing when what we want is AND.
@jsirois
Copy link
Member

left a comment

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:

pex/pex/bin/pex.py

Lines 314 to 323 in dbf92a9

group.add_option(
'--interpreter-constraint',
dest='interpreter_constraint',
default=[],
type='str',
action='append',
help='A constraint that determines the interpreter compatibility for '
'this pex, using the Requirement-style format, e.g. "CPython>=3", or ">=2.7" '
'for requirements agnostic to interpreter class. This option can be passed multiple '
'times.')

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

Eric-Arellano added some commits Mar 10, 2019

Fix other tests and add new test
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!
Restore use of sets in tests
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 pantsbuild:master Mar 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:or-constraints branch Mar 10, 2019

@jsirois jsirois referenced this pull request Mar 11, 2019

Merged

Prepare the 1.6.3 release. #682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.