Fix pydist native sources selection (#6205)

This adds a few more unit tests on top of #6201 -- specifically, testing in isolation that the platform is correctly set on a `python_dist()` which has no native sources itself, but depends on a `ctypes_compatible_c(pp)?_library()` (which is what #6201 fixes). It also moves all testing related to the `ctypes_compatible_c(pp)?_library()` targets into separate testing files to separate functionality more clearly.

Resolves #6201.

src/python/pants/backend/native/tasks/c_compile.py

@@ -15,6 +15,8 @@
 
 class CCompile(NativeCompile):
 
+  options_scope = 'c-compile'
+
   # Compile only C library targets.
   source_target_constraint = SubclassesOf(CLibrary)
 

src/python/pants/backend/native/tasks/cpp_compile.py

@@ -15,6 +15,8 @@
 
 class CppCompile(NativeCompile):
 
+  options_scope = 'cpp-compile'
+
   # Compile only C++ library targets.
   source_target_constraint = SubclassesOf(CppLibrary)
 

src/python/pants/backend/native/tasks/link_shared_libraries.py

@@ -8,9 +8,10 @@
 
 from pants.backend.native.config.environment import Linker, Platform
 from pants.backend.native.subsystems.native_toolchain import NativeToolchain
+from pants.backend.native.targets.native_artifact import NativeArtifact
 from pants.backend.native.targets.native_library import NativeLibrary
 from pants.backend.native.tasks.native_compile import NativeTargetDependencies, ObjectFiles
-from pants.backend.native.tasks.native_external_library_fetch import NativeExternalLibraryFetch
+from pants.backend.native.tasks.native_external_library_fetch import NativeExternalLibraryFiles
 from pants.backend.native.tasks.native_task import NativeTask
 from pants.base.exceptions import TaskError
 from pants.base.workunit import WorkUnit, WorkUnitLabel
@@ -24,16 +25,30 @@ class SharedLibrary(datatype(['name', 'path'])): pass
 
 
 class LinkSharedLibraryRequest(datatype([
-    'linker',
-    'object_files',
-    'native_artifact',
+    ('linker', Linker),
+    ('object_files', tuple),
+    ('native_artifact', NativeArtifact),
     'output_dir',
-    'external_libs_info'
-])): pass
+    ('external_lib_dirs', tuple),
+    ('external_lib_names', tuple),
+])):
+
+  @classmethod
+  def with_external_libs_product(cls, external_libs_product=None, *args, **kwargs):
+    if external_libs_product is None:
+      lib_dirs = ()
+      lib_names = ()
+    else:
+      lib_dirs = (external_libs_product.lib_dir,)
+      lib_names = external_libs_product.lib_names
+
+    return cls(*args, external_lib_dirs=lib_dirs, external_lib_names=lib_names, **kwargs)
 
 
 class LinkSharedLibraries(NativeTask):
 
+  options_scope = 'link-shared-libraries'
+
   @classmethod
   def product_types(cls):
     return [SharedLibrary]
@@ -43,7 +58,7 @@ def prepare(cls, options, round_manager):
     super(LinkSharedLibraries, cls).prepare(options, round_manager)
     round_manager.require(NativeTargetDependencies)
     round_manager.require(ObjectFiles)
-    round_manager.require(NativeExternalLibraryFetch.NativeExternalLibraryFiles)
+    round_manager.optional_product(NativeExternalLibraryFiles)
 
   @property
   def cache_target_dirs(self):
@@ -85,7 +100,7 @@ def execute(self):
     native_target_deps_product = self.context.products.get(NativeTargetDependencies)
     compiled_objects_product = self.context.products.get(ObjectFiles)
     shared_libs_product = self.context.products.get(SharedLibrary)
-    external_libs_product = self.context.products.get_data(NativeExternalLibraryFetch.NativeExternalLibraryFiles)
+    external_libs_product = self.context.products.get_data(NativeExternalLibraryFiles)
 
     all_shared_libs_by_name = {}
 
