Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Platform transition added to Rust binary rules. #2310

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

rickvanprim
Copy link
Contributor

Without much insight into core Bazel's plans for supporting "flagless" builds across platforms, this seems like the most direct way to select the platform for a given Rust binary target (all the terminal rules) that doesn't need additional work to forward providers.

Currently I'm using platform_transition_binary (https://github.com/aspect-build/bazel-lib/blob/main/docs/transitions.md), but it doesn't work with Rust Analyzer/Clippy/etc. presumably due to not forwarding the providers. In the case of this particular rule, it also doesn't work for something like rust_shared_library() which is explicitly not an executable = True target.

@illicitonion brought to my attention with_cfg (https://github.com/fmeum/with_cfg.bzl) which could be used as an alternative by consumers of rules_rust instead of this PR. That feels reasonable, though certainly more work for individual users and appears to rely on a lot of Bazel black magic.

Along the same lines, this could wait on rule inheritance as presumably a more formally supported with_cfg approach, which would also be up to individual users and not rules_rust.

Tagging @aiuto, @katre, and @comius per @illicitonion's suggestion of relevant inputs 🙂

@rickvanprim rickvanprim force-pushed the platform branch 3 times, most recently from 7f62a13 to aad11db Compare December 9, 2023 02:53
@illicitonion
Copy link
Collaborator

Tagged folks - @aiuto @katre @fmeum - I know there's been long-standing discussion around flagless builds and arbitrary targets being transitionable - this PR seems like a reasonable step in that direction, but feels like a very specific instance/implementation of a very general pattern... Any thoughts on whether we should do this as-is, and whether there's anything we should change to be forwards-compatible with any larger more general efforts folks may be considering?

@katre
Copy link
Member

katre commented Dec 11, 2023

This change is compatible with our goals, as outlined in Standard Platforms Transitions.

Once we have the common change_platform transition available, it'd be nice if you migrated to use it, but the intent is the same.

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I'll leave this open for a day or two in case any other maintainers have thoughts.

rust/private/rust.bzl Show resolved Hide resolved
rust/private/rust.bzl Show resolved Hide resolved
rust/private/rust.bzl Show resolved Hide resolved
rust/private/rust.bzl Show resolved Hide resolved
rust/private/rust.bzl Show resolved Hide resolved
@rickvanprim
Copy link
Contributor Author

@katre thanks for the link. I'd be happy to switch this over to change_platform whenever that's available 🙂

rust/private/rust.bzl Show resolved Hide resolved
rust/private/rust.bzl Show resolved Hide resolved
@rickvanprim rickvanprim force-pushed the platform branch 2 times, most recently from 64230fe to 752d01c Compare December 12, 2023 21:25
@illicitonion illicitonion merged commit 0e4be00 into bazelbuild:main Dec 13, 2023
3 checks passed
@rickvanprim rickvanprim deleted the platform branch December 13, 2023 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants