From aa99e9f2517d1c2bff47ea46defa89bf4895f2a3 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 14 Jul 2019 16:32:32 -0700 Subject: [PATCH 1/7] Fix some issues with absolute paths in dep-info files. --- src/cargo/core/compiler/fingerprint.rs | 31 +-- src/cargo/core/compiler/mod.rs | 3 +- tests/testsuite/dep_info.rs | 316 ++++++++++++++++++++++++- tests/testsuite/support/mod.rs | 45 +--- tests/testsuite/support/paths.rs | 51 ++++ 5 files changed, 379 insertions(+), 67 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index d5f2ec320e7..ae3e8e4ec52 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -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. @@ -1014,7 +1014,7 @@ 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); + let target_root = target_root(cx); fingerprint.check_filesystem(unit.pkg.root(), &target_root)?; let fingerprint = Arc::new(fingerprint); @@ -1046,7 +1046,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(); @@ -1219,8 +1219,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>| { if deps.rerun_if_changed.is_empty() && deps.rerun_if_env_changed.is_empty() { @@ -1247,7 +1247,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 +1346,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( @@ -1565,10 +1558,10 @@ pub fn translate_dep_info( 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) { + let (ty, path) = if let Ok(stripped) = file.strip_prefix(target_root) { (DepInfoPathType::TargetRootRelative, stripped) + } else if let Ok(stripped) = file.strip_prefix(pkg_root) { + (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 diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index eb55e0659af..5fc26975c00 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -223,6 +223,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 +318,7 @@ fn rustc<'a, 'cfg>( &dep_info_loc, &cwd, &pkg_root, - &root_output, + &target_dir, ) .chain_err(|| { internal(format!( diff --git a/tests/testsuite/dep_info.rs b/tests/testsuite/dep_info.rs index 37ac5e926d0..d0fb323f5b0 100644 --- a/tests/testsuite/dep_info.rs +++ b/tests/testsuite/dep_info.rs @@ -1,5 +1,53 @@ -use crate::support::{basic_bin_manifest, main_file, project}; +use crate::support::registry::Package; +use crate::support::{ + basic_bin_manifest, basic_manifest, main_file, paths, project, rustc_host, Project, +}; use filetime::FileTime; +use std::fs; +use std::path::Path; + +// Helper for testing dep-info files in the fingerprint dir. +fn assert_deps(project: &Project, fingerprint: &str, test_cb: impl Fn(&Path, &[(u8, &str)])) { + let mut files = project + .glob(fingerprint) + .map(|f| f.expect("unwrap glob result")) + // Filter out `.json` entries. + .filter(|f| f.extension().is_none()); + let info_path = files + .next() + .unwrap_or_else(|| panic!("expected 1 dep-info file at {}, found 0", fingerprint)); + assert!(files.next().is_none(), "expected only 1 dep-info file"); + let dep_info = fs::read(&info_path).unwrap(); + let deps: Vec<(u8, &str)> = dep_info + .split(|&x| x == 0) + .filter(|x| !x.is_empty()) + .map(|p| { + ( + p[0], + std::str::from_utf8(&p[1..]).expect("expected valid path"), + ) + }) + .collect(); + test_cb(&info_path, &deps); +} + +fn assert_deps_contains(project: &Project, fingerprint: &str, expected: &[(u8, &str)]) { + assert_deps(project, fingerprint, |info_path, entries| { + for (e_kind, e_path) in expected { + let pattern = glob::Pattern::new(e_path).unwrap(); + let count = entries + .iter() + .filter(|(kind, path)| kind == e_kind && pattern.matches(path)) + .count(); + if count != 1 { + panic!( + "Expected 1 match of {} {} in {:?}, got {}:\n{:#?}", + e_kind, e_path, info_path, count, entries + ); + } + } + }) +} #[cargo_test] fn build_dep_info() { @@ -18,8 +66,15 @@ fn build_dep_info() { let bin_path = p.bin("foo"); let src_path = p.root().join("src").join("foo.rs"); - let expected_depinfo = format!("{}: {}\n", bin_path.display(), src_path.display()); - assert_eq!(depinfo, expected_depinfo); + if !depinfo.lines().any(|line| { + line.starts_with(&format!("{}:", bin_path.display())) + && line.contains(src_path.to_str().unwrap()) + }) { + panic!( + "Could not find {:?}: {:?} in {:?}", + bin_path, src_path, depinfo_bin_path + ); + } } #[cargo_test] @@ -110,3 +165,258 @@ fn no_rewrite_if_no_change() { FileTime::from_last_modification_time(&metadata2), ); } + +#[cargo_test] +// Remove once https://github.com/rust-lang/rust/pull/61727 lands, and switch +// to a `nightly` check. +#[ignore] +fn relative_depinfo_paths_ws() { + // Test relative dep-info paths in a workspace with --target with + // proc-macros and other dependency kinds. + Package::new("regdep", "0.1.0") + .file("src/lib.rs", "pub fn f() {}") + .publish(); + Package::new("pmdep", "0.1.0") + .file("src/lib.rs", "pub fn f() {}") + .publish(); + Package::new("bdep", "0.1.0") + .file("src/lib.rs", "pub fn f() {}") + .publish(); + + let p = project() + /*********** Workspace ***********/ + .file( + "Cargo.toml", + r#" + [workspace] + members = ["foo"] + "#, + ) + /*********** Main Project ***********/ + .file( + "foo/Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2018" + + [dependencies] + pm = {path = "../pm"} + bar = {path = "../bar"} + regdep = "0.1" + + [build-dependencies] + bdep = "0.1" + bar = {path = "../bar"} + "#, + ) + .file( + "foo/src/main.rs", + r#" + pm::noop!{} + + fn main() { + bar::f(); + regdep::f(); + } + "#, + ) + .file("foo/build.rs", "fn main() { bdep::f(); }") + /*********** Proc Macro ***********/ + .file( + "pm/Cargo.toml", + r#" + [package] + name = "pm" + version = "0.1.0" + edition = "2018" + + [lib] + proc-macro = true + + [dependencies] + pmdep = "0.1" + "#, + ) + .file( + "pm/src/lib.rs", + r#" + extern crate proc_macro; + use proc_macro::TokenStream; + + #[proc_macro] + pub fn noop(_item: TokenStream) -> TokenStream { + pmdep::f(); + "".parse().unwrap() + } + "#, + ) + /*********** Path Dependency `bar` ***********/ + .file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0")) + .file("bar/src/lib.rs", "pub fn f() {}") + .build(); + + let host = rustc_host(); + p.cargo("build --target") + .arg(&host) + .with_stderr_contains("[COMPILING] foo [..]") + .run(); + + assert_deps_contains( + &p, + "target/debug/.fingerprint/pm-*/dep-lib-pm-*", + &[(1, "src/lib.rs"), (2, "debug/deps/libpmdep-*.rlib")], + ); + + assert_deps_contains( + &p, + &format!("target/{}/debug/.fingerprint/foo-*/dep-bin-foo-*", host), + &[ + (1, "src/main.rs"), + ( + 2, + &format!( + "debug/deps/{}pm-*.{}", + paths::get_lib_prefix("proc-macro"), + paths::get_lib_extension("proc-macro") + ), + ), + (2, &format!("{}/debug/deps/libbar-*.rlib", host)), + (2, &format!("{}/debug/deps/libregdep-*.rlib", host)), + ], + ); + + assert_deps_contains( + &p, + "target/debug/.fingerprint/foo-*/dep-build-script-build_script_build-*", + &[(1, "build.rs"), (2, "debug/deps/libbdep-*.rlib")], + ); + + // Make sure it stays fresh. + p.cargo("build --target") + .arg(&host) + .with_stderr("[FINISHED] dev [..]") + .run(); +} + +#[cargo_test] +// Remove once https://github.com/rust-lang/rust/pull/61727 lands, and switch +// to a `nightly` check. +#[ignore] +fn relative_depinfo_paths_no_ws() { + // Test relative dep-info paths without a workspace with proc-macros and + // other dependency kinds. + Package::new("regdep", "0.1.0") + .file("src/lib.rs", "pub fn f() {}") + .publish(); + Package::new("pmdep", "0.1.0") + .file("src/lib.rs", "pub fn f() {}") + .publish(); + Package::new("bdep", "0.1.0") + .file("src/lib.rs", "pub fn f() {}") + .publish(); + + let p = project() + /*********** Main Project ***********/ + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2018" + + [dependencies] + pm = {path = "pm"} + bar = {path = "bar"} + regdep = "0.1" + + [build-dependencies] + bdep = "0.1" + bar = {path = "bar"} + "#, + ) + .file( + "src/main.rs", + r#" + pm::noop!{} + + fn main() { + bar::f(); + regdep::f(); + } + "#, + ) + .file("build.rs", "fn main() { bdep::f(); }") + /*********** Proc Macro ***********/ + .file( + "pm/Cargo.toml", + r#" + [package] + name = "pm" + version = "0.1.0" + edition = "2018" + + [lib] + proc-macro = true + + [dependencies] + pmdep = "0.1" + "#, + ) + .file( + "pm/src/lib.rs", + r#" + extern crate proc_macro; + use proc_macro::TokenStream; + + #[proc_macro] + pub fn noop(_item: TokenStream) -> TokenStream { + pmdep::f(); + "".parse().unwrap() + } + "#, + ) + /*********** Path Dependency `bar` ***********/ + .file("bar/Cargo.toml", &basic_manifest("bar", "0.1.0")) + .file("bar/src/lib.rs", "pub fn f() {}") + .build(); + + p.cargo("build") + .with_stderr_contains("[COMPILING] foo [..]") + .run(); + + assert_deps_contains( + &p, + "target/debug/.fingerprint/pm-*/dep-lib-pm-*", + &[(1, "src/lib.rs"), (2, "debug/deps/libpmdep-*.rlib")], + ); + + assert_deps_contains( + &p, + "target/debug/.fingerprint/foo-*/dep-bin-foo-*", + &[ + (1, "src/main.rs"), + ( + 2, + &format!( + "debug/deps/{}pm-*.{}", + paths::get_lib_prefix("proc-macro"), + paths::get_lib_extension("proc-macro") + ), + ), + (2, "debug/deps/libbar-*.rlib"), + (2, "debug/deps/libregdep-*.rlib"), + ], + ); + + assert_deps_contains( + &p, + "target/debug/.fingerprint/foo-*/dep-build-script-build_script_build-*", + &[(1, "build.rs"), (2, "debug/deps/libbdep-*.rlib")], + ); + + // Make sure it stays fresh. + p.cargo("build").with_stderr("[FINISHED] dev [..]").run(); +} diff --git a/tests/testsuite/support/mod.rs b/tests/testsuite/support/mod.rs index 4e29d8d959e..75ddf5a4957 100644 --- a/tests/testsuite/support/mod.rs +++ b/tests/testsuite/support/mod.rs @@ -332,15 +332,9 @@ impl Project { /// `kind` should be one of: "lib", "rlib", "staticlib", "dylib", "proc-macro" /// ex: `/path/to/cargo/target/cit/t0/foo/target/debug/examples/libex.rlib` pub fn example_lib(&self, name: &str, kind: &str) -> PathBuf { - let prefix = Project::get_lib_prefix(kind); - - let extension = Project::get_lib_extension(kind); - - let lib_file_name = format!("{}{}.{}", prefix, name, extension); - self.target_debug_dir() .join("examples") - .join(&lib_file_name) + .join(paths::get_lib_filename(name, kind)) } /// Path to a debug binary. @@ -454,43 +448,6 @@ impl Project { .write_all(contents.replace("#", "").as_bytes()) .unwrap(); } - - fn get_lib_prefix(kind: &str) -> &str { - match kind { - "lib" | "rlib" => "lib", - "staticlib" | "dylib" | "proc-macro" => { - if cfg!(windows) { - "" - } else { - "lib" - } - } - _ => unreachable!(), - } - } - - fn get_lib_extension(kind: &str) -> &str { - match kind { - "lib" | "rlib" => "rlib", - "staticlib" => { - if cfg!(windows) { - "lib" - } else { - "a" - } - } - "dylib" | "proc-macro" => { - if cfg!(windows) { - "dll" - } else if cfg!(target_os = "macos") { - "dylib" - } else { - "so" - } - } - _ => unreachable!(), - } - } } // Generates a project layout diff --git a/tests/testsuite/support/paths.rs b/tests/testsuite/support/paths.rs index 59d12a91853..6d2e9fbc318 100644 --- a/tests/testsuite/support/paths.rs +++ b/tests/testsuite/support/paths.rs @@ -213,3 +213,54 @@ where } } } + +/// Get the filename for a library. +/// +/// `kind` should be one of: "lib", "rlib", "staticlib", "dylib", "proc-macro" +/// +/// For example, dynamic library named "foo" would return: +/// - macOS: "libfoo.dylib" +/// - Windows: "foo.dll" +/// - Unix: "libfoo.so" +pub fn get_lib_filename(name: &str, kind: &str) -> String { + let prefix = get_lib_prefix(kind); + let extension = get_lib_extension(kind); + format!("{}{}.{}", prefix, name, extension) +} + +pub fn get_lib_prefix(kind: &str) -> &str { + match kind { + "lib" | "rlib" => "lib", + "staticlib" | "dylib" | "proc-macro" => { + if cfg!(windows) { + "" + } else { + "lib" + } + } + _ => unreachable!(), + } +} + +pub fn get_lib_extension(kind: &str) -> &str { + match kind { + "lib" | "rlib" => "rlib", + "staticlib" => { + if cfg!(windows) { + "lib" + } else { + "a" + } + } + "dylib" | "proc-macro" => { + if cfg!(windows) { + "dll" + } else if cfg!(target_os = "macos") { + "dylib" + } else { + "so" + } + } + _ => unreachable!(), + } +} From b1b9b79c0260f19dda3c3c6523bafa8c19a6a5fc Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 14 Jul 2019 20:20:27 -0700 Subject: [PATCH 2/7] Track dep-info for all dependencies. This adds dep-info tracking for non-path dependencies. This avoids tracking package-relative paths (like source files) in non-path dependencies, since we assume they are static. It also adds an mtime cache to improve performance since when rustc starts tracking sysroot files (in the future), it can cause a large number of stat calls. --- src/cargo/core/compiler/context/mod.rs | 3 ++ src/cargo/core/compiler/fingerprint.rs | 57 +++++++++++++++++--------- src/cargo/core/compiler/mod.rs | 2 + tests/testsuite/dep_info.rs | 40 ++++++++++++++++++ 4 files changed, 82 insertions(+), 20 deletions(-) diff --git a/src/cargo/core/compiler/context/mod.rs b/src/cargo/core/compiler/context/mod.rs index 96a544759a0..9325c8c63e2 100644 --- a/src/cargo/core/compiler/context/mod.rs +++ b/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, BuildDeps>, pub fingerprints: HashMap, Arc>, + pub mtime_cache: HashMap, pub compiled: HashSet>, pub build_scripts: HashMap, Arc>, 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(), diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index ae3e8e4ec52..fdf9735f7af 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -187,7 +187,7 @@ //! See the `A-rebuild-detection` flag on the issue tracker for more: //! -use std::collections::HashMap; +use std::collections::hash_map::{Entry, HashMap}; use std::env; use std::fs; use std::hash::{self, Hasher}; @@ -539,6 +539,7 @@ impl LocalFingerprint { /// file accesses. fn find_stale_file( &self, + mtime_cache: &mut HashMap, pkg_root: &Path, target_root: &Path, ) -> CargoResult> { @@ -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, + 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(()); } @@ -1015,7 +1022,7 @@ 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); - fingerprint.check_filesystem(unit.pkg.root(), &target_root)?; + fingerprint.check_filesystem(&mut cx.mtime_cache, unit.pkg.root(), &target_root)?; let fingerprint = Arc::new(fingerprint); cx.fingerprints.insert(*unit, Arc::clone(&fingerprint)); @@ -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 @@ -1422,11 +1426,7 @@ pub fn parse_dep_info( } }) .collect::, _>>()?; - if paths.is_empty() { - Ok(None) - } else { - Ok(Some(paths)) - } + Ok(Some(paths)) } fn pkg_fingerprint(bcx: &BuildContext<'_, '_>, pkg: &Package) -> CargoResult { @@ -1439,7 +1439,11 @@ fn pkg_fingerprint(bcx: &BuildContext<'_, '_>, pkg: &Package) -> CargoResult(reference: &Path, paths: I) -> Option +fn find_stale_file( + mtime_cache: &mut HashMap, + reference: &Path, + paths: I, +) -> Option where I: IntoIterator, I::Item: AsRef, @@ -1451,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. @@ -1540,6 +1550,9 @@ 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 false, then package-relative paths are +/// skipped and ignored. +/// /// 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( @@ -1548,6 +1561,7 @@ 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 @@ -1561,6 +1575,9 @@ pub fn translate_dep_info( let (ty, path) = if let Ok(stripped) = file.strip_prefix(target_root) { (DepInfoPathType::TargetRootRelative, stripped) } else if let Ok(stripped) = 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 diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 5fc26975c00..d4004c55e82 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -319,6 +319,8 @@ fn rustc<'a, 'cfg>( &cwd, &pkg_root, &target_dir, + // Do not track source files in the fingerprint for registry dependencies. + current_id.source_id().is_path(), ) .chain_err(|| { internal(format!( diff --git a/tests/testsuite/dep_info.rs b/tests/testsuite/dep_info.rs index d0fb323f5b0..9933b3dda57 100644 --- a/tests/testsuite/dep_info.rs +++ b/tests/testsuite/dep_info.rs @@ -420,3 +420,43 @@ fn relative_depinfo_paths_no_ws() { // Make sure it stays fresh. p.cargo("build").with_stderr("[FINISHED] dev [..]").run(); } + +#[cargo_test] +fn reg_dep_source_not_tracked() { + // Make sure source files in dep-info file are not tracked for registry dependencies. + Package::new("regdep", "0.1.0") + .file("src/lib.rs", "pub fn f() {}") + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + regdep = "0.1" + "#, + ) + .file("src/lib.rs", "pub fn f() { regdep::f(); }") + .build(); + + p.cargo("build").run(); + + assert_deps( + &p, + "target/debug/.fingerprint/regdep-*/dep-lib-regdep-*", + |info_path, entries| { + for (kind, path) in entries { + if *kind == 1 { + panic!( + "Did not expect package root relative path type: {:?} in {:?}", + path, info_path + ); + } + } + }, + ); +} From 3595de3608d83bd64206c46bc7f4da2db38659c6 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 16 Jul 2019 10:24:18 -0700 Subject: [PATCH 3/7] Handle Windows extended-length path in dep-info. --- src/cargo/core/compiler/fingerprint.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index fdf9735f7af..4b384a6f904 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -1571,7 +1571,13 @@ pub fn translate_dep_info( let mut new_contents = Vec::new(); for file in deps { - let file = rustc_cwd.join(file); + let file = if cfg!(windows) && file.starts_with("\\\\?\\") { + // Remove Windows extended-length prefix, since functions like + // strip_prefix won't work if you mix with traditional dos paths. + PathBuf::from(&file[4..]) + } else { + rustc_cwd.join(file) + }; let (ty, path) = if let Ok(stripped) = file.strip_prefix(target_root) { (DepInfoPathType::TargetRootRelative, stripped) } else if let Ok(stripped) = file.strip_prefix(pkg_root) { From ff532eca950dc5880211883a83b5d8b760733442 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 16 Jul 2019 10:55:46 -0700 Subject: [PATCH 4/7] Clarify comment on `allow_package`. --- src/cargo/core/compiler/fingerprint.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index 4b384a6f904..5dab8663e82 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -1550,8 +1550,11 @@ 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 false, then package-relative paths are -/// skipped and ignored. +/// 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. From 4f6553ab553ca127ab653bec9f2f65e64735912f Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 24 Jul 2019 19:35:27 -0700 Subject: [PATCH 5/7] Use canonical paths when parsing dep-info. Instead of treating Windows differently, this just always uses canonical paths on all platforms. This fixes a problem where symlinks were not treated correctly on all platforms. Switching rm_rf to the remove_dir_all crate because deleting symbolic links on Windows is difficult. --- Cargo.toml | 1 + src/cargo/core/compiler/fingerprint.rs | 21 +++++------ tests/testsuite/dep_info.rs | 45 +++++++++++++++++++++-- tests/testsuite/support/mod.rs | 51 ++++++++++++++++++++++++++ tests/testsuite/support/paths.rs | 17 ++------- 5 files changed, 107 insertions(+), 28 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index f44432a7d2a..2fce01b9e04 100644 --- a/Cargo.toml +++ b/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"] } diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index 5dab8663e82..3ce3c04a0f4 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -1572,18 +1572,18 @@ pub fn translate_dep_info( .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 = if cfg!(windows) && file.starts_with("\\\\?\\") { - // Remove Windows extended-length prefix, since functions like - // strip_prefix won't work if you mix with traditional dos paths. - PathBuf::from(&file[4..]) - } else { - rustc_cwd.join(file) - }; - let (ty, path) = 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) = file.strip_prefix(pkg_root) { + } else if let Ok(stripped) = canon_file.strip_prefix(&pkg_root) { if !allow_package { continue; } @@ -1592,8 +1592,7 @@ pub fn translate_dep_info( // 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)?); diff --git a/tests/testsuite/dep_info.rs b/tests/testsuite/dep_info.rs index 9933b3dda57..c93aa6e488b 100644 --- a/tests/testsuite/dep_info.rs +++ b/tests/testsuite/dep_info.rs @@ -1,7 +1,6 @@ +use crate::support::paths::{self, CargoPathExt}; use crate::support::registry::Package; -use crate::support::{ - basic_bin_manifest, basic_manifest, main_file, paths, project, rustc_host, Project, -}; +use crate::support::{basic_bin_manifest, basic_manifest, main_file, project, rustc_host, Project}; use filetime::FileTime; use std::fs; use std::path::Path; @@ -460,3 +459,43 @@ fn reg_dep_source_not_tracked() { }, ); } + +#[cargo_test] +// Remove once https://github.com/rust-lang/rust/pull/61727 lands, and switch +// to a `nightly` check. +#[ignore] +fn canonical_path() { + if !crate::support::symlink_supported() { + return; + } + Package::new("regdep", "0.1.0") + .file("src/lib.rs", "pub fn f() {}") + .publish(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + regdep = "0.1" + "#, + ) + .file("src/lib.rs", "pub fn f() { regdep::f(); }") + .build(); + + let real = p.root().join("real_target"); + real.mkdir_p(); + p.symlink(real, "target"); + + p.cargo("build").run(); + + assert_deps_contains( + &p, + "target/debug/.fingerprint/foo-*/dep-lib-foo-*", + &[(1, "src/lib.rs"), (2, "debug/deps/libregdep-*.rlib")], + ); +} diff --git a/tests/testsuite/support/mod.rs b/tests/testsuite/support/mod.rs index 75ddf5a4957..05094af819f 100644 --- a/tests/testsuite/support/mod.rs +++ b/tests/testsuite/support/mod.rs @@ -448,6 +448,29 @@ impl Project { .write_all(contents.replace("#", "").as_bytes()) .unwrap(); } + + pub fn symlink(&self, src: impl AsRef, dst: impl AsRef) { + let src = self.root().join(src.as_ref()); + let dst = self.root().join(dst.as_ref()); + #[cfg(unix)] + { + if let Err(e) = os::unix::fs::symlink(&src, &dst) { + panic!("failed to symlink {:?} to {:?}: {:?}", src, dst, e); + } + } + #[cfg(windows)] + { + if src.is_dir() { + if let Err(e) = os::windows::fs::symlink_dir(&src, &dst) { + panic!("failed to symlink {:?} to {:?}: {:?}", src, dst, e); + } + } else { + if let Err(e) = os::windows::fs::symlink_file(&src, &dst) { + panic!("failed to symlink {:?} to {:?}: {:?}", src, dst, e); + } + } + } + } } // Generates a project layout @@ -1746,3 +1769,31 @@ pub fn clippy_is_available() -> bool { true } } + +#[cfg(windows)] +pub fn symlink_supported() -> bool { + let src = paths::root().join("symlink_src"); + fs::write(&src, "").unwrap(); + let dst = paths::root().join("symlink_dst"); + let result = match os::windows::fs::symlink_file(&src, &dst) { + Ok(_) => { + fs::remove_file(&dst).unwrap(); + true + } + Err(e) => { + eprintln!( + "symlinks not supported: {:?}\n\ + Windows 10 users should enable developer mode.", + e + ); + false + } + }; + fs::remove_file(&src).unwrap(); + return result; +} + +#[cfg(not(windows))] +pub fn symlink_supported() -> bool { + true +} diff --git a/tests/testsuite/support/paths.rs b/tests/testsuite/support/paths.rs index 6d2e9fbc318..7dfb65c69a3 100644 --- a/tests/testsuite/support/paths.rs +++ b/tests/testsuite/support/paths.rs @@ -113,22 +113,11 @@ impl CargoPathExt for Path { * care all that much for our tests */ fn rm_rf(&self) { - if !self.exists() { - return; - } - - for file in t!(fs::read_dir(self)) { - let file = t!(file); - if file.file_type().map(|m| m.is_dir()).unwrap_or(false) { - file.path().rm_rf(); - } else { - // On windows we can't remove a readonly file, and git will - // often clone files as readonly. As a result, we have some - // special logic to remove readonly files on windows. - do_op(&file.path(), "remove file", |p| fs::remove_file(p)); + if self.exists() { + if let Err(e) = remove_dir_all::remove_dir_all(self) { + panic!("failed to remove {:?}: {:?}", self, e) } } - do_op(self, "remove dir", |p| fs::remove_dir(p)); } fn mkdir_p(&self) { From 51a8206c38601402fe739f1553f40af7f038001d Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 24 Jul 2019 21:48:15 -0700 Subject: [PATCH 6/7] Be more consistent about detecting CI. --- crates/resolver-tests/tests/resolve.rs | 4 ++-- src/cargo/util/mod.rs | 5 +++++ src/cargo/util/progress.rs | 4 ++-- tests/testsuite/support/mod.rs | 10 +++++++--- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/crates/resolver-tests/tests/resolve.rs b/crates/resolver-tests/tests/resolve.rs index 1fd59d3c149..0c6512186d7 100644 --- a/crates/resolver-tests/tests/resolve.rs +++ b/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 { diff --git a/src/cargo/util/mod.rs b/src/cargo/util/mod.rs index a8806f577e5..50fbd8c2b1b 100644 --- a/src/cargo/util/mod.rs +++ b/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() +} diff --git a/src/cargo/util/progress.rs b/src/cargo/util/progress.rs index 69b9aad0fb5..42a6f162f6b 100644 --- a/src/cargo/util/progress.rs +++ b/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 }; } diff --git a/tests/testsuite/support/mod.rs b/tests/testsuite/support/mod.rs index 05094af819f..01abfdba550 100644 --- a/tests/testsuite/support/mod.rs +++ b/tests/testsuite/support/mod.rs @@ -118,7 +118,7 @@ use std::time::{self, Duration}; use std::usize; use cargo; -use cargo::util::{CargoResult, ProcessBuilder, ProcessError, Rustc}; +use cargo::util::{is_ci, CargoResult, ProcessBuilder, ProcessError, Rustc}; use filetime; use serde_json::{self, Value}; use url::Url; @@ -855,7 +855,7 @@ impl Execs { fn match_process(&self, process: &ProcessBuilder) -> MatchResult { println!("running {}", process); let res = if self.stream_output { - if env::var("CI").is_ok() { + if is_ci() { panic!("`.stream()` is for local debugging") } process.exec_with_streaming( @@ -1746,7 +1746,7 @@ pub fn is_coarse_mtime() -> bool { // This should actually be a test that `$CARGO_TARGET_DIR` is on an HFS // filesystem, (or any filesystem with low-resolution mtimes). However, // that's tricky to detect, so for now just deal with CI. - cfg!(target_os = "macos") && (env::var("CI").is_ok() || env::var("TF_BUILD").is_ok()) + cfg!(target_os = "macos") && is_ci() } /// Some CI setups are much slower then the equipment used by Cargo itself. @@ -1772,6 +1772,10 @@ pub fn clippy_is_available() -> bool { #[cfg(windows)] pub fn symlink_supported() -> bool { + if is_ci() { + // We want to be absolutely sure this runs on CI. + return true; + } let src = paths::root().join("symlink_src"); fs::write(&src, "").unwrap(); let dst = paths::root().join("symlink_dst"); From c6e626b33916327098224a4f51793ed2ef67e3b3 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 26 Jul 2019 09:06:58 -0700 Subject: [PATCH 7/7] Add -Z binary-dep-depinfo --- src/cargo/core/compiler/mod.rs | 3 +++ src/cargo/core/features.rs | 2 ++ tests/testsuite/dep_info.rs | 45 ++++++++++++++++++++++------------ 3 files changed, 35 insertions(+), 15 deletions(-) diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index d4004c55e82..4962e774506 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/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(); diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index b7f9d39d45b..9fd8161ce35 100644 --- a/src/cargo/core/features.rs +++ b/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), } diff --git a/tests/testsuite/dep_info.rs b/tests/testsuite/dep_info.rs index c93aa6e488b..dce5c4025d1 100644 --- a/tests/testsuite/dep_info.rs +++ b/tests/testsuite/dep_info.rs @@ -1,6 +1,8 @@ use crate::support::paths::{self, CargoPathExt}; use crate::support::registry::Package; -use crate::support::{basic_bin_manifest, basic_manifest, main_file, project, rustc_host, Project}; +use crate::support::{ + basic_bin_manifest, basic_manifest, is_nightly, main_file, project, rustc_host, Project, +}; use filetime::FileTime; use std::fs; use std::path::Path; @@ -166,10 +168,12 @@ fn no_rewrite_if_no_change() { } #[cargo_test] -// Remove once https://github.com/rust-lang/rust/pull/61727 lands, and switch -// to a `nightly` check. -#[ignore] fn relative_depinfo_paths_ws() { + if !is_nightly() { + // See https://github.com/rust-lang/rust/issues/63012 + return; + } + // Test relative dep-info paths in a workspace with --target with // proc-macros and other dependency kinds. Package::new("regdep", "0.1.0") @@ -257,8 +261,9 @@ fn relative_depinfo_paths_ws() { .build(); let host = rustc_host(); - p.cargo("build --target") + p.cargo("build -Z binary-dep-depinfo --target") .arg(&host) + .masquerade_as_nightly_cargo() .with_stderr_contains("[COMPILING] foo [..]") .run(); @@ -293,17 +298,20 @@ fn relative_depinfo_paths_ws() { ); // Make sure it stays fresh. - p.cargo("build --target") + p.cargo("build -Z binary-dep-depinfo --target") .arg(&host) + .masquerade_as_nightly_cargo() .with_stderr("[FINISHED] dev [..]") .run(); } #[cargo_test] -// Remove once https://github.com/rust-lang/rust/pull/61727 lands, and switch -// to a `nightly` check. -#[ignore] fn relative_depinfo_paths_no_ws() { + if !is_nightly() { + // See https://github.com/rust-lang/rust/issues/63012 + return; + } + // Test relative dep-info paths without a workspace with proc-macros and // other dependency kinds. Package::new("regdep", "0.1.0") @@ -382,7 +390,8 @@ fn relative_depinfo_paths_no_ws() { .file("bar/src/lib.rs", "pub fn f() {}") .build(); - p.cargo("build") + p.cargo("build -Z binary-dep-depinfo") + .masquerade_as_nightly_cargo() .with_stderr_contains("[COMPILING] foo [..]") .run(); @@ -417,7 +426,10 @@ fn relative_depinfo_paths_no_ws() { ); // Make sure it stays fresh. - p.cargo("build").with_stderr("[FINISHED] dev [..]").run(); + p.cargo("build -Z binary-dep-depinfo") + .masquerade_as_nightly_cargo() + .with_stderr("[FINISHED] dev [..]") + .run(); } #[cargo_test] @@ -461,10 +473,11 @@ fn reg_dep_source_not_tracked() { } #[cargo_test] -// Remove once https://github.com/rust-lang/rust/pull/61727 lands, and switch -// to a `nightly` check. -#[ignore] fn canonical_path() { + if !is_nightly() { + // See https://github.com/rust-lang/rust/issues/63012 + return; + } if !crate::support::symlink_supported() { return; } @@ -491,7 +504,9 @@ fn canonical_path() { real.mkdir_p(); p.symlink(real, "target"); - p.cargo("build").run(); + p.cargo("build -Z binary-dep-depinfo") + .masquerade_as_nightly_cargo() + .run(); assert_deps_contains( &p,