Split conan resolve by native_external_library targets (takeover). (#6630)

Takeover of #6492 (which has completely passed review) as it was blocked by progress on two other PRs I have up (#6486, #6628) due to potential merge conflicts, which I can resolve when they come up for each of these PRs to unblock landing them in parallel.

The body of #6492 was:

### Problem

As described in #6178, the `NativeExternalLibraryFiles` products of a `conan` resolve are not currently partitioned by target, which means it isn't possible to expose individual 3rdparty deps to only their declared dependents.

### Solution

Partition the `NativeExternalLibraryFiles` product using `UnionProduct` while producing it in `NativeExternalLibraryFetch` (and switch to using isolated `vt.results_dir` directories per `external_native_library` target), and consume the split product in `NativeCompile` and `LinkSharedLibraries`.

### Result

Only declared dependents have access to 3rdparty libraries. Fixes #6178.

Author: cosmicexplorer

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

@@ -32,17 +32,7 @@ class LinkSharedLibraryRequest(datatype([
     ('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)
+  pass
 
 
 class LinkSharedLibraries(NativeTask):
@@ -154,21 +144,30 @@ def _make_link_request(self,
     deps = self._retrieve_single_product_at_target_base(native_target_deps_product, vt.target)
 
     all_compiled_object_files = []
-
     for dep_tgt in deps:
-      self.context.log.debug("dep_tgt: {}".format(dep_tgt))
-      object_files = self._retrieve_single_product_at_target_base(compiled_objects_product, dep_tgt)
-      self.context.log.debug("object_files: {}".format(object_files))
-      object_file_paths = object_files.file_paths()
-      self.context.log.debug("object_file_paths: {}".format(object_file_paths))
-      all_compiled_object_files.extend(object_file_paths)
-
-    return LinkSharedLibraryRequest.with_external_libs_product(
+      if compiled_objects_product.get(dep_tgt):
+        self.context.log.debug("dep_tgt: {}".format(dep_tgt))
+        object_files = self._retrieve_single_product_at_target_base(compiled_objects_product, dep_tgt)
+        self.context.log.debug("object_files: {}".format(object_files))
+        object_file_paths = object_files.file_paths()
+        self.context.log.debug("object_file_paths: {}".format(object_file_paths))
+        all_compiled_object_files.extend(object_file_paths)
+
+    external_lib_dirs = []
+    external_lib_names = []
+    if external_libs_product is not None:
+      for nelf in external_libs_product.get_for_targets(deps):
+        if nelf.lib_dir:
+          external_lib_dirs.append(nelf.lib_dir)
+        external_lib_names.extend(nelf.lib_names)
+
+    return LinkSharedLibraryRequest(
       linker=self.linker,
       object_files=tuple(all_compiled_object_files),
       native_artifact=vt.target.ctypes_native_library,
       output_dir=vt.results_dir,
-      external_libs_product=external_libs_product)
+      external_lib_dirs=tuple(external_lib_dirs),
+      external_lib_names=tuple(external_lib_names))
 
   _SHARED_CMDLINE_ARGS = {
     'darwin': lambda: ['-Wl,-dylib'],

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

@@ -10,6 +10,7 @@
 from collections import defaultdict
 
 from pants.backend.native.config.environment import Executable
+from pants.backend.native.targets.external_native_library import ExternalNativeLibrary
 from pants.backend.native.targets.native_library import NativeLibrary
 from pants.backend.native.tasks.native_external_library_fetch import NativeExternalLibraryFiles
 from pants.backend.native.tasks.native_task import NativeTask
@@ -51,7 +52,7 @@ class NativeCompile(NativeTask, AbstractClass):
   # operate on for `strict_deps` calculation.
   # NB: `source_target_constraint` must be overridden.
   source_target_constraint = None
-  dependent_target_constraint = SubclassesOf(NativeLibrary)
+  dependent_target_constraint = SubclassesOf(ExternalNativeLibrary, NativeLibrary)
 
   # `NativeCompile` will use `workunit_label` as the name of the workunit when executing the
   # compiler process. `workunit_label` must be set to a string.
@@ -128,14 +129,14 @@ def execute(self):
     source_targets = self.context.targets(self.source_target_constraint.satisfied_by)
 
     with self.invalidated(source_targets, invalidate_dependents=True) as invalidation_check:
-      for vt in invalidation_check.invalid_vts:
+      for vt in invalidation_check.all_vts:
         deps = self.native_deps(vt.target)
         self._add_product_at_target_base(native_deps_product, vt.target, deps)
-        compile_request = self._make_compile_request(vt, deps, external_libs_product)
-        self.context.log.debug("compile_request: {}".format(compile_request))
-        self._compile(compile_request)
+        if not vt.valid:
+          compile_request = self._make_compile_request(vt, deps, external_libs_product)
+          self.context.log.debug("compile_request: {}".format(compile_request))
+          self._compile(compile_request)
 
-      for vt in invalidation_check.all_vts:
         object_files = self.collect_cached_objects(vt)
         self._add_product_at_target_base(object_files_product, vt.target, object_files)
 
@@ -192,18 +193,19 @@ def get_compiler(self):
   def _compiler(self):
     return self.get_compiler()
 
-  def _get_third_party_include_dirs(self, external_libs_product):
+  def _get_third_party_include_dirs(self, external_libs_product, dependencies):
     if not external_libs_product:
       return []
 
-    directory = external_libs_product.include_dir
-    return [directory] if directory else []
+    return [nelf.include_dir
+            for nelf in external_libs_product.get_for_targets(dependencies)
+            if nelf.include_dir]
 
   def _make_compile_request(self, versioned_target, dependencies, external_libs_product):
     target = versioned_target.target
 
     include_dirs = [self._include_dirs_for_target(dep_tgt) for dep_tgt in dependencies]
-    include_dirs.extend(self._get_third_party_include_dirs(external_libs_product))
+    include_dirs.extend(self._get_third_party_include_dirs(external_libs_product, dependencies))
 
     sources_and_headers = self.get_sources_headers_for_target(target)
 
@@ -221,12 +223,12 @@ def _make_compile_argv(self, compile_request):
     err_flags = ['-Werror'] if compile_request.fatal_warnings else []
 
     # 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] +
       compiler.extra_args +
       err_flags +
+      # TODO: If we need to produce static libs, don't add -fPIC! (could use Variants -- see #5788).
       ['-c', '-fPIC'] +
       [
         '-I{}'.format(os.path.join(buildroot, inc_dir))

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

@@ -15,10 +15,10 @@
 from pants.backend.native.targets.external_native_library import ExternalNativeLibrary
 from pants.base.build_environment import get_pants_cachedir
 from pants.base.exceptions import TaskError
+from pants.goal.products import UnionProducts
 from pants.invalidation.cache_manager import VersionedTargetSet
 from pants.task.task import Task
 from pants.util.contextutil import environment_as
-from pants.util.dirutil import safe_mkdir
 from pants.util.memo import memoized_property
 from pants.util.objects import Exactly, datatype
 from pants.util.process_handler import subprocess
@@ -94,6 +94,10 @@ def register_options(cls, register):
     register('--conan-remotes', type=list, default=['https://conan.bintray.com'], advanced=True,
              fingerprint=True, help='The conan remote to download conan packages from.')
 
+  @classmethod
+  def implementation_version(cls):
+    return super(NativeExternalLibraryFetch, cls).implementation_version() + [('NativeExternalLibraryFetch', 0)]
+
   @classmethod
   def subsystem_dependencies(cls):
     return super(NativeExternalLibraryFetch, cls).subsystem_dependencies() + (Conan.scoped(cls),)
@@ -103,7 +107,10 @@ def product_types(cls):
     return [NativeExternalLibraryFiles]
 
   @property
-  def cache_target_dirs(self):
+  def create_target_dirs(self):
+    # We create per-target directories in order to act as isolated collections of fetched files.
+    # But do not attempt to automatically cache then (cache_target_dirs), because the entire resolve
+    # must be have its own merged cachekey/VT.
     return True
 
   @memoized_property
@@ -128,40 +135,35 @@ def execute(self):
       with self.invalidated(native_lib_tgts,
                             invalidate_dependents=True) as invalidation_check:
         resolve_vts = VersionedTargetSet.from_versioned_targets(invalidation_check.all_vts)
-        vts_results_dir = self._prepare_vts_results_dir(resolve_vts)
         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._fetch_packages(vt)
 
-        native_external_libs_product = self._collect_external_libs(vts_results_dir)
+        native_external_libs_product = self._collect_external_libs(invalidation_check.all_vts)
         self.context.products.register_data(NativeExternalLibraryFiles,
                                             native_external_libs_product)
 
-  def _prepare_vts_results_dir(self, vts):
-    """
-    Given a `VersionedTargetSet`, prepare its results dir.
-    """
-    vt_set_results_dir = os.path.join(self.workdir, vts.cache_key.hash)
-    safe_mkdir(vt_set_results_dir)
-    return vt_set_results_dir
-
-  def _collect_external_libs(self, results_dir):
+  def _collect_external_libs(self, vts):
     """
     Sets the relevant properties of the task product (`NativeExternalLibraryFiles`) object.
     """
-    lib_dir = os.path.join(results_dir, 'lib')
-    include_dir = os.path.join(results_dir, 'include')
-
-    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:
-          lib_names.append(lib_name)
-
-    return NativeExternalLibraryFiles(include_dir=include_dir,
-                                      lib_dir=lib_dir,
-                                      lib_names=tuple(lib_names))
+    product = UnionProducts()
+    for vt in vts:
+      lib_dir = os.path.join(vt.results_dir, 'lib')
+      include_dir = os.path.join(vt.results_dir, 'include')
+
+      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:
+            lib_names.append(lib_name)
+
+      nelf = NativeExternalLibraryFiles(include_dir=include_dir,
+                                        lib_dir=lib_dir,
+                                        lib_names=tuple(lib_names))
+      product.add_for_target(vt.target, [nelf])
+    return product
 
   def _get_conan_data_dir_path_for_package(self, pkg_dir_path, pkg_sha):
     return os.path.join(self.workdir,
@@ -237,14 +239,13 @@ def _copy_package_contents_from_conan_dir(self, results_dir, conan_requirement,
     if os.path.exists(src_include):
       copy_tree(src_include, dest_include)
 
-  def _fetch_packages(self, vt, vts_results_dir):
+  def _fetch_packages(self, vt):
     """
     Invoke the conan pex to fetch conan packages specified by a
     `ExternalLibLibrary` target.
 
-    :param vt: a versioned target containing conan package specifications.
-    :param vts_results_dir: the results directory of the VersionedTargetSet
-      for the purpose of aggregating package contents.
+    :param vt: a versioned target containing conan package specifications, and with a results_dir
+      that we can clone outputs into.
     """
 
     # NB: CONAN_USER_HOME specifies the directory to use for the .conan data directory.
@@ -274,4 +275,4 @@ def _fetch_packages(self, vt, vts_results_dir):
           )
 
         pkg_sha = conan_requirement.parse_conan_stdout_for_pkg_sha(stdout)
-        self._copy_package_contents_from_conan_dir(vts_results_dir, conan_requirement, pkg_sha)
+        self._copy_package_contents_from_conan_dir(vt.results_dir, conan_requirement, pkg_sha)