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

satisfy python_dist setup_requires with a pex to resolve transitive deps, and some other unrelated native toolchain changes #6275

Merged
merged 25 commits into from Sep 4, 2018

Conversation

Projects
None yet
2 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Jul 30, 2018

Problem

This PR solves two separate problems at once. It should have been split into two separate PRs earlier, but has undergone significant review and it would be a significant amount of unnecessary effort to do that at this point.

Problem 1

The setup_requires kwarg of python_dist() holds addresses for python_requirement_library() targets. Currently, these requirements are fetched into a directory which is added to PYTHONPATH. This currently produces an invalid set of packages (for reasons I'm not quite sure of), which can cause errors on import statements within the setup.py.

Problem 2

See #6273 for more context on this second issue: distutils does all sorts of things with our environment variables (e.g. adding CFLAGS to LDFLAGS), so until #6273 is merged, trying to modify anything but the PATH in the environment in which we invoke setup.py in for python_dist() targets causes lots of failures in many scenarios.

Solution

Solution 1:

  • Extract out the pex-building logic from the Conan subsystem into an ExecutablePexTool base class.
  • Collect setup_requires into a pex, using ExecutablePexTool.

Solution 2

  • Only modify the PATH in the environment dict produced from SetupPyExecutionEnvironment, no other env vars relevant to compilation.
    • Also, prepend our toolchain to the current PATH (commented inline, with a TODO pointing to #6273).
  • Add LC_ALL=C to the environment as well so compilation error output doesn't have decode errors on smart quotes.

Result

Result 1

setup_requires now fetches all necessary transitive deps into a pex, which means you can reliably import python requirements declared in setup_requires in the python_dist()'s setup.py.

Result 2

Some of the native toolchain code is slightly simplified until #6273 is merged.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pydist-setup-requires-resolve-transitive-deps branch from 7718164 to ee99bca Aug 7, 2018

@cosmicexplorer cosmicexplorer added this to the 1.9.x milestone Aug 8, 2018

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pydist-setup-requires-resolve-transitive-deps branch from 1e7716b to ee99bca Aug 8, 2018

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Aug 8, 2018

The setup_requires kwarg of python_dist() holds addresses for python_requirement_library() targets. Currently, these requirements are fetched into a directory which is added to PYTHONPATH. This however means that the transitive dependencies are not fetched ...

This is not true. The resolving is done via resolve_multi which is transitive:

setup_requires_dists = resolve_multi(interpreter, reqs_to_resolve, platforms, None)

There must be some other problem.

@jsirois
Copy link
Member

jsirois left a comment

It's completely unclear to me what the differnce on the ground is in this change. I think its important to understand the actual mechanisms at play here and I definitely don't. You may just need to educate me on before underlying PYTHONPATH/commandline vs after.

@@ -41,6 +41,8 @@ def compute_fingerprint(self, python_target):
class SelectInterpreter(Task):
"""Select an Python interpreter that matches the constraints of all targets in the working set."""

options_scope = 'select-python-interpreter'

This comment has been minimized.

@jsirois

jsirois Aug 8, 2018

Member

What's this for? SelectInterpreter exports no options.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 9, 2018

Contributor

If you try to use a task in a test, it will error out if an options_scope is not provided (from test_base.py):

      scope = task_type.options_scope
      if scope is None:
        raise TaskError('You must set a scope on your task type before using it in tests.')

This comment has been minimized.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

Wow, thanks a lot. I will fix that now.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

Fixed as of 79dd8a8!

base_requirements = []

def bootstrap(self, interpreter, pex_file_path, extra_reqs=None):
pex_info = PexInfo.default()

This comment has been minimized.

@jsirois

jsirois Aug 8, 2018

Member

These PexInfo setup lines can be moved into the else clause.

This comment has been minimized.

@cosmicexplorer
]

@memoized_property
def _python_setup(self):

This comment has been minimized.

@jsirois

jsirois Aug 8, 2018

Member

Unused - kill.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

I'm not sure I understand -- self._python_setup.platforms is referenced in the get_targets_by_declared_platform method?


@classmethod
def subsystem_dependencies(cls):
return super(BuildSetupRequiresPex, cls).subsystem_dependencies() + (PythonSetup.scoped(cls),)

This comment has been minimized.

@jsirois

jsirois Aug 8, 2018

Member

Per below - kill this method override - not needed.

setup_runner = SetupPyRunner(
source_dir=dist_target_dir,
setup_command=setup_py_snapshot_version_argv,
interpreter=interpreter)
file_input='setup.py',
interpreter=setup_requires_interpreter)

setup_py_env = setup_py_execution_environment.as_environment()
with environment_as(**setup_py_env):
# Build a whl using SetupPyRunner and return its absolute path.

This comment has been minimized.

@jsirois

jsirois Aug 8, 2018

Member

The path is not returned, fix comment.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

Fixed in da7eefb!

with safe_concurrent_creation(pex_file_path) as safe_path:
builder = PEXBuilder(interpreter=interpreter, pex_info=pex_info)
all_reqs = list(self.base_requirements) + list(extra_reqs or [])
dump_requirements(builder, interpreter, all_reqs, logger)

This comment has been minimized.

@jsirois

jsirois Aug 8, 2018

Member

The dump_requirements method uses PythonSetup and PythonRepos behind your back - you need to declare subsystem dependencies on these two as things stand to be safe.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

Fixed, and comment added with that info as of fc6ffc5!

'setup-requires-{}.pex'.format(versioned_target_fingerprint))
setup_requires_pex = self._build_setup_requires_pex_settings.bootstrap(
interpreter, setup_reqs_pex_path,
# FIXME: remove this obvious hack! Without this building the dist will fail complaining that

This comment has been minimized.

@jsirois

jsirois Aug 8, 2018

Member

This should be understood now. The base_requirements of BuildSetupRequiresPex already has an un-constrained 'setuptools' requirement.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

Ah! That makes perfect sense, and let me remove this "obvious hack" in 8d431b2!

def run(self):
"""???/ripped from the pex installer class
Used so we can control the command line, and raise an exception on failure.

This comment has been minimized.

@jsirois

jsirois Aug 8, 2018

Member

This isn't making sense. The one use of SetupPyRunner that passes file_input, passes 'setup.py' which is exactly the file the original SetupPyRunner class operates on.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

Ok, but if we don't override the run method like this, the pex installer class generates a command line like <interpreter_path> '-' [ARGS...]. This causes the following error message on 1c56404:

> ./pants test tests/python/pants_test/backend/python/tasks:python_native_code_testing -- -vsk test_python_create_universal
# ...
 tests/python/pants_test/backend/python/tasks/test_build_local_python_distributions.py::TestBuildLocalDistsNativeSources::test_python_create_universal_distribution <- pyprep/sources/e748adc87afce18d6c648d5b1450b8a60c05af63/pants_test/backend/python/tasks/test_build_local_python_distributions.py **** Failed to install python_dist_subdir (caused by: NonZeroExit("received exit code 1 during execution of `[u'/tmp/tmp8iN8hX_BUILD_ROOT/.pants.d/pants_backend_python_tasks_build_local_python_distributions_BuildLocalPythonDistributions/ded05fd9ba17/src.python.dist.universal_dist/033964736752/setup_requires_site/setup-requires-67d1c918dfca701f0821785aaba9deb2d65ed3b6-DefaultFingerprintStrategy.da39a3ee5e6b_1c758847ad64.pex', '-', u'egg_info', u'--tag-build=+67d1c918dfca701f0821785aaba9deb2d65ed3b6-DefaultFingerprintStrategy.da39a3ee5e6b_1c758847ad64', u'bdist_wheel', u'--dist-dir', u'dist']` while trying to execute `[u'/tmp/tmp8iN8hX_BUILD_ROOT/.pants.d/pants_backend_python_tasks_build_local_python_distributions_BuildLocalPythonDistributions/ded05fd9ba17/src.python.dist.universal_dist/033964736752/setup_requires_site/setup-requires-67d1c918dfca701f0821785aaba9deb2d65ed3b6-DefaultFingerprintStrategy.da39a3ee5e6b_1c758847ad64.pex', '-', u'egg_info', u'--tag-build=+67d1c918dfca701f0821785aaba9deb2d65ed3b6-DefaultFingerprintStrategy.da39a3ee5e6b_1c758847ad64', u'bdist_wheel', u'--dist-dir', u'dist']`",)
                     ):
                     stdout:
                     
                     stderr:
                     Could not open - in the environment [/tmp/tmp8iN8hX_BUILD_ROOT/.pants.d/pants_backend_python_tasks_build_local_python_distributions_BuildLocalPythonDistributions/ded05fd9ba17/src.python.dist.universal_dist/033964736752/setup_requires_site/setup-requires-67d1c918dfca701f0821785aaba9deb2d65ed3b6-DefaultFingerprintStrategy.da39a3ee5e6b_1c758847ad64.pex]: [Errno 2] No such file or directory: '-'
                     
                     
                     FAILED
# ...

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 18, 2018

Contributor

I left a fixme in setup_py.py describing what I'm stuck on in that commit -- if there's an upstream fix we should be thinking about, I can do that too.

This comment has been minimized.

@jsirois

jsirois Aug 22, 2018

Member

OK - how about not using SetupPyRunner. What it does:

  1. ensures wheel and setuptools are present
  2. executes setup.py [args]

You already do 1 on your own, ensure setup.py exists and formulate args. So, just run your pex directly against setup.py:

diff --git a/src/python/pants/backend/python/tasks/build_local_python_distributions.py b/src/python/pants/backend/python/tasks/build_local_python_distributions.py
index a7ee08bc2..80f96791e 100644
--- a/src/python/pants/backend/python/tasks/build_local_python_distributions.py
+++ b/src/python/pants/backend/python/tasks/build_local_python_distributions.py
@@ -22,13 +22,13 @@ from pants.backend.python.subsystems.python_native_code import (BuildSetupRequir
                                                                 SetupPyNativeTools)
 from pants.backend.python.targets.python_requirement_library import PythonRequirementLibrary
 from pants.backend.python.tasks.pex_build_util import is_local_python_dist
-from pants.backend.python.tasks.setup_py import SetupPyRunner
 from pants.base.build_environment import get_buildroot
 from pants.base.exceptions import TargetDefinitionException, TaskError
+from pants.base.workunit import WorkUnitLabel
 from pants.build_graph.address import Address
 from pants.task.task import Task
 from pants.util.collections import assert_single_element
-from pants.util.contextutil import environment_as
+from pants.util.contextutil import pushd
 from pants.util.dirutil import safe_mkdir_for, split_basename_and_dirname
 from pants.util.memo import memoized_classproperty, memoized_property
 
@@ -254,7 +254,6 @@ class BuildLocalPythonDistributions(Task):
     self._create_dist(
       dist_target,
       dist_output_dir,
-      interpreter,
       setup_py_execution_environment,
       versioned_target_fingerprint,
       is_platform_specific)
@@ -279,51 +278,47 @@ class BuildLocalPythonDistributions(Task):
 
     dist_dir_args = ['--dist-dir', self._DIST_OUTPUT_DIR]
 
-    setup_py_command = egg_info_snapshot_tag_args + bdist_whl_args + platform_args + dist_dir_args
-    return setup_py_command
-
-  def _create_dist(self, dist_tgt, dist_target_dir, interpreter,
-                   setup_py_execution_environment, snapshot_fingerprint, is_platform_specific):
+    return (['setup.py'] +
+            egg_info_snapshot_tag_args +
+            bdist_whl_args +
+            platform_args +
+            dist_dir_args)
+
+  def _create_dist(self,
+                   dist_tgt,
+                   dist_target_dir,
+                   setup_py_execution_environment,
+                   snapshot_fingerprint,
+                   is_platform_specific):
     """Create a .whl file for the specified python_distribution target."""
     self._copy_sources(dist_tgt, dist_target_dir)
 
     setup_py_snapshot_version_argv = self._generate_snapshot_bdist_wheel_argv(
       snapshot_fingerprint, is_platform_specific)
 
-    setup_requires_interpreter = PythonInterpreter(
-      binary=setup_py_execution_environment.setup_requires_pex.path(),
-      identity=interpreter.identity,
-      extras=interpreter.extras)
-
-    setup_runner = SetupPyRunner(
-      source_dir=dist_target_dir,
-      setup_command=setup_py_snapshot_version_argv,
-      # TODO: explain why this is necessary!
-      explicit_setup_filename='setup.py',
-      interpreter=setup_requires_interpreter)
-
+    setup_requires_pex = setup_py_execution_environment.setup_requires_pex
     setup_py_env = setup_py_execution_environment.as_environment()
-    with environment_as(**setup_py_env):
-      # Build a whl using SetupPyRunner. The pex installer class's run() instance method performs
-      # the install, returns whether the install was successful, and if not, displays the stdout and
-      # stderr of the setup.py invocation directly to stderr.
-      # TODO: Would an upstream pex change make sense to allow more control of the output printed to
-      # stderr when using pex as a library (because pex prints directly to the parent process's
-      # stderr on failure)? Or is there a canonical way to just capture stderr in a with statement?
-      was_installed = setup_runner.run()
-      if not was_installed:
-        raise self.BuildLocalPythonDistributionsError(
-          "Installation of python distribution from target {target} into directory {into_dir} "
-          "failed (return value of run() was: {rc!r}).\n"
-          "The chosen interpreter was: {interpreter}.\n"
-          "The execution environment was: {env}.\n"
-          "The setup command was: {command}."
-          .format(target=dist_tgt,
-                  into_dir=dist_target_dir,
-                  rc=was_installed,
-                  interpreter=setup_requires_interpreter,
-                  env=setup_py_env,
-                  command=setup_py_snapshot_version_argv))
+
+    cmd = ' '.join(setup_requires_pex.cmdline(setup_py_snapshot_version_argv))
+    with self.context.new_workunit('setup.py', cmd=cmd, labels=[WorkUnitLabel.TOOL]) as workunit:
+      with pushd(dist_target_dir):
+        result = setup_requires_pex.run(args=setup_py_snapshot_version_argv,
+                                        env=setup_py_env,
+                                        stdout=workunit.output('stdout'),
+                                        stderr=workunit.output('stderr'))
+        if result != 0:
+          raise self.BuildLocalPythonDistributionsError(
+            "Installation of python distribution from target {target} into directory {into_dir} "
+            "failed (return value of run() was: {rc!r}).\n"
+            "The chosen interpreter was: {interpreter}.\n"
+            "The execution environment was: {env}.\n"
+            "The setup command was: {command}."
+            .format(target=dist_tgt,
+                    into_dir=dist_target_dir,
+                    rc=result,
+                    interpreter=setup_requires_pex.path(),
+                    env=setup_py_env,
+                    command=setup_py_snapshot_version_argv))
 
   def _inject_synthetic_dist_requirements(self, dist, req_lib_addr):
     """Inject a synthetic requirements library that references a local wheel.
diff --git a/src/python/pants/backend/python/tasks/setup_py.py b/src/python/pants/backend/python/tasks/setup_py.py
index 170581f4e..1f2fcf4b0 100644
--- a/src/python/pants/backend/python/tasks/setup_py.py
+++ b/src/python/pants/backend/python/tasks/setup_py.py
@@ -14,10 +14,8 @@ from builtins import map, object, str, zip
 from collections import OrderedDict, defaultdict
 
 from future.utils import PY2, PY3
-from pex.executor import Executor
 from pex.installer import InstallerBase, Packager
 from pex.interpreter import PythonInterpreter
-from pex.tracer import TRACER
 from twitter.common.collections import OrderedSet
 from twitter.common.dirutil.chroot import Chroot
 
@@ -53,10 +51,8 @@ setup(**
 class SetupPyRunner(InstallerBase):
   _EXTRAS = ('setuptools', 'wheel')
 
-  # `interpreter` is one of the kwargs often passed to this constructor.
-  def __init__(self, source_dir, setup_command, explicit_setup_filename=None, **kw):
+  def __init__(self, source_dir, setup_command, **kw):
     self.__setup_command = setup_command
-    self.explicit_setup_filename = explicit_setup_filename
     super(SetupPyRunner, self).__init__(source_dir, **kw)
 
   def mixins(self):
@@ -78,34 +74,6 @@ class SetupPyRunner(InstallerBase):
   def _setup_command(self):
     return self.__setup_command
 
-  class SetupPyError(TaskError): pass
-
-  def run(self):
-    if self._installed is not None:
-      return self._installed
-
-    with TRACER.timed('Installing {}'.format(self._install_tmp), V=2):
-      if self.explicit_setup_filename:
-        command = [self._interpreter.binary, self.explicit_setup_filename] + self._setup_command()
-      else:
-        command = [self._interpreter.binary, '-'] + self._setup_command()
-      try:
-        Executor.execute(command,
-                         env=self._interpreter.sanitized_environment(),
-                         cwd=self._source_dir,
-                         stdin_payload=self.bootstrap_script.encode('ascii'))
-        self._installed = True
-      except Executor.NonZeroExit as e:
-        self._installed = False
-        name = os.path.basename(self._source_dir)
-        raise self.SetupPyError(
-          "**** Failed to install {} (caused by: {!r}\n):\n"
-          "stdout:\n{}\nstderr:\n{}\n"
-          .format(name, e, e.stdout, e.stderr))
-
-    self._postprocess()
-    return self._installed
-
 
 class TargetAncestorIterator(object):
   """Supports iteration of target ancestor lineages."""
diff --git a/src/python/pants/binaries/executable_pex_tool.py b/src/python/pants/binaries/executable_pex_tool.py
index 659cd3ece..fc1a2830c 100644
--- a/src/python/pants/binaries/executable_pex_tool.py
+++ b/src/python/pants/binaries/executable_pex_tool.py
@@ -43,7 +43,7 @@ class ExecutablePexTool(Subsystem):
     if is_executable(pex_file_path):
       return PEX(pex_file_path, interpreter)
     else:
-      pex_info = PexInfo.default()
+      pex_info = PexInfo.default(interpreter=interpreter)
       if self.entry_point is not None:
         pex_info.entry_point = self.entry_point
 

You then get this: image

This comment has been minimized.

@jsirois

jsirois Aug 22, 2018

Member

Upstream fix pantsbuild/pex#543

NB though - using SetupPyRunner here was still a bit off since:

  1. The current code redundantly add wheel and setuptools requirements
  2. The code was reaching across to anthother task's helper without extracting it and a dedicated test for it.
@@ -74,6 +79,42 @@ def mixins(self):
def _setup_command(self):
return self.__setup_command

def run(self):

This comment has been minimized.

@jsirois

jsirois Aug 8, 2018

Member

I'm sceptical of the need to modify this class as noted below, but assuming it is somehow actually needed - this class needs to move to a shared file. >50% of its code is not needed locally.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 11, 2018

See #6338, which has a minimal example of the error this PR is trying to resolve. The specific error is actually that pex is failing to resolve transitive dependencies (raising an exception in that method), not that it only resolves the top-level dependencies as I incorrectly described in the OP. If we can focus on the use case in #6338, we may find we can close this one.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pydist-setup-requires-resolve-transitive-deps branch 3 times, most recently from d078a4b to 9e1b207 Aug 18, 2018

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Aug 21, 2018

Sorry for the delay. I'll dive into this by EOD.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Aug 22, 2018

Not sure if my reply gets burried by collapsed sections, but it's here: #6275 (comment)

In addition to working for pants pyprep --cache-ignore examples/src/python/example/python_distribution/hello/pants_setup_requires/ the tests as you have them (./pants test.pytest tests/python/pants_test/backend/python/tasks: -- -vk'test_build_local_python_distributions or test_ctypes') also pass with the exception of CTypesIntegrationTest.test_pants_native_source_detection_for_local_ctypes_dists_for_current_platform_only which seems to me should fail in build-local-dists when attempting to build for the this-platform-does_not-exist platform as things stand. To fix you need to pin dump_requirements with platforms=['current'] in the ExecutablePexTool.

Along with severing the unholy alliance with SetupPy this is with a delta of 3 files changed, 38 insertions(+), 75 deletions(-) - so a win all around I think.

jsirois added a commit to jsirois/pex that referenced this pull request Aug 22, 2018

Support `-` when running as an interpreter.
It's surprising when using a pex in python interpreter mode that `-` is
not recognized as a program coming from stdin as regular python
interpreter would. Add support for this with a test.

The surprise of this missing feature was rightly identified in the
context of pantsbuild/pants#6275.
@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 23, 2018

There's one failure in an integration test which just builds testprojects (Exception message: Could not satisfy all requirements for ctypes_test==0.0.1+36df877074a4d30bde4598fb0bc0a7ea1f58c1ce.defaultfingerprintstrategy.1f6528e9e3c3.83125fc5fcc6) which is not spurious, will look into fixing that today then updating the OP.

jsirois added a commit to pantsbuild/pex that referenced this pull request Aug 24, 2018

Support `-` when running as an interpreter. (#543)
It's surprising when using a pex in python interpreter mode that `-` is
not recognized as a program coming from stdin as regular python
interpreter would. Add support for this with a test.

The surprise of this missing feature was rightly identified in the
context of pantsbuild/pants#6275.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pydist-setup-requires-resolve-transitive-deps branch from 4e9707a to 7e25b7b Aug 25, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 31, 2018

I can reliably repro the above error on a Linux machine, looking into fixing it.

cosmicexplorer added some commits Jul 30, 2018

cosmicexplorer added some commits Aug 9, 2018

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pydist-setup-requires-resolve-transitive-deps branch 2 times, most recently from 163f0b4 to eb4b5b9 Aug 31, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 31, 2018

Two of the python_dist() testprojects having the same package name ('ctypes_test') appeared to cause this issue, which actually makes perfect sense given the error message. eb4b5b9 should fix that.

@cosmicexplorer cosmicexplorer removed this from the 1.9.x milestone Aug 31, 2018

@cosmicexplorer cosmicexplorer changed the title [WIP] satisfy python_dist setup_requires with a pex to resolve transitive deps satisfy python_dist setup_requires with a pex to resolve transitive deps, and some other unrelated native toolchain changes Aug 31, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Aug 31, 2018

The OP was updated and CI is green -- PTAL. The hypothesis above seems to have been correct.

@jsirois
Copy link
Member

jsirois left a comment

Hurrah!

python_dist(
sources=rglobs('*.py', exclude=[main_file]),
setup_requires=[
# FIXME: this currently fails because setup_requires doesn't fetch transitive deps!

This comment has been minimized.

@jsirois

jsirois Aug 31, 2018

Member

Still true?

As an aside - I'm super-uncomfortable with these FIXMEs hitting master. It may be just a vocabulary difference though. TODO (+ issue) - great, FIXME broken - like this implies - not great.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 31, 2018

Contributor

It's a bad habit I picked up reading LLVM sources a while ago, I have zero issues with never seeing it again.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Aug 31, 2018

Contributor

This line was removed because it was untrue now that the rest of this PR works, I turned a couple nearby FIXMEs into TODOs, and moving forward will avoid the use of FIXME altogether.


def bootstrap(self, interpreter, pex_file_path, extra_reqs=None):
# Caching is done just by checking if the file at the specified path is already executable.
if is_executable(pex_file_path):

This comment has been minimized.

@jsirois

jsirois Aug 31, 2018

Member

A bit cleaner to:

if not is_executable(pex_file_path):
  # create it
return PEX(pex_file_path, interpreter)

This comment has been minimized.

@cosmicexplorer

cosmicexplorer added some commits Aug 31, 2018

@cosmicexplorer cosmicexplorer merged commit 6b11c59 into pantsbuild:master Sep 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

cosmicexplorer added a commit that referenced this pull request Sep 5, 2018

Fix CI failures introduced by #6275 (#6454)
### Problem

When #6275 was merged to master, [it failed two test shards](https://travis-ci.org/pantsbuild/pants/builds/424559907), and these errors weren't spurious.

### Solution

- fixed (ish) the json error (from the py3 shard), and left a detailed TODO next to the workaround
- skipped the integration test that was failing (`test_pants_requirement_setup_requires_version()`, introduced in #6275) -- see #6455 for more info.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment