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

Bootstrap should skip preparing a linker for targets when checking std #128180

Closed
ChrisDenton opened this issue Jul 25, 2024 · 4 comments · Fixed by #128871
Closed

Bootstrap should skip preparing a linker for targets when checking std #128180

ChrisDenton opened this issue Jul 25, 2024 · 4 comments · Fixed by #128871
Labels
C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@ChrisDenton
Copy link
Member

ChrisDenton commented Jul 25, 2024

Currently ./x check std --stage 0 --target <target> will attempt to configure the target linker and other tools even though they're unused. For the most part this just results in needless cc spam (and presumably slower checks). E.g.:

cargo:warning=Compiler family detection failed due to error: ToolNotFound: Failed to find tool. Is `arm-kmc-eabi-gcc` installed? (see https://github.com/rust-lang/cc-rs#compile-time-requirements for help)
cargo:warning=Compiler family detection failed due to error: ToolNotFound: Failed to find tool. Is `arm-kmc-eabi-g++` installed? (see https://github.com/rust-lang/cc-rs#compile-time-requirements for help)
cargo:warning=Compiler family detection failed due to error: ToolNotFound: Failed to find tool. Is `arm-kmc-eabi-g++` installed? (see https://github.com/rust-lang/cc-rs#compile-time-requirements for help)

But for apple targets this is often a hard failure unless you have the relevant tools. E.g.:

> ./x check std --target aarch64-apple-ios --stage 0
Building bootstrap
    Finished `dev` profile [unoptimized] target(s) in 0.07s


error occurred: Failed to find tool. Is `xcrun` installed? (see https://github.com/rust-lang/cc-rs#compile-time-requirements for help)


Build completed unsuccessfully in 0:00:00

I managed to hack around this issue but I don't know bootstrap code very well so a proper fix would be appreciated.

Hack
diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs
index 78fbea2e810..9d3c606aa01 100644
--- a/src/bootstrap/src/core/builder.rs
+++ b/src/bootstrap/src/core/builder.rs
@@ -2433,7 +2433,9 @@ pub fn new(
         cmd: &str, // FIXME make this properly typed
     ) -> Cargo {
         let mut cargo = builder.cargo(compiler, mode, source_type, target, cmd);
-        cargo.configure_linker(builder);
+        if cmd != "check" {
+            cargo.configure_linker(builder);
+        }
         cargo
     }

diff --git a/src/bootstrap/src/utils/cc_detect.rs b/src/bootstrap/src/utils/cc_detect.rs
index 20d79e490ec..dcc56de121f 100644
--- a/src/bootstrap/src/utils/cc_detect.rs
+++ b/src/bootstrap/src/utils/cc_detect.rs
@@ -89,13 +89,11 @@ fn new_cc_build(build: &Build, target: TargetSelection) -> cc::Build {
 pub fn find(build: &Build) {
     // For all targets we're going to need a C compiler for building some shims
     // and such as well as for being a linker for Rust code.
-    let targets = build
-        .targets
-        .iter()
-        .chain(&build.hosts)
-        .cloned()
-        .chain(iter::once(build.build))
-        .collect::<HashSet<_>>();
+    let targets: HashSet<_> = if matches!(build.config.cmd, crate::Subcommand::Check { .. }) {
+        build.hosts.iter().cloned().chain(iter::once(build.build)).collect()
+    } else {
+        build.targets.iter().chain(&build.hosts).cloned().chain(iter::once(build.build)).collect()
+    };
     for target in targets.into_iter() {
         find_target(build, target);
     }
@ChrisDenton ChrisDenton added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-bug Category: This is a bug. labels Jul 25, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 25, 2024
@bjorn3
Copy link
Member

bjorn3 commented Jul 25, 2024

configure_linker is also setting the C/C++ compiler, which is used by the build script of compiler-builtins unless optimized-compiler-builtins is false (and in case of checking rustc, the build script of rustc_llvm uses a C++ compiler). Build scripts do not know if the current build is a check build or regular build.

@ChrisDenton
Copy link
Member Author

Which is why I called my code a "hack". But specifically for checking std without optimized-compiler-builtins (the default for local dev), xcrun is entirely unnecessary.

@saethlin
Copy link
Member

saethlin commented Jul 25, 2024

configure_linker is also setting the C/C++ compiler, which is used by the build script of compiler-builtins unless optimized-compiler-builtins is false

That seems fine to me, optimized-compiler-builtins is an annoying feature anyway because it requires the LLVM submodule.

and in case of checking rustc, the build script of rustc_llvm uses a C++ compiler

That is also fine, we're not checking the compiler. We're checking std.

What Chris is trying to do here used to work. This issue is a regression in bootstrap. Being able to cross-check is very useful; if/when anyone fixes this we should add cross-checking to CI to make sure this doesn't regress again.

@onur-ozkan onur-ozkan removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 26, 2024
@onur-ozkan
Copy link
Member

I managed to hack around this issue but I don't know bootstrap code very well so a proper fix would be appreciated.

Hack

diff --git a/src/bootstrap/src/core/builder.rs b/src/bootstrap/src/core/builder.rs
index 78fbea2e810..9d3c606aa01 100644
--- a/src/bootstrap/src/core/builder.rs
+++ b/src/bootstrap/src/core/builder.rs
@@ -2433,7 +2433,9 @@ pub fn new(
         cmd: &str, // FIXME make this properly typed
     ) -> Cargo {
         let mut cargo = builder.cargo(compiler, mode, source_type, target, cmd);
-        cargo.configure_linker(builder);
+        if cmd != "check" {
+            cargo.configure_linker(builder);
+        }
         cargo
     }

diff --git a/src/bootstrap/src/utils/cc_detect.rs b/src/bootstrap/src/utils/cc_detect.rs
index 20d79e490ec..dcc56de121f 100644
--- a/src/bootstrap/src/utils/cc_detect.rs
+++ b/src/bootstrap/src/utils/cc_detect.rs
@@ -89,13 +89,11 @@ fn new_cc_build(build: &Build, target: TargetSelection) -> cc::Build {
 pub fn find(build: &Build) {
     // For all targets we're going to need a C compiler for building some shims
     // and such as well as for being a linker for Rust code.
-    let targets = build
-        .targets
-        .iter()
-        .chain(&build.hosts)
-        .cloned()
-        .chain(iter::once(build.build))
-        .collect::<HashSet<_>>();
+    let targets: HashSet<_> = if matches!(build.config.cmd, crate::Subcommand::Check { .. }) {
+        build.hosts.iter().cloned().chain(iter::once(build.build)).collect()
+    } else {
+        build.targets.iter().chain(&build.hosts).cloned().chain(iter::once(build.build)).collect()
+    };
     for target in targets.into_iter() {
         find_target(build, target);
     }

These cmd checks in the builder module are going out of control I think. The proper solution would be to refactor Cargo, fix this FIXME, and handle all cmds from a single source (ideally covered by some unit tests). I can allocate some time this weekend to work on that.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 29, 2024
…ns, r=Mark-Simulacrum

improve cargo invocations on bootstrap

Fixes few of the `FIXME`s on cargo invocations and should be considered as blocker for rust-lang#128180.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 29, 2024
Rollup merge of rust-lang#128269 - onur-ozkan:improve-cargo-invocations, r=Mark-Simulacrum

improve cargo invocations on bootstrap

Fixes few of the `FIXME`s on cargo invocations and should be considered as blocker for rust-lang#128180.
@onur-ozkan onur-ozkan added C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such and removed C-bug Category: This is a bug. labels Aug 10, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Sep 2, 2024
bypass linker configuration and cross target check for specific commands

Avoids configuring the linker and checking cross-target-specific tools unless necessary.

Resolves rust-lang#128180

cc `@ChrisDenton`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 3, 2024
bypass linker configuration and cross target check for specific commands

Avoids configuring the linker and checking cross-target-specific tools unless necessary.

Resolves rust-lang#128180

cc ``@ChrisDenton``
tgross35 added a commit to tgross35/rust that referenced this issue Sep 4, 2024
bypass linker configuration and cross target check for specific commands

Avoids configuring the linker and checking cross-target-specific tools unless necessary.

Resolves rust-lang#128180

cc ```@ChrisDenton```
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 6, 2024
bypass linker configuration and cross target check for specific commands

Avoids configuring the linker and checking cross-target-specific tools unless necessary.

Resolves rust-lang#128180

cc `@ChrisDenton`
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 7, 2024
bypass linker configuration and cross target check for specific commands

Avoids configuring the linker and checking cross-target-specific tools unless necessary.

Resolves rust-lang#128180

cc `@ChrisDenton`
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 7, 2024
bypass linker configuration and cross target check for specific commands

Avoids configuring the linker and checking cross-target-specific tools unless necessary.

Resolves rust-lang#128180

cc `@ChrisDenton`
compiler-errors added a commit to compiler-errors/rust that referenced this issue Sep 7, 2024
bypass linker configuration and cross target check for specific commands

Avoids configuring the linker and checking cross-target-specific tools unless necessary.

Resolves rust-lang#128180

cc `@ChrisDenton`
@bors bors closed this as completed in 7468b69 Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants