Implement transition.and_then - #29542
Conversation
5091465 to
e33d3b9
Compare
|
FYI @UebelAndre |
katre
left a comment
There was a problem hiding this comment.
The Starlark transition composition implementation is great: it's a very nice approach that I hadn't considered.
I'm confused about the WASM compilation and CHANGELOG changes, are they from a different PR?
| // defined in Starlark via, cfg = transition | ||
| return new StarlarkRuleTransitionProvider(starlarkDefinedConfigTransition); | ||
| } | ||
| if (cfg instanceof ComposedConfigurationTransitionApi composition) { |
There was a problem hiding this comment.
Can this be combined with the equivalent code from StarlarkAttrModule?
| * subsequent transition reads the build settings produced by the previous one. Nested compositions | ||
| * are flattened, so {@link #getElements} is always a flat chain of non-composed transitions. | ||
| */ | ||
| public final class ComposedConfigurationTransitionApi implements ConfigurationTransitionApi { |
There was a problem hiding this comment.
Given that this doesn't define a specific Starlark API:
a) Should this be in a different package (which makes it difficult to use from ConfigurationTransitionApi)?
b) Should this have a different name (just remove the Api)?
There was a problem hiding this comment.
Renamed to ComposedConfigurationTransition (dropping Api) — agree it isn't a Starlark API surface in its own right, it's just the deferred holder backing transition.and_then. Kept it in starlarkbuildapi.config so ConfigurationTransitionApi's default and_then can still reference it; moving it under analysis.config would force every ConfigurationTransitionApi implementation to override and_then to escape the package layering.
| scratch.file( | ||
| "test/rules.bzl", | ||
| """ | ||
| bad_transition = config.exec().and_then(config.exec()) |
There was a problem hiding this comment.
Which line is reported as the error, line 262 or line 266? I want to be sure this is debugabble in more complex situations where transitions are defined in different Starlark files.
There was a problem hiding this comment.
Good question. With the original code the error fired at the use site (the attr.label(cfg = bad_transition) line), which for cross-file definitions left the user to grep for bad_transition to find the actual .and_then call.
To make this debuggable I now capture thread.getCallerLocation() at and_then time and store it on the ComposedConfigurationTransition. The materializer includes that location in the error message, so users see both the cfg= use site (in the standard Starlark traceback) and the original composition site. For this test the error reads:
File ".../test/rules.bzl", line 5, column 31, in <toplevel>
attrs = {"dep": attr.label(cfg = bad_transition)},
Error in label: invalid composed transition for `cfg`: can't compose two exec transitions (composed at /workspace/test/rules.bzl:1:40)
Test now asserts both substrings (attrs = {"dep": attr.label(cfg = bad_transition)}, and composed at /workspace/test/rules.bzl:1:).
There was a problem hiding this comment.
Thanks, this looks much easier to debug.
e33d3b9 to
bf62502
Compare
Adds an `and_then(other)` method to the `transition` Starlark builtin that returns a new transition applying this one followed by `other`. The result is itself a transition and can be composed further. A composition may be used as a rule or attribute transition wherever its component transitions could be used. At most one of the composed factories may target the exec configuration (enforced by `ComposingTransitionFactory.of`, which now throws the new checked `IncompatibleTransitionsException` for invalid compositions; an `ofUnchecked` wrapper is provided for internal call sites where the inputs are structurally guaranteed compatible). When two of the composed transitions are 1:2+, the result has the cross product of their splits; the key for each combined split is the comma-separated concatenation of the component keys. Documents the new feature in `site/en/extending/config.md`. Also fixes `ComposingTransitionFactory.transitionType` to return the more specific of its children's types (previously always the first child's, which was incorrect for `ANY + RULE/ATTRIBUTE` compositions), and updates `DependencyResolutionHelpers.getExecutionPlatformLabel` to find the exec transition factory anywhere inside a composition via `visit`. Adds `StarlarkTransitionCompositionTest` covering all combinations of 1-2 Starlark transitions and 0-1 exec transitions (including S+E+S and split+split), plus the error cases E+E and exec-in-rule-cfg.
bf62502 to
29ee03d
Compare
| scratch.file( | ||
| "test/rules.bzl", | ||
| """ | ||
| bad_transition = config.exec().and_then(config.exec()) |
There was a problem hiding this comment.
Thanks, this looks much easier to debug.
|
@bazel-io fork 9.2.0 |
|
@bazel-io fork 8.8.0 |
|
@aranguyen Could you review this for approval? |
gregestren
left a comment
There was a problem hiding this comment.
Nice to see this, @fmeum !
FYI @ajsinclair who's also interested in this.
|
|
||
| A composed transition can be attached to a rule or attribute wherever its | ||
| component transitions could be used (subject to the usual restriction that an | ||
| [incoming edge transition](#incoming-edge-transitions) must be 1:1). At most one |
There was a problem hiding this comment.
I can't remember how the code enforces this restriction.
A Starlark transition can dynamically produce 1 or >1 outputs in its implementation function, so this property can't be checked statically. I don't think this PR changes that, but do you remember how we protect against an accidental 1:2 incoming transition? Is it only checked on evaluation?
There was a problem hiding this comment.
|
@gregestren This would be great to get into 9.2.0 so that folks can start playing with it, do you think that's possible? |
For what it's worth I'll do some testing with it internal to Google once we have this submitted and released internally. Those changes might make their way out to rules_android depending on how we handle Bazel versioning requirements with that ruleset. |
I pinged the import merge. |
Implements the proposal https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md. This change in particular allows non-toolchain dependencies of rules to inherit both the execution platform (via `config.exec()`) as well as the current target platform (via a custom transition that records `//command_line_option:platforms`), which is important for certain cross-compilation scenarios. Yes, as discussed and approved in https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md. - [x] I have added tests for the new use cases (if any). - [x] I have updated the documentation (if applicable). RELNOTES[NEW]: The `and_then` method on `transition`s can be used to compose transitions. Both Starlark transitions and native transitions (e.g. `config.exec()`) are supported. Closes bazelbuild#29542. PiperOrigin-RevId: 934095515 Change-Id: I3f36ec0907c4af5bdf43e38323d166422b47051c (cherry picked from commit d089ae2)
Implements the proposal https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md. This change in particular allows non-toolchain dependencies of rules to inherit both the execution platform (via `config.exec()`) as well as the current target platform (via a custom transition that records `//command_line_option:platforms`), which is important for certain cross-compilation scenarios. Yes, as discussed and approved in https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md. - [x] I have added tests for the new use cases (if any). - [x] I have updated the documentation (if applicable). RELNOTES[NEW]: The `and_then` method on `transition`s can be used to compose transitions. Both Starlark transitions and native transitions (e.g. `config.exec()`) are supported. Closes bazelbuild#29542. PiperOrigin-RevId: 934095515 Change-Id: I3f36ec0907c4af5bdf43e38323d166422b47051c (cherry picked from commit d089ae2) <!-- Thank you for contributing to Bazel! Please read the contribution guidelines: https://bazel.build/contribute.html --> ### Description <!-- Please provide a brief summary of the changes in this PR. --> ### Motivation <!-- Why is this change important? Does it fix a specific bug or add a new feature? If this PR fixes an existing issue, please link it here (e.g. "Fixes bazelbuild#1234"). --> ### Build API Changes <!-- Does this PR affect the Build API? (e.g. Starlark API, providers, command-line flags, native rules) If yes, please answer the following: 1. Has this been discussed in a design doc or issue? (Please link it) 2. Is the change backward compatible? 3. If it's a breaking change, what is the migration plan? --> No ### Checklist - [ ] I have added tests for the new use cases (if any). - [ ] I have updated the documentation (if applicable). ### Release Notes <!-- If this is a new feature, please add 'RELNOTES[NEW]: <description>' here. If this is a breaking change, please add 'RELNOTES[INC]: <reason>' here. If this change should be mentioned in release notes, please add 'RELNOTES: <reason>' here. --> RELNOTES: None
### Description Implements the proposal https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md. ### Motivation This change in particular allows non-toolchain dependencies of rules to inherit both the execution platform (via `config.exec()`) as well as the current target platform (via a custom transition that records `//command_line_option:platforms`), which is important for certain cross-compilation scenarios. ### Build API Changes Yes, as discussed and approved in https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md. ### Checklist - [x] I have added tests for the new use cases (if any). - [x] I have updated the documentation (if applicable). ### Release Notes RELNOTES[NEW]: The `and_then` method on `transition`s can be used to compose transitions. Both Starlark transitions and native transitions (e.g. `config.exec()`) are supported. Closes bazelbuild#29542. PiperOrigin-RevId: 934095515 Change-Id: I3f36ec0907c4af5bdf43e38323d166422b47051c (cherry picked from commit d089ae2)
### Description Implements the proposal https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md. ### Motivation This change in particular allows non-toolchain dependencies of rules to inherit both the execution platform (via `config.exec()`) as well as the current target platform (via a custom transition that records `//command_line_option:platforms`), which is important for certain cross-compilation scenarios. ### Build API Changes Yes, as discussed and approved in https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md. ### Checklist - [x] I have added tests for the new use cases (if any). - [x] I have updated the documentation (if applicable). ### Release Notes RELNOTES[NEW]: The `and_then` method on `transition`s can be used to compose transitions. Both Starlark transitions and native transitions (e.g. `config.exec()`) are supported. Closes bazelbuild#29542. PiperOrigin-RevId: 934095515 Change-Id: I3f36ec0907c4af5bdf43e38323d166422b47051c (cherry picked from commit d089ae2) Closes bazelbuild#29620
Description
Implements the proposal https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md.
Motivation
This change in particular allows non-toolchain dependencies of rules to inherit both the execution platform (via
config.exec()) as well as the current target platform (via a custom transition that records//command_line_option:platforms), which is important for certain cross-compilation scenarios.Build API Changes
Yes, as discussed and approved in https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md.
Checklist
Release Notes
RELNOTES[NEW]: The
and_thenmethod ontransitions can be used to compose transitions. Both Starlark transitions and native transitions (e.g.config.exec()) are supported.