Trim input build settings after transition #13971
Conversation
gregestren
added
the
team-Configurability
src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
Outdated
Show resolved
Hide resolved
|
@sdtwigg relevant to your transition naming refactoring plans. |
|
This looks good and is a reasonable idea conceptually. @gregestren can you verify that this 'sits in the right place' wrt where you added Starlark transition caching? |
fmeum
force-pushed
the
trim-default-starlark-settings
branch
2 times, most recently
from
10d8b83
to
fd79299
Compare
fmeum
changed the title
It didn't feel that fully elegant and that's because it used the wrong approach - I missed that there already was logic that was trimming default values, just not all of them. In hindsight that is obvious: If it weren't the case, all no-op transitions would have caused action conflicts (rather than only those with input-only build settings). I updated the PR and would now call it fully "elegant". |
fmeum
force-pushed
the
trim-default-starlark-settings
branch
from
fd79299
to
a4cd39c
Compare
Yes. This code only triggers when the transition is actually being executed, and its output is prepared. Caching should purely wrap around that. |
Very nice catch! And indeed much simpler this round. Thanks for that update. And solid test coverage. |
There was a problem hiding this comment.
Re: #12888 (comment), can you confirm the return {} code path also get this optimization? Maybe a test case for that too?
By convention, BuildOptions should not contain Starlark build settings that are set to their default values. However, if a Starlark transition has a build setting as an input, the value of that setting has to be added to the input BuildOptions temporarily so that it can be read by the transition. Before this commit, if a build setting was an input but not an output of a transition, its default value remained in the BuildOptions and could cause action conflicts. With this commit, all build settings rather than only output build settings are trimmed post-transition if they are set to their default value. Fixes bazelbuild#12888.
fmeum
force-pushed
the
trim-default-starlark-settings
branch
from
a4cd39c
to
c417fe3
Compare
I added a test case verifying that input build settings are trimmed even for |
|
LGTM on my end. PS: Really like how elegant it is now :D |
Yes. Thank you. |
Starlark transition logic temporarily explicitly sets all input build settings of a transition to their defaults. Since bazelbuild#13971, these values are cleared after the transition. However, since then they have also been subject to type validation, which is not only unnecessary, but also breaks in the special case of a string build setting with allow_multiple. With this commit, input-only build settings are unconditionally stripped from the post-transition BuildOptions and do not undergo validation. This is made possible by a refactoring of `StarlarkTransition#validate` that extracts the validation logic into a separate function and also improves some variable names.
Starlark transition logic temporarily explicitly sets all input build settings of a transition to their defaults. Since bazelbuild#13971, these values are cleared after the transition. However, since then they have also been subject to type validation, which is not only unnecessary, but also breaks in the special case of a string build setting with allow_multiple. With this commit, input-only build settings at their literal default values are removed from the post-transition BuildOptions without going through validation. This is made possible by a refactoring of `StarlarkTransition#validate` that extracts the validation logic into a separate function and also improves some variable names.
Starlark transition logic temporarily explicitly sets all input build settings of a transition to their defaults. Since #13971, these values are cleared after the transition. However, since then they have also been subject to type validation, which is not only unnecessary, but also breaks in the special case of a string build setting with allow_multiple. With this commit, input-only build settings are unconditionally stripped from the post-transition BuildOptions and do not undergo validation. This is made possible by a refactoring of `StarlarkTransition#validate` that extracts the validation logic into a separate function and also improves some variable names. Fixes #14894 Closes #14972. PiperOrigin-RevId: 434589143
Starlark transition logic temporarily explicitly sets all input build settings of a transition to their defaults. Since bazelbuild#13971, these values are cleared after the transition. However, since then they have also been subject to type validation, which is not only unnecessary, but also breaks in the special case of a string build setting with allow_multiple. With this commit, input-only build settings are unconditionally stripped from the post-transition BuildOptions and do not undergo validation. This is made possible by a refactoring of `StarlarkTransition#validate` that extracts the validation logic into a separate function and also improves some variable names. Fixes bazelbuild#14894 Closes bazelbuild#14972. PiperOrigin-RevId: 434589143
Starlark transition logic temporarily explicitly sets all input build settings of a transition to their defaults. Since #13971, these values are cleared after the transition. However, since then they have also been subject to type validation, which is not only unnecessary, but also breaks in the special case of a string build setting with allow_multiple. With this commit, input-only build settings are unconditionally stripped from the post-transition BuildOptions and do not undergo validation. This is made possible by a refactoring of `StarlarkTransition#validate` that extracts the validation logic into a separate function and also improves some variable names. Fixes #14894 Closes #14972. PiperOrigin-RevId: 434589143
By convention, BuildOptions should not contain Starlark build settings
that are set to their default values. However, if a Starlark transition
has a build setting as an input, the value of that setting has to be
added to the input BuildOptions temporarily so that it can be read by
the transition.
Before this commit, if a build setting was an input but not an output of
a transition, its default value remained in the BuildOptions and could
cause action conflicts. With this commit, all build settings rather than
only output build settings are trimmed post-transition if they are set
to their default value.
Fixes #12888.