From 7dcedd85a4c7d8bef6287d52fec204abb5573634 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 12 Jan 2016 17:37:57 -0800 Subject: [PATCH] Handle rerun-if-changed directives in dependencies There was a failure mode of the handling of the rerun-if-changed directive where it would rerun the build script twice before hitting a steady state of actually processing the directives. The order of events that led to this were: 1. A project was built from a clean directory. Cargo recorded a fingerprint indicating this (for the build script), but the fingerprint indicated that the build script was a normal build script (no manually specified inputs) because Cargo had no prior knowledge. 2. A project was then rebuilt from the same directory. Cargo's new fingerprint for the build script now indicates that there is a custom list of dependencies, but the previous fingerprint indicates there wasn't, so the mismatch causes another rebuild. 3. All future rebuilds will agree that there are custom lists both before and after, so the directives are processed as one would expect. This commit does a bit of refactoring in the fingerprint module to fix this situation. The recorded fingerprint in step (1) is now recorded as a "custom dependencies are specified" fingerprint if, after the build script is run, custom dependencies were specified. Closes #2267 --- src/cargo/ops/cargo_rustc/custom_build.rs | 9 +- src/cargo/ops/cargo_rustc/fingerprint.rs | 299 ++++++++++++---------- tests/test_cargo_freshness.rs | 32 +++ tests/test_cargo_rustc.rs | 2 +- 4 files changed, 196 insertions(+), 146 deletions(-) diff --git a/src/cargo/ops/cargo_rustc/custom_build.rs b/src/cargo/ops/cargo_rustc/custom_build.rs index 6fe6814966f..9b974ab1cd3 100644 --- a/src/cargo/ops/cargo_rustc/custom_build.rs +++ b/src/cargo/ops/cargo_rustc/custom_build.rs @@ -131,10 +131,11 @@ fn build_work<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) // Check to see if the build script as already run, and if it has keep // track of whether it has told us about some explicit dependencies let prev_output = BuildOutput::parse_file(&output_file, &pkg_name).ok(); - if let Some(ref prev) = prev_output { - let val = (output_file.clone(), prev.rerun_if_changed.clone()); - cx.build_explicit_deps.insert(*unit, val); - } + let rerun_if_changed = match prev_output { + Some(ref prev) => prev.rerun_if_changed.clone(), + None => Vec::new(), + }; + cx.build_explicit_deps.insert(*unit, (output_file.clone(), rerun_if_changed)); try!(fs::create_dir_all(&cx.layout(unit.pkg, Kind::Host).build(unit.pkg))); try!(fs::create_dir_all(&cx.layout(unit.pkg, unit.kind).build(unit.pkg))); diff --git a/src/cargo/ops/cargo_rustc/fingerprint.rs b/src/cargo/ops/cargo_rustc/fingerprint.rs index 81d8c3e06b5..1cd37c33c62 100644 --- a/src/cargo/ops/cargo_rustc/fingerprint.rs +++ b/src/cargo/ops/cargo_rustc/fingerprint.rs @@ -1,5 +1,5 @@ use std::fs::{self, File, OpenOptions}; -use std::hash::{Hash, Hasher, SipHasher}; +use std::hash::{self, Hasher}; use std::io::prelude::*; use std::io::{BufReader, SeekFrom}; use std::path::{Path, PathBuf}; @@ -54,7 +54,7 @@ pub fn prepare_target<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, debug!("fingerprint at: {}", loc.display()); let fingerprint = try!(calculate(cx, unit)); - let compare = compare_old_fingerprint(&loc, &fingerprint); + let compare = compare_old_fingerprint(&loc, &*fingerprint); log_compare(unit, &compare); let root = cx.out_dir(unit); @@ -66,8 +66,17 @@ pub fn prepare_target<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, } let allow_failure = unit.profile.rustc_args.is_some(); - Ok(prepare(compare.is_ok() && !missing_outputs, - allow_failure, loc, fingerprint)) + let write_fingerprint = Work::new(move |_| { + match fingerprint.update_local() { + Ok(()) => {} + Err(..) if allow_failure => return Ok(()), + Err(e) => return Err(e) + } + write_fingerprint(&loc, &*fingerprint) + }); + + let fresh = compare.is_ok() && !missing_outputs; + Ok((if fresh {Fresh} else {Dirty}, write_fingerprint, Work::noop())) } /// A fingerprint can be considered to be a "short string" representing the @@ -82,7 +91,7 @@ pub fn prepare_target<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, /// as a fingerprint (all source files must be modified *before* this mtime). /// This dep-info file is not generated, however, until after the crate is /// compiled. As a result, this structure can be thought of as a fingerprint -/// to-be. The actual value can be calculated via `resolve()`, but the operation +/// to-be. The actual value can be calculated via `hash()`, but the operation /// may fail as some files may not have been generated. /// /// Note that dependencies are taken into account for fingerprints because rustc @@ -98,7 +107,7 @@ pub struct Fingerprint { profile: u64, deps: Vec<(String, Arc)>, local: LocalFingerprint, - resolved: Mutex>, + memoized_hash: Mutex>, } #[derive(RustcEncodable, RustcDecodable, Hash)] @@ -110,59 +119,50 @@ enum LocalFingerprint { struct MtimeSlot(Mutex>); impl Fingerprint { - fn resolve(&self, force: bool) -> CargoResult { - if !force { - if let Some(s) = *self.resolved.lock().unwrap() { - return Ok(s) - } - } - let mut s = SipHasher::new_with_keys(0, 0); - self.rustc.hash(&mut s); - self.features.hash(&mut s); - self.target.hash(&mut s); - self.profile.hash(&mut s); + fn update_local(&self) -> CargoResult<()> { match self.local { LocalFingerprint::MtimeBased(ref slot, ref path) => { - let mut slot = slot.0.lock().unwrap(); - if force { - let meta = try!(fs::metadata(path).chain_error(|| { - internal(format!("failed to stat {:?}", path)) - })); - *slot = Some(FileTime::from_last_modification_time(&meta)); - } - slot.hash(&mut s); + let meta = try!(fs::metadata(path).chain_error(|| { + internal(format!("failed to stat `{}`", path.display())) + })); + let mtime = FileTime::from_last_modification_time(&meta); + *slot.0.lock().unwrap() = Some(mtime); } - LocalFingerprint::Precalculated(ref p) => p.hash(&mut s), + LocalFingerprint::Precalculated(..) => return Ok(()) } - for &(_, ref dep) in self.deps.iter() { - try!(dep.resolve(force)).hash(&mut s); + *self.memoized_hash.lock().unwrap() = None; + Ok(()) + } + + fn hash(&self) -> u64 { + if let Some(s) = *self.memoized_hash.lock().unwrap() { + return s } - let ret = s.finish(); - *self.resolved.lock().unwrap() = Some(ret); - Ok(ret) + let ret = util::hash_u64(self); + *self.memoized_hash.lock().unwrap() = Some(ret); + return ret } fn compare(&self, old: &Fingerprint) -> CargoResult<()> { if self.rustc != old.rustc { - return Err(internal("rust compiler has changed")) + bail!("rust compiler has changed") } if self.features != old.features { - return Err(internal(format!("features have changed: {} != {}", - self.features, old.features))) + bail!("features have changed: {} != {}", self.features, old.features) } if self.target != old.target { - return Err(internal("target configuration has changed")) + bail!("target configuration has changed") } if self.profile != old.profile { - return Err(internal("profile configuration has changed")) + bail!("profile configuration has changed") } match (&self.local, &old.local) { (&LocalFingerprint::Precalculated(ref a), &LocalFingerprint::Precalculated(ref b)) => { if a != b { - return Err(internal(format!("precalculated components have \ - changed: {} != {}", a, b))) + bail!("precalculated components have changed: {} != {}", + a, b) } } (&LocalFingerprint::MtimeBased(ref a, ref ap), @@ -170,29 +170,40 @@ impl Fingerprint { let a = a.0.lock().unwrap(); let b = b.0.lock().unwrap(); if *a != *b { - return Err(internal(format!("mtime based components have \ - changed: {:?} != {:?}, paths \ - are {:?} and {:?}", - *a, *b, ap, bp))) + bail!("mtime based comopnents have changed: {:?} != {:?}, \ + paths are {:?} and {:?}", *a, *b, ap, bp) } } - _ => return Err(internal("local fingerprint type has changed")), + _ => bail!("local fingerprint type has changed"), } if self.deps.len() != old.deps.len() { - return Err(internal("number of dependencies has changed")) + bail!("number of dependencies has changed") } for (a, b) in self.deps.iter().zip(old.deps.iter()) { - let new = *a.1.resolved.lock().unwrap(); - let old = *b.1.resolved.lock().unwrap(); - if new != old { - return Err(internal(format!("new ({}) != old ({})", a.0, b.0))) + if a.1.hash() != b.1.hash() { + bail!("new ({}) != old ({})", a.0, b.0) } } Ok(()) } } +impl hash::Hash for Fingerprint { + fn hash(&self, h: &mut H) { + let Fingerprint { + rustc, + ref features, + target, + profile, + ref deps, + ref local, + memoized_hash: _, + } = *self; + (rustc, features, target, profile, deps, local).hash(h) + } +} + impl Encodable for Fingerprint { fn encode(&self, e: &mut E) -> Result<(), E::Error> { e.emit_struct("Fingerprint", 6, |e| { @@ -205,7 +216,7 @@ impl Encodable for Fingerprint { })); try!(e.emit_struct_field("deps", 5, |e| { self.deps.iter().map(|&(ref a, ref b)| { - (a, b.resolve(false).unwrap()) + (a, b.hash()) }).collect::>().encode(e) })); Ok(()) @@ -225,11 +236,11 @@ impl Decodable for Fingerprint { profile: try!(d.read_struct_field("profile", 2, decode)), local: try!(d.read_struct_field("local", 3, decode)), features: try!(d.read_struct_field("features", 4, decode)), - resolved: Mutex::new(None), + memoized_hash: Mutex::new(None), deps: { let decode = decode::, D>; let v = try!(d.read_struct_field("deps", 5, decode)); - v.into_iter().map(|(name, resolved)| { + v.into_iter().map(|(name, hash)| { (name, Arc::new(Fingerprint { rustc: 0, target: 0, @@ -237,7 +248,7 @@ impl Decodable for Fingerprint { local: LocalFingerprint::Precalculated(String::new()), features: String::new(), deps: Vec::new(), - resolved: Mutex::new(Some(resolved)), + memoized_hash: Mutex::new(Some(hash)), })) }).collect() } @@ -246,7 +257,7 @@ impl Decodable for Fingerprint { } } -impl Hash for MtimeSlot { +impl hash::Hash for MtimeSlot { fn hash(&self, h: &mut H) { self.0.lock().unwrap().hash(h) } @@ -315,10 +326,10 @@ fn calculate<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) // And finally, calculate what our own local fingerprint is let local = if use_dep_info(unit) { let dep_info = dep_info_loc(cx, unit); - let mtime = try!(calculate_target_mtime(&dep_info)); + let mtime = try!(dep_info_mtime_if_fresh(&dep_info)); LocalFingerprint::MtimeBased(MtimeSlot(Mutex::new(mtime)), dep_info) } else { - let fingerprint = try!(calculate_pkg_fingerprint(cx, unit.pkg)); + let fingerprint = try!(pkg_fingerprint(cx, unit.pkg)); LocalFingerprint::Precalculated(fingerprint) }; let mut deps = deps; @@ -330,7 +341,7 @@ fn calculate<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) features: format!("{:?}", features), deps: deps, local: local, - resolved: Mutex::new(None), + memoized_hash: Mutex::new(None), }); cx.fingerprints.insert(*unit, fingerprint.clone()); Ok(fingerprint) @@ -363,8 +374,8 @@ fn use_dep_info(unit: &Unit) -> bool { /// /// The currently implemented solution is option (1), although it is planned to /// migrate to option (2) in the near future. -pub fn prepare_build_cmd(cx: &mut Context, unit: &Unit) - -> CargoResult { +pub fn prepare_build_cmd<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) + -> CargoResult { let _p = profile::start(format!("fingerprint build cmd: {}", unit.pkg.package_id())); let new = dir(cx, unit); @@ -376,44 +387,79 @@ pub fn prepare_build_cmd(cx: &mut Context, unit: &Unit) // is just a hash of what it was overridden with. Otherwise the fingerprint // is that of the entire package itself as we just consider everything as // input to the build script. - let local = { + let (local, output_path) = { let state = cx.build_state.outputs.lock().unwrap(); match state.get(&(unit.pkg.package_id().clone(), unit.kind)) { Some(output) => { let s = format!("overridden build state with hash: {}", util::hash_u64(output)); - LocalFingerprint::Precalculated(s) + (LocalFingerprint::Precalculated(s), None) } None => { - match cx.build_explicit_deps.get(unit) { - Some(&(ref output, ref deps)) if deps.len() > 0 => { - let mtime = try!(calculate_explicit_fingerprint(unit, - output, - deps)); - LocalFingerprint::MtimeBased(MtimeSlot(Mutex::new(mtime)), - output.clone()) - } - _ => { - let s = try!(calculate_pkg_fingerprint(cx, unit.pkg)); - LocalFingerprint::Precalculated(s) - } - } + let &(ref output, ref deps) = &cx.build_explicit_deps[unit]; + + let local = if deps.len() == 0 { + let s = try!(pkg_fingerprint(cx, unit.pkg)); + LocalFingerprint::Precalculated(s) + } else { + let deps = deps.iter().map(|p| unit.pkg.root().join(p)); + let mtime = mtime_if_fresh(output, deps); + let mtime = MtimeSlot(Mutex::new(mtime)); + LocalFingerprint::MtimeBased(mtime, output.clone()) + }; + + (local, Some(output.clone())) } } }; - let new_fingerprint = Arc::new(Fingerprint { + + let mut fingerprint = Fingerprint { rustc: 0, target: 0, profile: 0, features: String::new(), deps: Vec::new(), local: local, - resolved: Mutex::new(None), + memoized_hash: Mutex::new(None), + }; + let compare = compare_old_fingerprint(&loc, &fingerprint); + log_compare(unit, &compare); + + // When we write out the fingerprint, we may want to actually change the + // kind of fingerprint being recorded. If we started out, then the previous + // run of the build script (or if it had never run before) may indicate to + // use the `Precalculated` variant with the `pkg_fingerprint`. If the build + // script then prints `rerun-if-changed`, however, we need to record what's + // necessary for that fingerprint. + // + // Hence, if there were some `rerun-if-changed` directives forcibly change + // the kind of fingerprint over to the `MtimeBased` variant where the + // relevant mtime is the output path of the build script. + let state = cx.build_state.clone(); + let key = (unit.pkg.package_id().clone(), unit.kind); + let write_fingerprint = Work::new(move |_| { + if let Some(output_path) = output_path { + let outputs = state.outputs.lock().unwrap(); + if outputs[&key].rerun_if_changed.len() > 0 { + let slot = MtimeSlot(Mutex::new(None)); + fingerprint.local = LocalFingerprint::MtimeBased(slot, + output_path); + try!(fingerprint.update_local()); + } + } + write_fingerprint(&loc, &fingerprint) }); - let compare = compare_old_fingerprint(&loc, &new_fingerprint); - log_compare(unit, &compare); - Ok(prepare(compare.is_ok(), false, loc, new_fingerprint)) + Ok((if compare.is_ok() {Fresh} else {Dirty}, write_fingerprint, Work::noop())) +} + +fn write_fingerprint(loc: &Path, fingerprint: &Fingerprint) -> CargoResult<()> { + let hash = fingerprint.hash(); + debug!("write fingerprint: {}", loc.display()); + try!(paths::write(&loc, util::to_hex(hash).as_bytes())); + try!(paths::write(&loc.with_extension("json"), + json::encode(&fingerprint).unwrap().as_bytes())); + Ok(()) } /// Prepare work for when a package starts to build @@ -430,31 +476,6 @@ pub fn prepare_init(cx: &mut Context, unit: &Unit) -> CargoResult<()> { Ok(()) } -/// Given the data to build and write a fingerprint, generate some Work -/// instances to actually perform the necessary work. -fn prepare(is_fresh: bool, - allow_failure: bool, - loc: PathBuf, - fingerprint: Arc) -> Preparation { - let write_fingerprint = Work::new(move |_| { - debug!("write fingerprint: {}", loc.display()); - let hash = match fingerprint.resolve(true) { - Ok(e) => e, - Err(..) if allow_failure => return Ok(()), - Err(e) => return Err(e).chain_error(|| { - internal("failed to resolve a pending fingerprint") - }) - - }; - try!(paths::write(&loc, util::to_hex(hash).as_bytes())); - try!(paths::write(&loc.with_extension("json"), - json::encode(&fingerprint).unwrap().as_bytes())); - Ok(()) - }); - - (if is_fresh {Fresh} else {Dirty}, write_fingerprint, Work::noop()) -} - /// Return the (old, new) location for fingerprints for a package pub fn dir(cx: &Context, unit: &Unit) -> PathBuf { cx.layout(unit.pkg, unit.kind).proxy().fingerprint(unit.pkg) @@ -465,20 +486,16 @@ pub fn dep_info_loc(cx: &Context, unit: &Unit) -> PathBuf { dir(cx, unit).join(&format!("dep-{}", filename(unit))) } -fn compare_old_fingerprint(loc: &Path, - new_fingerprint: &Fingerprint) +fn compare_old_fingerprint(loc: &Path, new_fingerprint: &Fingerprint) -> CargoResult<()> { let old_fingerprint_short = try!(paths::read(loc)); - let new_hash = try!(new_fingerprint.resolve(false).chain_error(|| { - internal(format!("failed to resolve new fingerprint")) - })); + let new_hash = new_fingerprint.hash(); if util::to_hex(new_hash) == old_fingerprint_short { return Ok(()) } let old_fingerprint_json = try!(paths::read(&loc.with_extension("json"))); - let old_fingerprint = try!(json::decode(&old_fingerprint_json).chain_error(|| { internal(format!("failed to deserialize json")) })); @@ -502,7 +519,7 @@ fn log_compare(unit: &Unit, compare: &CargoResult<()>) { } } -fn calculate_target_mtime(dep_info: &Path) -> CargoResult> { +fn dep_info_mtime_if_fresh(dep_info: &Path) -> CargoResult> { macro_rules! fs_try { ($e:expr) => (match $e { Ok(e) => e, Err(..) => return Ok(None) }) } @@ -517,14 +534,13 @@ fn calculate_target_mtime(dep_info: &Path) -> CargoResult> { Some(Ok(line)) => line, _ => return Ok(None), }; - let meta = try!(fs::metadata(&dep_info)); - let mtime = FileTime::from_last_modification_time(&meta); let pos = try!(line.find(": ").chain_error(|| { internal(format!("dep-info not in an understood format: {}", dep_info.display())) })); let deps = &line[pos + 2..]; + let mut paths = Vec::new(); let mut deps = deps.split(' ').map(|s| s.trim()).filter(|s| !s.is_empty()); loop { let mut file = match deps.next() { @@ -534,56 +550,57 @@ fn calculate_target_mtime(dep_info: &Path) -> CargoResult> { while file.ends_with("\\") { file.pop(); file.push(' '); - file.push_str(deps.next().unwrap()) - } - let meta = match fs::metadata(cwd.join(&file)) { - Ok(meta) => meta, - Err(..) => { info!("stale: {} -- missing", file); return Ok(None) } - }; - let file_mtime = FileTime::from_last_modification_time(&meta); - if file_mtime > mtime { - info!("stale: {} -- {} vs {}", file, file_mtime, mtime); - return Ok(None) + file.push_str(try!(deps.next().chain_error(|| { + internal(format!("malformed dep-info format, trailing \\")) + }))); } + paths.push(cwd.join(&file)); } - Ok(Some(mtime)) + Ok(mtime_if_fresh(&dep_info, paths.iter())) } -fn calculate_pkg_fingerprint(cx: &Context, - pkg: &Package) -> CargoResult { - let source = cx.sources - .get(pkg.package_id().source_id()) - .expect("BUG: Missing package source"); - +fn pkg_fingerprint(cx: &Context, pkg: &Package) -> CargoResult { + let source_id = pkg.package_id().source_id(); + let source = try!(cx.sources.get(source_id).chain_error(|| { + internal("missing package source") + })); source.fingerprint(pkg) } -fn calculate_explicit_fingerprint(unit: &Unit, - output: &Path, - deps: &[String]) - -> CargoResult> { +fn mtime_if_fresh(output: &Path, paths: I) -> Option + where I: IntoIterator, + I::Item: AsRef, +{ let meta = match fs::metadata(output) { Ok(meta) => meta, - Err(..) => return Ok(None), + Err(..) => return None, }; let mtime = FileTime::from_last_modification_time(&meta); - for path in deps.iter().map(|p| unit.pkg.root().join(p)) { - let meta = match fs::metadata(&path) { + let any_stale = paths.into_iter().any(|path| { + let path = path.as_ref(); + let meta = match fs::metadata(path) { Ok(meta) => meta, Err(..) => { - info!("bs stale: {} -- missing", path.display()); - return Ok(None) + info!("stale: {} -- missing", path.display()); + return true } }; let mtime2 = FileTime::from_last_modification_time(&meta); if mtime2 > mtime { - info!("bs stale: {} -- {} vs {}", path.display(), mtime2, mtime); - return Ok(None) + info!("stale: {} -- {} vs {}", path.display(), mtime2, mtime); + true + } else { + false } + }); + + if any_stale { + None + } else { + Some(mtime) } - Ok(Some(mtime)) } fn filename(unit: &Unit) -> String { diff --git a/tests/test_cargo_freshness.rs b/tests/test_cargo_freshness.rs index e694bb67e0e..b6801be1142 100644 --- a/tests/test_cargo_freshness.rs +++ b/tests/test_cargo_freshness.rs @@ -257,3 +257,35 @@ test!(no_rebuild_transitive_target_deps { {compiling} foo v0.0.1 ([..]) ", compiling = COMPILING))); }); + +test!(rerun_if_changed_in_dep { + let p = project("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.0.1" + authors = [] + + [dependencies] + a = { path = "a" } + "#) + .file("src/lib.rs", "") + .file("a/Cargo.toml", r#" + [package] + name = "a" + version = "0.0.1" + authors = [] + build = "build.rs" + "#) + .file("a/build.rs", r#" + fn main() { + println!("cargo:rerun-if-changed=build.rs"); + } + "#) + .file("a/src/lib.rs", ""); + + assert_that(p.cargo_process("build"), + execs().with_status(0)); + assert_that(p.cargo("build"), + execs().with_status(0).with_stdout("")); +}); diff --git a/tests/test_cargo_rustc.rs b/tests/test_cargo_rustc.rs index d79ccd9b6bd..cda7726060b 100644 --- a/tests/test_cargo_rustc.rs +++ b/tests/test_cargo_rustc.rs @@ -3,7 +3,7 @@ use std::path::MAIN_SEPARATOR as SEP; use support::{execs, project}; use support::{COMPILING, RUNNING}; -use hamcrest::{assert_that, existing_file}; +use hamcrest::assert_that; fn setup() { }