@@ -143,18 +158,25 @@ def _make_link_request(self,
       self.context.log.debug("object_file_paths: {}".format(object_file_paths))
       all_compiled_object_files.extend(object_file_paths)
 
-    return LinkSharedLibraryRequest(
+    return LinkSharedLibraryRequest.with_external_libs_product(
       linker=self.linker,
-      object_files=all_compiled_object_files,
+      object_files=tuple(all_compiled_object_files),
       native_artifact=vt.target.ctypes_native_library,
       output_dir=vt.results_dir,
-      external_libs_info=external_libs_product)
+      external_libs_product=external_libs_product)
 
   _SHARED_CMDLINE_ARGS = {
     'darwin': lambda: ['-mmacosx-version-min=10.11', '-Wl,-dylib'],
     'linux': lambda: ['-shared'],
   }
 
+  def _get_third_party_lib_args(self, link_request):
+    ext_libs = link_request.external_libs_info
+    if not ext_libs:
+      return []
+
+    return ext_libs.get_third_party_lib_args()
+
   def _execute_link_request(self, link_request):
     object_files = link_request.object_files
 
@@ -167,11 +189,11 @@ def _execute_link_request(self, link_request):
     output_dir = link_request.output_dir
     resulting_shared_lib_path = os.path.join(output_dir,
                                              native_artifact.as_shared_lib(self.platform))
+    self.context.log.debug("resulting_shared_lib_path: {}".format(resulting_shared_lib_path))
     # We are executing in the results_dir, so get absolute paths for everything.
     cmd = ([linker.exe_filename] +
            self.platform.resolve_platform_specific(self._SHARED_CMDLINE_ARGS) +
            linker.extra_args +
-           link_request.external_libs_info.get_third_party_lib_args() +
            ['-o', os.path.abspath(resulting_shared_lib_path)] +
            [os.path.abspath(obj) for obj in object_files])
 

src/python/pants/backend/native/tasks/native_compile.py

@@ -11,7 +11,7 @@
 
 from pants.backend.native.config.environment import Executable, Platform
 from pants.backend.native.targets.native_library import NativeLibrary
-from pants.backend.native.tasks.native_external_library_fetch import NativeExternalLibraryFetch
+from pants.backend.native.tasks.native_external_library_fetch import NativeExternalLibraryFiles
 from pants.backend.native.tasks.native_task import NativeTask
 from pants.base.build_environment import get_buildroot
 from pants.base.exceptions import TaskError
@@ -64,7 +64,7 @@ def product_types(cls):
   @classmethod
   def prepare(cls, options, round_manager):
     super(NativeCompile, cls).prepare(options, round_manager)
-    round_manager.require(NativeExternalLibraryFetch.NativeExternalLibraryFiles)
+    round_manager.optional_data(NativeExternalLibraryFiles)
 
   @property
   def cache_target_dirs(self):
@@ -124,9 +124,7 @@ def _add_product_at_target_base(product_mapping, target, value):
   def execute(self):
     object_files_product = self.context.products.get(ObjectFiles)
     native_deps_product = self.context.products.get(NativeTargetDependencies)
-    external_libs_product = self.context.products.get_data(
-      NativeExternalLibraryFetch.NativeExternalLibraryFiles
-    )
+    external_libs_product = self.context.products.get_data(NativeExternalLibraryFiles)
     source_targets = self.context.targets(self.source_target_constraint.satisfied_by)
 
     with self.invalidated(source_targets, invalidate_dependents=True) as invalidation_check:
@@ -195,6 +193,9 @@ def _compiler(self):
     return self.get_compiler()
 
   def _get_third_party_include_dirs(self, external_libs_product):
+    if not external_libs_product:
+      return []
+
     directory = external_libs_product.include_dir
     return [directory] if directory else []
 
@@ -232,14 +233,18 @@ def _make_compile_argv(self, compile_request):
 
     # We are going to execute in the target output, so get absolute paths for everything.
     # TODO: If we need to produce static libs, don't add -fPIC! (could use Variants -- see #5788).
+    buildroot = get_buildroot()
     argv = (
       [compiler.exe_filename] +
       platform_specific_flags +
       self.extra_compile_args() +
       err_flags +
       ['-c', '-fPIC'] +
-      ['-I{}'.format(os.path.abspath(inc_dir)) for inc_dir in compile_request.include_dirs] +
-      [os.path.abspath(src) for src in compile_request.sources])
+      [
+        '-I{}'.format(os.path.join(buildroot, inc_dir))
+        for inc_dir in compile_request.include_dirs
+      ] +
+      [os.path.join(buildroot, src) for src in compile_request.sources])
 
     return argv
 

src/python/pants/backend/native/tasks/native_external_library_fetch.py

@@ -61,31 +61,21 @@ def fetch_cmdline_args(self):
     return args
 
 
+class NativeExternalLibraryFiles(datatype([
+    'include_dir',
+    # TODO: we shouldn't have any `lib_names` if `lib_dir` is not set!
+    'lib_dir',
+    ('lib_names', tuple),
+])): pass
+
+
 class NativeExternalLibraryFetch(Task):
   options_scope = 'native-external-library-fetch'
   native_library_constraint = Exactly(ExternalNativeLibrary)
 
   class NativeExternalLibraryFetchError(TaskError):
     pass
 
-  class NativeExternalLibraryFiles(object):
-    def __init__(self):
-      self.include_dir = None
-      self.lib_dir = None
-      self.lib_names = []
-
-    def add_lib_name(self, lib_name):
-      self.lib_names.append(lib_name)
-
-    def get_third_party_lib_args(self):
-      lib_args = []
-      if self.lib_names:
-        for lib_name in self.lib_names:
-          lib_args.append('-l{}'.format(lib_name))
-        lib_dir_arg = '-L{}'.format(self.lib_dir)
-        lib_args.append(lib_dir_arg)
-      return lib_args
-
   @classmethod
   def _parse_lib_name_from_library_filename(cls, filename):
     match_group = re.match(r"^lib(.*)\.(a|so|dylib)$", filename)
@@ -105,7 +95,7 @@ def subsystem_dependencies(cls):
 
   @classmethod
   def product_types(cls):
-    return [cls.NativeExternalLibraryFiles]
+    return [NativeExternalLibraryFiles]
 
   @property
   def cache_target_dirs(self):
