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

Honor interpreter constraints even when PEX_PYTHON and PEX_PYTHON_PATH not set #668

Merged

Conversation

blorente
Copy link
Contributor

Solves issue #656, where if no PEX_PYTHON_PATH and no PEX_PYHON are configured, we still honour --interpreter-constraint flag when bootstrapping a PEX.

There is an integration test that worked only when applied the last commit, but there is a lot of nuance to running it that I'd like a hand with. In particular, I'd like to know if there's a way of:

  • Specifying "please ignore system-wide settings for PEX_PYTHON_PATH" in a test or Specifying
    "please override PEX_PYTHON_PATH with None". I think the first one should be easy to implement with a command-line flag defined in the same place as --rcfile.
  • Specifying "This test only runs on Python3.5", because I need two different interpreters to play with (one for the environment that the pex runs in, another one to pass to --interpreter-constraint).

@jsirois
Copy link
Member

jsirois commented Feb 11, 2019

@blorente for point #2 you want this:
https://github.com/pantsbuild/pex/blob/edc636ce6a004fa1bfc762d74f01c9fd3a862429/pex/testing.py#L324-L366

For point #1 you should just be able to run pex with an env=... that has what you want. And part of what you want is maybe:
https://github.com/pantsbuild/pex/blob/edc636ce6a004fa1bfc762d74f01c9fd3a862429/pex/variables.py#L322-L328

@blorente blorente force-pushed the blorente/656/interpreter_constraints branch from 47ea81a to d820a94 Compare February 15, 2019 15:18
@blorente
Copy link
Contributor Author

Depends on #673

@blorente blorente force-pushed the blorente/656/interpreter_constraints branch from 78e5ab7 to 8bdd0c3 Compare February 15, 2019 18:07
@blorente
Copy link
Contributor Author

For some reason I can repro failing test (and others) locally in master, even after blowing up ./.tox. I have no idea what's up or why they don't show up in other places. I will keep investigating further, but ideas are welcome.

@blorente
Copy link
Contributor Author

@jsirois, any idea why I can repro the failing tests in master? Meaning, how they made it past CI in the first place.

@blorente blorente force-pushed the blorente/656/interpreter_constraints branch from e1d90fb to 5297a21 Compare March 19, 2019 17:27
@blorente blorente requested a review from jsirois March 19, 2019 17:57
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.

Thank you Borja for working through this!

tests/test_integration.py Outdated Show resolved Hide resolved
tests/test_integration.py Show resolved Hide resolved
@Eric-Arellano
Copy link
Contributor

@jsirois or @CMLivingston a review of this would be quite helpful. While I'm more familiar with Pex than a few months ago, I'm still not aware of all the expectations and its history.

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.

LGTM - no issues that I can see.

@CMLivingston
Copy link
Contributor

CMLivingston commented Mar 20, 2019

@jsirois, any idea why I can repro the failing tests in master? Meaning, how they made it past CI in the first place.

In my experience, this happens because certain env conditions align locally that are not covered in CI, and the developer did not verify his/her changes by running tox locally before pushing the branch.

tests/test_integration.py Outdated Show resolved Hide resolved
tests/test_integration.py Outdated Show resolved Hide resolved
tests/test_integration.py Outdated Show resolved Hide resolved
@blorente blorente changed the title Interpreter constraints honored even with regular PATH Honor interpreter constraints even when PEX_PYTHON and PEX_PYTHON_PATH not set Mar 21, 2019
@blorente blorente merged commit 45ef407 into pex-tool:master Mar 21, 2019
@blorente blorente deleted the blorente/656/interpreter_constraints branch March 21, 2019 10:36
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.

4 participants