remove hard dependency on Android NDK presence when performing check builds

[landaire]

The unwind build script will attempt to invoke the Android compiler when the target platform is android. This unfortunately puts a hard dependency on the NDK which hurts the ability to run code targeting Android in Miri. For more context on the Miri side of things, see rust-lang/miri#2011.

Since Miri is emulating the target platform at an IR level and doesn't need to produce target binaries, we just check if we're building stdlib for Miri and assert that we have lunwind support without invoking a compiler.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

[bjorn3]

Maybe instead do an early return just after println!("cargo:rerun-if-changed=build.rs"); for miri? We need neither linker flags nor building libunwind when using miri.

[Mark-Simulacrum]

I would expect the fix here looks closer to this, where we gate on RUST_CHECK (an ad-hoc variable set by rustbuild during compiler builds today, but could be extended to check builds too). I suspect we want to do this not just when compiling for miri, but during all check builds, right?

[bjorn3]

an ad-hoc variable set by rustbuild during compiler builds today, but could be extended to check builds too

That would require rerunning build scripts when switching between check and build.

[Mark-Simulacrum]

I would expect that they are stored in different profiles (e.g., debug vs. release) and so don't share a cache anyway -- at the very least, I don't recall having too much trouble switching between check and build today.

[bjorn3]

I mean in general if cargo were to get that ability.

[landaire]

I suspect we want to do this not just when compiling for miri, but during all check builds, right?

That's a good point. I think you're right, yes.

I'm admittedly moving a little slow today so let me make sure I understand: would you like me to make the necessary changes to enable the RUST_CHECK env var in check builds invoked by cargo, or should this change be refactored to early return using the CARGO_CFG_MIRI env var?

[Mark-Simulacrum]

I would like this PR to use RUST_CHECK, yeah. It looks like that env variable is currently being used for specifically "do we already have an LLVM" (https://github.com/yanganto/rust/blob/38b96b9a6459facd05097b0b2d9d82fb2da6d774/src/bootstrap/builder.rs#L1014), but we can likely rename that use to something like RUST_BOOTSTRAP_SKIP_LLVM_BUILD and separately have a RUST_CHECK that is used for this build script.

[landaire]

Okay, sounds good. I'm going to update this PR to use the RUST_BOOTSTRAP_SKIP_LLVM_BUILD as proposed. Now I'm wondering, where should RUST_CHECK be set? I have a PoC locally where I've modified Cargo to set a CARGO_CHECK (which would replace RUST_CHECK) env var:

Expand to view diff

diff --git a/src/cargo/core/compiler/build_config.rs b/src/cargo/core/compiler/build_config.rs
index 4b488261c..cbe308dab 100644
--- a/src/cargo/core/compiler/build_config.rs
+++ b/src/cargo/core/compiler/build_config.rs
@@ -147,7 +147,7 @@ pub enum CompileMode {
     /// A target that will be tested with `rustdoc`.
     Doctest,
     /// A marker for Units that represent the execution of a `build.rs` script.
-    RunCustomBuild,
+    RunCustomBuild { parent_is_check: bool, },
 }
 
 impl ser::Serialize for CompileMode {
@@ -163,7 +163,7 @@ impl ser::Serialize for CompileMode {
             Bench => "bench".serialize(s),
             Doc { .. } => "doc".serialize(s),
             Doctest => "doctest".serialize(s),
-            RunCustomBuild => "run-custom-build".serialize(s),
+            RunCustomBuild { .. } => "run-custom-build".serialize(s),
         }
     }
 }
@@ -174,6 +174,11 @@ impl CompileMode {
         matches!(self, CompileMode::Check { .. })
     }
 
+    /// Returns `true` if the unit or any of its parents is being checked.
+    pub fn is_any_parent_unit_check(self) -> bool {
+        matches!(self, CompileMode::Check { .. } | CompileMode::RunCustomBuild { parent_is_check: true })
+    }
+
     /// Returns `true` if this is generating documentation.
     pub fn is_doc(self) -> bool {
         matches!(self, CompileMode::Doc { .. })
@@ -206,6 +211,6 @@ impl CompileMode {
 
     /// Returns `true` if this is the *execution* of a `build.rs` script.
     pub fn is_run_custom_build(self) -> bool {
-        self == CompileMode::RunCustomBuild
+        matches!(self, CompileMode::RunCustomBuild { .. })
     }
 }
diff --git a/src/cargo/core/compiler/build_context/target_info.rs b/src/cargo/core/compiler/build_context/target_info.rs
index 610749af9..5f729132c 100644
--- a/src/cargo/core/compiler/build_context/target_info.rs
+++ b/src/cargo/core/compiler/build_context/target_info.rs
@@ -445,7 +445,7 @@ impl TargetInfo {
                 }
             }
             CompileMode::Check { .. } => Ok((vec![FileType::new_rmeta()], Vec::new())),
