Split conan resolve product by target to enforce declared dependencies #6492

stuhood · 2018-09-12T00:35:23Z

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.

stuhood · 2018-09-12T20:52:36Z

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


  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))
+    print('>>> include_dirs: {}'.format(include_dirs))


stuhood · 2018-09-12T20:57:32Z

This is reviewable.

jsirois

A test would be nice.

jsirois · 2018-09-13T01:54:22Z

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

-  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


s/then/them/

jsirois · 2018-09-13T01:59:41Z

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


-        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)


This is confusion waiting to happen. The type is declared as NativeExternalLibraryFiles but the instance is a UnionProducts. Maybe s/NativeExternalLibraryFiles/NativeExternalLibrary/ and then subclass UnionProducts as NativeExternalLibraries which now contains a union of NativeExternalLibrary.

I think the above sounds like a good idea -- unsure if we would need to override the copy() method to return a NativeExternalLibrary instead of a UnionProducts, because I think the UnionProducts method would take precedence over the datatype version.

cosmicexplorer · 2018-09-13T18:19:52Z

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

-      all_compiled_object_files.extend(object_file_paths)
-
-    return LinkSharedLibraryRequest.with_external_libs_product(
+      if compiled_objects_product.get(dep_tgt):


Unsure whether we should accept the lack of a product for that target and not error out -- I think we should be able to assume the product for the compiled output exists in the mapping (even if the product contains no files), unless there's some polymorphism happening here I'm missing.

Ok, that actually makes sense. Maybe a comment on the fact that dep_tgt could be either an external library or a source library could be useful, as that spooky action happens at a distance in NativeCompile and isn't immediately evident in this file.

cosmicexplorer · 2018-09-13T18:19:58Z

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


-      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)


The execute method is easier to follow now, thanks.

cosmicexplorer · 2018-09-13T18:20:04Z

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


-        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)


I think the above sounds like a good idea -- unsure if we would need to override the copy() method to return a NativeExternalLibrary instead of a UnionProducts, because I think the UnionProducts method would take precedence over the datatype version.

cosmicexplorer · 2018-09-13T18:25:59Z

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

-      lib_names = external_libs_product.lib_names
-
-    return cls(*args, external_lib_dirs=lib_dirs, external_lib_names=lib_names, **kwargs)
+  pass


This factory method is definitely unnecessary thanks to this PR, I neglected to mention that, thanks for reading my mind.

cosmicexplorer · 2018-09-13T18:36:52Z

A very simple test for this could be in test_ctypes.py which just tests that compilation of a file with #include <rang.hpp> or something fails when you don't depend on the Conan library target (but the rang conan library target is defined in the same build file and used in another target? Let me know if it's not clear how to phrase the test). I'm pretty sure testing the link phase requires #6486, so I wouldn't worry about that for now.

CMLivingston

LGTM. Thanks for taking care of this! One simple way of integration testing could be to have two separate ctyes_compatible_cpp_library targets that both include cereal.h,and then assert that the build/run fails with the correct compilation error. This should be green on master due to the lack of target-based partitioning.

Also while reviewing this, it occurred to be that there might be static and dynamic object files flying around in various use cases, and there is not a whole lot of differentiation or option declarations around these. Afaict, a static archive might make it into a LinkSharedLibrariesRequest via NativeExternalLibrary. As I understand it, we can link both types when depending on a Conan package, so it may be worth it at some point in the future to refactor the class/method names to convey this flexibility.

cosmicexplorer · 2018-09-13T20:03:14Z

Also while reviewing this, it occurred to be that there might be static and dynamic object files flying around in various use cases, and there is not a whole lot of differentiation or option declarations around these. Afaict, a static archive might make it into a LinkSharedLibrariesRequest via NativeExternalLibrary. As I understand it, we can link both types when depending on a Conan package, so it may be worth it at some point in the future to refactor the class/method names to convey this flexibility.

Totally agree on specifically noting the difference between static and shared libs -- we'll find a way to do that cleanly (as you describe) in #6486 after this is merged, I think (because we don't have any examples of using 3rdparty shared libs in this PR yet). But I'm 100% with you on making these easily differentiable -- not munging together different types of native build products haphazardly is something I think is super important to maintain.

I mentioned the use of test_ctypes.py because (1) using unit tests wherever possible seems preferable in general (2) every test we add is another test to run on the osx "platform-specific testing" shard, so if we can avoid integration tests (which by virtue of calling pants are likely more resource-intensive), I would think we would try to do so if at all possible. If there's a good reason to make a new testproject anyway, that could change the calculus. But my impression is that integration tests are almost always going to take more time, and this seems testable without that.

