Design review for Standard Platform Transitions

[katre]

Hi, I've uploaded a new design proposal, Standard Platform Transitions. The goal is to define a standard set of rules and transitions in the @platforms repository, which rule authors and users can make use of, without needing changes to Bazel itself.

Please take a look and let me know if you have any comments or criticisms.

Reviewer: @gregestren

[rickeylev]

Should there be an inline macro variant, like [inline platform_data macro]

I would say no. Mostly because tools don't handle complex structures in BUILD files very well. It also just doesn't look idiomatic (usually BUILD files are a bunch of target definitions, and then they're wired together using their string names).

Also, in the example, the inline call doesn't have a name argument? I'm guess that's just a typo. If not, then it'd have to use e.g. the target arg to automatically generate a name to generate the non-inline equivalent. All it really saves is 1 line.

[lberki]

Agreed. My understanding is that the Starlark folks want to phase out macros (eventually, although it's an uphill battle) for this very reason, so let's not add more sanctioned macros if we can help, at least until it becomes very necessary.

[sluongng]

From the folks who I have had the chance to interact with, the ideal developer experience for the average engineer/developer is not having to craft BUILD files manually as much as possible.

I think most are trying to move toward Gazelle's model today by building custom "analyzers" to construct the BUILD files from parsing source code's AST and special comments. I would imagine that in the future, having a manually crafted BUILD file would be something optional, reserved for special edge cases that these "analyzers" have yet to cover.

So keeping BUILD files flat and simple would be a much more appreciated approach.

[katre]

SGTM, I'll remove this from the document.

[gregestren]

To be clear, are we talking specifically about Starlark functions consumed directly in BUILD files?

Even the base proposal adds new functions that delegate to underlying Starlark code which is what ultimately executes. It's conceptually similar to macros. Except the function, underlying definitions, and consumer are all in .bzl files. Do tool readability concerns extend to .bzl?

[katre]

The concern in this comment is about a macro intended to be used inline in another target, which does have definite tool concerns. A macro or rule intended to be used as a target directly in the BUILD file is not a concern.

[lberki]

How is this going to interact with configurable attributes, when there is a select() statement in the platforms= attribute?

[katre]

This should work without any additional steps. See the core documentation on accessing attribute values in transitions (the paragraph starting with "attr is a dictionary of attributes ...").

1. For a rule transition, using select on the attribute read the by the transition (either platform or something else) will cause an error to be reported. Users should be able to then correct and remove the offending select.
2. For attribute transitions, the value for the attribute will be the value after the select is resolved, so there will be no issue.

Does this need to be specifically called out in the design?

[gregestren]

Should the rule transition case be an error?

Is there an argument for rules being select()ively platform-restricted?

I really don't know. But this has shown to be useful for target_compatible_with: https://bazel.build/extending/platforms#expressive-constraints

[gregestren]

@philsc as our resident target_compatible_with expert.

[katre]

Using a select to choose the platform of a target sounds like a circular dependency waiting to happen. We should probably prevent using select with this at first, and then open it up if we can prove that we can detect and warn on circular configurations.

[sluongng]

If I read this correctly, this proposal is a generalization of what we have in rules_go today.

Given a go_binary target like this

go_binary(
  name = "a",
  srcs = ["a.go"],
)

you could create neighbor target that transition to a specific platform

go_binary(
  name = "a",
  srcs = ["a.go"],
)

go_binary(
  name = "a_m1",
  srcs = ["a.go"],
  goos = "darwin",
  goarch = "arm64",
)

And this proposal would help us simplify the rules a bit by providing change_platform and split_platform as reusable transition configs.

Additionally, platform_data would help us replace go_cross_binary, which was introduced in rules_go for the same purpose.

It's still unclear to me though, how much we would be able to simplify https://github.com/bazelbuild/rules_go/blob/master/go/private/rules/transition.bzl as most of our transition work on a combination of //command_line_option:platforms and rules_go's toolchain-specific custom config settings. Is it possible to chain multiple transition inside a cfg attribute? or could we re-use change_platform in our custom transition somehow?

cc: @fmeum @illicitonion who have both work on these code paths

[fmeum]

Yes, I think that's correct. It's surprisingly difficult to write a rule that is essentially an "alias but with a transition applied", so I expect platform_data to stay clear of that complexity.

[katre]

Possibly the platform_data rule can expose an is_executable attribute, or check the executableness of the underlying target. It looks like aspect's platform_transition_binary rule, which is explicitly for binary targets, is assuming the underlying target is executable and copying the relevant data and providers.

I wish we had a better way of writing "alias-but-with-a-transition" rules, too.

In terms of composable transitions, @sdtwigg is working on a design that would allow you to layer two transitions (a Starlark version of ComposingTransition, essentially), but I don't think that would actually help rules_go.

It seems like that rules_go would need is:

1. Go-specific logic to map from the goos and goarch attributes to a specific platform label
2. A standard function that could then be called to actually set --platforms and return the result from the transition

Part 2 could fairly easily be broken out of the implementation of the proposed change_platform transition, but it's also fairly trivial at that point, especially if there are further flags being changed. If this would be useful I'm happy to add it, however.

[gregestren]

I didn't know about platform_transition_binary, interesting. How well does it work? @kormide. Won't it still depend on the type of binary being depended on, and struggle with other variations like implicit outputs?

[illicitonion]

On platform_transition_binary - we're using it (and I think it was based on our implementation in bazelbuild/rules_rust#276 (comment) actually!), but have a couple of issues:

1. Ideally it wouldn't need to exist - I'd love to be able to use platform_data in its stead (with an explicit is_executable attr if needed, but I'd be even happier if its is_executable mirrored the target it's wrapping automatically).
2. The symlink situation isn't ideal - e.g. if you put a platform_transition_binary into a pkg_tar you'll get a warning output that pkg_tar is selecting one of the (equivalent - one is a symlink to the other, and it gets flattened) two identically named files. (In a fork I've added a copy_instead_of_symlink attribute to work around this).

That said, I don't think I've seen it try to be used anywhere that the binary wasn't either being run (either directly, or via runfiles), or being packaged up somehow. @gregestren do you have examples of where you think it may hit issues?

[katre]

When I start implementing I'll do some starlark debugging and see if the rule can tell that the dependency is executable. Might be possible, might be hidden, might be able to file a feature request.

[gregestren]

Acknowledged as reviewer.

I'll have to do deeper passes and think this through more (especially regarding fitting with all our other API ideas). A few quick comments for now:

1. platform_data looks like it could fulfill the exec_binary tool @murali42 wrote for google's repo? That wraps any binary in an exec-compatible configuration so it builds consistently as a compiler tool?
2. This isn't trying to solve "set platforms and flags" together? You still had ideas for that? Or alternative standard configs could handle that?
3. How does platform_data work? Doesn't it have to be an "alias-but-with-a-transition`, or otherwise expose providers in some reliable way?
4. This doesn't handle native rule self-transitions, right? (I guess it can't if it doesn't change core Bazel). We should probably clarify with major rule maintainers if we need to support that one away or another. Even with native rules "going away" I'm sure "going away" is a long timeframe.
5. Is this intended to fulfill the Apple rules owners' needs for cleaner Bazel flags + multiplatform support?

[katre]

@brentleyjones I agree with you that projects should define their own platforms, but I've seen some (google-internal) uses with a hard-coded platform for projects that have custom rules and also define their own platforms, so this seems like an easy enough use case to support.

[gregestren]

More thoughts:

1. What timeline are you thinking for this? The reason I'm asking is I'd really like to write a doc that stitches together our various API proposals and shows how they holistically work from an end-user perspective. In other words, we're proposing various new concepts. I'd like to see a clean review showing meaningful use cases and how each API blob fits in and works with the others. If you don't expect to do much in July that opens time for that.

2. Do we need change_platform and split_platforms as separate concepts? What if there was just one which is selectively single- vs. split transition depending on its args?

3. In service of 1., I'd still like to review the feedback from the original proposal and the inflexibility concerns behind it.

4. I still think we have to be very careful about the rule wrapping / platform_data part. Very clear about what it can and can't do, so it's not abused in ways it can't handle and users perceive the API a brittle.

5. After thinking about it more, I think that platform_data makes the most sense used as a wrapper for rules that generate an executable

   This is an interesting thought. I wonder if we should go farther and be strongly opinionated about transitions only applying to binaries as general principle. While not exhaustive, it's a major use case and maybe optimizing for that specifically offers a good balance of simplicity and usability and limiting abuse of the transition concept.

[katre]

What timeline are you thinking for this?

I'm going to be OOO for most of July, so I'd like to work on this in August. On the other hand, this is a pure Starlark change (no Bazel implications), so development time should be fairly quick. As always, I expect figuring out how to write tests to be the hard part.

I'm happy to delay this design and the implementation until after your named config stuff is ready to review.

Do we need change_platform and split_platforms as separate concepts?

I envision them as having largely the same implementation, but I feel like the actual use is sufficiently different to be worth exposing them as separate concepts.

Rule authors, any input?

I'd still like to review the feedback from the original proposal

We can take a look at that. The main issue was that those changes were hard-coded into Bazel, and so rule authors wouldn't be able to opt out of using the semantics from that proposal. Since this is a proposal for a pure-Starlark change, which rule authors can choose to opt into, I feel it addresses most of the concerns.

[gregestren]

I'm wondering if we have to directly support cc_binary at some point. i.e. if the use case specifically for cc_binary is common and strong enough that working out the awkwardness of the "imperfect Starlark alias" needs a deeper solution. Or alternatively any Starlark rule that a user is consuming vs. being able to directly modify?

[katre]

One option is to petition the CC rules owners to use the change_platform transition directly on cc_binary, reducing the need for platform_data. I suspect there will still be cases where the actual cc_binary target should be able to be built for any platform, but where a specific use needs platform_data to build for a specific target platform (things like firmware flashing harnesses make sense here).

This is why I think the generic rules are useful: they're building blocks for larger sets of targets in a multiplatform build.

[murali42]

exec_binary is specific to binaries (including tests) but not limited to exec mode. It can (and is) also be used for target mode tests. Re: platform selection. By default, it picks a configuration which is compatible with the current platform. For example, it could change the CPU. But when it does so implicitly relies on the assumption on all machines where the binary may be executed.. "if it supports old CPU it also supports new CPU". One thing to watch out... Initially it was an "alias with a transition" but that breaks many use cases. A binary "//pkg:bin" results in a file named "pkg/bin" in the runfiles tree. So we changed to using "native_binary" (which just pretends that a file is a binary).

…

On Tue, Jun 13, 2023, 6:25 AM John Cater ***@***.***> wrote: @brentleyjones <https://github.com/brentleyjones> I agree with you that projects should define their own platforms, but I've seen some (google-internal) uses with a hard-coded platform for projects that have custom rules and also define their own platforms, so this seems like an easy enough use case to support. — Reply to this email directly, view it on GitHub <#18628 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKHEQOPJJZH545JKMSEUOMDXLBS5PANCNFSM6AAAAAAZA64QZQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[katre]

Thanks for the details. I'll take the implementation into account when I start writing code for this.

[gregestren]

@murali42 can you clarify? Changing what to native_binary?

[katre]

After talking with @gregestren and looking at my calendar, I'm going to pause the discussion on this proposal: @gregestren has a related proposal to finish up and I will be out of the country for most of July.

I'd like to pick this back up when I'm back in the last week of July.

[gregestren]

More thoughts:

Principles:

Agree we should avoid getting involved in non-universal concerns (concerns that vary between rule types):

1. If we transform target T with any kind of wrapper W, which parts of T should W propagate? There's IMO no consistent answer: some properties it's fine to pass forward as if they're part of T. Some should be distinctly copied to be clear W is its own target. Especially if there's the possibility of building / running both W and T together in the same invocation. Which properties matter depend (I think) on the rule type.
2. If we alternatively add transition support directly into T, we're telling rule type T how its semantics work. It's plausible this is fairly standard, but I'm not sure how safe that ultimately is. @katre re: android_binary and --android_platforms, would that actually be a counter-example? Wouldn't --android_platforms still be consumed and set regardless of how --platforms changes, or is that too much of an assumption?

Furthermore for 2, I'm convinced Extending Bazel rules is the cleaner solution for that approach. I'm pretty certain that proposal as-is will be powerful enough (note it has multiple references to overriding cfg).

Conclusion: Take a practical user-focused approach. Solve for known useful, generic cases that don't require language specialization. Consider language-specialized approaches on their own terms, either hard-coded (platform directly in cc_binary) or with a properly focused extensibility API (Extending Bazel rules).

Also: document the intended use and limitations of the new APIs clearly. Point users to the right alternatives for more custom needs.

Motivating user needs

We have a clearly defined user need of supporting multi-platform embedded C++ binaries, particularly through discussion with Pigweed. We should be sure our solution addresses that. We can check with them but I'm pretty sure platform_data as written is sufficient.

We generally have interest in this space particularly for cc_binary. We should further vet cases where cc_binary owners want this and determine if we need anything more complex. For example, a runnable version: #18628 (reply in thread). Or platform directly in cc_binary. I don't think we yet have the call for these deeper variations. But if/when we do it should be easy to extend our implementations to support them.

All this makes me think we should take an incremental approach: start with the basic use cases we know will solve concrete problems. Write them in a generic way that we can expand as needed. Then expand as needed.

This doesn't really speak to the go_binary discussion #18628 (reply in thread). Could platform-based-flags help? And we provide enough of a building block (base_platform_binary_that_you_extend) to make that realistic? Or if (Extending Bazel rules) is available by that point we just point there?

Executables

I played around with some Starlark code. We can definitely support language-specific blaze run executables, and platform_transition_binary is a clear precedent (although I agree with #18628 (reply in thread) a copy-based approach is cleaner than a symlink approach).

I'm pretty sure we can't merge that directly into platform_data. platform_data has to set is_executable in its rule() definition. There's no way to configure that based on the properties of the target rule. I don't think we could even support platform_data(name = 'my_target', target = ':cc_foo', executable = True) for the same reason. I think we'd have no choice but to define a distinct platform_data_binary variant. Or propose adjustments to the Starlark API.

Aside from that, platform_data's implementation could figure out if the target rule is executable: https://bazel.build/rules/lib/builtins/ctx#executable. But at that point it's already too late.

@katre you mentioned it's not clear we should encourage transitioning *_library rules. I generally agree. We could alternatively restrict support to *_binary and just assume the target is executable, and error if it isn't. Although I'm not sure that fits with packaging targets. And it'll still have the language-specific semantics issue (like with runfiles).

Proposal nitpicks:

1. How would

my_rule = rule(
  cfg = change_platform(attribute = "my_platform"),
  attrs = {
    "my_platform": attr.label(providers = [platform_common.PlatformInfo]),
  },
)

work? If change_platform is a pure Starlark function, how would it have access to the rule's my_platform attribute? Especially if it's called during rule instantiation (not in the rule's implementation function)?

2. To be clear, what specific providers do you see platform_data returning?
3. Small nit: this slightly complicates the user experience: if they run $ bazel build //:all they'll build both the underlying binary and its wrapper version. Which may not be what they want. I hear you @katre that it's a good thing to keep these separate, and not assume the user wants to merge them. But users who don't play close attention could see more build work than they expect. At the very least we should be clear about this in documentation.
4. Small nit: just like the above, users trying to find all cc_binary (i.e. to fill out a CI run) will have to remember to also search for platform_data instances depending on a cc_binary. This is already true with aliases (I think?) and whatever ad hoc wrappers people write together. But another usability nit to be aware of.

That's all I have in mind. I think we've done a good job of getting to root principles.

Priorities:

All the above said, I suggest we do this to move forward:

1. Implement platform_data (with clearly defined semantics)
2. Verify 100% that platform_data is sufficient for Pigweed. And anyone else who wants to share concrete needs here.
3. Implement change_platform(platform = Label("//new/target:platform")),, re-using as much underlying code as possible
4. Not yet implement cfg = change_platform(attribute = "my_platform") unless it's clean and easy (note this and 3 may be eventually replaceable by "extending rules". Although that API is more verbose than this)
5. Not worry about the split variations until we have a concrete request
6. If there's a concrete request, implement platform_cc_binary for a C++-focused executable variation
7. If we prove that's insufficient, support platform directly in cc_binary
8. While C++ is more pertinent in practice, consider variants for other languages too (until "extending rules" is available)

Basically, we're in good shape as long as we know we're simplifying concrete user cases. Let's do enough work to support those use cases and no more. But make it easy to extend as the need comes up.

[gregestren]

I realize the above comment is huge. To summarize:

1. let's start with the most generic support (platform_data as a single-file data dependency) and only extend to more nuanced use cases as the need is clear.
2. Implement the work in a way that's easily extensible when there's the call.
3. According to clear principles outlined in our comments above.

[katre]

Lots of great points here. I think this implementation approach is fairly good and gives us the best benefits.

Re the implementation of change_platform: I envision change_platform as a Starlark function returning a transition. Since transition implementations can see rule attributes, it would look something like this:

def change_platform(attribute = "platform"):
    def _change_platform_impl(settings, attr):
        new_platform = attr[attribute]
        // other transition logic
      return new_attributes
    return transition(
        implementation = _change_platform_impl,
        inputs = ["//command_line_flags:platforms"],
        outputs = ["//command_line_flags:platforms"],
    )

As far as Extending Bazel rules goes, it's definitely a great way to attach a new transition to an existing rules, but the transition still has to be defined somewhere, which is where change_platforms comes in handy.

[katre]

Oh, and for android_binary and --android_platforms: This is a good example of a rule that needs more complex logic, and so needs custom flags and transitions. Possibly someday we can simplify this (and I'd be very happy if we could have used plain --platforms instead of adding a new flag), but it mainly serves of an example of where this proposal isn't helpful.

[gregestren]

Of course, how did I forget the attr parameter? Makes sense.

Good point about Extending Bazel rules & change_platforms working together.

Nothing more from my end. Proposal LGTM.

[gregestren]

if there's a need for, say, swift_library_with_platform then the user (or the rules_swift authors) would need to write a specific rule that knows what providers to forward.

1. That said, it might be possible to write a Starlark function that generates such a rule, given a base rule and a list of providers.
2. Would this be useful enough to add?

I think this is a good idea if there's the need for swift_library_with_platform, as you say. Once we've determined we want Swift (or whatever) support, I think the extra work of generalized implementation logic will be minimal.

But as of this moment we don't need to do either.

[brentleyjones]

I saw somewhere a while ago that in general we shouldn't try to directly build non-top-level targets. they should be transitioned via a top-level-target. And I agree with that. So I don't think it makes sense for a swift_library_with_platform to exist, when we really should build swift_binary_with_platform or apple_application (theoretical new rule that allow setting the platform instead of using a {ios,tvos,visionos,...}_application rule.

[katre]

I generally agree that swift_library_with_platform is unneeded, I was giving it as an example of a rule that the rules_swift authors could add if there was need from their users.

[katre]

Thanks for the comments, everyone! Now that this is accepted, I am locking the discussion to keep it archived, but feel free to reach out or file further feature requests if needed.