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

Add target_compatible_with from kwargs to build_script_kwargs #2133

Merged
merged 9 commits into from
Oct 10, 2023

Conversation

daivinhtran
Copy link
Contributor

The top-level _build_script_run target should also have target_compatible_with in order to work with platforms compatibility.

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

The way args are passed here seems backward to me. I think target folks think about when defining cargo_build_script targets is actually _build_script_run. Thus we should default to passing all args to that and set the rust_binary target to manual. You can then constrain the target with both target_compatible_with which I feel most applies to running the script where exec_compatible_with would apply to compiling the script as the script is in cfg = "exec". How do you feel about passing kwargs to _build_script_run instead?

cc @scentini

@UebelAndre UebelAndre requested a review from scentini August 25, 2023 14:22
@daivinhtran daivinhtran force-pushed the target_compatible_with-attr branch from 55eaa85 to 3385c97 Compare August 25, 2023 15:09
@daivinhtran
Copy link
Contributor Author

daivinhtran commented Aug 25, 2023

@UebelAndre Yes that's an approach I would prefer. Passing kwargs to _build_script_run make sense.

PTAL: 3385c97

I also introduced edition,srcs, crate_name, crate_root, and proc_macro_deps attributes to cargo_build_script macro so they can be passed to rust_binary to build. This ensures that all arguments to rust_binary is fully defined at the macro signature.

@daivinhtran daivinhtran force-pushed the target_compatible_with-attr branch 3 times, most recently from 20296df to 7235378 Compare August 25, 2023 15:29
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Seems good to me! I’ll give @scentini a chance to look since I think they have some more nuanced uses with “compatible_with”.

jrguzman-ms pushed a commit to msft-mirror-aosp/platform.build.bazel that referenced this pull request Aug 26, 2023
There's a bug in cargo_build_script rule that does propagate target_compatible_with attr correctly for device build.

See bazelbuild/rules_rust#2133

Bug: 297550356
Test: CI
Change-Id: Ifbcbdcb85b0c8aa230c2f147bc74a776b1c2d91b
@scentini scentini enabled auto-merge (squash) September 4, 2023 20:00
auto-merge was automatically disabled October 9, 2023 23:05

Head branch was pushed to by a user without write access

@daivinhtran daivinhtran force-pushed the target_compatible_with-attr branch from 61356cc to 4b52792 Compare October 9, 2023 23:11
@UebelAndre UebelAndre merged commit 3e124fb into bazelbuild:main Oct 10, 2023
2 checks passed
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.

3 participants