fix: `rustc_compile_action` emits `CcInfo` for `wasm32` targets #1979

rickvanprim · 2023-05-24T18:44:57Z

This change allows cc_binary(), cc_library(), etc. targets to depend on rust_libary(), rust_static_library(), etc. targets when targeting wasm32.

rickvanprim · 2023-05-24T18:51:37Z

@titanous your initial addition gated CcInfo on not being wasm32, so I'm hoping you can weigh in here on whether this is a bad idea/I'm overlooking something 😅

titanous · 2023-05-24T19:51:56Z

I don't actually have any code using the patch from #715 right now, so I'm not certain... I don't remember why I added that condition 😰

titanous · 2023-05-24T19:53:04Z

#627 said this:

For the rule to succeed it is also necessary to skip the output of the CcInfo, since it won't be generated. This issue also shows up and is resolved when trying to build a rust_library with crate_type=cdylib and --platforms=@rules_rust//rust/platform:wasm.

rickvanprim · 2023-05-24T20:18:50Z

Hmm that surprises me. I'd expect rust_wasm_bindgen to emit its own set of providers and whatever rust_library emitted to be ignored (beyond the purposes of bindgen running and producing the .wasm). I'll try to dig in further.

rickvanprim · 2023-05-25T04:54:40Z

It seems like that case should be covered by the targets in examples/wasm/BUILD.bazel which build just fine with this change. I'm thinking the original scenario where rust_library() accepted a crate_type may have been the culprit and it's no longer an issue with rust_shared_library(). Any thoughts on other stuff to test?

rickvanprim · 2023-06-05T20:59:28Z

@titanous any further thoughts on this or what I should do to move this forward?

UebelAndre

@rickvanprim @titanous I think this change is fine but could you add a unit test of sorts with some comments on the rationale for needing this? None of the maintainers really use WASM so we need to rely on the community and heavy documentation to make sure we don't break things.

UebelAndre

Awesome work! Sorry it took me so long to look at this 😅

Just a couple of requests

examples/nix_cross_compiling/bazel/toolchain_rules/cc_tools/wasm-ld.bzl

examples/nix_cross_compiling/BUILD.bazel

examples/nix_cross_compiling/bazel/cargo/platforms/BUILD

examples/nix_cross_compiling/BUILD.bazel

.bazelci/presubmit.yml

rickvanprim · 2023-11-21T06:30:08Z

Regarding the example and cargo-bazel, it looks like the other examples build cargo-bazel from source (cargo_bazel_bootstrap()) and that's what I have set up here (Nix builds it with Cargo via buildRustPackage), but this is fairly slow. Is there some trick that's speeding it up in the other examples?

UebelAndre

I think this is looking good! I just had one last question 😄

.bazelci/presubmit.yml

nix/flake.nix

UebelAndre · 2023-12-01T18:41:47Z

Looks like you have some nix errors? I can take another pass when CI is green.

rickvanprim · 2023-12-01T20:03:39Z

I think I've come up with an approach that just installs Nix as part of the setup step, which would mean no new container needed (yay). I'm not sure why I'm hitting the current error, which is presumably due to how bazelci.py invokes things, so unfortunately my iteration right now is "test in CI" 😞 Apologies for the email spam.

rickvanprim · 2023-12-01T20:46:53Z

It went green! 😱 That said, for whatever reason until I forced my local Nix to rebuild cargo-bazel, it was producing a different hash for the lockfile. I'm a little concerned that might manifest again, although I'm also wondering if the examples should be running with CARGO_BAZEL_REPIN=true anyways to test that portion of things, though at the expense of not testing that it works with the existing lockfile?

.bazelci/presubmit.yml

UebelAndre

Oh sorry! I didn't realize CI had been passing. I just had a few more requests and then this otherwise looks good to me!

.bazelci/presubmit.yml

UebelAndre · 2023-12-08T18:09:09Z

.envrc

@@ -0,0 +1,3 @@
+# Use `path:` syntax to avoid copying the entire repo to the Nix Store.


Can you add a comment to what the .envrc file is and link to some docs? For those who don't know anything about nix like me 😅

UebelAndre · 2023-12-08T18:10:08Z

nix/flake.nix

@@ -0,0 +1,25 @@
+{


Can you add a BUILD.bazel file to this package and a README explaining what the package is for?

(relates to #1979 (comment))

rickvanprim · 2023-12-08T21:00:31Z

@UebelAndre I have hopefully addressed your feedback and CI passed 🙂

UebelAndre

Awesome! Thank you for all your patience and hard work!

rickvanprim marked this pull request as ready for review May 24, 2023 18:50

rickvanprim force-pushed the wasm32_cc_info branch from 305acfc to 3a5b19b Compare June 5, 2023 20:59

UebelAndre requested changes Jun 12, 2023

View reviewed changes

rickvanprim force-pushed the wasm32_cc_info branch from 3a5b19b to be793fd Compare June 16, 2023 17:27

rickvanprim force-pushed the wasm32_cc_info branch 9 times, most recently from 27e716e to bb59ea0 Compare November 16, 2023 20:00

UebelAndre requested changes Nov 17, 2023

View reviewed changes

examples/nix_cross_compiling/bazel/toolchain_rules/cc_tools/wasm-ld.bzl Outdated Show resolved Hide resolved

examples/nix_cross_compiling/BUILD.bazel Outdated Show resolved Hide resolved

examples/nix_cross_compiling/bazel/cargo/platforms/BUILD Outdated Show resolved Hide resolved

UebelAndre requested changes Nov 17, 2023

View reviewed changes

examples/nix_cross_compiling/BUILD.bazel Outdated Show resolved Hide resolved

rickvanprim force-pushed the wasm32_cc_info branch 3 times, most recently from 6e6b169 to 2a19f00 Compare November 17, 2023 16:49

rickvanprim commented Nov 17, 2023

View reviewed changes

.bazelci/presubmit.yml Outdated Show resolved Hide resolved

rickvanprim force-pushed the wasm32_cc_info branch from 2a19f00 to 187f4a1 Compare November 17, 2023 17:10

UebelAndre requested changes Nov 19, 2023

View reviewed changes

.bazelci/presubmit.yml Show resolved Hide resolved

rickvanprim force-pushed the wasm32_cc_info branch 3 times, most recently from e51f42c to 155cfaf Compare November 21, 2023 05:33

UebelAndre requested changes Nov 22, 2023

View reviewed changes

.bazelci/presubmit.yml Show resolved Hide resolved

nix/flake.nix Show resolved Hide resolved

rickvanprim force-pushed the wasm32_cc_info branch 6 times, most recently from b9f6d5f to 76a4c9d Compare November 29, 2023 16:40

rickvanprim force-pushed the wasm32_cc_info branch 4 times, most recently from 7768002 to 8e19e11 Compare December 1, 2023 18:09

rickvanprim force-pushed the wasm32_cc_info branch from 8e19e11 to 68fe2be Compare December 1, 2023 20:02

rickvanprim force-pushed the wasm32_cc_info branch 4 times, most recently from 6150ef2 to 07e5bed Compare December 1, 2023 20:46

UebelAndre requested changes Dec 1, 2023

View reviewed changes

.bazelci/presubmit.yml Show resolved Hide resolved

UebelAndre requested changes Dec 8, 2023

View reviewed changes

rickvanprim added 2 commits December 8, 2023 13:15

fix: rustc_compile_action emits CcInfo for wasm32 targets

dba3b34

example: nix cross compiling

c6022d7

rickvanprim force-pushed the wasm32_cc_info branch from 07e5bed to c6022d7 Compare December 8, 2023 20:50

UebelAndre approved these changes Dec 8, 2023

View reviewed changes

UebelAndre merged commit 0a4c4df into bazelbuild:main Dec 8, 2023
3 checks passed

rickvanprim deleted the wasm32_cc_info branch December 8, 2023 21:44

cameron-martin mentioned this pull request Jan 3, 2024

Advertise CcInfo provider on rules #2126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `rustc_compile_action` emits `CcInfo` for `wasm32` targets #1979

fix: `rustc_compile_action` emits `CcInfo` for `wasm32` targets #1979

rickvanprim commented May 24, 2023 •

edited

Loading

rickvanprim commented May 24, 2023

titanous commented May 24, 2023 •

edited

Loading

titanous commented May 24, 2023

rickvanprim commented May 24, 2023

rickvanprim commented May 25, 2023

rickvanprim commented Jun 5, 2023

UebelAndre left a comment

UebelAndre left a comment

rickvanprim commented Nov 21, 2023

UebelAndre left a comment

UebelAndre commented Dec 1, 2023

rickvanprim commented Dec 1, 2023

rickvanprim commented Dec 1, 2023

UebelAndre left a comment

UebelAndre Dec 8, 2023

UebelAndre Dec 8, 2023 •

edited

Loading

rickvanprim commented Dec 8, 2023

UebelAndre left a comment

		@@ -0,0 +1,3 @@
		# Use `path:` syntax to avoid copying the entire repo to the Nix Store.

fix: rustc_compile_action emits CcInfo for wasm32 targets #1979

fix: rustc_compile_action emits CcInfo for wasm32 targets #1979

Conversation

rickvanprim commented May 24, 2023 • edited Loading

rickvanprim commented May 24, 2023

titanous commented May 24, 2023 • edited Loading

titanous commented May 24, 2023

rickvanprim commented May 24, 2023

rickvanprim commented May 25, 2023

rickvanprim commented Jun 5, 2023

UebelAndre left a comment

Choose a reason for hiding this comment

UebelAndre left a comment

Choose a reason for hiding this comment

rickvanprim commented Nov 21, 2023

UebelAndre left a comment

Choose a reason for hiding this comment

UebelAndre commented Dec 1, 2023

rickvanprim commented Dec 1, 2023

rickvanprim commented Dec 1, 2023

UebelAndre left a comment

Choose a reason for hiding this comment

UebelAndre Dec 8, 2023

Choose a reason for hiding this comment

UebelAndre Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

rickvanprim commented Dec 8, 2023

UebelAndre left a comment

Choose a reason for hiding this comment

fix: `rustc_compile_action` emits `CcInfo` for `wasm32` targets #1979

fix: `rustc_compile_action` emits `CcInfo` for `wasm32` targets #1979

rickvanprim commented May 24, 2023 •

edited

Loading

titanous commented May 24, 2023 •

edited

Loading

UebelAndre Dec 8, 2023 •

edited

Loading