Compiler option sets for Native targets by CMLivingston · Pull Request #6665 · pantsbuild/pants

CMLivingston · 2018-10-20T00:46:54Z

Problem

Native targets do not have support for compiler option sets.

Solution

Add support for compiler options sets for use in native compile tasks.

Result

Users can specify option set defaults and their corresponding default enabled values in Pants.ini. Native targets support option sets.

…header files

stuhood · 2018-10-23T00:04:20Z

@CMLivingston talked about this offline... I had initially suggested that it would be easier to add compiler_options_sets in a followup change, but I think that I was wrong about that. Sorry @CMLivingston !

@cosmicexplorer : If you agree that it is worthwhile to do compiler_option_sets here, that would be my vote.

CMLivingston · 2018-10-23T00:09:01Z

I'll move forward with compiler_option_sets.

CMLivingston · 2018-10-23T01:01:06Z

Took a first pass at option sets. @cosmicexplorer I tried to capture what you were talking about as using a mixin for shared default option set keys. Please let me know if this is at all related to what you were describing. More testing to come after I get the green light. Thanks!

stuhood

Looks reasonable, thanks.

stuhood · 2018-10-23T03:08:58Z

-    # flags!
-    register('--fatal-warnings', type=bool, default=True, fingerprint=True, advanced=True,
-             help='The default for the "fatal_warnings" argument for targets of this language.')
+    register('--fatal-warnings-enabled-args', advanced=True, type=list, fingerprint=True,


The changes to the fatal-warnings option should not be necessary here: can leave it as is, and then it can be deprecated in a followup.

stuhood · 2018-10-23T03:13:38Z

+             help='Extra compiler args to use when fatal warnings are disabled.')
+    register('--compiler-option-sets-enabled-args', advanced=True, type=dict, fingerprint=True,
+             default={
+              'fatal_warnings': list(cls.get_fatal_warnings_enabled_args_default()),


Given that this backend is fairly young, I don't think that you should worry about attempting to preserve backwards compatibility here. So it should be fine to have the default enabled/disabled args just be empty.

stuhood · 2018-10-23T03:14:54Z

    'include_dirs',
    'sources',
    ('fatal_warnings', bool),
+    ('ndebug', bool),


Will need an update.

stuhood · 2018-10-23T03:16:31Z

      include_dirs=include_dirs,
      sources=sources_and_headers,
      fatal_warnings=self._compile_settings.get_fatal_warnings_value_for_target(target),
+      compiler_option_sets_options=self._compile_settings.get_merged_compiler_options_for_target(target),


At this point they will have been flattened, so you can probably just call this field compiler_options.

CMLivingston · 2018-10-23T22:00:38Z

@stuhood regarding inheritance of NativeBuildSettings, is there any reason to use subsystem dependencies over direct inheritance here?

stuhood · 2018-10-24T04:04:25Z

@stuhood regarding inheritance of NativeBuildSettings, is there any reason to use subsystem dependencies over direct inheritance here?

There are a few things to think about, and I'm not sure I can recommend the appropriate thing here, but I know that @cosmicexplorer can:

Using a Subsystem always means creating a new "namespace". If you scope the subsystem (MySubsystem.scoped(cls)) then the new namespace is scoped above the dependee's scope (my-subsystem-scope.my-dependee-scope). If you use a global subsystem (MySubsystem), then you're just creating a new independent namespace/scope (my-subsystem-scope).

If you use inheritance, then you're just adding the options directly to some existing (Task, maybe/generally?) scope, rather than creating a new one.

stuhood

Thanks.

stuhood · 2018-10-24T22:09:28Z


 class NativeBuildStepSettingsBase(Subsystem, MirroredTargetOptionMixin):

+  options_scope = 'native-build-step-settings-base'


I think that this class is supposed to be abstract, and then mixed in to a language specific subsystem? If so, it shouldn't need an options_scope to be defined, because the subclass will handle that.

This is here to allow for base options set definitions that can be shared between C and CPP (different from default option set keys).

Ok. Do we think that that is a blocker? Might be easier to wait to add that flexibility if need be.

Or to go the other way? I know that in a previous revision @cosmicexplorer mentioned that he wasn't sure that the C/C++ language split was carrying its weight: #6628 (review)

...oh, nevermind. I misinterpreted that.

stuhood · 2018-10-24T22:11:02Z

+    compiler_options = set()
+
+    # Set values for enabled options.
+    if compiler_option_sets:


Below you directly access the compiler_option_sets variable, which implies that it is not None within this method... if that's the case, it should be fine to just have this loop at the top level rather than guarded by the if.

stuhood · 2018-10-24T22:13:13Z

+                         .get_fatal_warnings_value_for_target(target)),
+      compiler_options=(self._compile_settings
+                           ._native_build_step_settings
+                           .get_merged_compiler_options_for_target(target,


It looks like the get_merged_compiler_options_for_target method already includes the enabled/disabled args dicts... are the arguments here a different collection of options? If so, is there a usecase for that? It's sortof confusing.

These arguments are to compose the dicts for the subclasses into the calculation. The dicts that are pulled from self.get_options() belong to the base class.

CMLivingston · 2018-10-25T22:38:09Z

ping @stuhood, this is in a good state for review when you can.

stuhood

This is very close, but there is one big open question. Thanks!

stuhood · 2018-10-25T23:23:24Z

+from pants.util.memo import memoized_property
+
+
+class NativeBuildStepSettings(Subsystem, MirroredTargetOptionMixin):


I'm not completely clear on why this is a different subsystem than NativeBuildSettings.

Rather than adding this subsystem, would it be useful to just define the new options on that subsystem? And if they need to be split for some reason, could you add docstrings to both of them explaining why?

The difference is described in #6486 -- https://github.com/pantsbuild/pants/blob/5e320b721c234cdc6a56e5c762441a87741d85d7/src/python/pants/backend/native/subsystems/native_build_step_settings_base.py. They are named like this because that PR is still in development. The names could maybe be improved -- I don't like calling everything "settings" but want to differentiate it from e.g. the compile tasks.

cosmicexplorer

I would really like the implementation of compiler_option_sets to be shared with the JVM implementation for this to be merged, ideally as a mixin (something similar to e.g. MirroredTargetOptionMixin). Even getting the hang of understanding how to use compiler_option_sets has been described as difficult to me from users consuming the JVM version, and I really don't want to introduce further idiosyncracies.

The reason why I want this is because we'll never have to make any subsystems just to add a compiler option ever again, and because then every other subsystem will have a deep knowledge of which settings to apply in which scenarios, which could potentially make it extremely easy to then implement reliable and granular caching of native artifacts in a way, yet again, that no other build tool can do.

cosmicexplorer · 2018-10-26T00:03:56Z

+import os
+
+
+def get_generated_shared_lib(lib_name):


Question unrelated to this specific code -- is there a way we can package the ctypes dylibs in a way that allows us to access it like a JVM resource instead of hardcoding the relative path? Not anything to fix in this PR specifically.

I'm not sure. I think there would need to be coherence between the compile tasks and the resource target handling. However, I think resources just pull in the same dir structure as the path you provide to the resource files themselves (I'm not too familiar with that target type).

Made #6711 in case anyone gets any smart ideas.

cosmicexplorer · 2018-10-26T00:05:04Z

+    ]
+    pants_run = self.run_pants(command=command, config={
+      'native-build-step-settings.cpp-compile-settings': {
+        'compiler_option_sets_enabled_args': {


This makes me pretty happy to see, we are changing the C/C++ game every step of the way.

cosmicexplorer · 2018-10-26T00:10:32Z

+from pants.util.memo import memoized_property
+
+
+class NativeBuildStepSettings(Subsystem, MirroredTargetOptionMixin):


The difference is described in #6486 -- https://github.com/pantsbuild/pants/blob/5e320b721c234cdc6a56e5c762441a87741d85d7/src/python/pants/backend/native/subsystems/native_build_step_settings_base.py. They are named like this because that PR is still in development. The names could maybe be improved -- I don't like calling everything "settings" but want to differentiate it from e.g. the compile tasks.

… and native compiles

CMLivingston · 2018-10-31T00:30:55Z

This is in a great state for a final pass @stuhood @cosmicexplorer.

cosmicexplorer

I think we can extract --fatal-warnings-* args more in the future, but I think the notice I left on deprecating the specific options should be enough for this PR. I left a comment on #6710 to finish any cleanup work that might be necessary beyond that so it doesn't block this PR.

cosmicexplorer · 2018-11-01T06:31:38Z

+import os
+
+
+def get_generated_shared_lib(lib_name):


Made #6711 in case anyone gets any smart ideas.

stuhood

Looks good! Just a naming nit. Thanks for iterating here.

stuhood · 2018-11-01T21:55:40Z

+
+class NativeBuildStepSettings(CompilerOptionSetsMixin, MirroredTargetOptionMixin, Subsystem):
+
+  options_scope = 'native-build-step-settings'


This is still pretty verbose. Putting the words "options" or "settings" in the names of options is probably superfluous.

In this case you're ending up with a subsystem instance scope named native-build-step-settings.c-compile-settings.

Chris Livingston added 5 commits October 19, 2018 10:57

Bump conan and add ExternalNativeLibrarys as a valid native dep

45a8c06

Extra compiler options for native library targets and do not compile …

81df8cf

…header files

Add a simple int test for a compiler flag

4b6abdc

Cleanup comments

0f06f91

Add simple boolean options for ndebug and cxx abi compatibility

a23a830

CMLivingston changed the title ~~(WIP) Extra compiler args for cpp_library targets~~ Add 2 extra compiler args for native targets Oct 22, 2018

Refactor into compiler option sets

97f439f

stuhood reviewed Oct 23, 2018

View reviewed changes

stuhood requested a review from cosmicexplorer October 24, 2018 04:06

CMLivingston changed the title ~~Add 2 extra compiler args for native targets~~ Compiler option sets for Native targets Oct 24, 2018

Use subsystem deps rather than inheritance

6801834

CMLivingston force-pushed the clivingston/bump-conan-182 branch from 8eba2b6 to 6801834 Compare October 24, 2018 21:18

stuhood reviewed Oct 24, 2018

View reviewed changes

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

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

Chris Livingston added 2 commits October 24, 2018 15:31

Address Stu's comments

a322b5a

Get tests passing with scoped compiler options

9daaa15

stuhood reviewed Oct 25, 2018

View reviewed changes

cosmicexplorer requested changes Oct 26, 2018

View reviewed changes

Chris Livingston added 4 commits October 26, 2018 16:57

Refactor compiler option sets into a mixin for shared use between jvm…

c6dad29

… and native compiles

Add mirrored option map

9df8022

simplify helper API for getting merged compiler options

7941017

Refactor merging logic

90a4319

Remove Cruft

cff4b49

cosmicexplorer mentioned this pull request Nov 1, 2018

merge DependencyContext and MirroredTargetOptionMixin from the JVM and native backends #6710

Closed

cosmicexplorer approved these changes Nov 1, 2018

View reviewed changes

Chris Livingston added 2 commits November 1, 2018 13:05

Use classpropertys for overrides

cd96382

Kill fatal_warnings arg for native library targets

16a32e3

stuhood approved these changes Nov 1, 2018

View reviewed changes

Chris Livingston added 4 commits November 1, 2018 15:07

Trim long options scope name

967dd31

Fix small Travis CI issue

9635a53

Simplify the testproject to circumvent weirdness in CI

06bd64b

Skip testproject

f3e7997

stuhood merged commit f8c02be into pantsbuild:master Nov 2, 2018

stuhood removed needs-cherrypick [CI] labels Feb 5, 2019


		class NativeBuildStepSettingsBase(Subsystem, MirroredTargetOptionMixin):

		options_scope = 'native-build-step-settings-base'

		from pants.util.memo import memoized_property


		class NativeBuildStepSettings(Subsystem, MirroredTargetOptionMixin):


		class NativeBuildStepSettings(CompilerOptionSetsMixin, MirroredTargetOptionMixin, Subsystem):

		options_scope = 'native-build-step-settings'

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Problem

Solution

Result

Uh oh!

stuhood commented Oct 23, 2018

Uh oh!

CMLivingston commented Oct 23, 2018

Uh oh!

CMLivingston commented Oct 23, 2018

Uh oh!

stuhood 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!

CMLivingston commented Oct 23, 2018

Uh oh!

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

Uh oh!

Uh oh!

stuhood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CMLivingston Oct 24, 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!

stuhood Oct 24, 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!

Uh oh!

Uh oh!

CMLivingston commented Oct 25, 2018

Uh oh!

stuhood 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!

Uh oh!

cosmicexplorer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

CMLivingston commented Oct 20, 2018 •

edited

Loading

stuhood commented Oct 24, 2018 •

edited

Loading

CMLivingston Oct 24, 2018 •

edited

Loading

stuhood Oct 24, 2018 •

edited

Loading