Add testproject with C/C++ interdependent targets, fix native backend design mistake by cosmicexplorer · Pull Request #6628 · pantsbuild/pants

cosmicexplorer · 2018-10-14T01:35:14Z

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:

--strict-deps doesn't really make sense as two separate options for C and C++ targets which can depend on each other.
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.

stuhood

Nice cleanup. Thanks!

stuhood · 2018-10-14T16:17:01Z

-  """Any settings relevant to a compiler invocation."""
+class NativeBuildSettings(Subsystem):
+  """Any settings relevant to a compiler and/or linker invocation."""
+  options_scope = 'native-build-settings'


Going from independent C/CPP subsystems to a merged one is a slightly odd, but maybe understandable step. I'm guessing you initially modeled this after the Java and Scala subsystems. If people think of both languages as interchangeable (my guess is that they don't?) then this might be fine... otherwise, doing a bit of work to identify which language a target is building and apply slightly different settings could be valuable in the long run (it's hard to split them later).

Anecdotally: Java and Scala having different strict_deps settings has been a lifesaver, because Scala's compiler has drastically different dep requirements, and it's a much larger step to enable strict deps there.

I think people don't think of the two languages as interchangeable because it has literally not been possible to formulate code as such without sacrificing a goat or two, even in the case of mixed C and C++ code. The reason for the merged subsystem was to make it clear that C and C++ are going to depend on each other the exact same way -- my goal is 0% to let it become a kitchen sink. I think --fatal-warnings is an example of something that could be split back into C and C++-specific subsystems (I'll do that now). Does that address your concern in:

