Skip to content

Commit

Permalink
Check default features when skipping resolution of a package
Browse files Browse the repository at this point in the history
Previously if a package had been activated without the default feature and then
it was attempted to be reactivated with the default feature, resolution wouldn't
continue and actually activate the default feature. This means that the
fingerprint on the other end of compilation would be slightly different because
the set of activated features would be slightly different, causing spurious
recompiles.

Closes #1567
  • Loading branch information
alexcrichton committed May 5, 2015
1 parent 7f810ba commit 40d1c10
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 12 deletions.
32 changes: 20 additions & 12 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,12 @@ impl Resolve {

impl fmt::Debug for Resolve {
fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
self.graph.fmt(fmt)
try!(write!(fmt, "graph: {:?}\n", self.graph));
try!(write!(fmt, "\nfeatures: {{\n"));
for (pkg, features) in &self.features {
try!(write!(fmt, " {}: {:?}\n", pkg, features));
}
write!(fmt, "}}")
}
}

Expand Down Expand Up @@ -223,12 +228,17 @@ fn flag_activated(cx: &mut Context,
return false
}
debug!("checking if {} is already activated", summary.package_id());
let features = match *method {
Method::Required{features, ..} => features,
let (features, use_default) = match *method {
Method::Required { features, uses_default_features, .. } => {
(features, uses_default_features)
}
Method::Everything => return false,
};
match cx.resolve.features(id) {
Some(prev) => features.iter().all(|f| prev.contains(f)),
Some(prev) => {
features.iter().all(|f| prev.contains(f)) &&
(!use_default || prev.contains("default"))
}
None => features.len() == 0,
}
}
Expand Down Expand Up @@ -422,7 +432,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary,
(&'a Dependency, Vec<String>)>> {
let dev_deps = match method {
Method::Everything => true,
Method::Required{dev_deps, ..} => dev_deps,
Method::Required { dev_deps, .. } => dev_deps,
};

// First, filter by dev-dependencies
Expand Down Expand Up @@ -519,14 +529,13 @@ fn build_features(s: &Summary, method: Method)
}
match method {
Method::Everything |
Method::Required{uses_default_features: true, ..} => {
if s.features().get("default").is_some() &&
!visited.contains("default") {
Method::Required { uses_default_features: true, .. } => {
if s.features().get("default").is_some() {
try!(add_feature(s, "default", &mut deps, &mut used,
&mut visited));
}
}
_ => {}
Method::Required { uses_default_features: false, .. } => {}
}
return Ok((deps, used));

Expand Down Expand Up @@ -560,9 +569,8 @@ fn build_features(s: &Summary, method: Method)
used.insert(feat.to_string());
match s.features().get(feat) {
Some(recursive) => {
for f in recursive.iter() {
try!(add_feature(s, f, deps, used,
visited));
for f in recursive {
try!(add_feature(s, f, deps, used, visited));
}
}
None => {
Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/cargo_rustc/fingerprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ fn calculate<'a, 'b>(cx: &mut Context<'a, 'b>,
});
let extra = util::short_hash(&(cx.config.rustc_version(), target, &features,
profile));
debug!("extra {:?} {:?} {:?} = {}", target, profile, features, extra);

// Next, recursively calculate the fingerprint for all of our dependencies.
//
Expand Down
40 changes: 40 additions & 0 deletions tests/test_cargo_features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -655,3 +655,43 @@ test!(everything_in_the_lockfile {
assert!(lockfile.contains(r#"name = "d2""#), "d2 not found\n{}", lockfile);
assert!(lockfile.contains(r#"name = "d3""#), "d3 not found\n{}", lockfile);
});

test!(no_rebuild_when_frobbing_default_feature {
let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "foo"
version = "0.1.0"
authors = []
[dependencies]
a = { path = "a" }
b = { path = "b" }
"#)
.file("src/lib.rs", "")
.file("b/Cargo.toml", r#"
[package]
name = "b"
version = "0.1.0"
authors = []
[dependencies]
a = { path = "../a", features = ["f1"], default-features = false }
"#)
.file("b/src/lib.rs", "")
.file("a/Cargo.toml", r#"
[package]
name = "a"
version = "0.1.0"
authors = []
[features]
default = ["f1"]
f1 = []
"#)
.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(""));
assert_that(p.cargo("build"), execs().with_status(0).with_stdout(""));
});

0 comments on commit 40d1c10

Please sign in to comment.