Skip to content
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.

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

Only compile activated optional dependencies #1812

Merged
merged 2 commits into from
Jul 17, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 5 additions & 6 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ fn activate(mut cx: Box<Context>,

// Next, transform all dependencies into a list of possible candidates which
// can satisfy that dependency.
let mut deps = try!(deps.into_iter().map(|(_dep_name, (dep, features))| {
let mut deps = try!(deps.into_iter().map(|(dep, features)| {
let mut candidates = try!(registry.query(dep));
// When we attempt versions for a package, we'll want to start at the
// maximum version and work our way down.
Expand Down Expand Up @@ -430,8 +430,7 @@ fn compatible(a: &semver::Version, b: &semver::Version) -> bool {

fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary,
method: Method)
-> CargoResult<HashMap<&'a str,
(&'a Dependency, Vec<String>)>> {
-> CargoResult<Vec<(&'a Dependency, Vec<String>)>> {
let dev_deps = match method {
Method::Everything => true,
Method::Required { dev_deps, .. } => dev_deps,
Expand All @@ -452,7 +451,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary,
});

let (mut feature_deps, used_features) = try!(build_features(parent, method));
let mut ret = HashMap::new();
let mut ret = Vec::new();

// Next, sanitize all requested features by whitelisting all the requested
// features that correspond to optional dependencies
Expand All @@ -461,7 +460,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary,
if dep.is_optional() && !feature_deps.contains_key(dep.name()) {
continue
}
let mut base = feature_deps.remove(dep.name()).unwrap_or(vec![]);
let mut base = feature_deps.remove(dep.name()).unwrap_or(Vec::new());
for feature in dep.features().iter() {
base.push(feature.clone());
if feature.contains("/") {
Expand All @@ -471,7 +470,7 @@ fn resolve_features<'a>(cx: &mut Context, parent: &'a Summary,
feature)));
}
}
ret.insert(dep.name(), (dep, base));
ret.push((dep, base));
}

// All features can only point to optional dependencies, in which case they
Expand Down
10 changes: 8 additions & 2 deletions src/cargo/ops/cargo_rustc/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,6 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
pkg.dependencies().iter().filter(|d| {
d.name() == dep.name()
}).any(|d| {

// If this target is a build command, then we only want build
// dependencies, otherwise we want everything *other than* build
// dependencies.
Expand All @@ -364,7 +363,14 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
target.is_example() ||
profile.test;

is_correct_dep && is_actual_dep
// If the dependency is optional, then we're only activating it
// if the corresponding feature was activated
let activated = !d.is_optional() ||
self.resolve.features(pkg.package_id()).map(|f| {
Copy link
Contributor

Choose a reason for hiding this comment

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

this usage seems to imply that features() wants to return a set

Copy link
Member Author

Choose a reason for hiding this comment

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

How so? Currently it returns Option<&HashSet>, but you're saying it should return HashSet? Certainly possible, just wanted to avoid the extra work on the calling side for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, i incorrectly parsed the map call as being on an iterator. I think HashSet would be slightly cleaner, but it's not as important as I thought.

f.contains(d.name())
}).unwrap_or(false);

is_correct_dep && is_actual_dep && activated
})
}).filter_map(|pkg| {
pkg.targets().iter().find(|t| t.is_lib()).map(|t| {
Expand Down
28 changes: 28 additions & 0 deletions tests/test_cargo_features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -741,3 +741,31 @@ test!(unions_work_with_no_default_features {
assert_that(p.cargo("build"), execs().with_status(0).with_stdout(""));
assert_that(p.cargo("build"), execs().with_status(0).with_stdout(""));
});

test!(optional_and_dev_dep {
let p = project("foo")
.file("Cargo.toml", r#"
[package]
name = "test"
version = "0.1.0"
authors = []

[dependencies]
foo = { path = "foo", optional = true }
[dev-dependencies]
foo = { path = "foo" }
"#)
.file("src/lib.rs", "")
.file("foo/Cargo.toml", r#"
[package]
name = "foo"
version = "0.1.0"
authors = []
"#)
.file("foo/src/lib.rs", "");

assert_that(p.cargo_process("build"),
execs().with_status(0).with_stdout(format!("\
{compiling} test v0.1.0 ([..])
", compiling = COMPILING)));
});