Skip to content

Delete experimental_toolchain_generated_sysroot #2848

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

Closed
wants to merge 1 commit into from

Conversation

UebelAndre
Copy link
Collaborator

Closes #2298

@krasimirgg
Copy link
Collaborator

krasimirgg commented Sep 10, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

krasimirgg commented Sep 10, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

illicitonion commented Sep 10, 2024

(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

"x86_64-unknown-linux-gnu": [
"@//linker_config:unknown",
"@platforms//cpu:x86_64",
"@platforms//os:linux",
],
"x86_64-unknown-linux-musl": [
"@//linker_config:musl",
"@platforms//cpu:x86_64",
"@platforms//os:linux",
],
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
Copy link
Collaborator

krasimirgg commented Sep 10, 2024

(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

"x86_64-unknown-linux-gnu": [
"@//linker_config:unknown",
"@platforms//cpu:x86_64",
"@platforms//os:linux",
],
"x86_64-unknown-linux-musl": [
"@//linker_config:musl",
"@platforms//cpu:x86_64",
"@platforms//os:linux",
],

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 #1420 (comment).

@krasimirgg
Copy link
Collaborator

Prepped #2849 to keep this around as non-experimental.

@UebelAndre UebelAndre closed this Sep 11, 2024
auto-merge was automatically disabled September 11, 2024 13:41

Pull request was closed

github-merge-queue bot pushed a commit that referenced this pull request Sep 11, 2024
…2849)

No functional changes intended.

We've got a use case where the ability to turn this off is useful, see
(#2848 (comment)).

Closes #2298
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.

experimental_toolchain_generated_sysroot
3 participants