cargo:rerun-if-env-changed doesn't work with env configuration

[bsilver8192]

Problem

When a build.rs script emits cargo:rerun-if-env-changed, it is not re-run when the value of the specified variable is changed via the env configuration.

Steps

build.rs:

pub fn main() {
    println!("cargo:rerun-if-env-changed=FOO");
    if let Ok(foo) = std::env::var("FOO") {
        assert!(&foo != "bad");
    }
}

.cargo/config.toml:

[env]
FOO = "good"

1. Run cargo build, it succeeds.
2. Change .cargo/config.toml to this:

[env]
FOO = "bad"

3. Run cargo build again, and it uses the cached build and succeeds. It should run build.rs again, and it should fail.

Workaround

4. Run FOO=bar cargo build, ignore the result.
5. Run cargo build again, now it runs build.rs with FOO=bad from the config and fails as expected.

Possible Solution(s)

No response

Notes

No response

Version

cargo 1.58.0 (7f08ace4f 2021-11-24)
release: 1.58.0
commit-hash: 7f08ace4f1305de7f3b1b0e2f765911957226bd4
commit-date: 2021-11-24
host: x86_64-unknown-linux-gnu
libgit2: 1.3.0 (sys:0.13.23 vendored)
libcurl: 7.80.0-DEV (sys:0.4.51+curl-7.80.0 vendored ssl:OpenSSL/1.1.1l)
os: Debian 11 (bullseye) [64-bit]

[weihanglo]

This is somehow similar to #10094. Cargo does not take into account the new configurable-env when calculating local fingerprint of rerun-if-env-changed.

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

Lines 1571 to 1577 in 07e9d46

	for var in deps.rerun_if_env_changed.iter() {
	let val = env::var(var).ok();
	local.push(LocalFingerprint::RerunIfEnvChanged {
	var: var.clone(),
	val,
	});
	}

When dealing with this issue, perhaps we should consider corner cases mentioned it this comment as well.

[therealfrauholle]

To share my experience: This is a really nasty bug. We finally found this issue and are now aware of it, but we actually faced this issue often in the past, e.g. when configuring logging levels by the [env] section. It caused us a lot of pain since it is so subtle, yet has impact on what is actually compiled.

Looks like a small thing, but should be of high priority, I think - maybe add a warning?

[bczhc]

+1. Kept debugging and debugging, just to end up finding this issue. Cargo doesn't compile with the right envs as the configuration file writes, quite annoying and tricky...

[weihanglo]

If I understand correctly, this shares the same root cause with #12434. Just two different ways to get hurt by the bug.

[weihanglo]

Copying my comment in #12434 (comment):

We should perhaps switch from std::env::var to Config::get_env and Config::env_config.

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

Lines 776 to 784 in 3a34fca

	// TODO: This is allowed at this moment. Should figure out if it makes
	// sense if permitting to read env from the config system.
	#[allow(clippy::disallowed_methods)]
	fn from_env<K: AsRef<str>>(key: K) -> LocalFingerprint {
	let key = key.as_ref();
	let var = key.to_owned();
	let val = env::var(key).ok();
	LocalFingerprint::RerunIfEnvChanged { var, val }
	}

However, there are at least two hard things needing to consider.

• Environment variables in [env] can be “config-relative”. Should we hash the absolute path into local fingerprint? That seems to be the correct behavior, but it might also hurt the cache sharing story when a user has changed the project root path.
• To resolve config-relative paths to absolute, we need Config. However rerun-if-env-changed is computed within a unit of work Cargo spawned. The unit of work is executed in a multi-thread context so Config cannot be easily shared. The code might need a relative large overhual to make it work.

[Swoorup]

I ran into this today, this is quite troublesome and rather annoying.

[sxlijin]

Also just ran into this while building a Rust-Ruby library using rb-sys.

[heisen-li]

The problem seemed very annoying and lingered for a relatively long time. I've looked into it today, and I'm not sure if it's the optimal solution:
the crux of the problem seems to be that after modifying env.FOO in config.toml, it's compiling again without using the latest values, so I'm planning to use the environment variables in ProcessBuilder::env, where the values are guaranteed to be new.

I think this modification has the least impact, but what I'm not sure about is whether it has any impact on the overall plan for Cargo. Thanks.

@weihanglo

[Christiaan676]

However, there are at least two hard things needing to consider.

