Prevent action conflicts for exec-starlark-exec transitions

[fmeum]

With this commit, an execution transition will recompute the transition
hash appended to the output directory name if set by any previous
Starlark transition. This also takes into account that the Starlark
transition may transitively affect an option --foo_bar after the exec
transition if it previously affected the corresponding host option
--host_foo_bar.

Previously, an execution transition would reuse the existing hash, which
no longer reflected the current state of native options correctly and
thus led to action conflicts.

Fixes #13464.

[katre]

@sdtwigg is reviewing this.

[gregestren]

Continuing the conversation at #13464 to clarify approaches.

[sdtwigg]

To clarify, I/we have not forgotten about this. There are just a lot of potential concerns (mostly with where to take this API in the future) that we want to address before proceeding.

[fmeum]

@sdtwigg Do you think that we'll be able to get some version of this (possibly without the host_* accounting) into 5.0?

[fmeum]

@gregestren I'm seeing unexpected NullPointerExceptions in the tests: https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/36b11777-1957-4dc8-9914-3bdd82aa6e4a/src%5Ctest%5Cjava%5Ccom%5Cgoogle%5Cdevtools%5Cbuild%5Clib%5Crules%5Ccpp%5CCcLibraryConfiguredTargetTest%5Cattempt_1.log.
Do you have an idea why? I would have expected updateTransitionDirectoryNameFragment to be safe to call at any time.

[gregestren]

I'm not sure. Which line is FunctionTransitionUtil.java:442 in your dev build?

[fmeum]

I'm not sure. Which line is FunctionTransitionUtil.java:442 in your dev build?

This one:

bazel/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java

Line 442 in 01cbfce

.getDefinition()

[gregestren]

I'm not sure why the tests get NullPointerExceptions, I'm sorry. Maybe some mock test infrastructure is missing an important field?

More broadly, can we pause this while we work out #13464 (comment) and the discussion at #13587 (comment)?

The right next steps, in my mind, are for @sdtwigg and I to seed a proper design doc for all of us to focus our thoughts on. I want to be careful not to eliminate action conflicts by an endless layer of ad hoc changes.

[fmeum]

I'm not sure why the tests get NullPointerExceptions, I'm sorry. Maybe some mock test infrastructure is missing an important field?

More broadly, can we pause this while we work out #13464 (comment) and the discussion at #13587 (comment)?

The right next steps, in my mind, are for @sdtwigg and I to seed a proper design doc for all of us to focus our thoughts on. I want to be careful not to eliminate action conflicts by an endless layer of ad hoc changes.

Sounds very good to me, the proposal seems very reasonable and would resolve a cluster of usability and performance issues around transitions, beyond action conflicts.

Let me know when you have started a draft of the design doc and I will see what I can contribute.

Should I close the current PR in favor of this effort?

[sdtwigg]

Ok, I was originally confused by the part of the this change (and corresponding discussion) regarding transitioning on --host_* values. (For reference, that seems tricky but we should discuss the merits as a separate issue.)

I need to find a proper issue to post the longer-form summary of our plan around ST-hash computation; however, for this specific sub-issue, I was planning to fix this issue by moving transitionDirectoryNameFragment out of BuildOptions and into BuildConfigurationValue (previously called BuildConfiguration).

transitionDirectoryNameFragment should wholly be a function of BuildOptions and it is fragile to require all transitions to remember to update it when it touches associated options, native or otherwise. BuildConfigurationValue is already a container for potentially-expensive-to-compute values based off BuildOptions so movement there is a natural fit (e.g. notably OutputDirectories, which ultimately consumed transitionDirectoryNameFragment is already part of BuildConfigurationValue!)

This essentially makes this PR irrelevant (although considering it did expose other potential performance problems with the the execution transition).

[sdtwigg]

See #14023 (comment)

[fmeum]

Ok, I was originally confused by the part of the this change (and corresponding discussion) regarding transitioning on --host_* values. (For reference, that seems tricky but we should discuss the merits as a separate issue.)

I need to find a proper issue to post the longer-form summary of our plan around ST-hash computation; however, for this specific sub-issue, I was planning to fix this issue by moving transitionDirectoryNameFragment out of BuildOptions and into BuildConfigurationValue (previously called BuildConfiguration).

transitionDirectoryNameFragment should wholly be a function of BuildOptions and it is fragile to require all transitions to remember to update it when it touches associated options, native or otherwise. BuildConfigurationValue is already a container for potentially-expensive-to-compute values based off BuildOptions so movement there is a natural fit (e.g. notably OutputDirectories, which ultimately consumed transitionDirectoryNameFragment is already part of BuildConfigurationValue!)

This essentially makes this PR irrelevant (although considering it did expose other potential performance problems with the the execution transition).

I fully agree that moving the transitionDirectoryNameFragment computation is a much better solution. I will close this PR in favor of this approach. Depending on how it is implemented, the host_* options might lead to action conflicts again, but we can just keep that in mind and e.g. reuse the test case from this PR.

[fmeum]

@sdtwigg Should I create a new PR containing just the test (probably only the one without --host_ flags)? It could serve as a useful regression test for upcoming changes to transition logic.