Skip to content

Commit

Permalink
Auto merge of #9363 - ehuss:cargo-env-fingerprint, r=alexcrichton
Browse files Browse the repository at this point in the history
Track "CARGO" in environment fingerprint.

There is an issue where if a package includes an `env!("CARGO")`, that value is not tracked in the fingerprint. If different cargos are used, it will not rebuild. This changes it so that the path to cargo will be tracked in the fingerprint if the CARGO env var is in the dep-info file.

This came up with rust's build system where it [tracks the env](https://github.com/rust-lang/rust/blob/60158f4a7cf3e3063df6127d3f0d206921d285b0/src/bootstrap/config.rs#L574). If you build rust once, and then change the `cargo` value in `config.toml` and try building again, it would not pick up the change which caused me some confusion.

In theory, cargo could fingerprint this every time, but I figure that could be disruptive and trigger needlessly in some situations.

This diff is a little bigger than I would like for such an obscure case.  As an alternative, I think rustbuild could just print `cargo:rerun-if-env-changed=CARGO`, but I figure this could potentially be useful for other projects.
  • Loading branch information
bors committed Apr 16, 2021
2 parents e870eac + 2551535 commit 57b7597
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 4 deletions.
37 changes: 33 additions & 4 deletions src/cargo/core/compiler/fingerprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ use crate::util;
use crate::util::errors::{CargoResult, CargoResultExt};
use crate::util::interning::InternedString;
use crate::util::{internal, path_args, profile};
use crate::CARGO_ENV;

use super::custom_build::BuildDeps;
use super::job::{Job, Work};
Expand Down Expand Up @@ -712,6 +713,7 @@ impl LocalFingerprint {
mtime_cache: &mut HashMap<PathBuf, FileTime>,
pkg_root: &Path,
target_root: &Path,
cargo_exe: &Path,
) -> CargoResult<Option<StaleItem>> {
match self {
// We need to parse `dep_info`, learn about the crate's dependencies.
Expand All @@ -727,7 +729,21 @@ impl LocalFingerprint {
None => return Ok(Some(StaleItem::MissingFile(dep_info))),
};
for (key, previous) in info.env.iter() {
let current = env::var(key).ok();
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()
)
})?
.to_string(),
)
} else {
env::var(key).ok()
};
if current == *previous {
continue;
}
Expand Down Expand Up @@ -980,6 +996,7 @@ impl Fingerprint {
mtime_cache: &mut HashMap<PathBuf, FileTime>,
pkg_root: &Path,
target_root: &Path,
cargo_exe: &Path,
) -> CargoResult<()> {
assert!(!self.fs_status.up_to_date());

Expand Down Expand Up @@ -1071,7 +1088,9 @@ impl Fingerprint {
// files for this package itself. If we do find something log a helpful
// message and bail out so we stay stale.
for local in self.local.get_mut().unwrap().iter() {
if let Some(item) = local.find_stale_item(mtime_cache, pkg_root, target_root)? {
if let Some(item) =
local.find_stale_item(mtime_cache, pkg_root, target_root, cargo_exe)?
{
item.log();
return Ok(());
}
Expand Down Expand Up @@ -1256,7 +1275,13 @@ fn calculate(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Arc<Fingerpri
// After we built the initial `Fingerprint` be sure to update the
// `fs_status` field of it.
let target_root = target_root(cx);
fingerprint.check_filesystem(&mut cx.mtime_cache, unit.pkg.root(), &target_root)?;
let cargo_exe = cx.bcx.config.cargo_exe()?;
fingerprint.check_filesystem(
&mut cx.mtime_cache,
unit.pkg.root(),
&target_root,
cargo_exe,
)?;

let fingerprint = Arc::new(fingerprint);
cx.fingerprints
Expand Down Expand Up @@ -1850,9 +1875,13 @@ pub fn translate_dep_info(
// 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.
on_disk_info
.env
.retain(|(key, _)| !rustc_cmd.get_envs().contains_key(key));
.retain(|(key, _)| !rustc_cmd.get_envs().contains_key(key) || key == CARGO_ENV);

for file in depinfo.files {
// The path may be absolute or relative, canonical or not. Make sure
Expand Down
60 changes: 60 additions & 0 deletions tests/testsuite/freshness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2581,3 +2581,63 @@ fn env_build_script_no_rebuild() {
p.cargo("build").run();
p.cargo("build").with_stderr("[FINISHED] [..]").run();
}

#[cargo_test]
fn cargo_env_changes() {
// Checks that changes to the env var CARGO in the dep-info file triggers
// a rebuild.
let p = project()
.file("Cargo.toml", &basic_manifest("foo", "1.0.0"))
.file(
"src/main.rs",
r#"
fn main() {
println!("{:?}", env!("CARGO"));
}
"#,
)
.build();

let cargo_exe = cargo_test_support::cargo_exe();
let other_cargo_path = p.root().join(cargo_exe.file_name().unwrap());
std::fs::hard_link(&cargo_exe, &other_cargo_path).unwrap();
let other_cargo = || {
let mut pb = cargo_test_support::process(&other_cargo_path);
pb.cwd(p.root());
cargo_test_support::execs().with_process_builder(pb)
};

p.cargo("check").run();
other_cargo()
.arg("check")
.arg("-v")
.with_stderr(
"\
[CHECKING] foo [..]
[RUNNING] `rustc [..]
[FINISHED] [..]
",
)
.run();

// And just to confirm that without using env! it doesn't rebuild.
p.change_file("src/main.rs", "fn main() {}");
p.cargo("check")
.with_stderr(
"\
[CHECKING] foo [..]
[FINISHED] [..]
",
)
.run();
other_cargo()
.arg("check")
.arg("-v")
.with_stderr(
"\
[FRESH] foo [..]
[FINISHED] [..]
",
)
.run();
}

0 comments on commit 57b7597

Please sign in to comment.