From f666b19a8fed15be72f55f8ec5685a863356989a Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 26 Jun 2020 12:30:42 -0700 Subject: [PATCH 1/6] Parse `# env-dep` directives in dep-info files This commit updates Cargo's parsing of rustc's dep-info files to account for changes made upstream in rust-lang/rust#71858. This means that if `env!` or `option_env!` is used in crate files Cargo will correctly rebuild the crate if the env var changes. Closes #8417 --- src/cargo/core/compiler/fingerprint.rs | 276 ++++++++++++++++------ src/cargo/core/compiler/output_depinfo.rs | 4 +- tests/testsuite/dep_info.rs | 62 +++-- tests/testsuite/freshness.rs | 93 ++++++++ 4 files changed, 339 insertions(+), 96 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index 4adab7efbcf..b0add13d94d 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -312,6 +312,7 @@ use std::collections::hash_map::{Entry, HashMap}; use std::env; use std::hash::{self, Hasher}; use std::path::{Path, PathBuf}; +use std::str; use std::sync::{Arc, Mutex}; use std::time::SystemTime; @@ -674,14 +675,19 @@ enum LocalFingerprint { RerunIfEnvChanged { var: String, val: Option }, } -enum StaleFile { - Missing(PathBuf), - Changed { +enum StaleItem { + MissingFile(PathBuf), + ChangedFile { reference: PathBuf, reference_mtime: FileTime, stale: PathBuf, stale_mtime: FileTime, }, + ChangedEnv { + var: String, + previous: Option, + current: Option, + }, } impl LocalFingerprint { @@ -690,12 +696,12 @@ impl LocalFingerprint { /// /// This will use the absolute root paths passed in if necessary to guide /// file accesses. - fn find_stale_file( + fn find_stale_item( &self, mtime_cache: &mut HashMap, pkg_root: &Path, target_root: &Path, - ) -> CargoResult> { + ) -> CargoResult> { match self { // We need to parse `dep_info`, learn about all the files the crate // depends on, and then see if any of them are newer than the @@ -703,11 +709,22 @@ impl LocalFingerprint { // unit has never been compiled! 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(mtime_cache, &dep_info, paths.iter())) - } else { - Ok(Some(StaleFile::Missing(dep_info))) + let info = match parse_dep_info(pkg_root, target_root, &dep_info)? { + Some(info) => info, + None => return Ok(Some(StaleItem::MissingFile(dep_info))), + }; + for (key, previous) in info.env.iter() { + let current = env::var(key).ok(); + if current == *previous { + continue; + } + return Ok(Some(StaleItem::ChangedEnv { + var: key.clone(), + previous: previous.clone(), + current, + })); } + Ok(find_stale_file(mtime_cache, &dep_info, info.files.iter())) } // We need to verify that no paths listed in `paths` are newer than @@ -1025,8 +1042,8 @@ 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(mtime_cache, pkg_root, target_root)? { - file.log(); + if let Some(item) = local.find_stale_item(mtime_cache, pkg_root, target_root)? { + item.log(); return Ok(()); } } @@ -1138,7 +1155,7 @@ impl DepFingerprint { } } -impl StaleFile { +impl StaleItem { /// Use the `log` crate to log a hopefully helpful message in diagnosing /// what file is considered stale and why. This is intended to be used in /// conjunction with `CARGO_LOG` to determine why Cargo is recompiling @@ -1146,10 +1163,10 @@ impl StaleFile { /// that. fn log(&self) { match self { - StaleFile::Missing(path) => { + StaleItem::MissingFile(path) => { info!("stale: missing {:?}", path); } - StaleFile::Changed { + StaleItem::ChangedFile { reference, reference_mtime, stale, @@ -1159,6 +1176,14 @@ impl StaleFile { info!(" (vs) {:?}", reference); info!(" {:?} != {:?}", reference_mtime, stale_mtime); } + StaleItem::ChangedEnv { + var, + previous, + current, + } => { + info!("stale: changed env {:?}", var); + info!(" {:?} != {:?}", previous, current); + } } } } @@ -1600,28 +1625,26 @@ pub fn parse_dep_info( pkg_root: &Path, target_root: &Path, dep_info: &Path, -) -> CargoResult>> { +) -> CargoResult> { let data = match paths::read_bytes(dep_info) { Ok(data) => data, Err(_) => return Ok(None), }; - let paths = data - .split(|&x| x == 0) - .filter(|x| !x.is_empty()) - .map(|p| { - let ty = match DepInfoPathType::from_byte(p[0]) { - Some(ty) => ty, - None => return Err(internal("dep-info invalid")), - }; - let path = util::bytes2path(&p[1..])?; - match ty { - DepInfoPathType::PackageRootRelative => Ok(pkg_root.join(path)), - // N.B. path might be absolute here in which case the join will have no effect - DepInfoPathType::TargetRootRelative => Ok(target_root.join(path)), - } - }) - .collect::, _>>()?; - Ok(Some(paths)) + let info = match OnDiskDepInfo::parse(&data) { + Some(info) => info, + None => return Ok(None), + }; + let mut ret = RustcDepInfo::default(); + ret.env = info.env; + for (ty, path) in info.files { + let path = match ty { + DepInfoPathType::PackageRootRelative => pkg_root.join(path), + // N.B. path might be absolute here in which case the join will have no effect + DepInfoPathType::TargetRootRelative => target_root.join(path), + }; + ret.files.push(path); + } + Ok(Some(ret)) } fn pkg_fingerprint(bcx: &BuildContext<'_, '_>, pkg: &Package) -> CargoResult { @@ -1638,14 +1661,14 @@ fn find_stale_file( mtime_cache: &mut HashMap, reference: &Path, paths: I, -) -> Option +) -> Option where I: IntoIterator, I::Item: AsRef, { let reference_mtime = match paths::mtime(reference) { Ok(mtime) => mtime, - Err(..) => return Some(StaleFile::Missing(reference.to_path_buf())), + Err(..) => return Some(StaleItem::MissingFile(reference.to_path_buf())), }; for path in paths { @@ -1655,7 +1678,7 @@ where Entry::Vacant(v) => { let mtime = match paths::mtime(path) { Ok(mtime) => mtime, - Err(..) => return Some(StaleFile::Missing(path.to_path_buf())), + Err(..) => return Some(StaleItem::MissingFile(path.to_path_buf())), }; *v.insert(mtime) } @@ -1683,7 +1706,7 @@ where continue; } - return Some(StaleFile::Changed { + return Some(StaleItem::ChangedFile { reference: reference.to_path_buf(), reference_mtime, stale: path.to_path_buf(), @@ -1698,23 +1721,13 @@ where None } -#[repr(u8)] +#[derive(Serialize, Deserialize)] enum DepInfoPathType { // src/, e.g. src/lib.rs - PackageRootRelative = 1, + PackageRootRelative, // target/debug/deps/lib... // or an absolute path /.../sysroot/... - TargetRootRelative = 2, -} - -impl DepInfoPathType { - fn from_byte(b: u8) -> Option { - match b { - 1 => Some(DepInfoPathType::PackageRootRelative), - 2 => Some(DepInfoPathType::TargetRootRelative), - _ => None, - } - } + TargetRootRelative, } /// Parses the dep-info file coming out of rustc into a Cargo-specific format. @@ -1748,16 +1761,14 @@ pub fn translate_dep_info( 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 depinfo = parse_rustc_dep_info(rustc_dep_info)?; let target_root = target_root.canonicalize()?; let pkg_root = pkg_root.canonicalize()?; - let mut new_contents = Vec::new(); - for file in deps { + let mut on_disk_info = OnDiskDepInfo::default(); + on_disk_info.env = depinfo.env; + + for file in depinfo.files { // 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 abs_file = rustc_cwd.join(file); @@ -1779,28 +1790,145 @@ pub fn translate_dep_info( // effect. (DepInfoPathType::TargetRootRelative, &*abs_file) }; - new_contents.push(ty as u8); - new_contents.extend(util::path2bytes(path)?); - new_contents.push(0); + on_disk_info.files.push((ty, path.to_owned())); } - paths::write(cargo_dep_info, &new_contents)?; + paths::write(cargo_dep_info, on_disk_info.serialize()?)?; Ok(()) } +#[derive(Default)] +pub struct RustcDepInfo { + /// The list of files that the main target in the dep-info file depends on. + pub files: Vec, + /// The list of environment variables we found that the rustc compilation + /// depends on. + pub env: Vec<(String, Option)>, +} + +#[derive(Default)] +struct OnDiskDepInfo { + files: Vec<(DepInfoPathType, PathBuf)>, + env: Vec<(String, Option)>, +} + +impl OnDiskDepInfo { + fn parse(mut bytes: &[u8]) -> Option { + let bytes = &mut bytes; + let nfiles = read_usize(bytes)?; + let mut files = Vec::with_capacity(nfiles as usize); + for _ in 0..nfiles { + let ty = match read_u8(bytes)? { + 0 => DepInfoPathType::PackageRootRelative, + 1 => DepInfoPathType::TargetRootRelative, + _ => return None, + }; + let bytes = read_bytes(bytes)?; + files.push((ty, util::bytes2path(bytes).ok()?)); + } + + let nenv = read_usize(bytes)?; + let mut env = Vec::with_capacity(nenv as usize); + for _ in 0..nenv { + let key = str::from_utf8(read_bytes(bytes)?).ok()?.to_string(); + let val = match read_u8(bytes)? { + 0 => None, + 1 => Some(str::from_utf8(read_bytes(bytes)?).ok()?.to_string()), + _ => return None, + }; + env.push((key, val)); + } + return Some(OnDiskDepInfo { files, env }); + + fn read_usize(bytes: &mut &[u8]) -> Option { + let ret = bytes.get(..4)?; + *bytes = &bytes[4..]; + Some( + ((ret[0] as usize) << 0) + | ((ret[1] as usize) << 8) + | ((ret[2] as usize) << 16) + | ((ret[3] as usize) << 24), + ) + } + + fn read_u8(bytes: &mut &[u8]) -> Option { + let ret = *bytes.get(0)?; + *bytes = &bytes[1..]; + Some(ret) + } + + fn read_bytes<'a>(bytes: &mut &'a [u8]) -> Option<&'a [u8]> { + let n = read_usize(bytes)? as usize; + let ret = bytes.get(..n)?; + *bytes = &bytes[n..]; + Some(ret) + } + } + + fn serialize(&self) -> CargoResult> { + let mut ret = Vec::new(); + let dst = &mut ret; + write_usize(dst, self.files.len()); + for (ty, file) in self.files.iter() { + match ty { + DepInfoPathType::PackageRootRelative => dst.push(0), + DepInfoPathType::TargetRootRelative => dst.push(1), + } + write_bytes(dst, util::path2bytes(file)?); + } + + write_usize(dst, self.env.len()); + for (key, val) in self.env.iter() { + write_bytes(dst, key); + match val { + None => dst.push(0), + Some(val) => { + dst.push(1); + write_bytes(dst, val); + } + } + } + return Ok(ret); + + fn write_bytes(dst: &mut Vec, val: impl AsRef<[u8]>) { + let val = val.as_ref(); + write_usize(dst, val.len()); + dst.extend_from_slice(val); + } + + fn write_usize(dst: &mut Vec, val: usize) { + dst.push(val as u8); + dst.push((val >> 8) as u8); + dst.push((val >> 16) as u8); + dst.push((val >> 24) as u8); + assert!(val >> 32 == 0); + } + } +} + /// Parse the `.d` dep-info file generated by rustc. -/// -/// Result is a Vec of `(target, prerequisites)` tuples where `target` is the -/// rule name, and `prerequisites` is a list of files that it depends on. -pub fn parse_rustc_dep_info(rustc_dep_info: &Path) -> CargoResult)>> { +pub fn parse_rustc_dep_info(rustc_dep_info: &Path) -> CargoResult { let contents = paths::read(rustc_dep_info)?; - contents - .lines() - .filter_map(|l| l.find(": ").map(|i| (l, i))) - .map(|(line, pos)| { - let target = &line[..pos]; + let mut ret = RustcDepInfo::default(); + let mut found_deps = false; + + for line in contents.lines() { + let env_dep_prefix = "# env-dep:"; + if line.starts_with(env_dep_prefix) { + let rest = &line[env_dep_prefix.len()..]; + let mut parts = rest.splitn(2, '='); + let env_var = match parts.next() { + Some(s) => s.to_string(), + None => continue, + }; + let env_val = parts.next().map(|s| s.to_string()); + ret.env.push((env_var, env_val)); + } else if let Some(pos) = line.find(": ") { + if found_deps { + continue; + } + found_deps = true; let mut deps = line[pos + 2..].split_whitespace(); - let mut ret = Vec::new(); while let Some(s) = deps.next() { let mut file = s.to_string(); while file.ends_with('\\') { @@ -1810,9 +1938,9 @@ pub fn parse_rustc_dep_info(rustc_dep_info: &Path) -> CargoResult, unit: &Unit) -> CargoResult<()> // If nothing changed don't recreate the file which could alter // its mtime if let Ok(previous) = fingerprint::parse_rustc_dep_info(&output_path) { - if previous.len() == 1 && previous[0].0 == target_fn && previous[0].1 == deps { + if previous.files.iter().eq(deps.iter().map(Path::new)) { continue; } } diff --git a/tests/testsuite/dep_info.rs b/tests/testsuite/dep_info.rs index 4223fb31198..8e9b9fe4875 100644 --- a/tests/testsuite/dep_info.rs +++ b/tests/testsuite/dep_info.rs @@ -9,6 +9,7 @@ use cargo_test_support::{ use filetime::FileTime; use std::fs; use std::path::Path; +use std::str; // Helper for testing dep-info files in the fingerprint dir. fn assert_deps(project: &Project, fingerprint: &str, test_cb: impl Fn(&Path, &[(u8, &str)])) { @@ -22,17 +23,38 @@ fn assert_deps(project: &Project, fingerprint: &str, test_cb: impl Fn(&Path, &[( .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| { + let dep_info = &mut &dep_info[..]; + let deps = (0..read_usize(dep_info)) + .map(|_| { ( - p[0], - std::str::from_utf8(&p[1..]).expect("expected valid path"), + read_u8(dep_info), + str::from_utf8(read_bytes(dep_info)).unwrap(), ) }) - .collect(); + .collect::>(); test_cb(&info_path, &deps); + + fn read_usize(bytes: &mut &[u8]) -> usize { + let ret = &bytes[..4]; + *bytes = &bytes[4..]; + ((ret[0] as usize) << 0) + | ((ret[1] as usize) << 8) + | ((ret[2] as usize) << 16) + | ((ret[3] as usize) << 24) + } + + fn read_u8(bytes: &mut &[u8]) -> u8 { + let ret = bytes[0]; + *bytes = &bytes[1..]; + ret + } + + fn read_bytes<'a>(bytes: &mut &'a [u8]) -> &'a [u8] { + let n = read_usize(bytes) as usize; + let ret = &bytes[..n]; + *bytes = &bytes[n..]; + ret + } } fn assert_deps_contains(project: &Project, fingerprint: &str, expected: &[(u8, &str)]) { @@ -273,31 +295,31 @@ fn relative_depinfo_paths_ws() { assert_deps_contains( &p, "target/debug/.fingerprint/pm-*/dep-lib-pm", - &[(1, "src/lib.rs"), (2, "debug/deps/libpmdep-*.rlib")], + &[(0, "src/lib.rs"), (1, "debug/deps/libpmdep-*.rlib")], ); assert_deps_contains( &p, &format!("target/{}/debug/.fingerprint/foo-*/dep-bin-foo", host), &[ - (1, "src/main.rs"), + (0, "src/main.rs"), ( - 2, + 1, &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)), + (1, &format!("{}/debug/deps/libbar-*.rlib", host)), + (1, &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")], + &[(0, "build.rs"), (1, "debug/deps/libbdep-*.rlib")], ); // Make sure it stays fresh. @@ -401,31 +423,31 @@ fn relative_depinfo_paths_no_ws() { assert_deps_contains( &p, "target/debug/.fingerprint/pm-*/dep-lib-pm", - &[(1, "src/lib.rs"), (2, "debug/deps/libpmdep-*.rlib")], + &[(0, "src/lib.rs"), (1, "debug/deps/libpmdep-*.rlib")], ); assert_deps_contains( &p, "target/debug/.fingerprint/foo-*/dep-bin-foo", &[ - (1, "src/main.rs"), + (0, "src/main.rs"), ( - 2, + 1, &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"), + (1, "debug/deps/libbar-*.rlib"), + (1, "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")], + &[(0, "build.rs"), (1, "debug/deps/libbdep-*.rlib")], ); // Make sure it stays fresh. @@ -514,6 +536,6 @@ fn canonical_path() { assert_deps_contains( &p, "target/debug/.fingerprint/foo-*/dep-lib-foo", - &[(1, "src/lib.rs"), (2, "debug/deps/libregdep-*.rmeta")], + &[(0, "src/lib.rs"), (1, "debug/deps/libregdep-*.rmeta")], ); } diff --git a/tests/testsuite/freshness.rs b/tests/testsuite/freshness.rs index 9d9069cc849..718ce7d8e38 100644 --- a/tests/testsuite/freshness.rs +++ b/tests/testsuite/freshness.rs @@ -2472,3 +2472,96 @@ fn lld_is_fresh() { .with_stderr("[FRESH] foo [..]\n[FINISHED] [..]") .run(); } + +#[cargo_test] +fn env_in_code_causes_rebuild() { + // Only nightly has support in dep-info files for this + if !cargo_test_support::is_nightly() { + return; + } + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + "#, + ) + .file( + "src/main.rs", + r#" + fn main() { + println!("{:?}", option_env!("FOO")); + } + "#, + ) + .build(); + + p.cargo("build").env_remove("FOO").run(); + p.cargo("build") + .env_remove("FOO") + .with_stderr("[FINISHED] [..]") + .run(); + p.cargo("build") + .env("FOO", "bar") + .with_stderr("[COMPILING][..]\n[FINISHED][..]") + .run(); + p.cargo("build") + .env("FOO", "bar") + .with_stderr("[FINISHED][..]") + .run(); + p.cargo("build") + .env("FOO", "baz") + .with_stderr("[COMPILING][..]\n[FINISHED][..]") + .run(); + p.cargo("build") + .env("FOO", "baz") + .with_stderr("[FINISHED][..]") + .run(); + p.cargo("build") + .env_remove("FOO") + .with_stderr("[COMPILING][..]\n[FINISHED][..]") + .run(); + p.cargo("build") + .env_remove("FOO") + .with_stderr("[FINISHED][..]") + .run(); +} + +#[cargo_test] +fn env_build_script_no_rebuild() { + // Only nightly has support in dep-info files for this + if !cargo_test_support::is_nightly() { + return; + } + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + "#, + ) + .file( + "build.rs", + r#" + fn main() { + println!("cargo:rustc-env=FOO=bar"); + } + "#, + ) + .file( + "src/main.rs", + r#" + fn main() { + println!("{:?}", env!("FOO")); + } + "#, + ) + .build(); + + p.cargo("build").run(); + p.cargo("build").with_stderr("[FINISHED] [..]").run(); +} From 784ea96e8e71ab2606af8bc4edba13f21a4811b7 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 29 Jun 2020 08:26:10 -0700 Subject: [PATCH 2/6] Filter env vars to check against --- src/cargo/core/compiler/fingerprint.rs | 31 +++++++++++++++++++++++++- src/cargo/core/compiler/mod.rs | 7 +++--- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index b0add13d94d..37814be66d2 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -328,7 +328,7 @@ use crate::core::{InternedString, Package}; use crate::util; use crate::util::errors::{CargoResult, CargoResultExt}; use crate::util::paths; -use crate::util::{internal, profile}; +use crate::util::{internal, profile, ProcessBuilder}; use super::custom_build::BuildDeps; use super::job::{ @@ -1759,6 +1759,7 @@ pub fn translate_dep_info( rustc_cwd: &Path, pkg_root: &Path, target_root: &Path, + rustc_cmd: &ProcessBuilder, allow_package: bool, ) -> CargoResult<()> { let depinfo = parse_rustc_dep_info(rustc_dep_info)?; @@ -1768,6 +1769,34 @@ pub fn translate_dep_info( let mut on_disk_info = OnDiskDepInfo::default(); on_disk_info.env = depinfo.env; + // 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 returend 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... + on_disk_info + .env + .retain(|(key, _)| !rustc_cmd.get_envs().contains_key(key)); + for file in depinfo.files { // The path may be absolute or relative, canonical or not. Make sure // it is canonicalized so we are comparing the same kinds of paths. diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index dd2a3c5da20..c690d37f7d9 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -70,7 +70,7 @@ pub trait Executor: Send + Sync + 'static { /// this package. fn exec( &self, - cmd: ProcessBuilder, + cmd: &ProcessBuilder, id: PackageId, target: &Target, mode: CompileMode, @@ -93,7 +93,7 @@ pub struct DefaultExecutor; impl Executor for DefaultExecutor { fn exec( &self, - cmd: ProcessBuilder, + cmd: &ProcessBuilder, _id: PackageId, _target: &Target, _mode: CompileMode, @@ -281,7 +281,7 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc) -> Car state.build_plan(buildkey, rustc.clone(), outputs.clone()); } else { exec.exec( - rustc, + &rustc, package_id, &target, mode, @@ -299,6 +299,7 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc) -> Car &cwd, &pkg_root, &target_dir, + &rustc, // Do not track source files in the fingerprint for registry dependencies. is_local, ) From 68200151da73a66f798b472ea3afcda036fa6e54 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 29 Jun 2020 08:52:05 -0700 Subject: [PATCH 3/6] Get past internal test on CI --- tests/internal.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/internal.rs b/tests/internal.rs index 2631d787381..13ab8af23f2 100644 --- a/tests/internal.rs +++ b/tests/internal.rs @@ -24,6 +24,9 @@ fn check_forbidden_code() { } let c = fs::read_to_string(path).unwrap(); for (line_index, line) in c.lines().enumerate() { + if line.trim().starts_with("//") { + continue; + } if line_has_print(line) { if entry.file_name().to_str().unwrap() == "cargo_new.rs" && line.contains("Hello") { // An exception. From 643f12efcf74b290ab4fa75dbadcec2c918963a3 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 29 Jun 2020 11:19:10 -0700 Subject: [PATCH 4/6] Address review feedback --- src/cargo/core/compiler/fingerprint.rs | 72 +++++++++++++++++++------- tests/testsuite/freshness.rs | 4 +- 2 files changed, 55 insertions(+), 21 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index 37814be66d2..23e256db369 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -106,8 +106,9 @@ //! used to log details about *why* a fingerprint is considered dirty. //! `CARGO_LOG=cargo::core::compiler::fingerprint=trace cargo build` can be //! used to display this log information. -//! - A "dep-info" file which contains a list of source filenames for the -//! target. See below for details. +//! - A "dep-info" file which is a translation of rustc's `*.d` dep-info files +//! to a Cargo-specific format that tweaks file names and is optimized for +//! reading quickly. //! - An `invoked.timestamp` file whose filesystem mtime is updated every time //! the Unit is built. This is used for capturing the time when the build //! starts, to detect if files are changed in the middle of the build. See @@ -146,7 +147,10 @@ //! directory (`translate_dep_info`). The mtime of the fingerprint dep-info //! file itself is used as the reference for comparing the source files to //! determine if any of the source files have been modified (see below for -//! more detail). +//! more detail). Note that Cargo parses the special `# env-var:...` comments in +//! dep-info files to learn about environment variables that the rustc compile +//! depends on. Cargo then later uses this to trigger a recompile if a +//! referenced env var changes (even if the source didn't change). //! //! There is also a third dep-info file. Cargo will extend the file created by //! rustc with some additional information and saves this into the output @@ -692,10 +696,16 @@ enum StaleItem { impl LocalFingerprint { /// Checks dynamically at runtime if this `LocalFingerprint` has a stale - /// file. + /// item inside of it. /// - /// This will use the absolute root paths passed in if necessary to guide - /// file accesses. + /// The main purpose of this function is to handle two different ways + /// fingerprints can be invalidated: + /// + /// * One is a dependency listed in rustc's dep-info files is invalid. Note + /// that these could either be env vars or files. We check both here. + /// + /// * Another is the `rerun-if-changed` directive from build scripts. This + /// is where we'll find whether files have actually changed fn find_stale_item( &self, mtime_cache: &mut HashMap, @@ -703,10 +713,12 @@ impl LocalFingerprint { target_root: &Path, ) -> CargoResult> { match self { - // We need to parse `dep_info`, learn about all the files the crate - // depends on, and then see if any of them are newer than the - // dep_info file itself. If the `dep_info` file is missing then this - // unit has never been compiled! + // We need to parse `dep_info`, learn about the crate's dependencies. + // + // For each env var we see if our current process's env var still + // matches, and for each file we see if any of them are newer than + // the `dep_info` file itself whose mtime represents the start of + // rustc. LocalFingerprint::CheckDepInfo { dep_info } => { let dep_info = target_root.join(dep_info); let info = match parse_dep_info(pkg_root, target_root, &dep_info)? { @@ -1620,7 +1632,17 @@ fn log_compare(unit: &Unit, compare: &CargoResult<()>) { info!(" err: {:?}", ce); } -// Parse the dep-info into a list of paths +/// Parses Cargo's internal `EncodedDepInfo` structure that was previously +/// serialized to disk. +/// +/// Note that this is not rustc's `*.d` files. +/// +/// Also note that rustc's `*.d` files are translated to Cargo-specific +/// `EncodedDepInfo` files after compilations have finished in +/// `translate_dep_info`. +/// +/// Returns `None` if the file is corrupt or couldn't be read from disk. This +/// indicates that the crate should likely be rebuilt. pub fn parse_dep_info( pkg_root: &Path, target_root: &Path, @@ -1630,9 +1652,12 @@ pub fn parse_dep_info( Ok(data) => data, Err(_) => return Ok(None), }; - let info = match OnDiskDepInfo::parse(&data) { + let info = match EncodedDepInfo::parse(&data) { Some(info) => info, - None => return Ok(None), + None => { + log::warn!("failed to parse cargo's dep-info at {:?}", dep_info); + return Ok(None); + } }; let mut ret = RustcDepInfo::default(); ret.env = info.env; @@ -1721,7 +1746,6 @@ where None } -#[derive(Serialize, Deserialize)] enum DepInfoPathType { // src/, e.g. src/lib.rs PackageRootRelative, @@ -1766,7 +1790,7 @@ pub fn translate_dep_info( let target_root = target_root.canonicalize()?; let pkg_root = pkg_root.canonicalize()?; - let mut on_disk_info = OnDiskDepInfo::default(); + let mut on_disk_info = EncodedDepInfo::default(); on_disk_info.env = depinfo.env; // This is a bit of a tricky statement, but here we're *removing* the @@ -1831,17 +1855,27 @@ pub struct RustcDepInfo { pub files: Vec, /// The list of environment variables we found that the rustc compilation /// depends on. + /// + /// The first element of the pair is the name of the env var and the second + /// item is the value. `Some` means that the env var was set, and `None` + /// means that the env var wasn't actually set and the compilation depends + /// on it not being set. pub env: Vec<(String, Option)>, } +// Same as `RustcDepInfo` except avoids absolute paths as much as possible to +// allow moving around the target directory. +// +// This is also stored in an optimized format to make parsing it fast because +// Cargo will read it for crates on all future compilations. #[derive(Default)] -struct OnDiskDepInfo { +struct EncodedDepInfo { files: Vec<(DepInfoPathType, PathBuf)>, env: Vec<(String, Option)>, } -impl OnDiskDepInfo { - fn parse(mut bytes: &[u8]) -> Option { +impl EncodedDepInfo { + fn parse(mut bytes: &[u8]) -> Option { let bytes = &mut bytes; let nfiles = read_usize(bytes)?; let mut files = Vec::with_capacity(nfiles as usize); @@ -1866,7 +1900,7 @@ impl OnDiskDepInfo { }; env.push((key, val)); } - return Some(OnDiskDepInfo { files, env }); + return Some(EncodedDepInfo { files, env }); fn read_usize(bytes: &mut &[u8]) -> Option { let ret = bytes.get(..4)?; diff --git a/tests/testsuite/freshness.rs b/tests/testsuite/freshness.rs index 718ce7d8e38..d14bae64098 100644 --- a/tests/testsuite/freshness.rs +++ b/tests/testsuite/freshness.rs @@ -2475,7 +2475,7 @@ fn lld_is_fresh() { #[cargo_test] fn env_in_code_causes_rebuild() { - // Only nightly has support in dep-info files for this + // Only nightly 1.46 has support in dep-info files for this if !cargo_test_support::is_nightly() { return; } @@ -2531,7 +2531,7 @@ fn env_in_code_causes_rebuild() { #[cargo_test] fn env_build_script_no_rebuild() { - // Only nightly has support in dep-info files for this + // Only nightly 1.46 has support in dep-info files for this if !cargo_test_support::is_nightly() { return; } From a6bf36e0ac7d5b7f7d87ce36829e29460b5f10d2 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 29 Jun 2020 12:39:44 -0700 Subject: [PATCH 5/6] Bump hashed metadata version --- src/cargo/core/compiler/context/compilation_files.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/core/compiler/context/compilation_files.rs b/src/cargo/core/compiler/context/compilation_files.rs index 5d2cf4efce9..13ee773d450 100644 --- a/src/cargo/core/compiler/context/compilation_files.rs +++ b/src/cargo/core/compiler/context/compilation_files.rs @@ -490,7 +490,7 @@ fn compute_metadata( // cause a new hash due to the rustc version changing, but this allows // cargo to be extra careful to deal with different versions of cargo that // use the same rustc version. - 1.hash(&mut hasher); + 2.hash(&mut hasher); // Unique metadata per (name, source, version) triple. This'll allow us // to pull crates from anywhere without worrying about conflicts. From 0750a6f5040bced0ae20d6eff83bfb4b7dafb9e9 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 29 Jun 2020 12:57:09 -0700 Subject: [PATCH 6/6] Handle env var escaping --- src/cargo/core/compiler/fingerprint.rs | 33 ++++++++++++++++++++++---- tests/testsuite/freshness.rs | 14 +++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src/cargo/core/compiler/fingerprint.rs b/src/cargo/core/compiler/fingerprint.rs index 23e256db369..a75482e2da5 100644 --- a/src/cargo/core/compiler/fingerprint.rs +++ b/src/cargo/core/compiler/fingerprint.rs @@ -1980,11 +1980,14 @@ pub fn parse_rustc_dep_info(rustc_dep_info: &Path) -> CargoResult let rest = &line[env_dep_prefix.len()..]; let mut parts = rest.splitn(2, '='); let env_var = match parts.next() { - Some(s) => s.to_string(), + Some(s) => s, None => continue, }; - let env_val = parts.next().map(|s| s.to_string()); - ret.env.push((env_var, env_val)); + let env_val = match parts.next() { + Some(s) => Some(unescape_env(s)?), + None => None, + }; + ret.env.push((unescape_env(env_var)?, env_val)); } else if let Some(pos) = line.find(": ") { if found_deps { continue; @@ -2005,5 +2008,27 @@ pub fn parse_rustc_dep_info(rustc_dep_info: &Path) -> CargoResult } } } - Ok(ret) + return Ok(ret); + + // rustc tries to fit env var names and values all on a single line, which + // means it needs to escape `\r` and `\n`. The escape syntax used is "\n" + // which means that `\` also needs to be escaped. + fn unescape_env(s: &str) -> CargoResult { + let mut ret = String::with_capacity(s.len()); + let mut chars = s.chars(); + while let Some(c) = chars.next() { + if c != '\\' { + ret.push(c); + continue; + } + match chars.next() { + Some('\\') => ret.push('\\'), + Some('n') => ret.push('\n'), + Some('r') => ret.push('\r'), + Some(c) => bail!("unknown escape character `{}`", c), + None => bail!("unterminated escape character"), + } + } + Ok(ret) + } } diff --git a/tests/testsuite/freshness.rs b/tests/testsuite/freshness.rs index d14bae64098..26d898484fc 100644 --- a/tests/testsuite/freshness.rs +++ b/tests/testsuite/freshness.rs @@ -2493,6 +2493,7 @@ fn env_in_code_causes_rebuild() { r#" fn main() { println!("{:?}", option_env!("FOO")); + println!("{:?}", option_env!("FOO\nBAR")); } "#, ) @@ -2527,6 +2528,19 @@ fn env_in_code_causes_rebuild() { .env_remove("FOO") .with_stderr("[FINISHED][..]") .run(); + + let interesting = " #!$\nabc\r\\\t\u{8}\r\n"; + p.cargo("build").env("FOO", interesting).run(); + p.cargo("build") + .env("FOO", interesting) + .with_stderr("[FINISHED][..]") + .run(); + + p.cargo("build").env("FOO\nBAR", interesting).run(); + p.cargo("build") + .env("FOO\nBAR", interesting) + .with_stderr("[FINISHED][..]") + .run(); } #[cargo_test]