Fixup pex re-exec during bootstrap. #741
Conversation
|
NB: Integration tests are forthcoming, but the production code changes are reviewable. |
|
Now fully reviewable. |
| [sys.executable, test_pex, '-c', 'import json, os; print(json.dumps(os.environ.copy()))'], | ||
| env=env | ||
| ) | ||
| final_env = json.loads(output.decode('utf-8')) |
There was a problem hiding this comment.
Clever way to test this. Good idea.
| @@ -75,9 +75,6 @@ def __init__(self, environ=None, rc=None, use_defaults=True): | |||
| def copy(self): | |||
| return self._environ.copy() | |||
|
|
|||
| def delete(self, variable): | |||
Ensure `PEX_PYTHON` and `PEX_PYTHON_PATH` are actually scrubbed from the environment of the pex process when re-exec'd fixing pex-tool#710. Also uniformize re-exec interpreter selection to ensure that the current interpreter is preferred when it meets any constraints fixing pex-tool#709. Fixes pex-tool#709 Fixes pex-tool#710
Use explicit return, pass narrower types and improve docs & logging.
| @@ -116,12 +116,12 @@ def maybe_reexec_pex(compatibility_constraints=None): | |||
|
|
|||
| current_interpreter = PythonInterpreter.get() | |||
|
|
|||
| # NB: Used only for debugging and tests. | |||
| pex_exec_chain = [] | |||
| # NB: Used only for tests. | |||
| if '_PEX_EXEC_CHAIN' in os.environ: | |||
There was a problem hiding this comment.
I feel slightly less dirty with this tweak. Now user code will never see _PEX_EXEC_CHAIN in the environment in production.
There was a problem hiding this comment.
Was this used somehow before? Confused about the "now" verbage.
There was a problem hiding this comment.
Before 4efafc2 the _PEX_EXEC_CHAIN environment variable always leaked to the environment of user code - they could see it and read it. Now (with the fix in 4efafc2), the environment variable is never exposed by the pex runtime. It must 1st be present in the enviornment to be mutated. IE: Only if you run _PEX_EXEC_CHAIN=1 ./my.pex will the variable be mutated to populate it with the interpreter invocation chain. Tests do this, users won't know to do this.
| # We've already been here and selected an interpreter. Continue to execution. | ||
| return | ||
|
|
||
| target = None | ||
| with TRACER.timed('Selecting runtime interpreter based on pexrc', V=3): | ||
| with TRACER.timed('Selecting runtime interpreter', V=3): | ||
| if ENV.PEX_PYTHON and not ENV.PEX_PYTHON_PATH: | ||
| # preserve PEX_PYTHON re-exec for backwards compatibility | ||
| # TODO: Kill this off completely in favor of PEX_PYTHON_PATH |
There was a problem hiding this comment.
This something that should probably happen soon. Not sure if we have a deprecation policy around this but it is redundant with PPP and more complexity to support than necessary.
There was a problem hiding this comment.
I think the bit needed to kick off deprecation / removal is expanding "it's redundant" in an issue. FWICT it is not simply redundant. Today I can PEX_PYTHON=python2 ... to ensure a pex runs under some python2 interpreter on any machine. I can't do that with PEX_PYTHON_PATH since its entries are full paths or path prefixes, both of which are machine specific.
| Handle environment overrides for the Python interpreter to use when executing this pex. | ||
|
|
||
| def maybe_reexec_pex(compatibility_constraints=None): | ||
| """Handle environment overrides for the Python interpreter to use when executing this pex. |
There was a problem hiding this comment.
Is this a style change or does it change any presentations from help/pydocs, etc? Just curious.
There was a problem hiding this comment.
Just a style change - I wanted to make it clear None was an acceptable input. The general implication of def maybe_reexec_pex(compatibility_constraints) is that compatibility_constraints are required.
Ensure
PEX_PYTHONandPEX_PYTHON_PATHare actually scrubbed from theenvironment of the pex process when re-exec'd fixing #710. Also
uniformize re-exec interpreter selection to ensure that the current
interpreter is preferred when it meets any constraints fixing #709.
Fixes #709
Fixes #710