Skip to content

Avoid rebuilding a project when cwd changes #4788

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bors merged 2 commits into from
Dec 12, 2017
Merged

Avoid rebuilding a project when cwd changes #4788

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8647a87
Avoid rebuilding a project when cwd changes
alexcrichton Dec 6, 2017
f688e9c
Change Cargo's own dep-file format
alexcrichton Dec 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions src/cargo/core/manifest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::collections::{HashMap, BTreeMap};
use std::fmt;
use std::path::{PathBuf, Path};
use std::rc::Rc;
use std::hash::{Hash, Hasher};

use semver::Version;
use serde::ser;
Expand Down Expand Up @@ -199,7 +200,11 @@ pub struct Profiles {
pub struct Target {
kind: TargetKind,
name: String,
src_path: PathBuf,
// Note that the `src_path` here is excluded from the `Hash` implementation
// as it's absolute currently and is otherwise a little too brittle for
// causing rebuilds. Instead the hash for the path that we send to the
// compiler is handled elsewhere.
src_path: NonHashedPathBuf,
required_features: Option<Vec<String>>,
tested: bool,
benched: bool,
Expand All @@ -209,6 +214,17 @@ pub struct Target {
for_host: bool,
}

#[derive(Clone, PartialEq, Eq, Debug)]
struct NonHashedPathBuf {
path: PathBuf,
}

impl Hash for NonHashedPathBuf {
fn hash<H: Hasher>(&self, _: &mut H) {
// ...
}
}
Copy link
Contributor

@matklad matklad Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A slightly more elaborte design here would be to introduce a notion of explicitly relative path here and track path relative to workspace root and to other workspace members outside the root. Sort of like

struct PathRoots {
  // a number of anchors, such as current workspace dir, a particular workspace member, users's home directory, 
  // CARGO_HOME, etc. This structure is "global" for cargo and is stored in Workspace/config.
  roots: Vec::<PathBuf>, 
}

struct RelativePath {
  root_idx: usize // index in the global PathRoots
  rel_path: PathBuf
}

impl RelativePath {
   fn to_path(&self, roots: &PathRoots) -> PathBuf {
    roots[self.root_idx].join(&self.rel_path)
  }
}

Not sure that we need this, but this can cope with workspaces which are not entirely under a workspace root. (Or perhaps it was a mistake to allow non-subdirectory members?).


#[derive(Serialize)]
struct SerializedTarget<'a> {
/// Is this a `--bin bin`, `--lib`, `--example ex`?
Expand All @@ -227,7 +243,7 @@ impl ser::Serialize for Target {
kind: &self.kind,
crate_types: self.rustc_crate_types(),
name: &self.name,
src_path: &self.src_path,
src_path: &self.src_path.path,
}.serialize(s)
}
}
Expand Down Expand Up @@ -370,7 +386,7 @@ impl Target {
Target {
kind: TargetKind::Bin,
name: String::new(),
src_path: src_path,
src_path: NonHashedPathBuf { path: src_path },
required_features: None,
doc: false,
doctest: false,
Expand Down Expand Up @@ -459,7 +475,7 @@ impl Target {

pub fn name(&self) -> &str { &self.name }
pub fn crate_name(&self) -> String { self.name.replace("-", "_") }
pub fn src_path(&self) -> &Path { &self.src_path }
pub fn src_path(&self) -> &Path { &self.src_path.path }
pub fn required_features(&self) -> Option<&Vec<String>> { self.required_features.as_ref() }
pub fn kind(&self) -> &TargetKind { &self.kind }
pub fn tested(&self) -> bool { self.tested }
Expand Down
102 changes: 56 additions & 46 deletions src/cargo/ops/cargo_rustc/fingerprint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use std::env;
use std::fs::{self, File, OpenOptions};
use std::fs::{self, File};
use std::hash::{self, Hasher};
use std::io::prelude::*;
use std::io::{BufReader, SeekFrom};
use std::io::BufReader;
use std::path::{Path, PathBuf};
use std::sync::{Arc, Mutex};

Expand Down Expand Up @@ -99,8 +99,9 @@ pub fn prepare_target<'a, 'cfg>(cx: &mut Context<'a, 'cfg>,
}

let allow_failure = unit.profile.rustc_args.is_some();
let target_root = cx.target_root().to_path_buf();
let write_fingerprint = Work::new(move |_| {
match fingerprint.update_local() {
match fingerprint.update_local(&target_root) {
Ok(()) => {}
Err(..) if allow_failure => return Ok(()),
Err(e) => return Err(e)
Expand Down Expand Up @@ -139,6 +140,7 @@ pub struct Fingerprint {
features: String,
target: u64,
profile: u64,
path: u64,
#[serde(serialize_with = "serialize_deps", deserialize_with = "deserialize_deps")]
deps: Vec<(String, Arc<Fingerprint>)>,
local: Vec<LocalFingerprint>,
Expand All @@ -165,6 +167,7 @@ fn deserialize_deps<'de, D>(d: D) -> Result<Vec<(String, Arc<Fingerprint>)>, D::
rustc: 0,
target: 0,
profile: 0,
path: 0,
local: vec![LocalFingerprint::Precalculated(String::new())],
features: String::new(),
deps: Vec::new(),
Expand All @@ -181,15 +184,27 @@ enum LocalFingerprint {
EnvBased(String, Option<String>),
}

impl LocalFingerprint {
fn mtime(root: &Path, mtime: Option<FileTime>, path: &Path)
-> LocalFingerprint
{
let mtime = MtimeSlot(Mutex::new(mtime));
assert!(path.is_absolute());
let path = path.strip_prefix(root).unwrap_or(path);
LocalFingerprint::MtimeBased(mtime, path.to_path_buf())
}
}

struct MtimeSlot(Mutex<Option<FileTime>>);

impl Fingerprint {
fn update_local(&self) -> CargoResult<()> {
fn update_local(&self, root: &Path) -> CargoResult<()> {
let mut hash_busted = false;
for local in self.local.iter() {
match *local {
LocalFingerprint::MtimeBased(ref slot, ref path) => {
let meta = fs::metadata(path)
let path = root.join(path);
let meta = fs::metadata(&path)
.chain_err(|| {
internal(format!("failed to stat `{}`", path.display()))
})?;
Expand Down Expand Up @@ -227,11 +242,14 @@ impl Fingerprint {
if self.target != old.target {
bail!("target configuration has changed")
}
if self.path != old.path {
bail!("path to the compiler has changed")
}
if self.profile != old.profile {
bail!("profile configuration has changed")
}
if self.rustflags != old.rustflags {
return Err(internal("RUSTFLAGS has changed"))
bail!("RUSTFLAGS has changed")
}
if self.local.len() != old.local.len() {
bail!("local lens changed");
Expand Down Expand Up @@ -294,13 +312,14 @@ impl hash::Hash for Fingerprint {
rustc,
ref features,
target,
path,
profile,
ref deps,
ref local,
memoized_hash: _,
ref rustflags,
} = *self;
(rustc, features, target, profile, local, rustflags).hash(h);
(rustc, features, target, path, profile, local, rustflags).hash(h);

h.write_usize(deps.len());
for &(ref name, ref fingerprint) in deps {
Expand Down Expand Up @@ -375,8 +394,8 @@ 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 = dep_info_mtime_if_fresh(&dep_info)?;
LocalFingerprint::MtimeBased(MtimeSlot(Mutex::new(mtime)), dep_info)
let mtime = dep_info_mtime_if_fresh(unit.pkg, &dep_info)?;
LocalFingerprint::mtime(cx.target_root(), mtime, &dep_info)
} else {
let fingerprint = pkg_fingerprint(cx, unit.pkg)?;
LocalFingerprint::Precalculated(fingerprint)
Expand All @@ -392,6 +411,9 @@ fn calculate<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>)
rustc: util::hash_u64(&cx.config.rustc()?.verbose_version),
target: util::hash_u64(&unit.target),
profile: util::hash_u64(&unit.profile),
// Note that .0 is hashed here, not .1 which is the cwd. That doesn't
// actually affect the output artifact so there's no need to hash it.
path: util::hash_u64(&super::path_args(cx, unit).0),
features: format!("{:?}", cx.resolve.features_sorted(unit.pkg.package_id())),
deps: deps,
local: vec![local],
Expand Down Expand Up @@ -443,6 +465,7 @@ pub fn prepare_build_cmd<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>)
rustc: 0,
target: 0,
profile: 0,
path: 0,
features: String::new(),
deps: Vec::new(),
local: local,
Expand All @@ -464,16 +487,17 @@ pub fn prepare_build_cmd<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>)
// build script.
let state = Arc::clone(&cx.build_state);
let key = (unit.pkg.package_id().clone(), unit.kind);
let root = unit.pkg.root().to_path_buf();
let pkg_root = unit.pkg.root().to_path_buf();
let target_root = cx.target_root().to_path_buf();
let write_fingerprint = Work::new(move |_| {
if let Some(output_path) = output_path {
let outputs = state.outputs.lock().unwrap();
let outputs = &outputs[&key];
if !outputs.rerun_if_changed.is_empty() ||
!outputs.rerun_if_env_changed.is_empty() {
let deps = BuildDeps::new(&output_path, Some(outputs));
fingerprint.local = local_fingerprints_deps(&deps, &root);
fingerprint.update_local()?;
fingerprint.local = local_fingerprints_deps(&deps, &target_root, &pkg_root);
fingerprint.update_local(&target_root)?;
}
}
write_fingerprint(&loc, &fingerprint)
Expand Down Expand Up @@ -516,18 +540,19 @@ fn build_script_local_fingerprints<'a, 'cfg>(cx: &mut Context<'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((local_fingerprints_deps(deps, unit.pkg.root()), Some(output)))
Ok((local_fingerprints_deps(deps, cx.target_root(), unit.pkg.root()), Some(output)))
}

fn local_fingerprints_deps(deps: &BuildDeps, root: &Path) -> Vec<LocalFingerprint> {
fn local_fingerprints_deps(deps: &BuildDeps, target_root: &Path, pkg_root: &Path)
-> Vec<LocalFingerprint>
{
debug!("new local fingerprints deps");
let mut local = Vec::new();
if !deps.rerun_if_changed.is_empty() {
let output = &deps.build_script_output;
let deps = deps.rerun_if_changed.iter().map(|p| root.join(p));
let deps = deps.rerun_if_changed.iter().map(|p| pkg_root.join(p));
let mtime = mtime_if_fresh(output, deps);
let mtime = MtimeSlot(Mutex::new(mtime));
local.push(LocalFingerprint::MtimeBased(mtime, output.clone()));
local.push(LocalFingerprint::mtime(target_root, mtime, output));
}

for var in deps.rerun_if_env_changed.iter() {
Expand Down Expand Up @@ -584,23 +609,19 @@ fn log_compare(unit: &Unit, compare: &CargoResult<()>) {
};
info!("fingerprint error for {}: {}", unit.pkg, ce);

for cause in ce.iter() {
for cause in ce.iter().skip(1) {
info!(" cause: {}", cause);
}
}

// Parse the dep-info into a list of paths
pub fn parse_dep_info(dep_info: &Path) -> CargoResult<Option<Vec<PathBuf>>> {
pub fn parse_dep_info(cx: &Context, dep_info: &Path)
-> CargoResult<Option<Vec<PathBuf>>>
{
macro_rules! fs_try {
($e:expr) => (match $e { Ok(e) => e, Err(..) => return Ok(None) })
}
let mut f = BufReader::new(fs_try!(File::open(dep_info)));
// see comments in append_current_dir for where this cwd is manifested from.
let mut cwd = Vec::new();
if fs_try!(f.read_until(0, &mut cwd)) == 0 {
return Ok(None)
}
let cwd = util::bytes2path(&cwd[..cwd.len()-1])?;
Copy link
Contributor

@matklad matklad Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct that new versions of Cargo will fail to parse old depinfo, which is actually OK, because it'll cause only a rebuild, and not a build failure?

Copy link
Member Author

@alexcrichton alexcrichton Dec 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm I'm not actually sure what will happen but the hashes are all changing so I don't think newer cargo will use the same files from older cargo.

let f = BufReader::new(fs_try!(File::open(dep_info)));
let line = match f.lines().next() {
Some(Ok(line)) => line,
_ => return Ok(None),
Expand All @@ -622,13 +643,19 @@ pub fn parse_dep_info(dep_info: &Path) -> CargoResult<Option<Vec<PathBuf>>> {
internal("malformed dep-info format, trailing \\".to_string())
})?);
}
paths.push(cwd.join(&file));

// Note that paths emitted in dep info files may be relative, but due to
// `path_args` in the module above this the relative paths are always
// relative to the root of a workspace.
paths.push(cx.ws.root().join(&file));
}
Ok(Some(paths))
}

fn dep_info_mtime_if_fresh(dep_info: &Path) -> CargoResult<Option<FileTime>> {
if let Some(paths) = parse_dep_info(dep_info)? {
fn dep_info_mtime_if_fresh(cx: &Context, dep_info: &Path)
-> CargoResult<Option<FileTime>>
{
if let Some(paths) = parse_dep_info(cx, dep_info)? {
Ok(mtime_if_fresh(dep_info, paths.iter()))
} else {
Ok(None)
Expand Down Expand Up @@ -703,20 +730,3 @@ fn filename<'a, 'cfg>(cx: &mut Context<'a, 'cfg>, unit: &Unit<'a>) -> String {
};
format!("{}{}-{}", flavor, kind, file_stem)
}

// The dep-info files emitted by the compiler all have their listed paths
// relative to whatever the current directory was at the time that the compiler
// was invoked. As the current directory may change over time, we need to record
// what that directory was at the beginning of the file so we can know about it
// next time.
pub fn append_current_dir(path: &Path, cwd: &Path) -> CargoResult<()> {
debug!("appending {} <- {}", path.display(), cwd.display());
let mut f = OpenOptions::new().read(true).write(true).open(path)?;
let mut contents = Vec::new();
f.read_to_end(&mut contents)?;
f.seek(SeekFrom::Start(0))?;
f.write_all(util::path2bytes(cwd)?)?;
f.write_all(&[0])?;
f.write_all(&contents)?;
Ok(())
}
Loading