cosmicexplorer · 2018-10-14T03:49:27Z

I made this into #6630 because all the work/review here was done and it unblocks landing each of these in parallel since I can fix any merge conflicts.

stuhood · 2018-10-14T16:11:25Z

Thanks... sorry about that!

…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.

@CMLivingston

… design mistake (#6628) ### Problem *This probably blocks #6486 and #6492.* As @CMLivingston and I realized sometime this week (to my *immense* dismay), we had no tests of native library targets depending on each other. When making these examples, I realized two things: 1. `--strict-deps` doesn't really make sense as two separate options for C and C++ targets which can depend on each other. 2. The implementation of `--strict-deps` in the native backend ("caching" native target interdependencies in a product from the compiler tasks) was absurd and unnecessary. I think I vaguely recall that the fact that `LinkSharedLibraries` previously did not have a subsystem dependency on the `CCompileSettings` or `CppCompileSettings` subsystems may have led to a subtle caching bug in the link task when options in those subsystems were modified, but I don't remember all the details right now. Regardless, that bug would have been fixed with this PR -- see the below: ### Solution - Move dependency calculation into a single `NativeBuildSettings` subsystem which is consumed by compile and link tasks, and use it to calculate the dependencies in both tasks. - Drop the very unnecessary `NativeTargetDependencies` product. - Move a ton of logic into `NativeTask` that really should have been there instead of `NativeCompile`, but couldn't because of the hacked together dependency calculation. - Add a testproject with C and C++ targets depending on each other, and an integration test showing the above all works. ### Result We have a working example of C and C++ targets depending on each other, resulting in idiomatic enough C and C++, which also work extremely effectively in a `python_dist()` target.

Split conan resolve by native_external_library targets.

a4db043

stuhood commented Sep 12, 2018

View reviewed changes

stuhood changed the title ~~WIP: Split conan resolve by native_external_library targets.~~ Split conan resolve product by target to enforce declared dependencies Sep 12, 2018

stuhood requested review from cosmicexplorer, CMLivingston and jsirois September 12, 2018 20:57

stuhood mentioned this pull request Sep 12, 2018

Expose C++ 3rdparty libraries from Conan only to the targets that depend on the 3rdparty library #6178

Closed

jsirois approved these changes Sep 13, 2018

View reviewed changes

cosmicexplorer mentioned this pull request Sep 13, 2018

we can now depend on non-header-only libraries from conan by building them ourselves #6486

Closed

cosmicexplorer reviewed Sep 13, 2018

View reviewed changes

CMLivingston reviewed Sep 13, 2018

View reviewed changes

CMLivingston approved these changes Sep 13, 2018

View reviewed changes

This was referenced Oct 14, 2018

Add testproject with C/C++ interdependent targets, fix native backend design mistake #6628

Merged

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

Merged

cosmicexplorer closed this Oct 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split conan resolve product by target to enforce declared dependencies #6492

Split conan resolve product by target to enforce declared dependencies #6492

stuhood commented Sep 12, 2018 •

edited

Loading

stuhood Sep 12, 2018

stuhood commented Sep 12, 2018

jsirois left a comment

jsirois Sep 13, 2018

jsirois Sep 13, 2018

cosmicexplorer Sep 13, 2018

cosmicexplorer Sep 13, 2018

cosmicexplorer Sep 13, 2018

cosmicexplorer Sep 13, 2018

cosmicexplorer Sep 13, 2018

cosmicexplorer Sep 13, 2018

cosmicexplorer commented Sep 13, 2018 •

edited

Loading

CMLivingston left a comment •

edited

Loading

cosmicexplorer commented Sep 13, 2018

cosmicexplorer commented Oct 14, 2018 •

edited

Loading

stuhood commented Oct 14, 2018

Split conan resolve product by target to enforce declared dependencies #6492

Split conan resolve product by target to enforce declared dependencies #6492

Conversation

stuhood commented Sep 12, 2018 • edited Loading

Problem

Solution

Result

Choose a reason for hiding this comment

stuhood commented Sep 12, 2018

jsirois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer commented Sep 13, 2018 • edited Loading

CMLivingston left a comment • edited Loading

Choose a reason for hiding this comment

cosmicexplorer commented Sep 13, 2018

cosmicexplorer commented Oct 14, 2018 • edited Loading

stuhood commented Oct 14, 2018

stuhood commented Sep 12, 2018 •

edited

Loading

cosmicexplorer commented Sep 13, 2018 •

edited

Loading

CMLivingston left a comment •

edited

Loading

cosmicexplorer commented Oct 14, 2018 •

edited

Loading