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

Use both the deprecated and new locations of fatal_warnings args #6798

Merged
merged 3 commits into from Nov 21, 2018

Conversation

Projects
None yet
3 participants
@stuhood
Member

stuhood commented Nov 21, 2018

Problem

Currently it is necessary to empty the fatal_warnings_(enabled|disabled)_args list in order for the values specified in compiler_options_sets to be used. But emptying the list (rather than using its default) requires actually specifying the deprecated options, which triggers a deprecation warning.

Solution

Include either the values from fatal_warnings_(enabled|disabled)_args and compiler_options_sets depending on which has been explicitly set, to ensure that the options are consumed in either situation.

Result

It's possible to migrate to compiler_options_sets without triggering a deprecation warning.

Use both the deprecated and new locations of fatal_warnings enabled/d…
…isabled args, to support the case where someone has migrated from the old location to the new location.
@wisechengyi

This comment has been minimized.

Contributor

wisechengyi commented Nov 21, 2018

Failed test at

test_combination('fatal', expect_success=True,
extra_args=['--compile-zinc-fatal-warnings-enabled-args=[\'-C-Werror\']'])
seems legit

@wisechengyi

This comment has been minimized.

Contributor

wisechengyi commented Nov 21, 2018

The final compiler_options specifically for

./pants clean-all compile testprojects/src/scala/org/pantsbuild/testproject/compilation_warnings:fatal --compile-zinc-fatal-warnings-enabled-args="['-C-Werror']"

this patch:

set([u'-S-Xfatal-warnings', '-C-Werror'])

master:

set(['-C-Werror'])

the -S-X might be tricky here.

@stuhood

This comment has been minimized.

Member

stuhood commented Nov 21, 2018

@wisechengyi : Have applied a more rigorous approach and added a test. Please take a look.

@wisechengyi

Thanks! The logic is quite twisted. Let's get rid of them as early as possible. cc @cosmicexplorer

@wisechengyi

This comment has been minimized.

Contributor

wisechengyi commented Nov 21, 2018

hmm but the deprecation warning still shows up tho..does it require config change?

@stuhood

This comment has been minimized.

Member

stuhood commented Nov 21, 2018

hmm but the deprecation warning still shows up tho..does it require config change?

Yes: should remove usage of the fatal_warnings_.*_args options, and use only compiler_option_sets_.*_args.

@CMLivingston

LGTM

@stuhood stuhood merged commit 0e11666 into pantsbuild:master Nov 21, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/use-both-deprecated-and-new-fatal-warnings-locations branch Nov 21, 2018

@stuhood stuhood added this to the 1.12.x milestone Nov 21, 2018

wisechengyi added a commit that referenced this pull request Nov 26, 2018

Use both the deprecated and new locations of fatal_warnings args (#6798)
### Problem

Currently it is necessary to empty the `fatal_warnings_(enabled|disabled)_args` list in order for the values specified in `compiler_options_sets` to be used. But emptying the list (rather than using its default) requires actually specifying the deprecated options, which triggers a deprecation warning.

### Solution

Include either the values from `fatal_warnings_(enabled|disabled)_args` and `compiler_options_sets` depending on which has been explicitly set, to ensure that the options are consumed in either situation.

### Result

It's possible to migrate to `compiler_options_sets` without triggering a deprecation warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment