New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler option sets for Native targets #6665

Merged
merged 20 commits into from Nov 2, 2018

Conversation

Projects
None yet
3 participants
@CMLivingston
Contributor

CMLivingston commented Oct 20, 2018

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.

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

@stuhood

This comment has been minimized.

Member

stuhood commented Oct 23, 2018

@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

This comment has been minimized.

Contributor

CMLivingston commented Oct 23, 2018

I'll move forward with compiler_option_sets.

Chris Livingston
@CMLivingston

This comment has been minimized.

Contributor

CMLivingston commented Oct 23, 2018

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.

# 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,

This comment has been minimized.

@stuhood

stuhood Oct 23, 2018

Member

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.

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

This comment has been minimized.

@stuhood

stuhood Oct 23, 2018

Member

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.

@@ -28,6 +28,8 @@ class NativeCompileRequest(datatype([
'include_dirs',
'sources',
('fatal_warnings', bool),
('ndebug', bool),

This comment has been minimized.

@stuhood

stuhood Oct 23, 2018

Member

Will need an update.

@@ -176,21 +178,22 @@ def _make_compile_request(self, versioned_target, dependencies, external_libs_pr
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),

This comment has been minimized.

@stuhood

stuhood Oct 23, 2018

Member

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

@CMLivingston

This comment has been minimized.

Contributor

CMLivingston commented Oct 23, 2018

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

@stuhood

This comment has been minimized.

Member

stuhood commented Oct 24, 2018

@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 stuhood requested a review from cosmicexplorer Oct 24, 2018

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

@CMLivingston CMLivingston force-pushed the CMLivingston:clivingston/bump-conan-182 branch from 8eba2b6 to 6801834 Oct 24, 2018

Show resolved Hide resolved src/python/pants/backend/native/subsystems/conan.py Outdated
class NativeBuildStepSettingsBase(Subsystem, MirroredTargetOptionMixin):
options_scope = 'native-build-step-settings-base'

This comment has been minimized.

@stuhood

stuhood Oct 24, 2018

Member

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 comment has been minimized.

@CMLivingston

CMLivingston Oct 24, 2018

Contributor

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

This comment has been minimized.

@stuhood

stuhood Oct 24, 2018

Member

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)

This comment has been minimized.

@stuhood

stuhood Oct 24, 2018

Member

...oh, nevermind. I misinterpreted that.

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

This comment has been minimized.

@stuhood

stuhood Oct 24, 2018

Member

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.

This comment has been minimized.

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

This comment has been minimized.

@stuhood

stuhood Oct 24, 2018

Member

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.

This comment has been minimized.

@CMLivingston

CMLivingston Oct 24, 2018

Contributor

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.

Show resolved Hide resolved ...ython/pants/backend/native/subsystems/native_build_step_settings_base.py Outdated
Show resolved Hide resolved src/python/pants/backend/native/tasks/native_compile.py Outdated

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

Chris Livingston added some commits Oct 24, 2018

Chris Livingston
@CMLivingston

This comment has been minimized.

Contributor

CMLivingston commented Oct 25, 2018

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!

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

This comment has been minimized.

@stuhood

stuhood Oct 25, 2018

Member

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?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 26, 2018

Contributor

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.

Show resolved Hide resolved src/python/pants/backend/native/subsystems/native_build_step_settings.py Outdated
@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.

Show resolved Hide resolved src/python/pants/backend/native/tasks/native_compile.py Outdated
Show resolved Hide resolved src/python/pants/backend/native/subsystems/native_build_step_settings.py Outdated
import os
def get_generated_shared_lib(lib_name):

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 26, 2018

Contributor

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.

This comment has been minimized.

@CMLivingston

CMLivingston Oct 26, 2018

Contributor

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

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Nov 1, 2018

Contributor

Made #6711 in case anyone gets any smart ideas.

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

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 26, 2018

Contributor

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

Show resolved Hide resolved src/python/pants/backend/native/subsystems/native_build_step_settings.py Outdated
Show resolved Hide resolved src/python/pants/backend/native/subsystems/native_build_step_settings.py Outdated
from pants.util.memo import memoized_property
class NativeBuildStepSettings(Subsystem, MirroredTargetOptionMixin):

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Oct 26, 2018

Contributor

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.

Show resolved Hide resolved src/python/pants/backend/native/subsystems/conan.py Outdated

Chris Livingston added some commits Oct 26, 2018

Chris Livingston
Chris Livingston
Chris Livingston
@CMLivingston

This comment has been minimized.

Contributor

CMLivingston commented Oct 31, 2018

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

Chris Livingston
@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.

Show resolved Hide resolved src/python/pants/backend/native/tasks/native_compile.py Outdated
Show resolved Hide resolved src/python/pants/backend/native/tasks/native_compile.py Outdated
Show resolved Hide resolved src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
Show resolved Hide resolved src/python/pants/option/compiler_option_sets_mixin.py
Show resolved Hide resolved src/python/pants/option/compiler_option_sets_mixin.py
Show resolved Hide resolved .../python_distribution/ctypes_with_extra_compiler_flags/some_more_math.hpp Outdated
import os
def get_generated_shared_lib(lib_name):

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Nov 1, 2018

Contributor

Made #6711 in case anyone gets any smart ideas.

Chris Livingston added some commits Nov 1, 2018

@stuhood

stuhood approved these changes Nov 1, 2018

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

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

This comment has been minimized.

@stuhood

stuhood Nov 1, 2018

Member

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 some commits Nov 1, 2018

Chris Livingston
Chris Livingston
Chris Livingston

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment