Change uses of SplitTransitionProvider to TransitionFactory. #7833
Conversation
Removed SplitTransitionProvider. Part of bazelbuild#7814.
jin
added
the
team-Configurability
There was a problem hiding this comment.
Looks good in principle, but I'm somewhat wary of adding unnecessary abstractions (including classes and methods). Approving on the assumption that these flags and classes will have a good reason to exist, put please keep YAGNI in mind. It's better to add abstractions when you need them instead of preemptively.
| * | ||
| * <p>For starlark defined rule class transitions, see {@link StarlarkRuleTransitionProvider}. | ||
| * | ||
| * <p>TODO(bazel-team): Consider allowing dependency-typed attributes to actually return providers | ||
| * instead of just labels (see {@link SkylarkAttributesCollection#addAttribute}). | ||
| */ | ||
| public class StarlarkAttributeTransitionProvider implements SplitTransitionProvider { | ||
| public class StarlarkAttributeTransitionProvider | ||
| implements TransitionFactory<RuleTransitionData>, SplitTransitionProviderApi { |
There was a problem hiding this comment.
nit: rename SplitTransitionProviderApi to TransitionFactoryApi. After all, the class is not a SplitTransitionProvider anymore.
There was a problem hiding this comment.
I'll leave it up to @juliexxia whether to rename, she was discussing a future plan to separate starlark patch and split transitions.
There was a problem hiding this comment.
This is confusing to me too. What's the purpose of SplitTransitionProviderApi? It looks like just an empty interface to expose SplitTransitionProvider to Starlark? And what does it mean to say isSplit() is true below?
My understanding is this may or may not be a split. So for the scenarios where it isn't a split what do those words mean?
(I guess this is mostly a question for @juliexxia).
There was a problem hiding this comment.
I think how it stands makes sense for this PR. I can always do clean up later.
Currently all Starlark transitions attached to attributes (i.e., StarlarkAttributeTransitionProvider transitions) are natively treated like SplitTransitions. This isn't something that's exposed to the user (i.e. if you return a map instead of a map of maps, that works just fine), just something that was done so we didn't have to handle patch and split transitions here.
SplitTransitionProviderApi was originally introduced for the benefit of exposing MultiArchSplitTransitionProvider to Starlark. That's it's only use case other than this. So SplitTransitionProviderApi is still technically the truth. And TransitionFactory seems to obscure the difference between split and patch transitions - @katre as a catch up question, are those both going to fall under this? I think it's mostly important in regards to the restriction that split transitions can't be attached directly to rules.
There was a problem hiding this comment.
Yes, TransitionFactory can produce both PatchTransition or SplitTransition. I had considered making it generic in which type it returns (so the signature would become TransitionFactory<PatchTransition, RuleTransitionData>), but
- This ends up being cumbersome to use, and
- The actual usage would be that Attribute has TransitionFactory<?, RuleTransitionData> so the difference isn't meaningful.
Actual code that cares can check the value of TransitionFactory.isSplit to see if the factory thinks it's returning a SplitTransition or not.
...java/com/google/devtools/build/lib/analysis/skylark/StarlarkAttributeTransitionProvider.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/rules/objc/MultiArchSplitTransitionProvider.java
Show resolved
Hide resolved
...java/com/google/devtools/build/lib/analysis/skylark/StarlarkAttributeTransitionProvider.java
Show resolved
Hide resolved
| * | ||
| * <p>For starlark defined rule class transitions, see {@link StarlarkRuleTransitionProvider}. | ||
| * | ||
| * <p>TODO(bazel-team): Consider allowing dependency-typed attributes to actually return providers | ||
| * instead of just labels (see {@link SkylarkAttributesCollection#addAttribute}). | ||
| */ | ||
| public class StarlarkAttributeTransitionProvider implements SplitTransitionProvider { | ||
| public class StarlarkAttributeTransitionProvider | ||
| implements TransitionFactory<RuleTransitionData>, SplitTransitionProviderApi { |
There was a problem hiding this comment.
This is confusing to me too. What's the purpose of SplitTransitionProviderApi? It looks like just an empty interface to expose SplitTransitionProvider to Starlark? And what does it mean to say isSplit() is true below?
My understanding is this may or may not be a split. So for the scenarios where it isn't a split what do those words mean?
(I guess this is mostly a question for @juliexxia).
|
|
||
| @Override | ||
| public void repr(SkylarkPrinter printer) { | ||
| printer.append("<transition object>"); |
There was a problem hiding this comment.
How do these get printed in Starlark with this method removed?
There was a problem hiding this comment.
Only starlark transitions need to be printed from Starlark, and they all implement SplitTransitionProviderApi (and so have a repr method).
As @jcater said, we had a lot of discussion about exactly this. In an ideal world I 100% agree. But given the limitations of how these need to be applied the hope is this can consolidate the different abstractions we already have. |
| Preconditions.checkArgument(attributeMap instanceof ConfiguredAttributeMapper); | ||
| return new FunctionSplitTransition( | ||
| starlarkDefinedConfigTransition, (ConfiguredAttributeMapper) attributeMap); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
How is this method used? Like I mentioned above, StarlarkAttributeTransitions are always SplitTransitions according to native code (for now, I suppose this distinction could be made more clear) but might not actually map to multiple output BuildOptions in practice.
There was a problem hiding this comment.
Callers of TransitionFactory that care whether the transition is a PatchTransition or a SplitTransition can use this to distinguish. It's about the return type and the intent, not on how many actual splits are produced.
Ideally at some point we should unify these further, but I'm not working on that immediately.
| * | ||
| * <p>For starlark defined rule class transitions, see {@link StarlarkRuleTransitionProvider}. | ||
| * | ||
| * <p>TODO(bazel-team): Consider allowing dependency-typed attributes to actually return providers | ||
| * instead of just labels (see {@link SkylarkAttributesCollection#addAttribute}). | ||
| */ | ||
| public class StarlarkAttributeTransitionProvider implements SplitTransitionProvider { | ||
| public class StarlarkAttributeTransitionProvider | ||
| implements TransitionFactory<RuleTransitionData>, SplitTransitionProviderApi { |
There was a problem hiding this comment.
I think how it stands makes sense for this PR. I can always do clean up later.
Currently all Starlark transitions attached to attributes (i.e., StarlarkAttributeTransitionProvider transitions) are natively treated like SplitTransitions. This isn't something that's exposed to the user (i.e. if you return a map instead of a map of maps, that works just fine), just something that was done so we didn't have to handle patch and split transitions here.
SplitTransitionProviderApi was originally introduced for the benefit of exposing MultiArchSplitTransitionProvider to Starlark. That's it's only use case other than this. So SplitTransitionProviderApi is still technically the truth. And TransitionFactory seems to obscure the difference between split and patch transitions - @katre as a catch up question, are those both going to fall under this? I think it's mostly important in regards to the restriction that split transitions can't be attached directly to rules.
Removed SplitTransitionProvider.
Part of #7814.