* Environment variables in `[env]` can be [“config-relative”](https://doc.rust-lang.org/nightly/cargo/reference/config.html#config-relative-paths). Should we hash the absolute path into local fingerprint? That seems to be the correct behavior, but it might also hurt the cache sharing story when a user has changed the project root path.

* To resolve config-relative paths to absolute, we need [`Config`](https://doc.rust-lang.org/nightly/nightly-rustc/cargo/util/config/struct.Config.html). However `rerun-if-env-changed` is computed within [a unit of work Cargo spawned](https://github.com/rust-lang/cargo/blob/ac6bbb33293d8d424c17ecdb42af3aac25fb7295/src/cargo/core/compiler/fingerprint/mod.rs#L514). The unit of work is executed in a multi-thread context so `Config` cannot be easily shared. The code might need a relative large overhual to make it work.

Ran into this issue today, and took a while to figure out why things where not working correctly.

Regarding your question about the config-relative paths. Rebuilding more often then absolutely needed is in my opinion preferably over not rebuilding when things have changed. And this could be solved in two steps. First the absolute path is used and we accept the fact that if a user moves the directory around a rebuild will occur. This seams like minor inconvenience, and later upgrade to make handling of config-relative paths more efficient.

[epage]

This seams like minor inconvenience,

Keep in mind that for a lot of CIs, every build is in a unique directory so this would preclude CI caching until its fixed.

, and later upgrade to make handling of config-relative paths more efficient.

And, if its determined that we can fix it later in a compatible way, then doing this incrementally is preferred so long as it doesn't affect people not using this feature.

[Christiaan676]

This seams like minor inconvenience,

Keep in mind that for a lot of CIs, every build is in a unique directory so this would preclude CI caching until its fixed.
Sure, but I would still prefer a rebuild over a build that uses the old ENV value of some cached build. As to my understanding you currently have no idea what value was used to compile that cached build. When sharing the build cache between CI jobs this seams to me to be even more important as currently you would basically be forced to run a cargo clean && cargo build to make sure every thing is compiled as expected.

For the quick fix I'm assuming its possible to add the value of the ENV var to the fingerprint. And that it would only trigger a rebuild of the libraries that use that environment variable. But let me know if I'm wrong here.

Looking at the code in fingerprint/mod.rs the compile produces a .d file that contains the env variables used (# env-dep:CARGO_MANIFEST_DIR=...). Quick test on my side, shows that the env var set in the config.toml are missing there. So I'm guessing that this would mean a change to the compiler to add them there, so that cargo can pick that up for the fingerprint.

Edit:
Think I found the issue. For every "rerun-if-env-changed" the function LocalFingerprint::from_env will be called:

    /// Read the environment variable of the given env `key`, and creates a new
    /// [`LocalFingerprint::RerunIfEnvChanged`] for it.
    ///
    // TODO: This is allowed at this moment. Should figure out if it makes
    // sense if permitting to read env from the config system.
    #[allow(clippy::disallowed_methods)]
    fn from_env<K: AsRef<str>>(key: K) -> LocalFingerprint {
        let key = key.as_ref();
        let var = key.to_owned();
        let val = env::var(key).ok();
        LocalFingerprint::RerunIfEnvChanged { var, val }
    }

And as can be seen, it only tries to find the environment variable in the process environment variables and does not look at the configuration. (Wonder if this also means that the cargo::rustc-env=VAR=VALUE in combination with the "rerun-if-env-changed" will not work correctly)

cargo\core\compiler\mod.rs uses the following function to fill the fill the env variables for rustc:

/// Applies environment variables from config `[env]` to [`ProcessBuilder`].
pub(crate) fn apply_env_config(
    gctx: &crate::GlobalContext,
    cmd: &mut ProcessBuilder,
) -> CargoResult<()> {
    for (key, value) in gctx.env_config()?.iter() {
        // never override a value that has already been set by cargo
        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));
        }
    }
    Ok(())
}

My assumption is that we would need to change the from_env function to look more like the apply_env_config function. Or maybe we should look at the env variables that where set during the rustc invocation, as they after all are the true source.

And this is basically what @weihanglo already commented ...
And I agree with @heisen-li that taking the values from ProcessBuilder::env with a fallback to env! seams to be the most sensible solution. That should trigger a rebuild, when a env var in the configuration changes.