-            CompileMode::Doc { .. } | CompileMode::Doctest | CompileMode::RunCustomBuild => {
+            CompileMode::Doc { .. } | CompileMode::Doctest | CompileMode::RunCustomBuild { .. } => {
                 panic!("asked for rustc output for non-rustc mode")
             }
         }
diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs
index c1f5abce3..8589c01b2 100644
--- a/src/cargo/core/compiler/compilation.rs
+++ b/src/cargo/core/compiler/compilation.rs
@@ -176,7 +176,7 @@ impl<'cfg> Compilation<'cfg> {
         };
 
         let cmd = fill_rustc_tool_env(rustc, unit);
-        self.fill_env(cmd, &unit.pkg, None, unit.kind, true)
+        self.fill_env(cmd, unit,  &unit.pkg, None, unit.kind, true)
     }
 
     /// Returns a [`ProcessBuilder`] for running `rustdoc`.
@@ -187,7 +187,7 @@ impl<'cfg> Compilation<'cfg> {
     ) -> CargoResult<ProcessBuilder> {
         let rustdoc = ProcessBuilder::new(&*self.config.rustdoc()?);
         let cmd = fill_rustc_tool_env(rustdoc, unit);
-        let mut p = self.fill_env(cmd, &unit.pkg, script_meta, unit.kind, true)?;
+        let mut p = self.fill_env(cmd, unit,  &unit.pkg, script_meta, unit.kind, true)?;
         unit.target.edition().cmd_edition_arg(&mut p);
 
         for crate_type in unit.target.rustc_crate_types() {
@@ -206,10 +206,12 @@ impl<'cfg> Compilation<'cfg> {
     pub fn host_process<T: AsRef<OsStr>>(
         &self,
         cmd: T,
+        unit: &Unit,
         pkg: &Package,
     ) -> CargoResult<ProcessBuilder> {
         self.fill_env(
             ProcessBuilder::new(cmd),
+            unit,
             pkg,
             None,
             CompileKind::Host,
@@ -232,6 +234,7 @@ impl<'cfg> Compilation<'cfg> {
         &self,
         cmd: T,
         kind: CompileKind,
+        unit: &Unit,
         pkg: &Package,
         script_meta: Option<Metadata>,
     ) -> CargoResult<ProcessBuilder> {
@@ -243,7 +246,7 @@ impl<'cfg> Compilation<'cfg> {
         } else {
             ProcessBuilder::new(cmd)
         };
-        self.fill_env(builder, pkg, script_meta, kind, false)
+        self.fill_env(builder, unit, pkg, script_meta, kind, false)
     }
 
     /// Prepares a new process with an appropriate environment to run against
@@ -254,6 +257,7 @@ impl<'cfg> Compilation<'cfg> {
     fn fill_env(
         &self,
         mut cmd: ProcessBuilder,
+        unit: &Unit,
         pkg: &Package,
         script_meta: Option<Metadata>,
         kind: CompileKind,
@@ -354,6 +358,10 @@ impl<'cfg> Compilation<'cfg> {
             }
         }
 
+        if unit.mode.is_any_parent_unit_check() {
+            cmd.env("CARGO_CHECK", "1");
+        }
+
         Ok(cmd)
     }
 }
diff --git a/src/cargo/core/compiler/context/compilation_files.rs b/src/cargo/core/compiler/context/compilation_files.rs
index 37b11da84..1d953c77b 100644
--- a/src/cargo/core/compiler/context/compilation_files.rs
+++ b/src/cargo/core/compiler/context/compilation_files.rs
@@ -390,7 +390,7 @@ impl<'a, 'cfg: 'a> CompilationFiles<'a, 'cfg> {
                     flavor: FileFlavor::Normal,
                 }]
             }
-            CompileMode::RunCustomBuild => {
+            CompileMode::RunCustomBuild { .. } => {
                 // At this time, this code path does not handle build script
                 // outputs.
                 vec![]
diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs
index 1587f100e..fb26db015 100644
--- a/src/cargo/core/compiler/custom_build.rs
+++ b/src/cargo/core/compiler/custom_build.rs
@@ -180,7 +180,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> {
     // `Profiles::get_profile_run_custom_build` so that those flags get
     // carried over.
     let to_exec = to_exec.into_os_string();
-    let mut cmd = cx.compilation.host_process(to_exec, &unit.pkg)?;
+    let mut cmd = cx.compilation.host_process(to_exec, unit, &unit.pkg)?;
     let debug = unit.profile.debuginfo.unwrap_or(0) != 0;
     cmd.env("OUT_DIR", &script_out_dir)
         .env("CARGO_MANIFEST_DIR", unit.pkg.root())
diff --git a/src/cargo/core/compiler/job_queue.rs b/src/cargo/core/compiler/job_queue.rs
index e6ce2c614..a0577b257 100644
--- a/src/cargo/core/compiler/job_queue.rs
+++ b/src/cargo/core/compiler/job_queue.rs
@@ -946,7 +946,7 @@ impl<'cfg> DrainState<'cfg> {
         let pkg_name = unit.pkg.name();
         match unit.mode {
             CompileMode::Doc { .. } => format!("{}(doc)", pkg_name),
-            CompileMode::RunCustomBuild => format!("{}(build)", pkg_name),
+            CompileMode::RunCustomBuild { .. } => format!("{}(build)", pkg_name),
             _ => {
                 let annotation = match unit.target.kind() {
                     TargetKind::Lib(_) => return pkg_name.to_string(),
diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs
index 0551f7ce5..09750368e 100644
--- a/src/cargo/core/compiler/mod.rs
+++ b/src/cargo/core/compiler/mod.rs
@@ -956,6 +956,13 @@ fn build_base_args(
             cmd.env(&key, exe_path);
         }
     }
+
+    // Add `CARGO_CHECK` environment variable to notify build scripts that no final
+    // code generation will be performed
+    if unit.mode.is_any_parent_unit_check() {
+        cmd.env("CARGO_CHECK", "1");
+    }
+
     Ok(())
 }
 
diff --git a/src/cargo/core/compiler/timings.rs b/src/cargo/core/compiler/timings.rs
index 836acac5c..06f07355f 100644
--- a/src/cargo/core/compiler/timings.rs
+++ b/src/cargo/core/compiler/timings.rs
@@ -176,7 +176,7 @@ impl<'cfg> Timings<'cfg> {
             CompileMode::Bench => target.push_str(" (bench)"),
             CompileMode::Doc { .. } => target.push_str(" (doc)"),
             CompileMode::Doctest => target.push_str(" (doc test)"),
-            CompileMode::RunCustomBuild => target.push_str(" (run)"),
+            CompileMode::RunCustomBuild { .. } => target.push_str(" (run)"),
         }
         let unit_time = UnitTime {
             unit,
diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs
index cfcda149b..3af5ac9ca 100644
--- a/src/cargo/core/compiler/unit_dependencies.rs
+++ b/src/cargo/core/compiler/unit_dependencies.rs
@@ -543,7 +543,7 @@ fn dep_build_script(
                 t,
                 script_unit_for,
                 unit.kind,
-                CompileMode::RunCustomBuild,
+                CompileMode::RunCustomBuild { parent_is_check: unit.mode.is_check() },
                 profile,
             )
         })
@@ -646,7 +646,7 @@ fn connect_run_custom_build_deps(state: &mut State<'_, '_>) {
         let mut reverse_deps_map = HashMap::new();
         for (unit, deps) in state.unit_dependencies.iter() {
             for dep in deps {
-                if dep.unit.mode == CompileMode::RunCustomBuild {
+                if dep.unit.mode.is_run_custom_build() {
                     reverse_deps_map
                         .entry(dep.unit.clone())
                         .or_insert_with(HashSet::new)
@@ -667,7 +667,7 @@ fn connect_run_custom_build_deps(state: &mut State<'_, '_>) {
         for unit in state
             .unit_dependencies
             .keys()
-            .filter(|k| k.mode == CompileMode::RunCustomBuild)
+            .filter(|k| k.mode.is_run_custom_build())
         {
             // This list of dependencies all depend on `unit`, an execution of
             // the build script.
@@ -708,7 +708,7 @@ fn connect_run_custom_build_deps(state: &mut State<'_, '_>) {
                 .filter_map(|other| {
                     state.unit_dependencies[&other.unit]
                         .iter()
-                        .find(|other_dep| other_dep.unit.mode == CompileMode::RunCustomBuild)
+                        .find(|other_dep|  other_dep.unit.mode.is_run_custom_build())
                         .cloned()
                 })
                 .collect::<HashSet<_>>();
diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs
index a86f0899e..297f8131f 100644
--- a/src/cargo/core/profiles.rs
+++ b/src/cargo/core/profiles.rs
@@ -313,7 +313,7 @@ impl Profiles {
                         )
                     }
                 }
-                CompileMode::Build | CompileMode::Check { .. } | CompileMode::RunCustomBuild => {
+                CompileMode::Build | CompileMode::Check { .. } | CompileMode::RunCustomBuild { .. } => {
                     // Note: `RunCustomBuild` doesn't normally use this code path.
                     // `build_unit_profiles` normally ensures that it selects the
                     // ancestor's profile. However, `cargo clean -p` can hit this
diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs
index 44945d919..89718eade 100644
--- a/src/cargo/ops/cargo_compile.rs
+++ b/src/cargo/ops/cargo_compile.rs
@@ -344,7 +344,7 @@ pub fn create_bcx<'a, 'cfg>(
         | CompileMode::Build
         | CompileMode::Check { .. }
         | CompileMode::Bench
-        | CompileMode::RunCustomBuild => {
+        | CompileMode::RunCustomBuild { .. }=> {
             if std::env::var("RUST_FLAGS").is_ok() {
                 config.shell().warn(
                     "Cargo does not read `RUST_FLAGS` environment variable. Did you mean `RUSTFLAGS`?",
@@ -747,7 +747,7 @@ impl CompileFilter {
                     } => examples.is_specific() || tests.is_specific() || benches.is_specific(),
                 }
             }
-            CompileMode::RunCustomBuild => panic!("Invalid mode"),
+            CompileMode::RunCustomBuild { .. } => panic!("Invalid mode"),
         }
     }
 
@@ -1261,7 +1261,7 @@ fn filter_default_targets(targets: &[Target], mode: CompileMode) -> Vec<&Target>
                 })
                 .collect()
         }
-        CompileMode::Doctest | CompileMode::RunCustomBuild => panic!("Invalid mode {:?}", mode),
+        CompileMode::Doctest | CompileMode::RunCustomBuild { .. } => panic!("Invalid mode {:?}", mode),
     }
 }
 
diff --git a/src/cargo/ops/cargo_run.rs b/src/cargo/ops/cargo_run.rs
index 69bae2c59..eb5229d7f 100644
--- a/src/cargo/ops/cargo_run.rs
+++ b/src/cargo/ops/cargo_run.rs
@@ -92,7 +92,7 @@ pub fn run(
         Err(_) => path.to_path_buf(),
     };
     let pkg = bins[0].0;
-    let mut process = compile.target_process(exe, unit.kind, pkg, *script_meta)?;
+    let mut process = compile.target_process(exe, unit.kind, unit, pkg, *script_meta)?;
     process.args(args).cwd(config.cwd());
 
     config.shell().status("Running", process.to_string())?;
diff --git a/src/cargo/ops/cargo_test.rs b/src/cargo/ops/cargo_test.rs
index 9fcb94f13..1232146bc 100644
--- a/src/cargo/ops/cargo_test.rs
+++ b/src/cargo/ops/cargo_test.rs
@@ -103,7 +103,7 @@ fn run_unit_tests(
             )
         };
 
-        let mut cmd = compilation.target_process(path, unit.kind, &unit.pkg, *script_meta)?;
+        let mut cmd = compilation.target_process(path, unit.kind, unit, &unit.pkg, *script_meta)?;
         cmd.args(test_args);
         if unit.target.harness() && config.shell().verbosity() == Verbosity::Quiet {
             cmd.arg("--quiet");
diff --git a/tests/testsuite/check.rs b/tests/testsuite/check.rs
index 0bfd5b923..c814c5f13 100644
--- a/tests/testsuite/check.rs
+++ b/tests/testsuite/check.rs
@@ -1001,3 +1001,55 @@ fn rustc_workspace_wrapper_excludes_published_deps() {
         .with_stdout_does_not_contain("WRAPPER CALLED: rustc --crate-name baz [..]")
         .run();
 }
+
+#[cargo_test]
+fn check_crate_env_vars() {
+    let foo = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.0.1"
+                authors = []
+
+                [dependencies.bar]
+                path = "../bar"
+            "#,
+        )
+        .file(
+            "src/main.rs",
+            "extern crate bar; fn main() { ::bar::baz(); }",
+        )
+        .file(
+            "build.rs",
+            r#"
+            pub fn main() {
+                for (key, value) in std::env::vars() {
+                    println!("{}: {}", key, value);
+                }
+
+                assert!(std::env::var_os("CARGO_CHECK").is_some())
+            }"#,
+        )
+        .build();
+
+    let _bar = project()
+        .at("bar")
+        .file("Cargo.toml", &basic_manifest("bar", "0.1.0"))
+        .file("src/lib.rs", "pub fn baz() {}")
+        .file(
+            "build.rs",
+            r#"
+            pub fn main() {
+                for (key, value) in std::env::vars() {
+                    println!("{}: {}", key, value);
+                }
+                assert!(std::env::var_os("CARGO_CHECK").is_some())
+            }
+        "#,
+        )
+        .build();
+
+    foo.cargo("check").run();
+}

I think that this has to be in Cargo since Miri invokes xargo check (and not the rust repo custom check command), but I wasn't sure if there was a different approach I should take in rustc instead? Let me know your thoughts and if you agree that Cargo is the appropriate place I'll make a PR there.

[Mark-Simulacrum]

Cargo is unlikely to make a change here soon (it's a harder design problem than just doing it for this repo). Not sure what the best solution will need to be then, will put in some thought.

[landaire]

(it's a harder design problem than just doing it for this repo)

Could you explain this a little bit? I'm by no means plugged in to the cargo/rustc internals discussion, but the patch I proposed exposes the CARGO_CHECK env var for any build.rs file. Are there other scenarios that I'm not taking into account such as using cargo as a library that my changes may break?

Not sure what the best solution will need to be then, will put in some thought.

Cargo invokes rustc with --emit=dep-info,metadata -- perhaps this could be an acceptable signal to "assume" a check build? Although I think implicit behavior is kinda bad to rely on.

Thanks again for helping me brainstorm here.

[Mark-Simulacrum]

Could you explain this a little bit? I'm by no means plugged in to the cargo/rustc internals discussion, but the patch I proposed exposes the CARGO_CHECK env var for any build.rs file. Are there other scenarios that I'm not taking into account such as using cargo as a library that my changes may break?

As @bjorn3 mentions here #94813 (comment) there's a few questions around the appropriate behavior when you're switching between check and regular builds. Typically today we can reuse build script results I believe in that case, which is definitely nice (particularly for larger scripts, that build C libraries etc), but introducing this environment variable likely makes it necessary to not share the results of the script in that scenario.

I am also not too closely familiar, but my recollection is that the Cargo team was relatively negative about such an expansion, though that was several years ago that I had/read a discussion there, I think.

Cargo invokes rustc with --emit=dep-info,metadata -- perhaps this could be an acceptable signal to "assume" a check build? Although I think implicit behavior is kinda bad to rely on.

It might be OK to rely on this (at least in std), but even so, we can't really - the build script doesn't know anything about that invocation I think.

---

Let's try poking @rust-lang/cargo to see if there's thoughts on the appropriate design here. It seems to me that to some extent, the problem we're running into is fairly common (cargo check target compilations during effectively cross-compilation not actually needing system libraries or headers, and that code running causing problems). I'm not sure if there's design bandwidth to review or standup a good solution here right now, but maybe someone has a good idea I'm missing (e.g., an unstable feature maybe?).

One intermediate option is that we just merge this PR (almost as-is, I would want to make sure we emit a rerun-if-env-changed), and add another small 'weird edge case' to std -- wouldn't be the first one by a long ways. But I'm increasingly trying to push us away from that kind of thing, so trying a ping of Cargo here.

[Eh2406]

I think our most resent discussion was at rust-lang/cargo#9532

[JohnCSimon]

ping from triage:
@landaire
What is the status of this PR? Did the conversation stall or should this PR be closed.

[landaire]

@JohnCSimon life things got in the way and it stalled on my end. I went back and prioritized getting the Miri PR landed first without this change (and therefore lacking testing support), then circling back here.

The conversation @Eh2406 linked to was a bit much for me to dive into, process, and circle back here with what I thought the correct approach would be in combination with Mark's feedback.

I apologize for moving slow on this and will try to circle back around as soon as I can. Thank you for the nudge.

[RalfJung]

I think that this has to be in Cargo since Miri invokes xargo check (and not the rust repo custom check command), but I wasn't sure if there was a different approach I should take in rustc instead? Let me know your thoughts and if you agree that Cargo is the appropriate place I'll make a PR there.

We can just make xargo-check (or Miri's logic that invokes it -- but this seems better to be shared with all xargo users) also set that variable.

So if I understood correctly, the plan for action is the following?

• Rename the existing RUST_CHECK to RUST_BOOTSTRAP_SKIP_LLVM_BUILD
• Add a new RUST_CHECK variable that is set in this branch (but outside the LLVM check), and if RUST_CHECK is set then bail our early in library/unwind/build.rs? Or maybe RUST_BOOTSTRAP_CHECK if we are keeping with the theme of the variable name.

[RalfJung]

My concern with that plan is, won't that cause rebuilds when switching between x.py build and x.py check on the same checkout?

[JohnCSimon]

@landaire
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive