Auto merge of #7137 - ehuss:dep-info-absolute-paths, r=alexcrichton

Fix some issues with absolute paths in dep-info files.

There were some issues with how #7030 was handling translating paths in dep-info files. The main consequence is that when coupled with rust-lang/rust#61727 (which has not yet merged), the fingerprint would fail and be considered dirty when it should be fresh.

It was joining [`target_root`](https://github.com/rust-lang/cargo/blame/1140c527c4c3b3e2533b9771d67f88509ef7fc16/src/cargo/core/compiler/fingerprint.rs#L1352-L1360) which had 3 different values, but stripping [only one](https://github.com/rust-lang/cargo/blame/1140c527c4c3b3e2533b9771d67f88509ef7fc16/src/cargo/core/compiler/mod.rs#L323). This means for different scenarios (like using `--target`), it was creating incorrect paths in some cases. For example a target having a proc-macro dependency which would be in the host directory.

The solution here is to always use CARGO_TARGET_DIR as the base that all relative paths are checked against. This should handle all host/target differences.

The tests are marked with `#[ignore]` because 61727 has not yet merged.

This includes a second commit (which I can open as a separate PR if needed) which is an alternate solution to #7034. It adds dep-info tracking for registry dependencies. However, it tries to limit which kinds of paths it tracks. It will not track package-relative paths (like source files). It also adds an mtime cache to significantly reduce the number of stat calls (because every dependency was stating every file in sysroot).

Finally, I've run some tests using this PR with 61727 in rust-lang/rust. I can confirm that a second build is fresh (it doesn't erroneously rebuild). I can also confirm that the problem in rust-lang/rust#59105 has *finally* been fixed!

My confidence in all this is pretty low, but I've been unable to think of any specific ways to make it fail. If anyone has ideas on how to better test this, let me know. Also, feel free to ask questions since I've been thinking about this a lot for the past few weeks, and there is quite a bit of subtle stuff going on.

Author: bors

Cargo.toml

@@ -47,6 +47,7 @@ memchr = "2.1.3"
 num_cpus = "1.0"
 opener = "0.4"
 percent-encoding = "2.0"
+remove_dir_all = "0.5.2"
 rustfix = "0.4.4"
 same-file = "1"
 semver = { version = "0.9.0", features = ["serde"] }

crates/resolver-tests/tests/resolve.rs

@@ -2,7 +2,7 @@ use std::env;
 
 use cargo::core::dependency::Kind;
 use cargo::core::{enable_nightly_features, Dependency};
-use cargo::util::Config;
+use cargo::util::{is_ci, Config};
 
 use resolver_tests::{
     assert_contains, assert_same, dep, dep_kind, dep_loc, dep_req, dep_req_kind, loc_names, names,
@@ -22,7 +22,7 @@ use proptest::prelude::*;
 proptest! {
     #![proptest_config(ProptestConfig {
         max_shrink_iters:
-            if env::var("CI").is_ok() || !atty::is(atty::Stream::Stderr) {
+            if is_ci() || !atty::is(atty::Stream::Stderr) {
                 // This attempts to make sure that CI will fail fast,
                 0
             } else {

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

@@ -5,6 +5,7 @@ use std::fmt::Write;
 use std::path::PathBuf;
 use std::sync::Arc;
 
+use filetime::FileTime;
 use jobserver::Client;
 
 use crate::core::compiler::compilation;
@@ -34,6 +35,7 @@ pub struct Context<'a, 'cfg> {
     pub build_script_overridden: HashSet<(PackageId, Kind)>,
     pub build_explicit_deps: HashMap<Unit<'a>, BuildDeps>,
     pub fingerprints: HashMap<Unit<'a>, Arc<Fingerprint>>,
+    pub mtime_cache: HashMap<PathBuf, FileTime>,
     pub compiled: HashSet<Unit<'a>>,
     pub build_scripts: HashMap<Unit<'a>, Arc<BuildScripts>>,
     pub links: Links,
@@ -82,6 +84,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
             compilation: Compilation::new(bcx)?,
             build_state: Arc::new(BuildState::new(&bcx.host_config, &bcx.target_config)),
             fingerprints: HashMap::new(),
+            mtime_cache: HashMap::new(),
             compiled: HashSet::new(),
             build_scripts: HashMap::new(),
             build_explicit_deps: HashMap::new(),

src/cargo/core/compiler/fingerprint.rs

@@ -187,7 +187,7 @@
 //! See the `A-rebuild-detection` flag on the issue tracker for more:
 //! <https://github.com/rust-lang/cargo/issues?q=is%3Aissue+is%3Aopen+label%3AA-rebuild-detection>
 
-use std::collections::HashMap;
+use std::collections::hash_map::{Entry, HashMap};
 use std::env;
 use std::fs;
 use std::hash::{self, Hasher};
@@ -213,7 +213,7 @@ use super::job::{
     Freshness::{Dirty, Fresh},
     Job, Work,
 };
-use super::{BuildContext, Context, FileFlavor, Kind, Unit};
+use super::{BuildContext, Context, FileFlavor, Unit};
 
 /// Determines if a `unit` is up-to-date, and if not prepares necessary work to
 /// update the persisted fingerprint.
@@ -539,6 +539,7 @@ impl LocalFingerprint {
     /// file accesses.
     fn find_stale_file(
         &self,
+        mtime_cache: &mut HashMap<PathBuf, FileTime>,
         pkg_root: &Path,
         target_root: &Path,
     ) -> CargoResult<Option<StaleFile>> {
@@ -550,7 +551,7 @@ impl LocalFingerprint {
             LocalFingerprint::CheckDepInfo { dep_info } => {
                 let dep_info = target_root.join(dep_info);
                 if let Some(paths) = parse_dep_info(pkg_root, target_root, &dep_info)? {
-                    Ok(find_stale_file(&dep_info, paths.iter()))
+                    Ok(find_stale_file(mtime_cache, &dep_info, paths.iter()))
                 } else {
                     Ok(Some(StaleFile::Missing(dep_info)))
                 }
@@ -559,6 +560,7 @@ impl LocalFingerprint {
             // We need to verify that no paths listed in `paths` are newer than
             // the `output` path itself, or the last time the build script ran.
             LocalFingerprint::RerunIfChanged { output, paths } => Ok(find_stale_file(
+                mtime_cache,
                 &target_root.join(output),
                 paths.iter().map(|p| pkg_root.join(p)),
             )),
@@ -756,7 +758,12 @@ impl Fingerprint {
     /// dependencies up to this unit as well. This function assumes that the
     /// unit starts out as `FsStatus::Stale` and then it will optionally switch
     /// it to `UpToDate` if it can.
-    fn check_filesystem(&mut self, pkg_root: &Path, target_root: &Path) -> CargoResult<()> {
+    fn check_filesystem(
+        &mut self,
+        mtime_cache: &mut HashMap<PathBuf, FileTime>,
+        pkg_root: &Path,
+        target_root: &Path,
+    ) -> CargoResult<()> {
         assert!(!self.fs_status.up_to_date());
 
         let mut mtimes = HashMap::new();
@@ -840,7 +847,7 @@ 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(file) = local.find_stale_file(pkg_root, target_root)? {
+            if let Some(file) = local.find_stale_file(mtime_cache, pkg_root, target_root)? {
                 file.log();
                 return Ok(());
             }
@@ -1014,8 +1021,8 @@ fn calculate<'a, 'cfg>(
 
     // After we built the initial `Fingerprint` be sure to update the
     // `fs_status` field of it.
-    let target_root = target_root(cx, unit);
-    fingerprint.check_filesystem(unit.pkg.root(), &target_root)?;
+    let target_root = target_root(cx);
+    fingerprint.check_filesystem(&mut cx.mtime_cache, unit.pkg.root(), &target_root)?;
 
     let fingerprint = Arc::new(fingerprint);
     cx.fingerprints.insert(*unit, Arc::clone(&fingerprint));
@@ -1046,7 +1053,7 @@ fn calculate_normal<'a, 'cfg>(
     // correctly, but otherwise upstream packages like from crates.io or git
     // get bland fingerprints because they don't change without their
     // `PackageId` changing.
-    let target_root = target_root(cx, unit);
+    let target_root = target_root(cx);
     let local = if use_dep_info(unit) {
         let dep_info = dep_info_loc(cx, unit);
         let dep_info = dep_info.strip_prefix(&target_root).unwrap().to_path_buf();
@@ -1098,13 +1105,10 @@ fn calculate_normal<'a, 'cfg>(
     })
 }
 
-// We want to use the mtime for files if we're a path source, but if we're a
-// git/registry source, then the mtime of files may fluctuate, but they won't
-// change so long as the source itself remains constant (which is the
-// responsibility of the source)
+/// Whether or not the fingerprint should track the dependencies from the
+/// dep-info file for this unit.
 fn use_dep_info(unit: &Unit<'_>) -> bool {
-    let path = unit.pkg.summary().source_id().is_path();
-    !unit.mode.is_doc() && path
+    !unit.mode.is_doc()
 }
 
 /// Calculate a fingerprint for an "execute a build script" unit.  This is an
@@ -1219,8 +1223,8 @@ fn build_script_local_fingerprints<'a, 'cfg>(
     // package. Remember that the fact that this is an `Option` is a bug, but a
     // longstanding bug, in Cargo. Recent refactorings just made it painfully
     // obvious.
-    let script_root = cx.files().build_script_run_dir(unit);
     let pkg_root = unit.pkg.root().to_path_buf();
+    let target_dir = target_root(cx);
     let calculate =
         move |deps: &BuildDeps, pkg_fingerprint: Option<&dyn Fn() -> CargoResult<String>>| {
             if deps.rerun_if_changed.is_empty() && deps.rerun_if_env_changed.is_empty() {
@@ -1247,7 +1251,7 @@ fn build_script_local_fingerprints<'a, 'cfg>(
             // Ok so now we're in "new mode" where we can have files listed as
             // dependencies as well as env vars listed as dependencies. Process
             // them all here.
-            Ok(Some(local_fingerprints_deps(deps, &script_root, &pkg_root)))
+            Ok(Some(local_fingerprints_deps(deps, &target_dir, &pkg_root)))
         };
 
     // Note that `false` == "not overridden"
@@ -1346,17 +1350,10 @@ pub fn dep_info_loc<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> Pa
         .join(&format!("dep-{}", filename(cx, unit)))
 }
 
-/// Returns an absolute path that the `unit`'s outputs should always be relative
-/// to. This `target_root` variable is used to store relative path names in
-/// `Fingerprint` instead of absolute pathnames (see module comment).
-fn target_root<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> PathBuf {
-    if unit.mode.is_run_custom_build() {
-        cx.files().build_script_run_dir(unit)
-    } else if unit.kind == Kind::Host {
-        cx.files().host_root().to_path_buf()
-    } else {
-        cx.files().target_root().to_path_buf()
-    }
+/// Returns an absolute path that target directory.
+/// All paths are rewritten to be relative to this.
+fn target_root(cx: &Context<'_, '_>) -> PathBuf {
+    cx.bcx.ws.target_dir().into_path_unlocked()
 }
 
 fn compare_old_fingerprint(
@@ -1429,11 +1426,7 @@ pub fn parse_dep_info(
             }
         })
         .collect::<Result<Vec<_>, _>>()?;
-    if paths.is_empty() {
-        Ok(None)
-    } else {
-        Ok(Some(paths))
-    }
+    Ok(Some(paths))
 }
 
 fn pkg_fingerprint(bcx: &BuildContext<'_, '_>, pkg: &Package) -> CargoResult<String> {
@@ -1446,7 +1439,11 @@ fn pkg_fingerprint(bcx: &BuildContext<'_, '_>, pkg: &Package) -> CargoResult<Str
     source.fingerprint(pkg)
 }
 
-fn find_stale_file<I>(reference: &Path, paths: I) -> Option<StaleFile>
+fn find_stale_file<I>(
+    mtime_cache: &mut HashMap<PathBuf, FileTime>,
+    reference: &Path,
+    paths: I,
+) -> Option<StaleFile>
 where
     I: IntoIterator,
     I::Item: AsRef<Path>,
@@ -1458,9 +1455,15 @@ where
 
     for path in paths {
         let path = path.as_ref();
-        let path_mtime = match paths::mtime(path) {
-            Ok(mtime) => mtime,
-            Err(..) => return Some(StaleFile::Missing(path.to_path_buf())),
+        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) {
+                    Ok(mtime) => mtime,
+                    Err(..) => return Some(StaleFile::Missing(path.to_path_buf())),
+                };
+                *v.insert(mtime)
+            }
         };
 
         // TODO: fix #5918.
@@ -1547,6 +1550,12 @@ impl DepInfoPathType {
 /// The `rustc_cwd` argument is the absolute path to the cwd of the compiler
 /// when it was invoked.
 ///
+/// If the `allow_package` argument is true, then package-relative paths are
+/// included. If it is false, then package-relative paths are skipped and
+/// ignored (typically used for registry or git dependencies where we assume
+/// the source never changes, and we don't want the cost of running `stat` on
+/// all those files).
+///
 /// 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.
 pub fn translate_dep_info(
@@ -1555,26 +1564,35 @@ pub fn translate_dep_info(
     rustc_cwd: &Path,
     pkg_root: &Path,
     target_root: &Path,
+    allow_package: bool,
 ) -> CargoResult<()> {
     let target = parse_rustc_dep_info(rustc_dep_info)?;
     let deps = &target
         .get(0)
         .ok_or_else(|| internal("malformed dep-info format, no targets".to_string()))?
         .1;
 
+    let target_root = target_root.canonicalize()?;
+    let pkg_root = pkg_root.canonicalize()?;
     let mut new_contents = Vec::new();
     for file in deps {
-        let file = rustc_cwd.join(file);
-        let (ty, path) = if let Ok(stripped) = file.strip_prefix(pkg_root) {
-            (DepInfoPathType::PackageRootRelative, stripped)
-        } else if let Ok(stripped) = file.strip_prefix(target_root) {
+        // The path may be absolute or relative, canonical or not. Make sure
+        // it is canonicalized so we are comparing the same kinds of paths.
+        let canon_file = rustc_cwd.join(file).canonicalize()?;
+        let abs_file = rustc_cwd.join(file);
+
+        let (ty, path) = if let Ok(stripped) = canon_file.strip_prefix(&target_root) {
             (DepInfoPathType::TargetRootRelative, stripped)
+        } else if let Ok(stripped) = canon_file.strip_prefix(&pkg_root) {
+            if !allow_package {
+                continue;
+            }
+            (DepInfoPathType::PackageRootRelative, stripped)
         } else {
             // It's definitely not target root relative, but this is an absolute path (since it was
             // joined to rustc_cwd) and as such re-joining it later to the target root will have no
             // effect.
-            assert!(file.is_absolute(), "{:?} is absolute", file);
-            (DepInfoPathType::TargetRootRelative, &*file)
+            (DepInfoPathType::TargetRootRelative, &*abs_file)
         };
         new_contents.push(ty as u8);
         new_contents.extend(util::path2bytes(path)?);

src/cargo/core/compiler/mod.rs

@@ -214,6 +214,9 @@ fn rustc<'a, 'cfg>(
     let dep_info_loc = fingerprint::dep_info_loc(cx, unit);
 
     rustc.args(cx.bcx.rustflags_args(unit));
+    if cx.bcx.config.cli_unstable().binary_dep_depinfo {
+        rustc.arg("-Zbinary-dep-depinfo");
+    }
     let mut output_options = OutputOptions::new(cx, unit);
     let package_id = unit.pkg.package_id();
     let target = unit.target.clone();
@@ -223,6 +226,7 @@ fn rustc<'a, 'cfg>(
     let exec = exec.clone();
 
     let root_output = cx.files().host_root().to_path_buf();
+    let target_dir = cx.bcx.ws.target_dir().into_path_unlocked();
     let pkg_root = unit.pkg.root().to_path_buf();
     let cwd = rustc
         .get_cwd()
@@ -317,7 +321,9 @@ fn rustc<'a, 'cfg>(
                 &dep_info_loc,
                 &cwd,
                 &pkg_root,
-                &root_output,
+                &target_dir,
+                // Do not track source files in the fingerprint for registry dependencies.
+                current_id.source_id().is_path(),
             )
             .chain_err(|| {
                 internal(format!(

src/cargo/core/features.rs

@@ -333,6 +333,7 @@ pub struct CliUnstable {
     pub mtime_on_use: bool,
     pub install_upgrade: bool,
     pub cache_messages: bool,
+    pub binary_dep_depinfo: bool,
 }
 
 impl CliUnstable {
@@ -378,6 +379,7 @@ impl CliUnstable {
             "mtime-on-use" => self.mtime_on_use = true,
             "install-upgrade" => self.install_upgrade = true,
             "cache-messages" => self.cache_messages = true,
+            "binary-dep-depinfo" => self.binary_dep_depinfo = true,
             _ => failure::bail!("unknown `-Z` flag specified: {}", k),
         }
 

src/cargo/util/mod.rs

@@ -82,3 +82,8 @@ pub fn validate_package_name(name: &str, what: &str, help: &str) -> CargoResult<
     }
     Ok(())
 }
+
+/// Whether or not this running in a Continuous Integration environment.
+pub fn is_ci() -> bool {
+    std::env::var("CI").is_ok() || std::env::var("TF_BUILD").is_ok()
+}

src/cargo/util/progress.rs

@@ -3,7 +3,7 @@ use std::env;
 use std::time::{Duration, Instant};
 
 use crate::core::shell::Verbosity;
-use crate::util::{CargoResult, Config};
+use crate::util::{is_ci, CargoResult, Config};
 
 use unicode_width::UnicodeWidthChar;
 
@@ -45,7 +45,7 @@ impl<'cfg> Progress<'cfg> {
             Ok(term) => term == "dumb",
             Err(_) => false,
         };
-        if cfg.shell().verbosity() == Verbosity::Quiet || dumb || env::var("CI").is_ok() {
+        if cfg.shell().verbosity() == Verbosity::Quiet || dumb || is_ci() {
             return Progress { state: None };
         }