Skip to content

Commit

Permalink
attempt to refactor the pytest pex invocation to use the interpreter …
Browse files Browse the repository at this point in the history
…file path
  • Loading branch information
cosmicexplorer committed Apr 30, 2019
1 parent bd6fdb1 commit 9a68c3c
Showing 1 changed file with 14 additions and 8 deletions.
22 changes: 14 additions & 8 deletions src/python/pants/backend/python/tasks/pytest_run.py
Expand Up @@ -34,7 +34,7 @@
from pants.util.objects import datatype
from pants.util.process_handler import SubprocessProcessHandler
from pants.util.py2_compat import configparser
from pants.util.strutil import safe_shlex_split
from pants.util.strutil import create_path_env_var, safe_shlex_split
from pants.util.xml_parser import XmlParser


Expand Down Expand Up @@ -483,7 +483,7 @@ def _test_runner(self, workdirs, test_targets, sources_map):
pytest_binary.pex) as coverage_args:
yield pytest_binary, [conftest] + coverage_args, get_pytest_rootdir

def _constrain_pytest_interpreter_search_path(self):
def _constrain_pytest_interpreter_search_path(self, args):
"""Return an environment for invoking a pex which ensures the use of the selected interpreter.
When creating the merged pytest pex, we already have an interpreter, and we only invoke that pex
Expand All @@ -493,11 +493,15 @@ def _constrain_pytest_interpreter_search_path(self):
"""
pytest_prep_binary_product = self.context.products.get_data(PytestPrep.PytestBinary)
chosen_interpreter_binary_path = pytest_prep_binary_product.interpreter.binary
return {
'PEX_PYTHON': chosen_interpreter_binary_path,
env = {
'PEX_PYTHON_PATH': chosen_interpreter_binary_path,
'PEX_IGNORE_RCFILES': '1',
'PATH': create_path_env_var([os.path.dirname(chosen_interpreter_binary_path)],

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 30, 2019

Member

This is not a PR blocker, but investigating the commit you pointed to as your PEX_IGNORE_RCFILES=1 attempt, why did you set PATH? For an invocation like PEX_PYTHON_PATH=<python binary (1)> <python binary (2)> <pex> ... no PATH is needed to find a python interpreter since by this construction the sys.executable (2) matches PEX_PYTHON_PATH (1) short-circuiting any PATH lookup. In addition, since the executable in this construction is python and not pex, the pex shebang which may have something like #!/usr/bin/env python3.6 will no be run avoiding the other possible PATH lookup.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 30, 2019

Author Contributor

It didn't work differently at all by adding the PATH variable. I had added that out of the vague concern that something could be happening like in #7615 where python or pex was looking for an executable of some specific name, so we had to ensure the temporary rustup-init file was named rustup-init. You've confirmed that there isn't a PATH lookup we're missing here, so this part should have been removed -- I wanted to see if travis would treat this any differently.

env=os.environ, prepend=True)
}

return (env, [chosen_interpreter_binary_path] + args)

def _do_run_tests_with_args(self, pex, args):
try:
env = dict(os.environ)
Expand Down Expand Up @@ -534,8 +538,9 @@ def _do_run_tests_with_args(self, pex, args):
cmd=' '.join(pex.cmdline(args)),
labels=[WorkUnitLabel.TOOL, WorkUnitLabel.TEST]) as workunit:
# NB: Constrain the pex environment to ensure the use of the selected interpreter!
env.update(self._constrain_pytest_interpreter_search_path())
rc = self.spawn_and_wait(pex, workunit=workunit, args=args, setsid=True, env=env)
extra_env, new_cmdline = self._constrain_pytest_interpreter_search_path(args)
env.update(extra_env)
rc = self.spawn_and_wait(pex, workunit=workunit, args=new_cmdline, setsid=True, env=env)
return PytestResult.rc(rc)
except ErrorWhileTesting:
# spawn_and_wait wraps the test runner in a timeout, so it could
Expand Down Expand Up @@ -755,8 +760,9 @@ def _pex_run(self, pex, workunit_name, args, env):
cmd=' '.join(pex.cmdline(args)),
labels=[WorkUnitLabel.TOOL, WorkUnitLabel.TEST]) as workunit:
# NB: Constrain the pex environment to ensure the use of the selected interpreter!
env.update(self._constrain_pytest_interpreter_search_path())
process = self._spawn(pex, workunit, args, setsid=False, env=env)
extra_env, new_cmdline = self._constrain_pytest_interpreter_search_path(args)
env.update(extra_env)
process = self._spawn(pex, workunit, new_cmdline, setsid=False, env=env)
return process.wait()

@contextmanager
Expand Down

0 comments on commit 9a68c3c

Please sign in to comment.