Skip to content
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

Fix ordering of args from compiler_option_sets and add test for scalac profiling #7683

Merged

Conversation

Projects
None yet
5 participants
@cosmicexplorer
Copy link
Contributor

commented May 8, 2019

Problem

The options from --compiler-option-sets-enabled-args or --compiler-option-sets-disabled-args for tasks which mix in CompilerOptionSetsMixin don't keep their ordering, see:

This breaks the ability to use options which require two successive arguments via compiler option sets. Scalac profiling is an example of this, requiring a successive output file argument.

Solution

  • Use a list to collect compiler_option_sets arguments.
  • Add an integration test for scalac profiling.

Result

Scalac profiling is now possible using a compiler option set!

@stuhood

stuhood approved these changes May 8, 2019

@CMLivingston
Copy link
Contributor

left a comment

The set usage was mainly to de-dupe args in the event that they are specified twice and this adversely affects the compilation process somehow. Admittedly, I don't know whether this impacts zinc or clang/gcc. If this came up we could probably cast as an OrderedSet and return a list from that. LGTM.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Ah, ok, thanks for the rationale! I'll add a comment about deduplication / casting to an OrderedSet.

@stuhood

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Ah, ok, thanks for the rationale! I'll add a comment about deduplication / casting to an OrderedSet.

List is probably preferable, because both ordering and dupes can matter. If you end up with --enable --disable --enable, the result should be enable.

@cosmicexplorer cosmicexplorer changed the title Fix ordering of args from compiler_option_sets and add test for scalac profiling [WIP] Fix ordering of args from compiler_option_sets and add test for scalac profiling May 9, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Sorry, yes, list is absolutely the correct behavior. As noted in a slack channel with @stuhood and @ShaneDelmore just now, we almost definitely want to make sure that option sets get applied at the end of the argv in order to give them the highest precedence -- that should be fixed and tested in this PR too.

@cosmicexplorer cosmicexplorer added this to the 1.16.x milestone May 9, 2019

cosmicexplorer added some commits May 8, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-option-sets-ordering branch from 0b25eba to 65fb671 May 10, 2019

@cosmicexplorer cosmicexplorer changed the title [WIP] Fix ordering of args from compiler_option_sets and add test for scalac profiling Fix ordering of args from compiler_option_sets and add test for scalac profiling May 10, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

we almost definitely want to make sure that option sets get applied at the end of the argv in order to give them the highest precedence -- that should be fixed and tested in this PR too.

It turns out this was already the case, so I'm comfortable enough about not adding further testing.

@Eric-Arellano
Copy link
Contributor

left a comment

Great use of comprehensions in the source file.

Thanks!

@@ -38,6 +38,31 @@ def test_scala_compile_jar(self):
self.assertTrue(jar.getinfo(SHAPELESS_CLSFILE),
'Expected a jar containing the expected class.')

# TODO: this could be converted into a unit test!
def test_scala_profile_can_be_generated(self):
with temporary_dir() as tmp_dir:

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 10, 2019

Contributor

It's not immediately clear from the title why this would not be the case. I think it would be helpful to link to this PR's description with a brief note explaining why we have to test this case.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 10, 2019

Author Contributor

Will do!

@mateor

mateor approved these changes May 10, 2019

@@ -62,16 +62,23 @@ def get_merged_args_for_compiler_option_sets(self, compiler_option_sets):
compiler_option_sets = self.get_options().default_compiler_option_sets
logger.debug('using default option sets: {}'.format(compiler_option_sets))

compiler_options = set()
compiler_options = []

This comment has been minimized.

Copy link
@mateor

mateor May 10, 2019

Member

This feature could really use some docs, not sure who originally added the feature. I struggled to trace the logic when I tripped over it a few months ago.

Not a blocking issue for your PR, just floating a casual feature request in case the original author is reading. 🙈

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 10, 2019

Author Contributor

It was added at the very end of an intern rotation last year (for which we were very grateful), but as a result didn't get the depth of initial review it probably should have. This could definitely be better-documented, especially as its semantics are being fixed here, and a related-but-different feature recently just changed again in #7594. I'll make a followup issue/PR for adding a section to https://www.pantsbuild.org/options.html on this I think?

@cosmicexplorer cosmicexplorer merged commit fd3a865 into pantsbuild:master May 14, 2019

cosmicexplorer added a commit that referenced this pull request May 14, 2019

Fix ordering of args from compiler_option_sets and add test for scala…
…c profiling (#7683)

### Problem

The options from `--compiler-option-sets-enabled-args` or `--compiler-option-sets-disabled-args` for tasks which mix in `CompilerOptionSetsMixin` don't keep their ordering, see: https://github.com/pantsbuild/pants/blob/5eed2e7be5aa4672485f06e043bcec24bb9c668d/src/python/pants/option/compiler_option_sets_mixin.py#L65

This breaks the ability to use options which require two successive arguments via compiler option sets. Scalac profiling is an example of this, requiring a successive output file argument.

### Solution

- Use a list to collect `compiler_option_sets` arguments.
- Add an integration test for scalac profiling.

### Result

Scalac profiling is now possible using a compiler option set!

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request May 14, 2019

Fix ordering of args from compiler_option_sets and add test for scala…
…c profiling (pantsbuild#7683)

### Problem

The options from `--compiler-option-sets-enabled-args` or `--compiler-option-sets-disabled-args` for tasks which mix in `CompilerOptionSetsMixin` don't keep their ordering, see: https://github.com/pantsbuild/pants/blob/5eed2e7be5aa4672485f06e043bcec24bb9c668d/src/python/pants/option/compiler_option_sets_mixin.py#L65

This breaks the ability to use options which require two successive arguments via compiler option sets. Scalac profiling is an example of this, requiring a successive output file argument.

### Solution

- Use a list to collect `compiler_option_sets` arguments.
- Add an integration test for scalac profiling.

### Result

Scalac profiling is now possible using a compiler option set!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.