Auto merge of #14701 - linyihai:trace-env-table, r=weihanglo

Fix: trace `config` `[env]` table in dep-info.

### What does this PR try to resolve?

The `env` defined in `config.toml` or `--config env`  are stripped before recording in the dep-info file.  The absence of the env table doesn't rebuild when env changed

By tracing the `env` table into the `dep-info` file, If the envs changed then the rebuild can be triggered.

Fixes #13280

### How should we test and review this PR?
One commit add the test, the latter commit update the test through the fixes.

### Additional information
The PR only fixed the `env` table changes can't trigger a rebuild, other `CARGO-like` envs
 doesn't take into account.

Author: bors

src/cargo/core/compiler/fingerprint/mod.rs

@@ -362,8 +362,8 @@
 mod dirty_reason;
 
 use std::collections::hash_map::{Entry, HashMap};
-
 use std::env;
+use std::ffi::OsString;
 use std::fmt::{self, Display};
 use std::fs::{self, File};
 use std::hash::{self, Hash, Hasher};
@@ -849,7 +849,11 @@ impl LocalFingerprint {
                                 .to_string(),
                         )
                     } else {
-                        gctx.get_env(key).ok()
+                        if let Some(value) = gctx.env_config()?.get(key) {
+                            value.to_str().and_then(|s| Some(s.to_string()))
+                        } else {
+                            gctx.get_env(key).ok()
+                        }
                     };
                     if current == *previous {
                         continue;
@@ -2124,6 +2128,9 @@ enum DepInfoPathType {
 ///
 /// The serialized Cargo format will contain a list of files, all of which are
 /// relative if they're under `root`. or absolute if they're elsewhere.
+///
+/// The `env_config` argument is a set of environment variables that are
+/// defined in `[env]` table of the `config.toml`.
 pub fn translate_dep_info(
     rustc_dep_info: &Path,
     cargo_dep_info: &Path,
@@ -2132,6 +2139,7 @@ pub fn translate_dep_info(
     target_root: &Path,
     rustc_cmd: &ProcessBuilder,
     allow_package: bool,
+    env_config: &Arc<HashMap<String, OsString>>,
 ) -> CargoResult<()> {
     let depinfo = parse_rustc_dep_info(rustc_dep_info)?;
 
@@ -2168,9 +2176,11 @@ pub fn translate_dep_info(
     // This also includes `CARGO` since if the code is explicitly wanting to
     // know that path, it should be rebuilt if it changes. The CARGO path is
     // not tracked elsewhere in the fingerprint.
-    on_disk_info
-        .env
-        .retain(|(key, _)| !rustc_cmd.get_envs().contains_key(key) || key == CARGO_ENV);
+    //
+    // For cargo#13280, We trace env vars that are defined in the `[env]` config table.
+    on_disk_info.env.retain(|(key, _)| {
+        env_config.contains_key(key) || !rustc_cmd.get_envs().contains_key(key) || key == CARGO_ENV
+    });
 
     let serialize_path = |file| {
         // The path may be absolute or relative, canonical or not. Make sure

src/cargo/core/compiler/mod.rs

@@ -330,7 +330,7 @@ fn rustc(
     if hide_diagnostics_for_scrape_unit {
         output_options.show_diagnostics = false;
     }
-
+    let env_config = Arc::clone(build_runner.bcx.gctx.env_config()?);
     return Ok(Work::new(move |state| {
         // Artifacts are in a different location than typical units,
         // hence we must assure the crate- and target-dependent
@@ -459,6 +459,7 @@ fn rustc(
                 &rustc,
                 // Do not track source files in the fingerprint for registry dependencies.
                 is_local,
+                &env_config,
             )
             .with_context(|| {
                 internal(format!(
@@ -1987,10 +1988,7 @@ pub(crate) fn apply_env_config(
         if cmd.get_envs().contains_key(key) {
             continue;
         }
-
-        if value.is_force() || gctx.get_env_os(key).is_none() {
-            cmd.env(key, value.resolve(gctx));
-        }
+        cmd.env(key, value);
     }
     Ok(())
 }

src/cargo/util/context/mod.rs

@@ -63,7 +63,7 @@ use std::io::SeekFrom;
 use std::mem;
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
-use std::sync::Once;
+use std::sync::{Arc, Once};
 use std::time::Instant;
 
 use self::ConfigValue as CV;
@@ -227,7 +227,7 @@ pub struct GlobalContext {
     target_cfgs: LazyCell<Vec<(String, TargetCfgConfig)>>,
     doc_extern_map: LazyCell<RustdocExternMap>,
     progress_config: ProgressConfig,
-    env_config: LazyCell<EnvConfig>,
+    env_config: LazyCell<Arc<HashMap<String, OsString>>>,
     /// This should be false if:
     /// - this is an artifact of the rustc distribution process for "stable" or for "beta"
     /// - this is an `#[test]` that does not opt in with `enable_nightly_features`
@@ -1833,34 +1833,47 @@ impl GlobalContext {
         &self.progress_config
     }
 
-    pub fn env_config(&self) -> CargoResult<&EnvConfig> {
-        let env_config = self
-            .env_config
-            .try_borrow_with(|| self.get::<EnvConfig>("env"))?;
-
-        // Reasons for disallowing these values:
-        //
-        // - CARGO_HOME: The initial call to cargo does not honor this value
-        //   from the [env] table. Recursive calls to cargo would use the new
-        //   value, possibly behaving differently from the outer cargo.
-        //
-        // - RUSTUP_HOME and RUSTUP_TOOLCHAIN: Under normal usage with rustup,
-        //   this will have no effect because the rustup proxy sets
-        //   RUSTUP_HOME and RUSTUP_TOOLCHAIN, and that would override the
-        //   [env] table. If the outer cargo is executed directly
-        //   circumventing the rustup proxy, then this would affect calls to
-        //   rustc (assuming that is a proxy), which could potentially cause
-        //   problems with cargo and rustc being from different toolchains. We
-        //   consider this to be not a use case we would like to support,
-        //   since it will likely cause problems or lead to confusion.
-        for disallowed in &["CARGO_HOME", "RUSTUP_HOME", "RUSTUP_TOOLCHAIN"] {
-            if env_config.contains_key(*disallowed) {
-                bail!(
-                    "setting the `{disallowed}` environment variable is not supported \
-                    in the `[env]` configuration table"
-                );
-            }
-        }
+    /// Get the env vars from the config `[env]` table which
+    /// are `force = true` or don't exist in the env snapshot [`GlobalContext::get_env`].
+    pub fn env_config(&self) -> CargoResult<&Arc<HashMap<String, OsString>>> {
+        let env_config = self.env_config.try_borrow_with(|| {
+            CargoResult::Ok(Arc::new({
+                let env_config = self.get::<EnvConfig>("env")?;
+                // Reasons for disallowing these values:
+                //
+                // - CARGO_HOME: The initial call to cargo does not honor this value
+                //   from the [env] table. Recursive calls to cargo would use the new
+                //   value, possibly behaving differently from the outer cargo.
+                //
+                // - RUSTUP_HOME and RUSTUP_TOOLCHAIN: Under normal usage with rustup,
+                //   this will have no effect because the rustup proxy sets
+                //   RUSTUP_HOME and RUSTUP_TOOLCHAIN, and that would override the
+                //   [env] table. If the outer cargo is executed directly
+                //   circumventing the rustup proxy, then this would affect calls to
+                //   rustc (assuming that is a proxy), which could potentially cause
+                //   problems with cargo and rustc being from different toolchains. We
+                //   consider this to be not a use case we would like to support,
+                //   since it will likely cause problems or lead to confusion.
+                for disallowed in &["CARGO_HOME", "RUSTUP_HOME", "RUSTUP_TOOLCHAIN"] {
+                    if env_config.contains_key(*disallowed) {
+                        bail!(
+                            "setting the `{disallowed}` environment variable is not supported \
+                            in the `[env]` configuration table"
+                        );
+                    }
+                }
+                env_config
+                    .into_iter()
+                    .filter_map(|(k, v)| {
+                        if v.is_force() || self.get_env_os(&k).is_none() {
+                            Some((k, v.resolve(self).to_os_string()))
+                        } else {
+                            None
+                        }
+                    })
+                    .collect()
+            }))
+        })?;
 
         Ok(env_config)
     }

tests/testsuite/cargo_env_config.rs

@@ -276,3 +276,168 @@ MAIN ENV_TEST:from-env
 "#]])
         .run();
 }
+
+#[cargo_test]
+fn env_changed_defined_in_config_toml() {
+    let p = project()
+        .file("Cargo.toml", &basic_bin_manifest("foo"))
+        .file(
+            "src/main.rs",
+            r#"
+        use std::env;
+        fn main() {
+            println!( "{}", env!("ENV_TEST") );
+        }
+        "#,
+        )
+        .file(
+            ".cargo/config.toml",
+            r#"
+                [env]
+                ENV_TEST = "from-config"
+            "#,
+        )
+        .build();
+
+    p.cargo("run")
+        .with_stdout_data(str![[r#"
+from-config
+
+"#]])
+        .with_stderr_data(str![[r#"
+[COMPILING] foo v0.5.0 ([ROOT]/foo)
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE]`
+
+"#]])
+        .run();
+
+    p.cargo("run")
+        .env("ENV_TEST", "from-env")
+        .with_stdout_data(str![[r#"
+from-env
+
+"#]])
+        .with_stderr_data(str![[r#"
+[COMPILING] foo v0.5.0 ([ROOT]/foo)
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE]`
+
+"#]])
+        .run();
+    // This identical cargo invocation is to ensure no rebuild happen.
+    p.cargo("run")
+        .env("ENV_TEST", "from-env")
+        .with_stdout_data(str![[r#"
+from-env
+
+"#]])
+        .with_stderr_data(str![[r#"
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE]`
+
+"#]])
+        .run();
+}
+
+#[cargo_test]
+fn forced_env_changed_defined_in_config_toml() {
+    let p = project()
+        .file("Cargo.toml", &basic_bin_manifest("foo"))
+        .file(
+            "src/main.rs",
+            r#"
+        use std::env;
+        fn main() {
+            println!( "{}", env!("ENV_TEST") );
+        }
+        "#,
+        )
+        .file(
+            ".cargo/config.toml",
+            r#"
+                [env]
+                ENV_TEST = {value = "from-config", force = true}
+            "#,
+        )
+        .build();
+
+    p.cargo("run")
+        .with_stdout_data(str![[r#"
+from-config
+
+"#]])
+        .with_stderr_data(str![[r#"
+[COMPILING] foo v0.5.0 ([ROOT]/foo)
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE]`
+
+"#]])
+        .run();
+
+    p.cargo("run")
+        .env("ENV_TEST", "from-env")
+        .with_stdout_data(str![[r#"
+from-config
+
+"#]])
+        .with_stderr_data(str![[r#"
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE]`
+
+"#]])
+        .run();
+}
+
+#[cargo_test]
+fn env_changed_defined_in_config_args() {
+    let p = project()
+        .file("Cargo.toml", &basic_bin_manifest("foo"))
+        .file(
+            "src/main.rs",
+            r#"
+        use std::env;
+        fn main() {
+            println!( "{}", env!("ENV_TEST") );
+        }
+        "#,
+        )
+        .build();
+    p.cargo(r#"run --config 'env.ENV_TEST="one"'"#)
+        .with_stdout_data(str![[r#"
+one
+
+"#]])
+        .with_stderr_data(str![[r#"
+[COMPILING] foo v0.5.0 ([ROOT]/foo)
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE]`
+
+"#]])
+        .run();
+
+    p.cargo(r#"run --config 'env.ENV_TEST="two"'"#)
+        .with_stdout_data(str![[r#"
+two
+
+"#]])
+        .with_stderr_data(str![[r#"
+[COMPILING] foo v0.5.0 ([ROOT]/foo)
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE]`
+
+"#]])
+        .run();
+    // This identical cargo invocation is to ensure no rebuild happen.
+    p.cargo(r#"run --config 'env.ENV_TEST="two"'"#)
+        .with_stdout_data(str![[r#"
+two
+
+"#]])
+        .with_stderr_data(str![[r#"
+[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+[RUNNING] `target/debug/foo[EXE]`
+
+"#]])
+        .run();
+}