Resolve all platforms from all python targets (pantsbuild#7156)

Don't just use the default configured targets.

This means that _all_ transitive 3rdparty python will need to be
resolvable in _all_ platforms in any target in the graph. This is not
ideal (we really want to be doing per-root resolves), but because we
currently do one global resolve, this is a decent fit.

examples/src/python/example/3rdparty_py.md

@@ -80,6 +80,9 @@ with which your binary is intended to be compatible in the `platforms` field of
 <a href="https://pip.pypa.io/en/stable/reference/pip_wheel/">wheel</a> files for each package
 and platform available at build time.
 
+Pants will use the explicitly specified `platforms` field of your <a pantsref="bdict_python_binary">`python_binary`</a>
+target if set for both itself and its dependencies, or will otherwise fall back to the `python-setup.platforms` option value.
+
 Pants will look for those files in the location specified in the
 [[`python-repos`|pants('src/docs:setup_repo')#redirecting-python-requirements-to-other-servers]] field
 in pants.ini. It can understand either a simple local directory of .whl files or a "find links"-friendly

src/python/pants/backend/python/subsystems/pex_build_util.py

@@ -7,6 +7,7 @@
 import logging
 import os
 from builtins import str
+from collections import defaultdict
 
 from future.utils import PY2
 from pex.fetcher import Fetcher
@@ -50,6 +51,19 @@ def has_python_requirements(tgt):
   return isinstance(tgt, PythonRequirementLibrary)
 
 
+def can_have_python_platform(tgt):
+  return isinstance(tgt, (PythonBinary, PythonDistribution))
+
+
+def targets_by_platform(targets, python_setup):
+  d = defaultdict(OrderedSet)
+  for target in targets:
+    if can_have_python_platform(target):
+      for platform in target.platforms if target.platforms else python_setup.platforms:
+        d[platform].add(target)
+  return d
+
+
 def _create_source_dumper(builder, tgt):
   if type(tgt) == Files:
     # Loose `Files` as opposed to `Resources` or `PythonTarget`s have no (implied) package structure

src/python/pants/backend/python/subsystems/python_native_code.py

@@ -5,13 +5,12 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
 
 from builtins import str
-from collections import defaultdict
 
 from pants.backend.native.subsystems.native_toolchain import NativeToolchain
 from pants.backend.native.targets.native_library import NativeLibrary
 from pants.backend.python.python_requirement import PythonRequirement
+from pants.backend.python.subsystems import pex_build_util
 from pants.backend.python.subsystems.python_setup import PythonSetup
-from pants.backend.python.targets.python_binary import PythonBinary
 from pants.backend.python.targets.python_distribution import PythonDistribution
 from pants.base.exceptions import IncompatiblePlatformsError
 from pants.binaries.executable_pex_tool import ExecutablePexTool
@@ -75,7 +74,7 @@ def _any_targets_have_native_sources(self, targets):
           return True
     return False
 
-  def get_targets_by_declared_platform(self, targets):
+  def _get_targets_by_declared_platform_with_placeholders(self, targets_by_platform):
     """
     Aggregates a dict that maps a platform string to a list of targets that specify the platform.
     If no targets have platforms arguments, return a dict containing platforms inherited from
@@ -84,19 +83,12 @@ def get_targets_by_declared_platform(self, targets):
     :param tgts: a list of :class:`Target` objects.
     :returns: a dict mapping a platform string to a list of targets that specify the platform.
     """
-    targets_by_platforms = defaultdict(list)
 
-    for tgt in targets:
-      for platform in tgt.platforms:
-        targets_by_platforms[platform].append(tgt)
-
-    if not targets_by_platforms:
+    if not targets_by_platform:
       for platform in self._python_setup.platforms:
-        targets_by_platforms[platform] = ['(No target) Platform inherited from either the '
+        targets_by_platform[platform] = ['(No target) Platform inherited from either the '
                                           '--platforms option or a pants.ini file.']
-    return targets_by_platforms
-
-  _PYTHON_PLATFORM_TARGETS_CONSTRAINT = SubclassesOf(PythonBinary, PythonDistribution)
+    return targets_by_platform
 
   def check_build_for_current_platform_only(self, targets):
     """
@@ -110,9 +102,8 @@ def check_build_for_current_platform_only(self, targets):
     if not self._any_targets_have_native_sources(targets):
       return False
 
-    targets_with_platforms = [target for target in targets
-                              if self._PYTHON_PLATFORM_TARGETS_CONSTRAINT.satisfied_by(target)]
-    platforms_with_sources = self.get_targets_by_declared_platform(targets_with_platforms)
+    targets_by_platform = pex_build_util.targets_by_platform(targets, self._python_setup)
+    platforms_with_sources = self._get_targets_by_declared_platform_with_placeholders(targets_by_platform)
     platform_names = list(platforms_with_sources.keys())
 
     if len(platform_names) < 1:

src/python/pants/backend/python/tasks/resolve_requirements_task_base.py

@@ -13,8 +13,10 @@
 from pex.pex_builder import PEXBuilder
 
 from pants.backend.python.python_requirement import PythonRequirement
+from pants.backend.python.subsystems import pex_build_util
 from pants.backend.python.subsystems.pex_build_util import PexBuilderWrapper
 from pants.backend.python.subsystems.python_native_code import PythonNativeCode
+from pants.backend.python.subsystems.python_setup import PythonSetup
 from pants.backend.python.targets.python_requirement_library import PythonRequirementLibrary
 from pants.base.hash_utils import hash_all
 from pants.invalidation.cache_manager import VersionedTargetSet
@@ -36,12 +38,17 @@ def subsystem_dependencies(cls):
     return super(ResolveRequirementsTaskBase, cls).subsystem_dependencies() + (
       PexBuilderWrapper.Factory,
       PythonNativeCode.scoped(cls),
+      PythonSetup.scoped(cls),
     )
 
   @memoized_property
   def _python_native_code_settings(self):
     return PythonNativeCode.scoped_instance(self)
 
+  @memoized_property
+  def _python_setup(self):
+    return PythonSetup.global_instance()
+
   @classmethod
   def prepare(cls, options, round_manager):
     super(ResolveRequirementsTaskBase, cls).prepare(options, round_manager)
@@ -70,11 +77,11 @@ def resolve_requirements(self, interpreter, req_libs):
 
       # We need to ensure that we are resolving for only the current platform if we are
       # including local python dist targets that have native extensions.
-      tgts = self.context.targets()
-      if self._python_native_code_settings.check_build_for_current_platform_only(tgts):
-        maybe_platforms = ['current']
+      targets_by_platform = pex_build_util.targets_by_platform(self.context.targets(), self._python_setup)
+      if self._python_native_code_settings.check_build_for_current_platform_only(targets_by_platform):
+        platforms = ['current']
       else:
-        maybe_platforms = None
+        platforms = list(sorted(targets_by_platform.keys()))
 
       path = os.path.realpath(os.path.join(self.workdir, str(interpreter.identity), target_set_id))
       # Note that we check for the existence of the directory, instead of for invalid_vts,
@@ -84,7 +91,7 @@ def resolve_requirements(self, interpreter, req_libs):
           pex_builder = PexBuilderWrapper.Factory.create(
             builder=PEXBuilder(path=safe_path, interpreter=interpreter, copy=True),
             log=self.context.log)
-          pex_builder.add_requirement_libs_from(req_libs, platforms=maybe_platforms)
+          pex_builder.add_requirement_libs_from(req_libs, platforms=platforms)
           pex_builder.freeze()
     return PEX(path, interpreter=interpreter)
 

src/python/pants/build_graph/build_graph.py

@@ -448,7 +448,7 @@ def _walk_rec(addr):
       _walk_rec(address)
 
   def transitive_dependees_of_addresses(self, addresses, predicate=None, postorder=False):
-    """Returns all transitive dependees of `address`.
+    """Returns all transitive dependees of `addresses`.
 
     Note that this uses `walk_transitive_dependee_graph` and the predicate is passed through,
     hence it trims graphs rather than just filtering out Targets that do not match the predicate.

tests/python/pants_test/backend/python/tasks/test_python_binary_integration.py

@@ -15,6 +15,12 @@
 from pants_test.pants_run_integration_test import PantsRunIntegrationTest
 
 
+_LINUX_PLATFORM = "linux-x86_64"
+_LINUX_WHEEL_SUBSTRING = "manylinux"
+_OSX_PLATFORM = "macosx-10.13-x86_64"
+_OSX_WHEEL_SUBSTRING = "macosx"
+
+
 class PythonBinaryIntegrationTest(PantsRunIntegrationTest):
   @staticmethod
   @contextmanager
@@ -68,9 +74,52 @@ def test_zipsafe_caching(self):
       self.assert_success(build())
       self.assert_pex_attribute(test_pex, 'zip_safe', True)
 
-  def test_platforms(self):
-    """Ensure that changing platforms invalidates the generated pex binaries."""
-
+  def test_platform_defaults_to_config(self):
+    self.platforms_test_impl(
+      target_platforms=None,
+      config_platforms=[_OSX_PLATFORM],
+      want_present_platforms=[_OSX_WHEEL_SUBSTRING],
+      want_missing_platforms=[_LINUX_PLATFORM],
+    )
+
+  def test_target_platform_without_config(self):
+    self.platforms_test_impl(
+      target_platforms=[_LINUX_PLATFORM],
+      config_platforms=None,
+      want_present_platforms=[_LINUX_WHEEL_SUBSTRING],
+      want_missing_platforms=[_OSX_WHEEL_SUBSTRING],
+    )
+
+  def test_target_platform_overrides_config(self):
+    self.platforms_test_impl(
+      target_platforms=[_LINUX_PLATFORM],
+      config_platforms=[_OSX_WHEEL_SUBSTRING],
+      want_present_platforms=[_LINUX_WHEEL_SUBSTRING],
+      want_missing_platforms=[_OSX_WHEEL_SUBSTRING],
+    )
+
+  def test_target_platform_narrows_config(self):
+    self.platforms_test_impl(
+      target_platforms=[_LINUX_PLATFORM],
+      config_platforms=[_LINUX_WHEEL_SUBSTRING, _OSX_WHEEL_SUBSTRING],
+      want_present_platforms=[_LINUX_WHEEL_SUBSTRING],
+      want_missing_platforms=[_OSX_WHEEL_SUBSTRING],
+    )
+
+  def test_target_platform_expands_config(self):
+    self.platforms_test_impl(
+      target_platforms=[_LINUX_PLATFORM, _OSX_PLATFORM],
+      config_platforms=[_LINUX_WHEEL_SUBSTRING],
+      want_present_platforms=[_LINUX_WHEEL_SUBSTRING, _OSX_WHEEL_SUBSTRING],
+    )
+
+  def platforms_test_impl(
+    self,
+    target_platforms,
+    config_platforms,
+    want_present_platforms,
+    want_missing_platforms=(),
+  ):
     def numpy_deps(deps):
       return [d for d in deps if 'numpy' in d]
     def assertInAny(substring, collection):
@@ -79,6 +128,7 @@ def assertInAny(substring, collection):
     def assertNotInAny(substring, collection):
       self.assertTrue(all(substring not in d for d in collection),
         'Expected an entry matching "{}" in {}'.format(substring, collection))
+
     test_project = 'testprojects/src/python/cache_fields'
     test_build = os.path.join(test_project, 'BUILD')
     test_src = os.path.join(test_project, 'main.py')
@@ -88,46 +138,43 @@ def assertNotInAny(substring, collection):
       config['python-setup'] = {
         'platforms': None
       }
-      build = functools.partial(
-        self.run_pants_with_workdir,
-        command=['binary', test_project],
-        workdir=os.path.join(buildroot.new_buildroot, '.pants.d'),
-        config=config,
-        build_root=buildroot.new_buildroot
-      )
 
       buildroot.write_file(test_src, '')
 
       buildroot.write_file(test_build,
         dedent("""
-        python_binary(source='main.py', dependencies=[':numpy'])
+        python_binary(
+          source='main.py',
+          dependencies=[':numpy'],
+          {target_platforms}
+        )
         python_requirement_library(
           name='numpy',
           requirements=[
             python_requirement('numpy==1.14.5')
           ]
         )
 
-        """)
+        """.format(
+          target_platforms="platforms = [{}],".format(", ".join(["'{}'".format(p) for p in target_platforms])) if target_platforms is not None else "",
+        ))
       )
       # When only the linux platform is requested,
       # only linux wheels should end up in the pex.
-      config['python-setup']['platforms'] = ['linux-x86_64']
-      build()
-
-      with open_zip(test_pex) as z:
-        deps = numpy_deps(z.namelist())
-        assertInAny('manylinux', deps)
-        assertNotInAny('macosx', deps)
-
-      # When both linux and macosx platforms are requested,
-      # wheels for both should end up in the pex.
-      config['python-setup']['platforms'] = [
-        'linux-x86_64',
-        'macosx-10.13-x86_64']
-      build()
+      if config_platforms is not None:
+        config['python-setup']['platforms'] = config_platforms
+      result = self.run_pants_with_workdir(
+        command=['binary', test_project],
+        workdir=os.path.join(buildroot.new_buildroot, '.pants.d'),
+        config=config,
+        build_root=buildroot.new_buildroot,
+        tee_output=True,
+      )
+      self.assert_success(result)
 
       with open_zip(test_pex) as z:
         deps = numpy_deps(z.namelist())
-        assertInAny('manylinux', deps)
-        assertInAny('macosx', deps)
+        for platform in want_present_platforms:
+          assertInAny(platform, deps)
+        for platform in want_missing_platforms:
+          assertNotInAny(platform, deps)