bazelbuild / rules_rust

Rust rules for Bazel
https://bazelbuild.github.io/rules_rust/
Apache License 2.0
670 stars 432 forks source link

Delete `experimental_toolchain_generated_sysroot` #2848

Closed UebelAndre closed 2 months ago

UebelAndre commented 2 months ago

Closes https://github.com/bazelbuild/rules_rust/issues/2298

krasimirgg commented 2 months ago

Could we functionally keep this around (possibly renaming it to non-experimental)?

We've got a use case (quite convolved admittedly, so bear with me) for this where we need the ability to prevent passing the computed --sysroot as a rustc flag: we've got a version of rustc bootstrapping defined in bazel. There's a "dynamic sysroot" situation while building rustc itself the first time around. At a specific bootstrapping stage, we have a beta rustc toolchain and beta stdlibs and freshly-built nightly stdlibs, and we're aiming to build the new rustc. When building standard rlib dependencies of rustc, they need the freshly-built nightly stdlibs on their sysroot; when building proc_macro deps or their dependencies, they need the beta stdlibs as a sysroot (since they're later loaded by the beta rustc).

For this, we currently have a custom single rust_toolchain that collects the beta rustc and both the beta and freshly-built nightly stdlibs and a custom script that wraps the rustc binary that passes the correct --sysroot based on BAZEL_RULES_RUST_IS_PROC_MACRO_DEP.

(Ideally, we'd have two separate rust_toolchain-s: one for the standard libs that packages the nightly stdlibs, and another one for the proc_macro-s that packages the beta stdlibs, but I don't know of a way to do that via toolchain resolution: if the target platform is the same as the exec platform, the only thing on bazel toolchain resolution side that distinguishes them is that one of them is exec, and you can't use that for toolchain resolution purposes.)

(We could still work around not having this ability by updating the script to strip off the --sysroot generated by rules_rust and replacing it with a custom one, but that's a bit complicated as it will need to handle the various ways there are to pass flags, like via param files and stuff.)

UebelAndre commented 2 months ago

Could we keep this around? We've got a use case (quite convolved admittedly, so bear with me) for this where we need the ability to prevent passing the computed --sysroot as a rustc flag: we've got a version of rustc bootstrapping defined in bazel. There's a "dynamic sysroot" situation while building rustc itself the first time around. At a specific bootstrapping stage, we have a beta rustc toolchain and beta stdlibs and freshly-built nightly stdlibs, and we're aiming to build the new rustc. When building standard rlib dependencies of rustc, they need the freshly-built nightly stdlibs on their sysroot; when building proc_macro deps or their dependencies, they need the beta stdlibs as a sysroot (since they're later loaded by the beta rustc).

For this, we currently have a custom single rust_toolchain that collects the beta rustc and both the beta and freshly-built nightly stdlibs and a custom script that wraps the rustc binary that passes the correct --sysroot based on BAZEL_RULES_RUST_IS_PROC_MACRO_DEP.

(Ideally, we'd have two separate rust_toolchain-s: one for the standard libs that packages the nightly stdlibs, and another one for the proc_macro-s that packages the beta stdlibs, but I don't know of a way to do that via toolchain resolution: if the target platform is the same as the exec platform, the only thing on bazel toolchain resolution side that distinguishes them is that one of them is exec, and you can't use that for toolchain resolution purposes.)

(We could still work around not having this ability by updating the script to strip off the --sysroot generated by rules_rust and replacing it with a custom one, but that's a bit complicated as it will need to handle the various ways there are to pass flags, like via param files and stuff.)

Wait, I'm confused, do you have a use for it or are you using it? If the former it kinda sounds like you want something like C++ toolchain features where you can control what flags are used and optionally disable some. And if that's the case then I'd say lets merge this and start working on that but admidetly this change isn't absolutely needed and can wait. I just don't wanna keep incompatibility/experimental to permanently persist in lieu of actually building useful functionality.

krasimirgg commented 2 months ago

Could we keep this around? We've got a use case (quite convolved admittedly, so bear with me) for this where we need the ability to prevent passing the computed --sysroot as a rustc flag: we've got a version of rustc bootstrapping defined in bazel. There's a "dynamic sysroot" situation while building rustc itself the first time around. At a specific bootstrapping stage, we have a beta rustc toolchain and beta stdlibs and freshly-built nightly stdlibs, and we're aiming to build the new rustc. When building standard rlib dependencies of rustc, they need the freshly-built nightly stdlibs on their sysroot; when building proc_macro deps or their dependencies, they need the beta stdlibs as a sysroot (since they're later loaded by the beta rustc). For this, we currently have a custom single rust_toolchain that collects the beta rustc and both the beta and freshly-built nightly stdlibs and a custom script that wraps the rustc binary that passes the correct --sysroot based on BAZEL_RULES_RUST_IS_PROC_MACRO_DEP. (Ideally, we'd have two separate rust_toolchain-s: one for the standard libs that packages the nightly stdlibs, and another one for the proc_macro-s that packages the beta stdlibs, but I don't know of a way to do that via toolchain resolution: if the target platform is the same as the exec platform, the only thing on bazel toolchain resolution side that distinguishes them is that one of them is exec, and you can't use that for toolchain resolution purposes.) (We could still work around not having this ability by updating the script to strip off the --sysroot generated by rules_rust and replacing it with a custom one, but that's a bit complicated as it will need to handle the various ways there are to pass flags, like via param files and stuff.)

Wait, I'm confused, do you have a use for it or are you using it? If the former it kinda sounds like you want something like C++ toolchain features where you can control what flags are used and optionally disable some. And if that's the case then I'd say lets merge this and start working on that but admidetly this change isn't absolutely needed and can wait. I just don't wanna keep incompatibility/experimental to permanently persist in lieu of actually building useful functionality.

We are using this (specifically the ability to disable passing the generated --sysroot by setting the build setting to false in some build configurations). And I agree and think the default behavior makes sense, so I suggest to just promote this flag to be non-experimental.

UebelAndre commented 2 months ago

Could we keep this around? We've got a use case (quite convolved admittedly, so bear with me) for this where we need the ability to prevent passing the computed --sysroot as a rustc flag: we've got a version of rustc bootstrapping defined in bazel. There's a "dynamic sysroot" situation while building rustc itself the first time around. At a specific bootstrapping stage, we have a beta rustc toolchain and beta stdlibs and freshly-built nightly stdlibs, and we're aiming to build the new rustc. When building standard rlib dependencies of rustc, they need the freshly-built nightly stdlibs on their sysroot; when building proc_macro deps or their dependencies, they need the beta stdlibs as a sysroot (since they're later loaded by the beta rustc). For this, we currently have a custom single rust_toolchain that collects the beta rustc and both the beta and freshly-built nightly stdlibs and a custom script that wraps the rustc binary that passes the correct --sysroot based on BAZEL_RULES_RUST_IS_PROC_MACRO_DEP. (Ideally, we'd have two separate rust_toolchain-s: one for the standard libs that packages the nightly stdlibs, and another one for the proc_macro-s that packages the beta stdlibs, but I don't know of a way to do that via toolchain resolution: if the target platform is the same as the exec platform, the only thing on bazel toolchain resolution side that distinguishes them is that one of them is exec, and you can't use that for toolchain resolution purposes.) (We could still work around not having this ability by updating the script to strip off the --sysroot generated by rules_rust and replacing it with a custom one, but that's a bit complicated as it will need to handle the various ways there are to pass flags, like via param files and stuff.)

Wait, I'm confused, do you have a use for it or are you using it? If the former it kinda sounds like you want something like C++ toolchain features where you can control what flags are used and optionally disable some. And if that's the case then I'd say lets merge this and start working on that but admidetly this change isn't absolutely needed and can wait. I just don't wanna keep incompatibility/experimental to permanently persist in lieu of actually building useful functionality.

We are using this (specifically the ability to disable passing the generated --sysroot by setting the build setting to false in some build configurations). And I agree and think the default behavior makes sense, so I suggest to just promote this flag to be non-experimental.

Can you put that PR together and close the tracking issue then?

illicitonion commented 2 months ago

(Ideally, we'd have two separate rust_toolchain-s: one for the standard libs that packages the nightly stdlibs, and another one for the proc_macro-s that packages the beta stdlibs, but I don't know of a way to do that via toolchain resolution: if the target platform is the same as the exec platform, the only thing on bazel toolchain resolution side that distinguishes them is that one of them is exec, and you can't use that for toolchain resolution purposes.)

You can probably make this work by transitioning your stdlibs to a platform that has an additional constraint (let's call it "has-sysroot-flag=true" for now). You'll need to register your toolchains all having an explicit value for that constraint ("has-sysroot-flag=true" and "has-sysroot-flag=false") because a missing constraint on a toolchain is treated as "compatible with all values of this constraint".

You can see a similar-ish example at https://github.com/bazelbuild/rules_rust/blob/9ad0b00a5e13de52f4ec495ea592a4998a53c1e6/examples/musl_cross_compiling/WORKSPACE.bazel#L86-L95 where we register different rust toolchains for "linker=musl" and "linker!=musl", which means that on the one exec=target platform targets we transition to "linker=musl" use the musl toolchain, and all other targets (e.g. process-wrapper and friends) end up using the "linker!=musl" toolchain.

(by krasimir: sorry I accidentally edited your comment initially instead of posting a reply 🤦 )

krasimirgg commented 2 months ago

(Ideally, we'd have two separate rust_toolchain-s: one for the standard libs that packages the nightly stdlibs, and another one for the proc_macro-s that packages the beta stdlibs, but I don't know of a way to do that via toolchain resolution: if the target platform is the same as the exec platform, the only thing on bazel toolchain resolution side that distinguishes them is that one of them is exec, and you can't use that for toolchain resolution purposes.)

You can probably make this work by transitioning your stdlibs to a platform that has an additional constraint (let's call it "has-sysroot-flag=true" for now). You'll need to register your toolchains all having an explicit value for that constraint ("has-sysroot-flag=true" and "has-sysroot-flag=false") because a missing constraint on a toolchain is treated as "compatible with all values of this constraint".

You can see a similar-ish example at

https://github.com/bazelbuild/rules_rust/blob/9ad0b00a5e13de52f4ec495ea592a4998a53c1e6/examples/musl_cross_compiling/WORKSPACE.bazel#L86-L95

where we register different rust toolchains for "linker=musl" and "linker!=musl", which means that on the one exec=target platform targets we transition to "linker=musl" use the musl toolchain, and all other targets (e.g. process-wrapper and friends) end up using the "linker!=musl" toolchain.

Thank you for the suggestion! Naively, this situation is a bit different in that we still need to use two different sysroots even when exec=target: one for the normal deps and another one for proc_macros (and their dependencies). We can define two separate rust_toolchains distinguished by the "has-sysroot-flag" (or just their stdlib-s customized based on the value of that flag). The thing that's tricky is on the end-user side, to get the deps and proc_macro_deps pick up these two different toolchains consistently.

# these two toolchains are distinuished by has-sysroot-flag
rust_toolchain(
  name = "for_deps",
  rust_std = "std_for_deps",
)

rust_toolchain(
  name = "for_proc_macro_deps",
  rust_std = "std_for_proc_macro_deps",
)

rust_binary(
  name = "rustc",
  deps = [
    "rustc_lib1", # needs std_for_deps sysroot
  ],
  proc_macro_deps = [
    "rustc_pm1", # needs std_for_proc_macro_deps sysroot
  ],
)

There are several approaches that can solve this, but all that we have considered bad tradeoffs with other features: we could apply a custom user-defined transition that flips the has-sysroot-flag on the proc_macro_deps attribute, but we don't want to introduce extra transitions there since that has negative build speed implications. There's already an exec transition involved in the rust_proc_macro; if there was a way to say that a toolchain is compatible with exec-only, that would have worked (but folks have reasons to not allow selecting on this). A few more details about this are on https://github.com/bazelbuild/rules_rust/pull/1420#issue-1283830685.

krasimirgg commented 2 months ago

Prepped https://github.com/bazelbuild/rules_rust/pull/2849 to keep this around as non-experimental.