-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Upgrade to pex 1.6.6. #7186
Upgrade to pex 1.6.6. #7186
Conversation
All these code changes look reasonable and I don't see an immediate reason why most CI shards would be failing. Something that does looks fishy to me though is that the Py2 blacklist shards are failing by complaining the command to build the wheel for Cryptography with Python 3 fails: https://travis-ci.org/pantsbuild/pants/jobs/486183205#L531. Note that we know this parent process is being run with Python 2 because it renders a unicode literal I doubt this is relevant, but one potential thing to sanity check is issues with OpenSSL leading to cryptography issues with Py3. See how we resolve this with OSX shards: https://github.com/pantsbuild/pants/blob/master/.travis.yml#L173. -- Beyond CI issues, I'm able to run this locally as expected. It also fixes the Python 3.7 Let me know if you'd like me to test anything specific locally. |
This appears to be deeper than CI env issues. Straight-up pex 1.6.1 fails the same way on my machine. For pex under python3.7:
And same for pex 1.6.1 under python2.7:
Works though prior to pex 1.6:
The issue then must have to do with the improved isolation in pex 1.6+; either the new more constrained |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't comment on what might be causing CI issues, but the code looks great, and I love the simplification of interpreter cache.
OK - The cryptography thing was a doozy. I'll need to fix this over in pex-tool/pex#661, release 1.6.2 and then return to this PR. |
b348e97
to
b350621
Compare
Both production code and a few tests assert that the stderr of pex execution is empty. Since pex now warns (to stderr) when setuptools is used but not declared as a dependency, we have some red. More fixes coming... |
5c4614e
to
0237fa9
Compare
Although I'd argue some to all of the stderr sensitivity in these tests is unwarranted, I've filed pex-tool/pex#677 to add a knob for pex runtime warning suppression. I'll do a small release with that and consume here. |
…erpreter constraints requested (#7285) ### Problem When running `./pants binary`, we do not consider the global interpreter constraints in passing them to the Pex builder. This resulted in a bug where the interpreter used to build a PEX in CI differed from the Python used to run the PEX. See pex-tool/pex#676. To resolve this issue, we need some way to specify the Pex builder must use the specific Python version 2.7.13 (UCS4). This blocks #7235. ### Solution Modify `PexBuilderWrapper` to use `compatibility_or_constrains()`. This will first try to get the compatibility constraints from the target itself, and then resort to using the PythonSetup subsystem's constraints (for the global instance, resolved from `pants.ini`, `PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS`, and `--interpreter-constraints`). ### Result Users of the function `add_interpreter_constraints_from()` will now add the global `--interpreter-constraints` to the Pex builder if the targets do not themselves specify a constraint. At the moment, this only impacts `./pants binary`, as `python_binary_create.py` is the only file which uses this function. Note this is still not a great solution. It would be better to kill `add_interpreter_constraint()` and `add_interpreter_constraints_from()` to instead automatically set the interpreter constraints from the targets' graph. This PR does not make that change to avoid scope creep. #### Skipped tests Due to pex-tool/pex#655, we must now skip several tests that now fail. PEX does not correctly OR interpreter constraints, so these tests complain that they cannot find an appropriate interpreter to satisfy `['CPython>=3.6,<4', 'CPython>=2.7,<3']`. Because this PR blocks #7235, which blocks #7197, we skip these tests until the upstream fix pex-tool/pex#678 is merged into PEX and Pants upgrades its PEX to the new version #7186.
Ran into pex-tool/pex#684 here - the warnings fix broke lambdex :/ |
c20a09d
to
1635569
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks John!
OK... the python 3.6 hangs are actually infinite pex re-exec loops encountered when executing pants.pex using an interpreter (our pyenv 3.6 shim) that is not on the PATH: pex-tool/pex#700 I'll cut a release with a fix for that and consume pex 1.6.6. |
d08fa8c
to
23dbe44
Compare
Using interpreter constraints to attempt to steer interpreter selection is causing alot of pain. We know the exact interpreter to use and should be able to tell pants this (you can tell pex this). Filed #7508 to make this better going forward though I doubt I'll hold up this PR for it. |
46ce1c6
to
108a4bb
Compare
This allows us to get rid of resolving setuptools and wheel in our interpreter cache. Fixes pantsbuild#6927
Rebased after breaking out independent changes. A CI burn is in progress and once green, I'll circle back and flesh out the PR description a bit to describe both major fixes / features brought by the pex upgrade that Pants will leverage / benefit from as well as describing the major updates to the Pants codebase in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this green! Thanks for all your work to land this, John.
Will review the PR description once that's ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just nits. Fire away!
This allows us to get rid of resolving setuptools and wheel in our
interpreter cache and generally adapts us to more modern hermetic pex
distributions.
In addition pants now:
Pex used to do this, but really only for Pants use cases. The code
was stripped from Pex and moved into PexBuilderWrapper.
By default, Pex warns about various unseemly characteristics of built
pexes discovered when bootstrapping their run-time. Pants now plumbs
both a global PythonSetup option and a local python_binary option to
control the pex warning behavior of pexes it builds.
Fixes #6927