Review Request 4374: Use generic option to pass extra zinc compile options

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4374/

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang. Bugs: 4052 Repository: pants Description Problem: We have introduced multiple additional zinc options that can be turned on and off at the target level, for example, fatal_warnings, zinc_file_manager. Adding additional param becomes clunky. Solution: Introduce two parameters --compile-zinc-extra-compile-options and compile-zinc-default-extra-compile-options where settings can be referred by names and can be controlled at the global as well as target level. Note: In this review the control of both fatal_warnings and zinc_file_manager is moved out of the language level, I don't see how they need to be treated differently java vs scala. strict_deps is still at the language level. Testing https://travis-ci.org/peiyuwang/pants/builds/175808198 Diffs contrib/scalajs/src/python/pants/contrib/scalajs/targets/scala_js_target.py (2bd4beb88d6549220bc62d49ae5906fa872b3aae) src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (cb8d40acbabbc1ef9d5cb404cfa46ad1f15b9a82) src/python/pants/backend/jvm/targets/jvm_target.py (ea4cf2b132b9c18b7f77b3251fba284f7325d808) src/python/pants/backend/jvm/tasks/jvm_compile/BUILD (b7915069680ba4290900c5f44f5b0e46f20893c4) src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (355fd9d228fd78fdbc64bec4c7a6f05d528c63c7) src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (2c3af071fa08a227da77560b8d37b3256b5eed42) src/python/pants/util/filtering.py (7d3dbc451e5481c82fce00b5be99ad790b00784a) testprojects/src/java/org/pantsbuild/testproject/bench/BUILD (72be0e85ae8e3ea24fd87670b60f1a41acc02fd1) testprojects/src/java/org/pantsbuild/testproject/compilation_warnings/BUILD (a4244eb731b0af87686d354d49f1907849d1a0b5) testprojects/src/scala/org/pantsbuild/testproject/compilation_warnings/BUILD (e39f9c4c78fb2803388027ea141bd9f65955283c) tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py (bf3a0d66fc4fa90600449e7228a59a688150c978) tests/python/pants_test/backend/jvm/tasks/jvm_compile/zinc/zinc_compile_integration_base.py (58cb7069dafecce836efd0bd30ff2d6c1f6a1c68) View Diff

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4374/

src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (Diff revision 1)
		27	removal_hint='Please use --compile-zinc-default-extra-compile-options.')

It remains important to be able to specify different defaults for these settings based on which language is in play... so I think that the extra-compile-options should likely be on the zinc_language_mixin rather than on zinc itself.

src/python/pants/backend/jvm/targets/jvm_target.py (Diff revision 1)
		78	:param extra_compile_options: List of zinc options that reference to compile option settings
		79	by their names.
		80	:type extra_compile_options: list

This will be the public documentation string for this option, so it deserves a lot more attention... in particular, this should make it clear where the option sets are configured (under --${lang}-extra-compile-options, etc), and that this is not the literal options themselves.

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (Diff revision 1)
		205	"Optionally prefix '+' to enable, and '-' to disable")

This is already natively supported by list options? Might be good to just refer to the documentation for list options, or refer to this technique by name: http://www.pantsbuild.org/options.html#list-options

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (Diff revision 1)
		434	for option_name in option_names:
		435	modifier, option_name_configured = extract_modifier(option_name)

Aha, I see what you were doing with the modifiers. This makes sense, but it's going to be very, very important to ensure that the behaviour is consistent with the command line behaviour of list_option.
I also wonder whether it wouldn't be a good idea to delay adding support for +/- modifiers for now, since we're not expecting huge lists of options to exist anytime soon.

- Stu Hood

On November 14th, 2016, 7:30 p.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang.

Updated Nov. 14, 2016, 7:30 p.m.

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4374/

On November 14th, 2016, 8:50 p.m. UTC, Stu Hood wrote:

src/python/pants/backend/jvm/targets/jvm_target.py (Diff revision 1)
		78	:param extra_compile_options: List of zinc options that reference to compile option settings
		79	by their names.
		80	:type extra_compile_options: list

This will be the public documentation string for this option, so it deserves a lot more attention... in particular, this should make it clear where the option sets are configured (under --${lang}-extra-compile-options, etc), and that this is not the literal options themselves.

To be clear, where fatal-warnings lives now is on --scala-platform-fatal-warnings or --java-fatal-warnings, and that's a good thing.

Also, to take back one thing I said earlier: I think that we might not want to move strict_deps to this system. In particular, strict_deps is not an extra argument to the compiler... rather, it affects pants' behaviour. And we want it to affect pants' behaviour even more by affecting invalidation.

- Stu

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4374/

On November 14th, 2016, 8:50 p.m. UTC, Stu Hood wrote:

src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (Diff revision 1)
		27	removal_hint='Please use --compile-zinc-default-extra-compile-options.')

It remains important to be able to specify different defaults for these settings based on which language is in play... so I think that the extra-compile-options should likely be on the zinc_language_mixin rather than on zinc itself.

will move extra_compile_options into language mixin.

On November 14th, 2016, 8:50 p.m. UTC, Stu Hood wrote:

src/python/pants/backend/jvm/targets/jvm_target.py (Diff revision 1)
		78	:param extra_compile_options: List of zinc options that reference to compile option settings
		79	by their names.
		80	:type extra_compile_options: list

This will be the public documentation string for this option, so it deserves a lot more attention... in particular, this should make it clear where the option sets are configured (under --${lang}-extra-compile-options, etc), and that this is not the literal options themselves.

On November 14th, 2016, 8:53 p.m. UTC, Stu Hood wrote:

To be clear, where fatal-warnings lives now is on --scala-platform-fatal-warnings or --java-fatal-warnings, and that's a good thing.

Also, to take back one thing I said earlier: I think that we might not want to move strict_deps to this system. In particular, strict_deps is not an extra argument to the compiler... rather, it affects pants' behaviour. And we want it to affect pants' behaviour even more by affecting invalidation.

will improve comment

On November 14th, 2016, 8:50 p.m. UTC, Stu Hood wrote:

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (Diff revision 1)
		434	for option_name in option_names:
		435	modifier, option_name_configured = extract_modifier(option_name)

Aha, I see what you were doing with the modifiers. This makes sense, but it's going to be very, very important to ensure that the behaviour is consistent with the command line behaviour of list_option.
I also wonder whether it wouldn't be a good idea to delay adding support for +/- modifiers for now, since we're not expecting huge lists of options to exist anytime soon.

+/- or enable/disable is needed because there are three states: enable, disable, not present. If it's just two, being present and not present is sufficient.
from twitter repo:
fatal_warnings_enabled_args: [
    '-S-deprecation:false',
    '-C-Xlint:-deprecation',
    '-S-Xfatal-warnings',
    '-C-Werror',
  ]
fatal_warnings_disabled_args: [
    '-S-deprecation',
  ]


to be clear, +/- here is a little arbitrary, I just need a more concise representation for negation, ! would also work, i guess, if that helps with confusion.

- Peiyu

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4374/

On November 14th, 2016, 8:50 p.m. UTC, Stu Hood wrote:

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (Diff revision 1)
		434	for option_name in option_names:
		435	modifier, option_name_configured = extract_modifier(option_name)

Aha, I see what you were doing with the modifiers. This makes sense, but it's going to be very, very important to ensure that the behaviour is consistent with the command line behaviour of list_option.
I also wonder whether it wouldn't be a good idea to delay adding support for +/- modifiers for now, since we're not expecting huge lists of options to exist anytime soon.

On November 15th, 2016, 1:15 a.m. UTC, Peiyu Wang wrote:

+/- or enable/disable is needed because there are three states: enable, disable, not present. If it's just two, being present and not present is sufficient.
from twitter repo:
fatal_warnings_enabled_args: [
    '-S-deprecation:false',
    '-C-Xlint:-deprecation',
    '-S-Xfatal-warnings',
    '-C-Werror',
  ]
fatal_warnings_disabled_args: [
    '-S-deprecation',
  ]


to be clear, +/- here is a little arbitrary, I just need a more concise representation for negation, ! would also work, i guess, if that helps with confusion.

should have mentioned, the alternative is not using modifier but just add fatal_warning_disabled, zinc_file_manager_disabled as separate config entries, which is fine too.

- Peiyu

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4374/

On November 14th, 2016, 8:50 p.m. UTC, Stu Hood wrote:

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (Diff revision 1)
		434	for option_name in option_names:
		435	modifier, option_name_configured = extract_modifier(option_name)

Aha, I see what you were doing with the modifiers. This makes sense, but it's going to be very, very important to ensure that the behaviour is consistent with the command line behaviour of list_option.
I also wonder whether it wouldn't be a good idea to delay adding support for +/- modifiers for now, since we're not expecting huge lists of options to exist anytime soon.

