Starlark Transition Composition

[katre]

We've had native transition composition for a long time, let's make that available to Starlark users!

Design proposal here: https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md

Tagging @gregestren as reviewer, but anyone with comments feel free to contribute.

[fmeum]

I am overall very supportive of this addition, it would help clean up a bunch of indirections required to combine an exec transition with a non-exec transition.

3. The actual configuration change applies the first transition, and then applies the second transition to each split of the result.

What if the second transition in transition1 + transition2 is the split transition? It could make sense to formalize this section with references to the individual transitions in such a composition.

2. The string "exec", for the execution transition on the default exec group.

Now that there is config.exec, I'm not a fan of this kind of syntactic sugar. transition + "exec" looks magical (or even wrong) to me and breaks in a very bad way as soon as transition == "target". It also requires special-casing in LSPs.

[katre]

To clarify further: the semantics of the exec transition are that trans + "exec" would effectively only be the exec transition, because the current exec transition resets almost all flags to stable values.

[fmeum]

Why does transition == "target" break more than any other transition?

If you have some code in Starlark that does transition + "exec" for a transition obtained from elsewhere and someone passes in "target" or "exec", the result will suddenly be a string such as "execexec", not a transition.

[gregestren]

@fmeum which indirections specifically inspired you?

IMO 99% of the conceptual complexity in this proposal is the exec transition integration. We could even do wild things like invoke a build with a custom exec transition that's defined as a composition of trans1 plus the default exec transition. I think these issues scope-bleed into wider philosophy about what we want the exec transition to be.

If the use cases you're thinking about aren't trying to actually "modify" the exec transition but remain clear cases of "apply arbitrary transition B, then apply the exec transition, recognizing these are unrelated transitions and just happen to be sequenced together", that's IMO easier to reason about now.

[katre]

@fmeum You're right , "exec" + "target" would give the wrong result, which is unfortunate.

To clarify, does this mean you support:

1. Don't allow using shorthand names "exec" and "target" with composition (and expose something like config.target to mean the no-op transition)?
2. Support both transition + "exec" and "exec" + transition (but not, sadly, "exec" + "exec")?

[fmeum]

I support 1. I don't know whether we even need the config.target noop, I would consider usages such as transition + config.exec() if transition else config.exec() reasonable as well.

[sluongng]

Nit: I don't like using the plus + operator here as this does not have the commutative property that's often a part of addition.

Could we use a different operator? If it's not allowed in Starlark, I'm ok with a new built-in function as well.

[fmeum]

@katre Sorry, I accidentally edited your comment on mobile. I restored it.

My reply:

I am fine with the method on a transition, but then we should entirely drop support for the legacy string names for clarity.

Do you mean even disallowing cfg = "exec" on rules? That would be very breaking - why would that need to happen as a precondition?

[katre]

No, those we would leave, but for composition you'd need to use an actual transition object. Is that too restrictive?

[fmeum]

No, those we would leave, but for composition you'd need to use an actual transition object. Is that too restrictive?

No, that sounds good!

[gregestren]

Will config.exec() seamlessly work in the implementation? That's an ExcecutionTransitionFactory and transition is a ConfigurationTransitionApi which if I read correctly is a distinct interface/class.

[katre]

It will require some coding to make it work (and either some overloads or a common superinterface) but it should be supportable.

Or we could get rid of transitions and only use transition factories, but that's a fairly big change IIRC.

I'll clarify the spec on this point as part of marking it accepted.

[katre]

Pushed an update to clarify the precise semantics. @fmeum, I know you were interested, let me know if this is more clear.

[fmeum]

I think it is, thanks!

[gregestren]

LGTM on latest revision.