-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
Add interpreter constraints option and use constraints to search for compatible interpreters at exec time #427
Add interpreter constraints option and use constraints to search for compatible interpreters at exec time #427
Conversation
…NFO; filter interpreters used to build/run the pex based on these constraints.
…d add a unit test for new method
…raints matching and search api for pex bootstrap re-exec; add associated unit + integration tests
pex/interpreter_constraints.py
Outdated
@@ -0,0 +1,58 @@ | |||
# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md). |
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.
This new library is extent of the intersecting interpreter constraints logic that makes sense to move out of Pants and into Pex. It will be cleanly consumable by Pants and the only modification I made is to add a meet_all_constraints kwarg for meeting all filters passed instead of at least one, which is backward compatible with the Pants use cases.
I operated on the assumption that the other methods of this lib have been tested or are functioning properly so I did not add any new tests for them but I am more than happy to.
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.
Should this get bumped to 2017?
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.
✅
…ific installations
…eter selection tests green; choose highest compatible interpreter in pex python path instead of lowest.
…ston/pex into clivingston/add-pex-python-path
pex/interpreter_constraints.py
Outdated
from .interpreter import PythonIdentity | ||
|
||
|
||
def _matches(interpreter, filters, meet_all_constraints=False): |
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.
I added meet_all_constraints because for some reason the Pants usage of this code (currently in Pants codebase) only requires a match against one of the filters to be considered a "matching interpreter".
…le, but maintain support for pex_python if still set. Add relevant integration tests to test each of the test cases that deal with interpreter constraints.
5c435f1
to
233918e
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.
initial pass through the sources - will circle back to review tests.
pex/bin/pex.py
Outdated
if options.interpreter_constraint: | ||
constraints = options.interpreter_constraint | ||
# TODO: add check to see if constraints are mutually exclusive (bad) so no time is wasted | ||
check_requirements_are_well_formed(constraints) |
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.
how about calling this validate_constraints()
and moving the TODO here into it's function definition?
pex/bin/pex.py
Outdated
# TODO: add check to see if constraints are mutually exclusive (bad) so no time is wasted | ||
check_requirements_are_well_formed(constraints) | ||
interpreters = list(matched_interpreters(interpreters, constraints, meet_all_constraints=True)) | ||
|
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.
seems to me like in the case where no --python
args are explicitly passed, that interpreters
here pre-constraint filtration will only contain the currently executing interpreter which is probably insufficient for any users targeting e.g. python3 with constraints while executing pex with python2.
this would mean that anytime a user passes --constraint
they'd also need to pass one or more --python
args to surface the interpreters to actually apply the constraints to - which feels awkward.
I think we'll probably need to:
- make
--python
mutually exclusive with--constraints
- or at least warn when both are passed. - fall back to path or PPP search when
--constraints
is passed without--python
. - potentially deprecate
--python
in favor of--constraints
, if it makes sense.
this will pave a way for a future where both build and runtime interpreter selection behave identically, afaict.
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.
I like the idea of a combination of 1 and 2. Disallow both options as using both seems like an anti-pattern and fall back to PPP/PATH when --constraints is passed without --python.
pex/bin/pex.py
Outdated
@@ -535,7 +545,8 @@ def build_pex(args, options, resolver_option_builder): | |||
# options.preamble_file is None | |||
preamble = None | |||
|
|||
interpreter = _lowest_version_interpreter(interpreters) | |||
# TODO: make whether to take min or max version interpreter configurable | |||
interpreter = min(interpreters) |
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.
this TODO might be better as a github issue link instead?
pex/interpreter_constraints.py
Outdated
if meet_all_constraints: | ||
return all(interpreter.identity.matches(filt) for filt in filters) | ||
else: | ||
return any(interpreter.identity.matches(filt) for filt in filters) |
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.
how about parameterizing the function applied here to enforce DRY?
check = all if meet_all_constraints else any
return check(interpreter.identity.matches(filt) for filt in filters)
pex/interpreter_constraints.py
Outdated
try: | ||
PythonIdentity.parse_requirement(req) | ||
except ValueError as e: | ||
from .common import die |
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.
this import should move to the top of the file
pex/testing.py
Outdated
|
||
def bootstrap_python_installer(): | ||
install_location = os.getcwd() + '/.pyenv_test' | ||
if not os.path.exists(install_location) or not os.path.exists(install_location + '/bin/pyenv'): |
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.
ditto.
pex/testing.py
Outdated
install_location = os.getcwd() + '/.pyenv_test/versions/' + version | ||
if not os.path.exists(install_location): | ||
os.environ['PYENV_ROOT'] = os.getcwd() + '/.pyenv_test' | ||
subprocess.call([os.getcwd() + '/.pyenv_test/bin/pyenv', 'install', version]) |
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.
os.path.join
throughout this function.
pex/variables.py
Outdated
def PEX_PYTHON_PATH(self): | ||
"""String | ||
|
||
A colon-seperated string containing paths to binaires of blessed Python interpreters |
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.
binaries
pex/variables.py
Outdated
of a second execution of maybe_reexec_pex if neither PEX_PYTHON or PEX_PYTHON_PATH are set but | ||
interpreter constraints are specified. | ||
""" | ||
value = self._environ.get('SHOULD_EXIT_REEXEC', 'False') |
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.
would expect the environ var to map 1:1 to the property name here.
pex/variables.py
Outdated
interpreter constraints are specified. | ||
""" | ||
value = self._environ.get('SHOULD_EXIT_REEXEC', 'False') | ||
return False if value == 'False' else True |
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.
would be better to not rely on the string value here as anything other than indicator of truthyness, i.e.:
return bool(self._environ.get(..., False))
noting that this also seems to be failing on the |
5877b3c
to
851cc53
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.
looks good overall, a couple of cleanup suggestions
pex/pex_bootstrapper.py
Outdated
"""Find all compatible interpreters on the system within the supplied constraints and use | ||
PEX_PYTHON_PATH env variable if it is set. If not, fall back to interpreters on $PATH. | ||
""" | ||
if pex_python_path: |
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.
same codes as pex/bin/pex.py? If so, we should extract this to a library.
pex/interpreter_constraints.py
Outdated
:param meet_all_constraints: whether to match against all filters. | ||
Defaults to matching interpreters that match at least one filter. | ||
""" | ||
for match in _matching(interpreters, constraints, meet_all_constraints): |
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.
Seems like we can simplify to
def matched_interpreters(interpreters, constraints, meet_all_constraints=False):
_matching(interpreters, constraints, meet_all_constraints=meet_all_constraints)
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.
you'd need to return
the generator, but it'd probably be simpler to just fold _matching()
directly into this method.
96af91b
to
b5a86ea
Compare
…x bootstrapper to be cleaner and simpler
pex/variables.py
Outdated
"""Read pex runtime configuration variables from a pexrc file. | ||
|
||
:param rc: an absolute path to a pexrc file | ||
:return ret_vars: a dict object containing key values pairs found in processed pexrc files |
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.
these and docstring continuations should indent to align with the opening """
, e.g.
"""The docstring summary which might
spill onto a new line.
Extended summary would go here.
:param etc: Would go here.
"""
…ular testing for the interpreter selection method in pex bootstrapper
pex/variables.py
Outdated
os.path.join(os.path.dirname(sys.argv[0]), '.pexrc'), | ||
os.getcwd()] | ||
if not rc: | ||
rc_locations.append('~/.pexrc') |
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.
The only time ~/.pexrc
is the default is if we are calling the Variables constructor; the from_rc
api requires a string so I added support for adding ~/.pexrc
if the input is blank. I suppose I could make it a default arg or pass it in PEX CLI...
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.
Added it as default arg.
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.
seems to me like ~/.pexrc
needs to be considered in all cases (i.e. as a static member of rc_locations
) or else we aren't reading from all of the places that pexrc configuration can exist. (essentially, I don't see any existing supporting usage of Variables
in pex/pants to warrant keeping the rc
param here the way it is).
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.
looking great - handful of remaining feedback and a larger question on whether PEX_PYTHON
should still be supported at all.
pex/bin/pex.py
Outdated
@@ -525,6 +530,16 @@ def build_pex(args, options, resolver_option_builder): | |||
for interpreter in options.python or [None] | |||
] | |||
|
|||
if options.interpreter_constraint: | |||
# Overwrite the current interpreter as defined by sys.executable. |
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.
this comment doesn't make sense here.
pex/interpreter_constraints.py
Outdated
def _matching(interpreters, constraints, meet_all_constraints=False): | ||
for interpreter in interpreters: | ||
check = all if meet_all_constraints else any | ||
if check(interpreter.identity.matches(filt) for filt in constraints): |
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.
check
should be defined outside of the for
body
PythonIdentity.parse_requirement(req) | ||
except ValueError as e: | ||
die("Compatibility requirements are not formatted properly: %s" % str(e)) | ||
|
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.
specify the requirement that failed to parse's string.
pex/pex_bootstrapper.py
Outdated
if not target: | ||
die('Failed to find interpreter specified by PEX_PYTHON: %s' % target) | ||
elif compatibility_constraints: |
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.
s/elif/if
- these all act as guard clauses.
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.
the e
string contains this requirement.
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.
_matching
got folded into the matched_interpreters
method so I will remove the method.
pex/pex_bootstrapper.py
Outdated
with TRACER.timed('Selecting runtime interpreter based on pexrc', V=3): | ||
if ENV.PEX_PYTHON and not ENV.PEX_PYTHON_PATH: | ||
# preserve PEX_PYTHON re-exec for backwards compatibility | ||
selected_interpreter = _select_pex_python_interpreter(ENV.PEX_PYTHON, |
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.
I know we discussed this earlier - but what was the ultimate reasoning here to leave PEX_PYTHON
functionality as a dual path thing alongside PPP
?
I was sort of thinking we could yank PEX_PYTHON
in favor of PPP
and cut a new minor version of pex w/ doc'd release notes as a cutover. it was the configuration (e.g. /etc/pexrc) that we'd need to keep there throughout the lifetime of old pex runtimes - but afaict, we can yank this in pex itself and simplify things. thoughts?
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.
I agree about the simplification. We discussed leaving it in as a way to give us flexibility when migrating the /etc/pexrc configs in prod and in MDE, as well as provide backwards interoperability for pex users who want to continue using a single python interpreter. However, IIRC, I think the main reason was to give us flexibility with the whole migration of /etc/pexrc and to give us a safety net in case we find that PEX_PYTHON_PATH ends up disastrously breaking things or causing other unexpected problems.
I like the idea of a safety net and backwards compatibility (at least for now), and I think it wouldn't be a bad idea to keep it in for now until we have success using PPP internally, maybe with a reasonable subset of Python users successfully running and deploying py3 PEX both locally and in prod? What do you think? @UnrememberMe any thoughts here?
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.
works for me. in that case, would be good to link to a ticket here that covers the eventual removal of PEX_PYTHON
.
pex/pex_builder.py
Outdated
if interpreter: | ||
self._pex_info = pex_info or PexInfo.default(interpreter) | ||
else: | ||
self._pex_info = pex_info or PexInfo.default() |
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.
this could be simplified to:
self._pex_info = pex_info or PexInfo.default(interpreter)
given interpreter
is None
as the default here.
pex/variables.py
Outdated
os.path.join(os.path.dirname(sys.argv[0]), '.pexrc'), | ||
os.getcwd()] | ||
if not rc: | ||
rc_locations.append('~/.pexrc') |
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.
seems to me like ~/.pexrc
needs to be considered in all cases (i.e. as a static member of rc_locations
) or else we aren't reading from all of the places that pexrc configuration can exist. (essentially, I don't see any existing supporting usage of Variables
in pex/pants to warrant keeping the rc
param here the way it is).
pex/variables.py
Outdated
rc_locations = ['/etc/pexrc', | ||
rc, | ||
os.path.join(os.path.dirname(sys.argv[0]), '.pexrc'), | ||
os.getcwd()] |
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.
afaict, this should reference an rcfile, not the current working dir directly?
but maybe it's not actually needed?
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.
I think it is needed for the case where the pex you are invoking and the .pexrc is in the cwd.
Also, idk why the pi = interpreter or PythonInterpreter.get()
line was not already changed to that, I am not sure why this reverted, but I will re-add, sorry about that.
pex/bin/pex.py
Outdated
# affect usages of the interpreter(s) specified by the "--python" command line flag. | ||
constraints = options.interpreter_constraint | ||
validate_constraints(constraints) | ||
rc_variables = Variables.from_rc(build_time_rc_dir=os.path.dirname(options.pex_name)) |
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.
its still puzzling to me why we need to look at a .pexrc
relative to the output pex name for PPP
configuration at build time.. can you clarify that with a comment here?
if this is purely for tests, we should probably re-evaluate.
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.
The use case is when the output pex is not in cwd, i.e. -o dir/output.pex
. In that case, we should be supporting the case where the .pexrc is in dir
as per a precedent set in the tests, however I see this being practical beyond testing situations.
Previously, the check was for os.path.dirname(sys.argv[0]) to find this .pexrc in the same dir as the output pex at runtime, which works fine because sys.argv[0] is the pex itself, however, at build time, sys.argv[0] is the pex cli binary, which can be different than the dir of the output pex i.e. pex requests -o dir/output.pex
. In this case, where I have .pexrc in dir
, special accommodations are needed to make sure that the pex build routine can find that pexrc at build time.
Like you say, this precedent is set largely by the tests, however having a pexrc in the same location as the pex itself seems like a reasonable case to cover to me.
pex/variables.py
Outdated
def PEX_PYTHON_PATH(self): | ||
"""String | ||
|
||
A colon-separated string containing binaires of blessed Python interpreters |
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.
s/binaires/paths/
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.
I will remove rc
.
…Cleanup docs and refactor a few one-liners.
@@ -289,6 +291,25 @@ def configure_clp_pex_environment(parser): | |||
'can be passed multiple times to create a multi-interpreter compatible pex. ' | |||
'Default: Use current interpreter.') | |||
|
|||
group.add_option( | |||
'--interpreter-constraint', | |||
dest='interpreter_constraint', |
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.
IIRC, a-b-c will be automatically convert to a_b_c for variable names, so you don't necessarily need to specify dest here.
'times.') | ||
|
||
group.add_option( | ||
'--rcfile', |
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.
suggestion: for consistency, change option to --rc-file
PythonIdentity.parse_requirement(req) | ||
except ValueError as e: | ||
die("Compatibility requirements are not formatted properly: %s" % str(e)) | ||
|
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.
second. Including the failed string will help user to debug.
'PEX_PYTHON_PATH="%s", COMPATIBILITY_CONSTRAINTS="%s"' | ||
% (cmdline, sys.executable, ENV.PEX_PYTHON, ENV.PEX_PYTHON_PATH, | ||
compatibility_constraints)) | ||
os.execve(selected_interpreter, cmdline, ENV.copy()) |
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.
won't this become MyPython MyPython original_params
since on line 152, cmdline has already prepended selected_interpreter?
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.
the first arg of cmdline
is ignored in the os.execve API (not sure why exactly, but it is).
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 for the answer. Ship it!
pex/interpreter_constraints.py
Outdated
|
||
|
||
def validate_constraints(constraints): | ||
# TODO: add check to see if constraints are mutually exclusive (bad) so no time is wasted |
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.
Did this TODO exist already when you moved it? If not, can we start an issue for it to track?
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.
✅
pex, using the Requirement-style format, e.g. ``'CPython>=3', or just ['>=2.7','<3']`` | ||
for requirements agnostic to interpreter class. | ||
:param meet_all_constraints: whether to match against all filters. | ||
Defaults to matching interpreters that match at least one filter. |
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.
Should we add a line to this docstring for what exactly we're yielding?
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.
✅
with TRACER.timed('Selecting runtime interpreter based on pexrc', 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start an issue for this TODO?
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.
It is on the line below this one.
@@ -123,6 +123,8 @@ def __init__(self, info=None): | |||
'%s of type %s' % (info, type(info))) | |||
self._pex_info = info or {} | |||
self._distributions = self._pex_info.get('distributions', {}) | |||
# cast as set because pex info from json must store interpreter_constraints as a list |
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.
Not sure I understand this comment, or maybe it's a typo. You're casting it as a set, but interpreter constraints needs to be stored as a list?
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.
Reading in a pex info using from_pex
or from_json
will instantiate a PexInfo object with json loaded as a dict. Interpreter constraints get serialized to json as a list b.c. json does not support Python sets, so casting it here is to cover the case where we read in a populated pex-info from json (which will be a list), however we want to store it internally as a set.
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.
Looks good, just some minor nits and questions.
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.
lgtm, one last round of minor stuff.
pex/pex_bootstrapper.py
Outdated
with TRACER.timed('Selecting runtime interpreter based on pexrc', V=3): | ||
if ENV.PEX_PYTHON and not ENV.PEX_PYTHON_PATH: | ||
# preserve PEX_PYTHON re-exec for backwards compatibility | ||
selected_interpreter = _select_pex_python_interpreter(ENV.PEX_PYTHON, |
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.
works for me. in that case, would be good to link to a ticket here that covers the eventual removal of PEX_PYTHON
.
pex/bin/pex.py
Outdated
default=None, | ||
help='A path to a pexrc file. NOTE: this flag is for testing purposes only. It is to ' | ||
'be used in the case that a pexrc lives in the same directory as the created ' | ||
'output pex as is the case with the pex integration tests.') |
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.
how about shortening this to:
An additional path to a pexrc file to read during configuration parsing. Used primarily for testing.
pex/bin/pex.py
Outdated
pexrc = None | ||
if options.rc_file: | ||
pexrc = options.rc_file | ||
rc_variables = Variables.from_rc(rc=pexrc) |
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.
given options.rc_file
defaults to None
, can reduce this to:
rc_variables = Variables.from_rc(rc=options.rc_file)
pex/pex_bootstrapper.py
Outdated
try: | ||
interpreters.append(PythonInterpreter.from_binary(binary)) | ||
except Executor.ExecutionError: | ||
TRACER.log("Python interpreter %s in PEX_PYTHON_PATH failed to load properly." % binary) |
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.
this should probably get logged as a hard warning (i.e. print(msg, file=sys.stderr)
) vs masked in tracer logging only.
pex/variables.py
Outdated
"""Read pex runtime configuration variables from a pexrc file. | ||
|
||
:param rc: an absolute path to a pexrc file. | ||
:return ret_vars: a dict object containing key values pairs found in processed pexrc files |
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.
specifying an arg name for a return doesn't make sense.
should look like:
:return: A dict of key value pairs found in processed pexrc files.
:rtype: dict
pex/variables.py
Outdated
rc_locations = ['/etc/pexrc', | ||
'~/.pexrc', | ||
os.path.join(os.path.dirname(sys.argv[0]), '.pexrc'), | ||
os.path.join(os.getcwd(), '.pexrc')] |
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.
did the cwd variant here actually end up being needed given the new --rcfile
flag? if so, why?
seems to me like tests were passing without it before?
tests/test_integration.py
Outdated
with temporary_dir() as td: | ||
pexrc_path = os.path.join(td, '.pexrc') | ||
with open(pexrc_path, 'w') as pexrc: | ||
pex_python = os.getcwd() + '/.pyenv_test/versions/3.6.3/bin/python3.6' |
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.
this should get the path from ensure_python_interpreter
vs hardcoding.
tests/test_integration.py
Outdated
# test PEX_PYTHON with incompatible constraints | ||
pexrc_path = os.path.join(td, '.pexrc') | ||
with open(pexrc_path, 'w') as pexrc: | ||
pex_python = os.getcwd() + '/.pyenv_test/versions/2.7.10/bin/python2.7' |
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.
this should get the path from ensure_python_interpreter
vs hardcoding.
tests/test_variables.py
Outdated
@@ -84,27 +85,30 @@ def test_pex_get_kv(): | |||
|
|||
|
|||
def test_pex_from_rc(): | |||
with named_temporary_file(mode='w') as pexrc: | |||
with open('.pexrc', 'w') as pexrc: |
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.
I think the named_temporary_file
before was slightly safer?
Fix failing tests in master that have gone undetected (as a result of bad PATH setting in #427).
Problem:
PEX does not support interpreter compatibility ranges for selecting an interpreter to build/run a PEX with. Twitter is aiming at killing off PEX_PYTHON overriding and replacing with a PEX_PYTHON_PATH in /etc/pexrc, which will contain a path to directories containing canonical interpreters for searching against at pex bootstrap re-exec time. Interpreter constraints on a PEX should inform the searching of PEX_PYTHON_PATH (if present in the env) and error out if no compatible interpreter can be found, replacing the searching of $PATH as a fallback.
Solution:
Add an interpreter constraints option to pex CLI and write these constraints to PEX-INFO metadata. Add an env variable to variables.py to support the setting of PEX_PYTHON_PATH. When the bootstrapper pex re-execs at runtime, read constraints from PEX-INFO and attempt to resolve an interpreter to exec with from the PPP. If no interpreter can be found in PPP, error out indicating that no match can be found.
Result:
PEXes will be able to be built and executed with specified interpreter constraints. Users will be able to define and control interpreter selection in managed development environments by setting the PEX_PYTHON_PATH so that PEXes are executed with supported interpreters only.