otherwise, doing a bit of work to identify which language a target is building and apply slightly different settings could be valuable in the long run (it's hard to split them later).

I think that --strict-deps as a concept doesn't really exist in traditional C/C++ build systems, and separately, I think if we can support zero-(user-)overhead interdependence of C and C++ targets, it would be nice to uphold. I can make it clear in the docstring that NativeBuildSettings is for "pants stuff and not things like compiler arguments".

Anecdotally: Java and Scala having different strict_deps settings has been a lifesaver, because Scala's compiler has drastically different dep requirements, and it's a much larger step to enable strict deps there.

Could you elaborate on this? It is currently my impression (as in the above paragraph) that because --strict-deps is already this new thing we are introducing into C/C++, and is entirely managed by pants, it makes more sense to merge that setting. What does "drastically different dep requirements" mean?

Also, I wouldn't really call this "modelled" after the Java and Scala subsystems -- I had already made up a quick subsystem base class to calculate subsystem/target mirrored options, I just added a bool option to it, and used the existing strict deps calculation methods. I had originally done this before you did the zinc upgrade and literally made a new DependencyContext base class like I think you did or whatever, regardless that merge conflict was fun.

I want you to know that it took all of my strength to not implement --compiler-option-sets just now but I saved that for later because this PR otherwise works.

Separately, the test failure (in testprojects integration) is because we don't currently have a way to allow any command-line option to supersede a target option like --strict-deps (which was the idea, but it makes it hard to test). I will figure out a low-overhead solution that allows us to have these building in that integration test.

I think I'll just rewrite that BUILD file temporarily in the test, that's easy.

Done, that test passes locally.

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

The indirect method of "caching" dependent target calculation we had previously resulted in `LinkSharedLibraries` not incorporating the value of e.g. `--strict-deps` into its task fingerprint. This fell apart because although we allow C and C++ targets to depend on each other (and therefore `--strict-deps` being different for those target types is undefined -- this led to errors). We collapse the `NativeCompileSettings` subclasses (which take care of calculating dependencies given the value of `--strict-deps`) into a single `NativeBuildSettings` subsystem, which is consumed by compile and link tasks, and calculate the dependencies twice (which there was no reason we shouldn't have been doing before).

CMLivingston · 2018-10-15T17:38:31Z

+    super(NativeBuildSettings, cls).register_options(register)
+
+    register('--strict-deps', type=bool, default=True, fingerprint=True, advanced=True,
+             help='The default for the "strict_deps" argument for targets of this language.')


This help message does not really explain anything to the user who doesn't know about strict deps. Is the full explanation included anywhere in a help message?

This is a trash message that I distinctly remember telling myself to fix a long time ago. Will fix, thanks a lot for catching this.

Should be fixed!

CMLivingston

Just one small comment - Great work, ship it!

jsirois · 2018-10-15T17:47:57Z

-    return NativeToolchain.scoped_instance(self)
-
-  def get_compile_settings(self):
+  def _cpp_compile_settings(self):


Any reason not to inline the @memoized_property and CppCompileSettings.scoped_instance(self) to get_compile_settings?

None! Will remove.

jsirois · 2018-10-15T17:50:11Z


  @memoized_property
  def _cpp_toolchain(self):
    return self._request_single(LLVMCppToolchain, self._native_toolchain).cpp_toolchain


This still confuses me mightily (we're in the context of CppCompile and both LLVMCppToolchain and GCCCppToolchain exist, but LLVMCppToolchain is picked here unconditionally and without explanation), but out of scope.

That's true. Since we now have testing using both compilers (I think not having this was the initial reason for the unconditional selection which nobody else called me out on -- I don't remember why that wasn't resolved the last time you brought this up) this should probably be fixed.

I'm thinking of an enum option --toolchain-type for GCC vs LLVM on NativeBuildSettings which is the easiest thing in the entire world to do.

Left a comment in the thread -- I have a WIP PR that does exactly that but would depend on this getting merged first. It's not that many lines.

jsirois · 2018-10-15T17:54:51Z

+  mapping this subsystem's options attribute name (with underscores) to the corresponding target's
+  keyword argument name.
+  """
+  mirrored_option_to_kwarg_map = None


In NativeCompile below you use

@classproperty def workunit_label(cls): raise NotImplementedError(...)

Any reason not to use this here as well both to gain a method to carry the doc and a bit more robustness?

I literally made the docstring above by moving comments from above that field -- will make this change.

Fixed -- if there's a useful value for what's inside the NotImplementedError(...) I can edit that -- just couldn't think of anything.

jsirois · 2018-10-15T17:57:34Z

+  def get_compile_settings(self):
+    """Return a subclass of NativeBuildStepSettingsBase.
+
+    NB: the result of this method is cached in self._compile_settings and therefore it is only


"...in self._compile_settings" - too much detail. How about just:
"NB: Subclasses will be queried for the compile settings once and the result cached."

Will do, this was a lazy docstring cleanup.

jsirois · 2018-10-15T17:58:49Z

  def get_compiler(self):
    """An instance of `Executable` which can be invoked to compile files.

+    NB: the result of this method is cached in self._compiler and therefore it is only called once!


Ditto. Referring a subclass to internal methods they do not need not know about instead of the behavior they may need to know about.

Will fix, noted.

jsirois · 2018-10-15T18:16:39Z

+  # to compile, and the `dependent_target_constraint` to determine which dependent targets to
+  # operate on for `strict_deps` calculation.
+  # NB: `source_target_constraint` must be overridden.
+  source_target_constraint = None


Another instance of indecision on how to do class properties that must be over-ridden. Pick a style.

= None is a placeholder and always annoyed me with its inflexibility but the prospect of a classproperty hadn't occurred to me. Also noting the indecision, maintaining style across files in a pr isn't something I currently check enough in self-review.

Fixed! Also unclear if there is a useful string to pass to NotImplementedError(...) but otherwise fixed.

cosmicexplorer · 2018-10-16T22:38:32Z

I broke out #6638 to track some errors which might be occurring locally, but which shouldn't affect this PR. I'm going to address the last comment on toolchain selection with a WIP PR that I will create immediately after this lands.

jsirois · 2018-10-17T15:18:45Z

+    """
+    raise NotImplementedError()
+
+  @memoized_classproperty


I'm happy with this change if you are, but I was not bothered by dependent_target_constraint since it just looked like a class constant. It was only source_target_constraint which was commented as needing an override that seemed out of line with what you had done elsewhere for class-level bits that needed to be overridden.

I was thinking it could be useful to have the docstring right there instead of in NativeTask. It certainly doesn't need to be memoized.

jsirois · 2018-10-17T15:19:40Z

-  def _c_toolchain(self):
-    return self._request_single(LLVMCToolchain, self._native_toolchain).c_toolchain
-
  def get_compile_settings(self):


Just checking you mean to drop the memoization when inlining here and in cpp_compile.py below.

This bit was intentional! This was in response to clearing up the docstrings for these abstractmethods in NativeTask to make it clear that this method is only called once (see NativeTask). If it would seem to make more sense to let the leaf tasks handle the memoization, we can move it there.

cosmicexplorer requested review from CMLivingston, jsirois and stuhood October 14, 2018 01:35

cosmicexplorer mentioned this pull request Oct 14, 2018

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

Merged

stuhood approved these changes Oct 14, 2018

View reviewed changes

cosmicexplorer force-pushed the add-dependencies-ctypes-example branch from 030d62b to 8367461 Compare October 14, 2018 20:20

cosmicexplorer force-pushed the add-dependencies-ctypes-example branch from 8367461 to a4e3b90 Compare October 15, 2018 04:25

cosmicexplorer added 6 commits October 14, 2018 21:26

add ctypes_interop testproject to test depending on other native targets

bcbca4f

add some more pizazz to the TODOs

5039ae4

make the tests pass

f9fbbb1

use self._add_product_at_target_base uniformly

902fc75

split --fatal-warnings into C/C++-specific subsystems

71bda6a

cosmicexplorer force-pushed the add-dependencies-ctypes-example branch from a4e3b90 to 71bda6a Compare October 15, 2018 05:00

CMLivingston reviewed Oct 15, 2018

View reviewed changes

CMLivingston approved these changes Oct 15, 2018

View reviewed changes

jsirois approved these changes Oct 15, 2018

View reviewed changes

cosmicexplorer mentioned this pull request Oct 16, 2018

requirements resolution for some python_dist() target fails when 3rdparty/python/requirements.txt is modified (maybe) #6638

Closed

respond to review comments

c15cacf

jsirois approved these changes Oct 17, 2018

View reviewed changes

cosmicexplorer merged commit f5120a2 into pantsbuild:master Oct 18, 2018

stuhood added the needs-cherrypick [CI] label Oct 18, 2018

stuhood added this to the 1.11.x milestone Oct 18, 2018

stuhood removed the needs-cherrypick [CI] label Oct 18, 2018

stuhood removed this from the 1.11.x milestone Oct 18, 2018

stuhood mentioned this pull request Oct 24, 2018

Compiler option sets for Native targets #6665

Merged

Uh oh!

Uh oh!

Conversation

cosmicexplorer commented Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Result

Uh oh!

stuhood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosmicexplorer Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosmicexplorer Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosmicexplorer Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CMLivingston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosmicexplorer Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosmicexplorer Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cosmicexplorer commented Oct 14, 2018 •

edited

Loading

cosmicexplorer Oct 14, 2018 •

edited

Loading

cosmicexplorer Oct 14, 2018 •

edited

Loading

cosmicexplorer Oct 16, 2018 •

edited

Loading

cosmicexplorer Oct 16, 2018 •

edited

Loading

cosmicexplorer Oct 16, 2018 •

edited

Loading