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

Add interpreter constraints option and use constraints to search for compatible interpreters at exec time #427

Merged
merged 44 commits into from
Dec 2, 2017

Conversation

CMLivingston
Copy link
Contributor

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.

Chris Livingston added 4 commits October 20, 2017 12:12
…NFO; filter interpreters used to build/run the pex based on these constraints.
…raints matching and search api for pex bootstrap re-exec; add associated unit + integration tests
@@ -0,0 +1,58 @@
# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
Copy link
Contributor Author

@CMLivingston CMLivingston Oct 23, 2017

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.

Copy link

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CMLivingston CMLivingston changed the title [WIP] Add interpreter constraints option and use constraints to search for compatible interpreters at exec time Add interpreter constraints option and use constraints to search for compatible interpreters at exec time Oct 23, 2017
from .interpreter import PythonIdentity


def _matches(interpreter, filters, meet_all_constraints=False):
Copy link
Contributor Author

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.
@CMLivingston CMLivingston force-pushed the clivingston/add-pex-python-path branch from 5c435f1 to 233918e Compare November 8, 2017 20:11
Copy link
Contributor

@kwlzn kwlzn left a 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)
Copy link
Contributor

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))

Copy link
Contributor

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:

  1. make --python mutually exclusive with --constraints - or at least warn when both are passed.
  2. fall back to path or PPP search when --constraints is passed without --python.
  3. 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.

Copy link
Contributor Author

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)
Copy link
Contributor

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?

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)
Copy link
Contributor

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)

try:
PythonIdentity.parse_requirement(req)
except ValueError as e:
from .common import die
Copy link
Contributor

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'):
Copy link
Contributor

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])
Copy link
Contributor

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
Copy link
Contributor

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')
Copy link
Contributor

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
Copy link
Contributor

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))

@kwlzn
Copy link
Contributor

kwlzn commented Nov 15, 2017

noting that this also seems to be failing on the pypy shard after a few restarts.

@CMLivingston CMLivingston force-pushed the clivingston/add-pex-python-path branch 2 times, most recently from 5877b3c to 851cc53 Compare November 17, 2017 18:35
Copy link

@UnrememberMe UnrememberMe left a 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

"""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:

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.

: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):

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)

Copy link
Contributor

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.

@CMLivingston CMLivingston force-pushed the clivingston/add-pex-python-path branch from 96af91b to b5a86ea Compare November 20, 2017 21:42
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
Copy link
Contributor

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')
Copy link
Contributor Author

@CMLivingston CMLivingston Nov 28, 2017

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

@kwlzn kwlzn left a 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.
Copy link
Contributor

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.

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):
Copy link
Contributor

@kwlzn kwlzn Nov 29, 2017

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))

Copy link
Contributor

@kwlzn kwlzn Nov 29, 2017

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.

if not target:
die('Failed to find interpreter specified by PEX_PYTHON: %s' % target)
elif compatibility_constraints:
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

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,
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

if interpreter:
self._pex_info = pex_info or PexInfo.default(interpreter)
else:
self._pex_info = pex_info or PexInfo.default()
Copy link
Contributor

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')
Copy link
Contributor

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()]
Copy link
Contributor

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?

Copy link
Contributor Author

@CMLivingston CMLivingston Nov 29, 2017

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))
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/binaires/paths/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove rc.

@@ -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',

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

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))

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())

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?

Copy link
Contributor Author

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

Copy link

@UnrememberMe UnrememberMe left a 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!



def validate_constraints(constraints):
# TODO: add check to see if constraints are mutually exclusive (bad) so no time is wasted
Copy link

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?

Copy link
Contributor Author

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.
Copy link

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?

Copy link
Contributor Author

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
Copy link

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?

Copy link
Contributor Author

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
Copy link

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?

Copy link
Contributor Author

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.

Copy link

@lgvital lgvital left a 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.

Copy link
Contributor

@kwlzn kwlzn left a 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.

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,
Copy link
Contributor

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.')
Copy link
Contributor

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)
Copy link
Contributor

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)

try:
interpreters.append(PythonInterpreter.from_binary(binary))
except Executor.ExecutionError:
TRACER.log("Python interpreter %s in PEX_PYTHON_PATH failed to load properly." % binary)
Copy link
Contributor

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
Copy link
Contributor

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')]
Copy link
Contributor

@kwlzn kwlzn Dec 1, 2017

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?

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'
Copy link
Contributor

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.

# 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'
Copy link
Contributor

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.

@@ -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:
Copy link
Contributor

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?

@kwlzn kwlzn merged commit df326ff into pex-tool:master Dec 2, 2017
kwlzn pushed a commit that referenced this pull request May 12, 2018
Fix failing tests in master that have gone undetected (as a result of bad PATH setting in #427).
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