fix(build-rs): Implicitly report rerun-if-env-changed for input

[epage]

What does this PR try to resolve?

As we abstract away the env variables, users can't do a good job of
reporting these, so we'll do it ourselves.

How should we test and review this PR?

This isn't the shortest path to this solution because I had originally designed this like more like build_env but in a way to allow users to choose what policies they want (rather than baking in their HOST/TARGET scheme).
I decided to not support a custom env yet to make this more incremental and to see what feedback we get but I left the implementation in a way that would make it easy to support it.

I made is_present part of Env because I was unsure if callers of the API would want to handle that differently for rerun-if-env-changed or other parts.

Additional information

CC @CAD97

[rustbot]

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[CAD97]

👍 on direction from me.

cargo::rerun-on-env-changed is purely additive and doesn't disable a default watch set like cargo::rerun-on-changed does, so emitting it automatically seems like a good idea.

Although, I'm not sure it's actually needed for the CARGO_ environment? It depends on how aggressive Cargo is at caching the buildscript and if any of this environment can change without necessarily requiring a recompile.

[CAD97]

Changing this to e.g. ENV.require(key) may be a good idea to keep env access more uniform, and paves the way towards maybe supporting a different protocol in the future.

[epage]

I intentionally kept it off.

I see two ways of going

• Minimum interface for what is needed
• Every input being an inherent function on the trait

I went with the former. I only included is_present on the trait because I was concerned about what the right policy is for re-run if changed in that case. The direction I went with doesn't specialize it but I figured I'd still leave that door open. I'm not seeing a reason to specialize this atm and would rather wait for use cases rather than speculatively adding functionality to the trait.

Since we can add this as an inherent function, we can likely do so without a breaking change (yes, there are caveats). Even then, its not a big deal to make a breaking change.

[CAD97]

Especially since the trait/type aren't pub (yet?) and are just internal helpers.

[epage]

Although, I'm not sure it's actually needed for the CARGO_ environment? It depends on how aggressive Cargo is at caching the buildscript and if any of this environment can change without necessarily requiring a recompile.

The only cases I saw us specially handle are

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

Lines 852 to 865 in 2f74b54

	let current = if key == CARGO_ENV {
	Some(cargo_exe.to_str().ok_or_else(|| {
	format_err!(
	"cargo exe path {} must be valid UTF-8",
	cargo_exe.display()
	)
	})?)
	} else {
	if let Some(value) = gctx.env_config()?.get(key) {
	value.to_str()
	} else {
	gctx.get_env(key).ok()
	}
	};

and

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

Lines 306 to 338 in 2f74b54

	// This is a bit of a tricky statement, but here we're *removing* the
	// dependency on environment variables that were defined specifically for
	// the command itself. Environment variables returned by `get_envs` includes
	// environment variables like:
	//
	// * `OUT_DIR` if applicable
	// * env vars added by a build script, if any
	//
	// The general idea here is that the dep info file tells us what, when
	// changed, should cause us to rebuild the crate. These environment
	// variables are synthesized by Cargo and/or the build script, and the
	// intention is that their values are tracked elsewhere for whether the
	// crate needs to be rebuilt.
	//
	// For example a build script says when it needs to be rerun and otherwise
	// it's assumed to produce the same output, so we're guaranteed that env
	// vars defined by the build script will always be the same unless the build
	// script itself reruns, in which case the crate will rerun anyway.
	//
	// For things like `OUT_DIR` it's a bit sketchy for now. Most of the time
	// that's used for code generation but this is technically buggy where if
	// you write a binary that does `println!("{}", env!("OUT_DIR"))` we won't
	// recompile that if you move the target directory. Hopefully that's not too
	// bad of an issue for now...
	//
	// 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.
	//
	// 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
	});

Which makes sense. We can set a lot and we likely don't want to re-run if all of it has changed. Some we'll rebuild for anyways because some of the fingerprint has changed but I figure its better to be over aggressive than under aggressive or having to maintain a perfectly curated set of ENVs

[Muscraft]

Thanks! This seems like a great incremental step overall.

[weihanglo]

Which makes sense. We can set a lot and we likely don't want to re-run if all of it has changed. Some we'll rebuild for anyways because some of the fingerprint has changed but I figure its better to be over aggressive than under aggressive or having to maintain a perfectly curated set of ENVs

Agree with this. While we don't recommend user to hand-write rerun-if-env-changed with cargo internal envs, as a build script helper crate it should be fine as an implementation detail.