@@ -116,9 +106,6 @@ def _conan_binary(self):
     return Conan.scoped_instance(self).bootstrap_conan()
 
   def execute(self):
-    task_product = self.context.products.get_data(self.NativeExternalLibraryFiles,
-                                                  self.NativeExternalLibraryFiles)
-
     native_lib_tgts = self.context.targets(self.native_library_constraint.satisfied_by)
     if native_lib_tgts:
       with self.invalidated(native_lib_tgts,
@@ -128,7 +115,10 @@ def execute(self):
         if invalidation_check.invalid_vts or not resolve_vts.valid:
           for vt in invalidation_check.all_vts:
             self._fetch_packages(vt, vts_results_dir)
-        self._populate_task_product(vts_results_dir, task_product)
+
+        native_external_libs_product = self._collect_external_libs(vts_results_dir)
+        self.context.products.register_data(NativeExternalLibraryFiles,
+                                            native_external_libs_product)
 
   def _prepare_vts_results_dir(self, vts):
     """
@@ -138,22 +128,23 @@ def _prepare_vts_results_dir(self, vts):
     safe_mkdir(vt_set_results_dir)
     return vt_set_results_dir
 
-  def _populate_task_product(self, results_dir, task_product):
+  def _collect_external_libs(self, results_dir):
     """
     Sets the relevant properties of the task product (`NativeExternalLibraryFiles`) object.
     """
-    lib = os.path.join(results_dir, 'lib')
-    include = os.path.join(results_dir, 'include')
+    lib_dir = os.path.join(results_dir, 'lib')
+    include_dir = os.path.join(results_dir, 'include')
 
-    if os.path.exists(lib):
-      task_product.lib_dir = lib
-      for filename in os.listdir(lib):
+    lib_names = []
+    if os.path.isdir(lib_dir):
+      for filename in os.listdir(lib_dir):
         lib_name = self._parse_lib_name_from_library_filename(filename)
         if lib_name:
-          task_product.add_lib_name(lib_name)
+          lib_names.append(lib_name)
 
-    if os.path.exists(include):
-      task_product.include_dir = include
+    return NativeExternalLibraryFiles(include_dir=include_dir,
+                                      lib_dir=lib_dir,
+                                      lib_names=tuple(lib_names))
 
   def _get_conan_data_dir_path_for_package(self, pkg_dir_path, pkg_sha):
     return os.path.join(self.workdir,

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

@@ -15,7 +15,7 @@
 from pants.base.exceptions import IncompatiblePlatformsError
 from pants.subsystem.subsystem import Subsystem
 from pants.util.memo import memoized_property
-from pants.util.objects import Exactly
+from pants.util.objects import SubclassesOf
 
 
 class PythonNativeCode(Subsystem):
@@ -63,8 +63,8 @@ def native_target_has_native_sources(self, target):
   @memoized_property
   def _native_target_matchers(self):
     return {
-      Exactly(PythonDistribution): self.pydist_has_native_sources,
-      Exactly(NativeLibrary): self.native_target_has_native_sources,
+      SubclassesOf(PythonDistribution): self.pydist_has_native_sources,
+      SubclassesOf(NativeLibrary): self.native_target_has_native_sources,
     }
 
   def _any_targets_have_native_sources(self, targets):
@@ -97,7 +97,7 @@ def get_targets_by_declared_platform(self, targets):
                                           '--platforms option or a pants.ini file.']
     return targets_by_platforms
 
-  _PYTHON_PLATFORM_TARGETS_CONSTRAINT = Exactly(PythonBinary, PythonDistribution)
+  _PYTHON_PLATFORM_TARGETS_CONSTRAINT = SubclassesOf(PythonBinary, PythonDistribution)
 
   def check_build_for_current_platform_only(self, targets):
     """

testprojects/src/python/python_distribution/ctypes/BUILD

@@ -31,4 +31,5 @@ python_binary(
   dependencies=[
     ':ctypes',
   ],
+  platforms=['current'],
 )

tests/python/pants_test/backend/python/tasks/BUILD

@@ -24,22 +24,25 @@ python_library(
 python_native_code_test_files = [
   'test_build_local_python_distributions.py',
   'test_python_distribution_integration.py',
+  'test_ctypes.py',
   'test_ctypes_integration.py',
 ]
 
 python_tests(
   name='python_native_code_testing',
   sources=python_native_code_test_files,
   dependencies=[
-    ':python_task_test_base',
     '3rdparty/python:future',
+    '3rdparty/python/twitter/commons:twitter.common.collections',
     'src/python/pants/backend/native',
+    'src/python/pants/backend/native/targets',
     'src/python/pants/backend/python:plugin',
     'src/python/pants/backend/python/targets',
     'src/python/pants/backend/python/tasks',
     'src/python/pants/util:contextutil',
     'src/python/pants/util:process_handler',
     'tests/python/pants_test:int-test',
+    'tests/python/pants_test/backend/python/tasks/util',
     'tests/python/pants_test/engine:scheduler_test_base',
   ],
   tags={'platform_specific_behavior', 'integration'},