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

Enable passing option sets to the compiler #6065

Merged
merged 7 commits into from Jul 12, 2018

Conversation

Projects
None yet
2 participants
@alanbato
Copy link
Member

alanbato commented Jul 3, 2018

Problem

The issue at #5360

Solution

I've added another option, --compiler-option-sets, which takes a list of option sets which then get translated to the actual flags that you want to pass to the compiler. These flags should be defined in pants.ini or through the command line.

Result

Users can now enable compiler flags on a project or session basis instead of the whole repo.

PS. This is a work in progress, existing tests are failing and new ones have not been implemented. Making the PR now so that it can be reviewed as it progresses.

@stuhood
Copy link
Member

stuhood left a comment

This looks like a great start, thanks Alan!

@@ -22,6 +22,10 @@ def register_options(cls, register):
fingerprint=True,
help='The default for the "fatal_warnings" argument for targets of this language.')

register('--compiler-option-sets', advanced=True, default=[], type=list,
fingerprint=True,
help='The option sets to be enabled for the compilation of this target.')

This comment has been minimized.

@stuhood

stuhood Jul 3, 2018

Member

In this case, the --compiler-option-sets flag will end up defining the "default" options sets that are used for targets of a particular type (the ZincLanguageMixin is mixed in for "scala" and "java").

If a target passes the compiler_option_sets= argument, they'll be overriding the options sets.

This comment has been minimized.

@alanbato

alanbato Jul 3, 2018

Member

Awesome!
But 🤔
So we don't want them to pass down the compiler_option_sets= argument?
Either way, we can't hardcode the default option sets just yet, can we?

I'm not entirely sure where you were going with this comment 😅

PS. I'm using a list instead of a set because sets are not json serializable!

This comment has been minimized.

@stuhood

stuhood Jul 3, 2018

Member

So we don't want them to pass down the compiler_option_sets= argument?

Yes, they are allowed to do that. But if they do not do that, we use the default from this option.

This comment has been minimized.

@stuhood

stuhood Jul 3, 2018

Member

This should be the same as with fatal_warnings... the option defined in ZincLanguageMixin is the default value of fatal_warnings for a target... if they don't set the option explicitly on their target, we use the default.

This comment has been minimized.

@stuhood

stuhood Jul 12, 2018

Member

The help for this option should likely be updated to align with the help for the deprecated fatal_warnings option: ie:

The default for the "compiler_option_sets" argument for targets of this language.

@alanbato alanbato force-pushed the alanbato:compiler_option_sets branch from dcfd0c7 to 2183b3c Jul 9, 2018

@alanbato alanbato force-pushed the alanbato:compiler_option_sets branch from 2183b3c to 7ffa217 Jul 10, 2018

@alanbato alanbato changed the title [WIP] Enable passing option sets to the compiler Enable passing option sets to the compiler Jul 10, 2018

@@ -104,16 +104,22 @@ def __init__(self,
entity_description='fatal_warnings',
hint_message="fatal_warnings should be defined as part of the target compiler_option_sets"
)
if fatal_warnings and 'fatal_warnings' not in compiler_option_sets:
compiler_option_sets.append('fatal_warnings')
if fatal_warnings is not None:

This comment has been minimized.

@alanbato

alanbato Jul 10, 2018

Member

This bunch of code is handling the case where fatal_warnings is set to false on the target, but it might be also either in the compiler_option_sets target params or CLI options.
This is the minimal way I could think of, thoughts?
I'd like to see this all removed once fatal_warnings is removed as an actual option/param :P

This comment has been minimized.

@stuhood

stuhood Jul 10, 2018

Member

One option might be to have the internal representation of compiler_options_sets temporarily be a dict from key to bool?

This comment has been minimized.

@stuhood

stuhood Jul 10, 2018

Member

...oh. This is already nicer than that.

alanbato added some commits Jul 11, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks a lot! Looks good. Two little issues.

@@ -24,6 +24,7 @@ def __init__(self, address=None, payload=None, **kwargs):
payload = payload or Payload()
payload.add_fields({
'platform': PrimitiveField(None),
'compiler_option_sets': SetOfPrimitivesField(None)

This comment has been minimized.

@stuhood

stuhood Jul 12, 2018

Member

This should also use PrimitivesSetField.

@@ -22,6 +22,10 @@ def register_options(cls, register):
fingerprint=True,
help='The default for the "fatal_warnings" argument for targets of this language.')

register('--compiler-option-sets', advanced=True, default=[], type=list,
fingerprint=True,
help='The option sets to be enabled for the compilation of this target.')

This comment has been minimized.

@stuhood

stuhood Jul 12, 2018

Member

The help for this option should likely be updated to align with the help for the deprecated fatal_warnings option: ie:

The default for the "compiler_option_sets" argument for targets of this language.
@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jul 12, 2018

So, given that this is green and CI is borked, I'm going to go ahead and merge it.

@alanbato : Can you include the changes I mentioned in a followup PR?

@stuhood stuhood merged commit 9f13b5e into pantsbuild:master Jul 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alanbato

This comment has been minimized.

Copy link
Member

alanbato commented Jul 13, 2018

Sure!
Thanks a lot for your help Stu!

stuhood added a commit that referenced this pull request Jul 13, 2018

Use PrimitivesSetField in ScalaJs target and minor help text fixup (#…
…6113)

### Problem

Fixup the things requested at #6065

### Solution

s/SetOfPrimitivesField/PrimitivesSetField

and a change to the wording in the help text of the `compiler_option_sets` default options.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment