-
Notifications
You must be signed in to change notification settings - Fork 458
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
Avoid double building cargo_build_script.data
targets
#2826
Conversation
1df4f85
to
e4784d1
Compare
@scentini This change fails the
|
e4784d1
to
e26b20f
Compare
I believe I've fixed it. |
d181682
to
0a0f254
Compare
0a0f254
to
f6c0be0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cute, thanks for putting it together!
Are you imagining putting together any affordances for using this from crate_universe, or leaving folks to do that on their own?
Before this change targets passed to `cargo_build_script.data` would get built multiple times (once per configuration) ``` % bazel cquery 'filter(//test/cargo_build_script/location_expansion:target_data.txt, deps(//test/cargo_build_script/location_expansion:build_rs))' //test/cargo_build_script/location_expansion:target_data.txt (be33641) //test/cargo_build_script/location_expansion:target_data.txt (a1c326f) ``` We can see that this is the exec (`a1c326f`) and target (`be33641`) configurations. ``` % bazel config a1c326f be33641 INFO: Displaying diff between configs a1c326f and be33641 Displaying diff between configs a1c326f and be33641 FragmentOptions com.google.devtools.build.lib.analysis.PlatformOptions { platforms: [@@internal_platforms_do_not_use//host:host], [@@bazel_tools//tools:host_platform] } FragmentOptions com.google.devtools.build.lib.analysis.config.CoreOptions { compilation_mode: opt, fastbuild is exec configuration: true, false platform_suffix: exec, null } FragmentOptions com.google.devtools.build.lib.rules.android.AndroidConfiguration$Options { fat_apk_cpu: [], [armeabi-v7a] } FragmentOptions com.google.devtools.build.lib.rules.cpp.CppOptions { copt: [-g0], [] cxxopt: [-g0], [] strip: always, sometimes } FragmentOptions com.google.devtools.build.lib.rules.java.JavaOptions { java_runtime_version: remotejdk_11, local_jdk jvmopt: [-XX:ErrorFile=/dev/stderr], [] } ``` This is caused by `data` being passed to the underlying rust binary in `cargo_build_script_wrapper.bzl`. The fix for this ends up requiring some gymnastics as many existing uses of `cargo_build_script` rely on `data` files being available within the runfiles of `cargo_build_script.script`. This change accounts for that by creating a wrapper rule for the build script that symlinks the `cfg=exec` Rust binary (which is the actual [Cargo build script](https://doc.rust-lang.org/cargo/reference/build-scripts.html)) and updating the `cargo_build_script` rule to consume it as `cfg=target`. Within the `cargo_build_script` rule this means the script target and it's `data` attribute files will be in `cfg=target` but the binary itself will have been built for `cfg=exec` and will be runnable on the exec platform. We see after this change that data targets only appear once now ``` % bazel cquery 'filter(//test/cargo_build_script/location_expansion:target_data.txt, deps(//test/cargo_build_script/location_expansion:build_rs))' //test/cargo_build_script/location_expansion:target_data.txt (be33641) ```
What do you mean? This change shouldn't require any interface changes so any direct uses of |
I mean right now there are annotations like |
…ata (#2855) #2826 ended up introducing a regression for crates with build scripts that require extra data at compile time. This change adds a default glob for all `cargo_build_script` targets generated by `crate_universe` and also introduces an `annotation.build_script_compile_data` attribute to add custom targets to generated outputs.
After #2826 was merged, I started seeing flaky builds on Windows related to build script executables (#2891 (comment)). This appears to be related to bazelbuild/bazel#21747 so to avoid the issue, this change ensures that Windows build script executables are copied instead of symlinked.
Before this change targets passed to
cargo_build_script.data
would get built multiple times (once per configuration)We can see that this is the exec (
a1c326f
) and target (be33641
) configurations.This is caused by
data
being passed to the underlying rust binary incargo_build_script_wrapper.bzl
.The fix for this ends up requiring some gymnastics as many existing uses of
cargo_build_script
rely ondata
files being available within the runfiles ofcargo_build_script.script
. This change accounts for that by creating a wrapper rule for the build script that symlinks thecfg=exec
Rust binary (which is the actual Cargo build script) and updating thecargo_build_script
rule to consume it ascfg=target
. Within thecargo_build_script
rule this means the script target and it'sdata
attribute files will be incfg=target
but the binary itself will have been built forcfg=exec
and will be runnable on the exec platform.We see after this change that data targets only appear once now