On November 15th, 2016, 1:15 a.m. UTC, Peiyu Wang wrote:

+/- or enable/disable is needed because there are three states: enable, disable, not present. If it's just two, being present and not present is sufficient.
from twitter repo:
fatal_warnings_enabled_args: [
    '-S-deprecation:false',
    '-C-Xlint:-deprecation',
    '-S-Xfatal-warnings',
    '-C-Werror',
  ]
fatal_warnings_disabled_args: [
    '-S-deprecation',
  ]


to be clear, +/- here is a little arbitrary, I just need a more concise representation for negation, ! would also work, i guess, if that helps with confusion.

On November 15th, 2016, 1:18 a.m. UTC, Peiyu Wang wrote:

should have mentioned, the alternative is not using modifier but just add fatal_warning_disabled, zinc_file_manager_disabled as separate config entries, which is fine too.

IMO: "not present" == "disable", but it would be good to check with Matt Landis about this. I think the case of "disabled options" and "enabled options" like that could potentially be better modeled as just independent sets (perhaps via a no- prefix, similar to command line arguments).
In particular, I think the fatal_warnings_enabled_args and fatal_warnings_disabled_args sets are more a symptom of not having enough flexibility in how options are specified.

- Stu

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4374/

On November 14th, 2016, 8:50 p.m. UTC, Stu Hood wrote:

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (Diff revision 1)
		434	for option_name in option_names:
		435	modifier, option_name_configured = extract_modifier(option_name)

Aha, I see what you were doing with the modifiers. This makes sense, but it's going to be very, very important to ensure that the behaviour is consistent with the command line behaviour of list_option.
I also wonder whether it wouldn't be a good idea to delay adding support for +/- modifiers for now, since we're not expecting huge lists of options to exist anytime soon.

On November 15th, 2016, 1:15 a.m. UTC, Peiyu Wang wrote:

+/- or enable/disable is needed because there are three states: enable, disable, not present. If it's just two, being present and not present is sufficient.
from twitter repo:
fatal_warnings_enabled_args: [
    '-S-deprecation:false',
    '-C-Xlint:-deprecation',
    '-S-Xfatal-warnings',
    '-C-Werror',
  ]
fatal_warnings_disabled_args: [
    '-S-deprecation',
  ]


to be clear, +/- here is a little arbitrary, I just need a more concise representation for negation, ! would also work, i guess, if that helps with confusion.

On November 15th, 2016, 1:18 a.m. UTC, Peiyu Wang wrote:

should have mentioned, the alternative is not using modifier but just add fatal_warning_disabled, zinc_file_manager_disabled as separate config entries, which is fine too.

On November 15th, 2016, 1:48 a.m. UTC, Stu Hood wrote:

IMO: "not present" == "disable", but it would be good to check with Matt Landis about this. I think the case of "disabled options" and "enabled options" like that could potentially be better modeled as just independent sets (perhaps via a no- prefix, similar to command line arguments).
In particular, I think the fatal_warnings_enabled_args and fatal_warnings_disabled_args sets are more a symptom of not having enough flexibility in how options are specified.

Another example: --warning-args also has 3 states:
compile.zinc.no_warning_args = ['-S-nowarn'] (from CONFIG)
compile.zinc.warning_args = ['-S-deprecation', '-S-unchecked', '-S-feature'] (from CONFIG)


This looks pretty reasonable: no_warning needs to set -S-nowarn

- Peiyu

[Benjy Weinberger]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4374/

I like it in general, but it might make sense for fatal_warnings should remain separate, because it can be thought of as a language-level setting, not a compiler implementation detail setting.

- Benjy Weinberger

On November 14th, 2016, 7:30 p.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang.

Updated Nov. 14, 2016, 7:30 p.m. Bugs: 4052

[Mateo Rodriguez]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4374/

So I am all for moving to a generic way of passing options to zinc. But how to interact with the new system is not very clear - sometimes the hints just say use extra_compile_options and other times it recommends default_extra_options. I eventually saw more info in the tests, but that would not have been clear to me if I had just hit the deprecation warning.
Can you add an example to the docs or at least inlined in the code?

- Mateo Rodriguez

On November 14th, 2016, 1:30 p.m. CST, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang.

Updated Nov. 14, 2016, 1:30 p.m.