From cee088b0db01076deb11c037fe8b64b238b005a2 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 12 Dec 2020 13:59:51 -0800 Subject: [PATCH 1/2] Check if rerun-if-changed points to a directory. --- src/cargo/core/compiler/fingerprint.rs | 2 +- src/cargo/util/paths.rs | 36 ++++++++++ src/doc/src/reference/build-scripts.md | 17 ++--- tests/testsuite/build_script.rs | 91 +++++++++++++++++++++++++- 4 files changed, 133 insertions(+), 13 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index 8a5113d713c..7e4a543fca4 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -1700,7 +1700,7 @@ where let path_mtime = match mtime_cache.entry(path.to_path_buf()) { Entry::Occupied(o) => *o.get(), Entry::Vacant(v) => { - let mtime = match paths::mtime(path) { + let mtime = match paths::mtime_recursive(path) { Ok(mtime) => mtime, Err(..) => return Some(StaleItem::MissingFile(path.to_path_buf())), }; diff --git a/src/cargo/util/paths.rs b/src/cargo/util/paths.rs index b8b217813b9..34e4b6b3764 100644 --- a/src/cargo/util/paths.rs +++ b/src/cargo/util/paths.rs @@ -180,6 +180,42 @@ pub fn mtime(path: &Path) -> CargoResult { Ok(FileTime::from_last_modification_time(&meta)) } +/// Returns the maximum mtime of the given path, recursing into +/// subdirectories, and following symlinks. +pub fn mtime_recursive(path: &Path) -> CargoResult { + let meta = fs::metadata(path).chain_err(|| format!("failed to stat `{}`", path.display()))?; + if !meta.is_dir() { + return Ok(FileTime::from_last_modification_time(&meta)); + } + let max_meta = walkdir::WalkDir::new(path) + .follow_links(true) + .into_iter() + .filter_map(|e| e.ok()) + .filter_map(|e| { + if e.path_is_symlink() { + // Use the mtime of both the symlink and its target, to + // handle the case where the symlink is modified to a + // different target. + let sym_meta = std::fs::symlink_metadata(e.path()).ok()?; + let sym_mtime = FileTime::from_last_modification_time(&sym_meta); + // Walkdir follows symlinks. + match e.metadata() { + Ok(target_meta) => { + let target_mtime = FileTime::from_last_modification_time(&target_meta); + Some(sym_mtime.max(target_mtime)) + } + Err(_) => Some(sym_mtime), + } + } else { + let meta = e.metadata().ok()?; + Some(FileTime::from_last_modification_time(&meta)) + } + }) + .max() + .unwrap_or_else(|| FileTime::from_last_modification_time(&meta)); + Ok(max_meta) +} + /// Record the current time on the filesystem (using the filesystem's clock) /// using a file at the given directory. Returns the current time. pub fn set_invocation_time(path: &Path) -> CargoResult { diff --git a/src/doc/src/reference/build-scripts.md b/src/doc/src/reference/build-scripts.md index 3bdbb6254f2..6e4083a7a3c 100644 --- a/src/doc/src/reference/build-scripts.md +++ b/src/doc/src/reference/build-scripts.md @@ -258,19 +258,16 @@ filesystem last-modified "mtime" timestamp to determine if the file has changed. It compares against an internal cached timestamp of when the build script last ran. -If the path points to a directory, it does *not* automatically traverse the -directory for changes. Only the mtime change of the directory itself is -considered (which corresponds to some types of changes within the directory, -depending on platform). To request a re-run on any changes within an entire -directory, print a line for the directory and separate lines for everything -inside it, recursively. +If the path points to a directory, it will scan the entire directory for +any modifications. If the build script inherently does not need to re-run under any circumstance, then emitting `cargo:rerun-if-changed=build.rs` is a simple way to prevent it -from being re-run. Cargo automatically handles whether or not the script -itself needs to be recompiled, and of course the script will be re-run after -it has been recompiled. Otherwise, specifying `build.rs` is redundant and -unnecessary. +from being re-run (otherwise, the default if no `rerun-if` instructions are +emitted is to scan the entire package directory for changes). Cargo +automatically handles whether or not the script itself needs to be recompiled, +and of course the script will be re-run after it has been recompiled. +Otherwise, specifying `build.rs` is redundant and unnecessary. #### `cargo:rerun-if-env-changed=NAME` diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index 40b61b90b87..94a3c409892 100644 --- a/tests/testsuite/build_script.rs +++ b/tests/testsuite/build_script.rs @@ -8,8 +8,8 @@ use std::thread; use cargo::util::paths::remove_dir_all; use cargo_test_support::paths::CargoPathExt; use cargo_test_support::registry::Package; -use cargo_test_support::{basic_manifest, cross_compile, project}; -use cargo_test_support::{rustc_host, sleep_ms, slow_cpu_multiplier}; +use cargo_test_support::{basic_manifest, cross_compile, is_coarse_mtime, project}; +use cargo_test_support::{rustc_host, sleep_ms, slow_cpu_multiplier, symlink_supported}; #[cargo_test] fn custom_build_script_failed() { @@ -4071,3 +4071,90 @@ fn dev_dep_with_links() { .build(); p.cargo("check --tests").run() } + +#[cargo_test] +fn rerun_if_directory() { + if !symlink_supported() { + return; + } + + // rerun-if-changed of a directory should rerun if any file in the directory changes. + let p = project() + .file("Cargo.toml", &basic_manifest("foo", "0.1.0")) + .file("src/lib.rs", "") + .file( + "build.rs", + r#" + fn main() { + println!("cargo:rerun-if-changed=somedir"); + } + "#, + ) + .build(); + + let dirty = || { + p.cargo("check") + .with_stderr( + "[COMPILING] foo [..]\n\ + [FINISHED] [..]", + ) + .run(); + }; + + let fresh = || { + p.cargo("check").with_stderr("[FINISHED] [..]").run(); + }; + + // Start with a missing directory. + dirty(); + // Because the directory doesn't exist, it will trigger a rebuild every time. + // https://github.com/rust-lang/cargo/issues/6003 + dirty(); + + if is_coarse_mtime() { + sleep_ms(1000); + } + + // Empty directory. + fs::create_dir(p.root().join("somedir")).unwrap(); + dirty(); + fresh(); + + if is_coarse_mtime() { + sleep_ms(1000); + } + + // Add a file. + p.change_file("somedir/foo", ""); + p.change_file("somedir/bar", ""); + dirty(); + fresh(); + + if is_coarse_mtime() { + sleep_ms(1000); + } + + // Add a symlink. + p.symlink("foo", "somedir/link"); + dirty(); + fresh(); + + if is_coarse_mtime() { + sleep_ms(1000); + } + + // Move the symlink. + fs::remove_file(p.root().join("somedir/link")).unwrap(); + p.symlink("bar", "somedir/link"); + dirty(); + fresh(); + + if is_coarse_mtime() { + sleep_ms(1000); + } + + // Remove a file. + fs::remove_file(p.root().join("somedir/foo")).unwrap(); + dirty(); + fresh(); +} From 4c68a4ad793142cdc321aa8e6ff687ff9c927a97 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Mon, 14 Dec 2020 08:55:10 -0800 Subject: [PATCH 2/2] mtime_recursive: Add some comments and debugging diagnostics. --- src/cargo/util/paths.rs | 53 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/src/cargo/util/paths.rs b/src/cargo/util/paths.rs index 34e4b6b3764..9698ae2252e 100644 --- a/src/cargo/util/paths.rs +++ b/src/cargo/util/paths.rs @@ -190,13 +190,34 @@ pub fn mtime_recursive(path: &Path) -> CargoResult { let max_meta = walkdir::WalkDir::new(path) .follow_links(true) .into_iter() - .filter_map(|e| e.ok()) + .filter_map(|e| match e { + Ok(e) => Some(e), + Err(e) => { + // Ignore errors while walking. If Cargo can't access it, the + // build script probably can't access it, either. + log::debug!("failed to determine mtime while walking directory: {}", e); + None + } + }) .filter_map(|e| { if e.path_is_symlink() { // Use the mtime of both the symlink and its target, to // handle the case where the symlink is modified to a // different target. - let sym_meta = std::fs::symlink_metadata(e.path()).ok()?; + let sym_meta = match std::fs::symlink_metadata(e.path()) { + Ok(m) => m, + Err(err) => { + // I'm not sure when this is really possible (maybe a + // race with unlinking?). Regardless, if Cargo can't + // read it, the build script probably can't either. + log::debug!( + "failed to determine mtime while fetching symlink metdata of {}: {}", + e.path().display(), + err + ); + return None; + } + }; let sym_mtime = FileTime::from_last_modification_time(&sym_meta); // Walkdir follows symlinks. match e.metadata() { @@ -204,14 +225,38 @@ pub fn mtime_recursive(path: &Path) -> CargoResult { let target_mtime = FileTime::from_last_modification_time(&target_meta); Some(sym_mtime.max(target_mtime)) } - Err(_) => Some(sym_mtime), + Err(err) => { + // Can't access the symlink target. If Cargo can't + // access it, the build script probably can't access + // it either. + log::debug!( + "failed to determine mtime of symlink target for {}: {}", + e.path().display(), + err + ); + Some(sym_mtime) + } } } else { - let meta = e.metadata().ok()?; + let meta = match e.metadata() { + Ok(m) => m, + Err(err) => { + // I'm not sure when this is really possible (maybe a + // race with unlinking?). Regardless, if Cargo can't + // read it, the build script probably can't either. + log::debug!( + "failed to determine mtime while fetching metadata of {}: {}", + e.path().display(), + err + ); + return None; + } + }; Some(FileTime::from_last_modification_time(&meta)) } }) .max() + // or_else handles the case where there are no files in the directory. .unwrap_or_else(|| FileTime::from_last_modification_time(&meta)); Ok(max_meta) }