Add --compilation_mode support in transitions

[moroten]

Add support for converting //command_line_option:compilation_mode from
Java value to Starlark value so that it can be read in a transition.

Fixes --compilation_mode in #10690, but not the general case.

This patch does not contain any tests. What kind of test is expected and where should it be implemented?

[aiuto]

I would look at the tests in src/test/j.c.g/devtools/build/lib to find examples of how we test for this.

@alandonovan You had comments in #10690

[juliexxia]

I think StarlarkAttrTransitionProviderTest would be a good place to write a test where the set up would be a transition that transitions on //command_line_option:compilation_mode and asserts that the transitioned value is as expected. A thorough test would also read //command_line_option:compilation_mode to make sure that's working as well.

Here's a specific test case that is somewhat close to that (no need to make it a split transition) https://cs.opensource.google/bazel/bazel/+/master:src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java;l=120

Could also add a similar e2e test here if you're interested: https://cs.opensource.google/bazel/bazel/+/master:src/test/shell/integration/starlark_configurations_test.sh?q=starlark_configurations_test.sh%20&ss=bazel

[sdtwigg]

The broader issue here has come up in conversation before where our current Starlark-Java interface for these commandline options in transitions easily allows Java objects without a well-defined Starlark API to leak into the Starlark interpreter context.

I'm hesitant to approve piecemeal fixes since they could interfere with future broad fixes.

The (rough and currently not being worked on) plan for this is to add another interface that options parsing Converters can inherit, tentatively called StarlarkConvert. It would contain toStarlark (T -> Object) and fromStarlark (Object -> T) methods. The transitions infrastructure would be amended to use these methods when passing values to and from Starlark code (options not inheriting that interface cannot be used in transitions). There are a bunch of commonly used Converters that, if amended, would immediately apply the feature to a majority of options.

[moroten]

Rebased and added a test.

I agree with @sdtwigg that a broader solution would be welcome, but the time plan for it is unfortunately suffering.

In this PR, I only address //command_line_option:compilation_mode. Being able to read cpu, compilation_mode and platform_suffix helps when setting a manual output directory name (see #7477), which can improve cache hit ratio before #6526 has been solved.

[gregestren]

@moroten which cache miss scenarios are you trying to address?

[moroten]

The cache miss scenario is when building the following:

bazel test --platforms=//:my_armv8_platform --run_under=qemu //...
bazel build //:release_package

The //:release_package does a split transition to put binaries for many platforms in a .tar.gz file, including documentation. By setting //command_line_option:output directory name manually in the transition, the second command can reuse the compiled object files and libraries from the first command. (Ideally, we would like to add test and coverage report in the package as well.)

Annoyingly, the analysis cache is discarded between these commands so checking for cache hits remotely takes some time (see #10875). Running both steps in a single command is not possible due to the //... target pattern and because you can't depend (and apply a transition) on test and coverage results.

[moroten]

By enabling //command_line_option:compilation_mode as an input, I can allow the user to build the release package using either --compilation_mode=opt or --compilation_mode=fastbuild and still produce the matching //command_line_option:output directory name.

[gregestren]

I see, thanks.

@sdtwigg would this PR actually interfere with a broader solution? As ad hoc changes go it's not terribly hackish, and I don't see this opening floodgate exceptions for all the other options. If it's this vs. nothing maybe it's worthwhile.

[sdtwigg]

After the weekend mental refresh, I am less concerned that this would interfere with the broader solution.

That said, can you add a TODO (you can link back to this bug or specifically my comment before) that other places should strongly avoid replicating this fix and look in to implementing the proper fix for other desired flags.

[meisterT]

What's the state of this PR?

[moroten]

I am sorry I missed @sdtwigg's last comment. I have now added the missing TODO in the code.

[sdtwigg]

I think this line specifically out of date. Need to amend this so the tests still pass and I can import this into Bazel.

[moroten]

I've rebased and letting Buildkite run the tests.

[gregestren]

Thanks for rebasing, Fredrik. @sdtwigg ready to merge?