Same transition dirs when only cmd line options differs#13580
Same transition dirs when only cmd line options differs#13580ulrfa wants to merge 1 commit intobazelbuild:masterfrom
Conversation
|
@gregestren have you got the time to review this yet? |
|
I'm switching to giving this more attention around this week. Thanks for your patience. |
|
Now I waited more than 11 weeks. @gregestren, when do you think you will have time to review? |
|
I apologize for the wait and appreciate your patience. I was looking at this issue in earnest yesterday - not just this PR but the collection of related issues I think speak to this theme (like #13591 and #12171). I'm trying to stitch together how they'll all fit in my head. Expect a more technical update from me today. I'm also happy to share contact info if you want better ping access. |
gregestren
left a comment
There was a problem hiding this comment.
Thanks again for your patience. Overall this looks great to me.
Before this review I ran through the technicalities on my own to map my own understanding of the right approach. This is exactly the change I would have made. I'm encouraged by this convergence.
I haven't reviewed the test code yet, but hopefully these comments help. No major comments - all in all the base approach looks spot on.
src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
Outdated
Show resolved
Hide resolved
|
Thanks @gregestren for reviewing! Please, see my follow up questions. Yes, encouraging with the convergence about the approach! 😃 |
|
The failing windows test cases seems related to python version, and not this PR. Perhaps they will be resolved when I rebase. |
Ignore user defined build settings only set on
command line and not affected by any transition,
when calculating transitionDirectoryNameFragment
output directory.
Native options on command line already have this
behavior, and this commit provides the same also
for user defined build settings.
This allows reducing transition scalability issues,
by combining transitions with command line options:
a) Setting a limited number of common options via
transitions, which affects large parts of the
code base, but with limited number of variants.
b) When needed, *also* setting a few of many
specific options, each of them affecting only
their parts of the code base, but with many
variants.
Resolves bazelbuild#12731
104f1dd to
8f521dc
Compare
|
I tried to resolve the comments and rebased. @gregestren, note that there is one open conversation waiting for your feedback. @gregestren, is it something more you want from me before this can land? |
|
I also experimentally verified this at the command line with a simple project (building the same target that applies a transition twice, first time with |
|
Hurray! 🎉 🎈 I'm very happy! Thank you @gregestren! |
|
Hey @ulrfa. #14239 (comment) suggests the null check from this PR may not have actually been necessary? Do you remember the considerations behind that line? It seems like the relevant tests still pass with a small modification @sdtwigg made in a pending change. I wonder if the problem was failing if its value is null. |
|
Hi @gregestren, see my answer in #14239 |
Ignore user defined build settings only set on
command line and not affected by any transition,
when calculating transitionDirectoryNameFragment
output directory.
Native options on command line already have this
behavior, and this commit provides the same also
for user defined build settings.
This allows reducing transition scalability issues,
by combining transitions with command line options:
a) Setting a limited number of common options via
transitions, which affects large parts of the
code base, but with limited number of variants.
b) When needed, also setting a few of many
specific options, each of them affecting only
their parts of the code base, but with many
variants.
Resolves #12731
Resolves #13317