Skip to content

Add feature dependencies to all targets but lib #2056

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

Closed
wants to merge 3 commits into from
Closed

Add feature dependencies to all targets but lib #2056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
030976f
add feature dependency to all targets but lib
tsurai Oct 10, 2015
671b7ae
fix check style errors
tsurai Oct 14, 2015
beaa672
improve code and add tests
tsurai Oct 22, 2015
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
19 changes: 15 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 @@ -138,6 +138,7 @@ pub struct Target {
name: String,
src_path: PathBuf,
metadata: Option<Metadata>,
features: Option<Vec<String>>,
tested: bool,
benched: bool,
doc: bool,
Expand Down Expand Up @@ -226,6 +227,7 @@ impl Target {
name: String::new(),
src_path: PathBuf::new(),
metadata: None,
features: None,
doc: false,
doctest: false,
harness: true,
Expand All @@ -250,12 +252,14 @@ impl Target {
}

pub fn bin_target(name: &str, src_path: &Path,
metadata: Option<Metadata>) -> Target {
metadata: Option<Metadata>,
features: Option<Vec<String>>) -> Target {
Target {
kind: TargetKind::Bin,
name: name.to_string(),
src_path: src_path.to_path_buf(),
metadata: metadata,
features: features,
doc: true,
..Target::blank()
}
Expand All @@ -276,35 +280,41 @@ impl Target {
}
}

pub fn example_target(name: &str, src_path: &Path) -> Target {
pub fn example_target(name: &str, src_path: &Path,
features: Option<Vec<String>>) -> Target {
Target {
kind: TargetKind::Example,
name: name.to_string(),
src_path: src_path.to_path_buf(),
features: features,
benched: false,
..Target::blank()
}
}

pub fn test_target(name: &str, src_path: &Path,
metadata: Metadata) -> Target {
metadata: Metadata,
features: Option<Vec<String>>) -> Target {
Target {
kind: TargetKind::Test,
name: name.to_string(),
src_path: src_path.to_path_buf(),
metadata: Some(metadata),
features: features,
benched: false,
..Target::blank()
}
}

pub fn bench_target(name: &str, src_path: &Path,
metadata: Metadata) -> Target {
metadata: Metadata,
features: Option<Vec<String>>) -> Target {
Target {
kind: TargetKind::Bench,
name: name.to_string(),
src_path: src_path.to_path_buf(),
metadata: Some(metadata),
features: features,
tested: false,
..Target::blank()
}
Expand All @@ -314,6 +324,7 @@ impl Target {
pub fn crate_name(&self) -> String { self.name.replace("-", "_") }
pub fn src_path(&self) -> &Path { &self.src_path }
pub fn metadata(&self) -> Option<&Metadata> { self.metadata.as_ref() }
pub fn features(&self) -> Option<&Vec<String>> { self.features.as_ref() }
pub fn kind(&self) -> &TargetKind { &self.kind }
pub fn tested(&self) -> bool { self.tested }
pub fn harness(&self) -> bool { self.harness }
Expand Down
20 changes: 16 additions & 4 deletions src/cargo/ops/cargo_compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
//! previously compiled dependency
//!

use std::collections::HashMap;
use std::collections::{HashMap, HashSet};
use std::default::Default;
use std::path::{Path, PathBuf};
use std::sync::Arc;
Expand Down Expand Up @@ -153,6 +153,7 @@ pub fn compile_pkg<'a>(root_package: &Package,

(packages, resolved_with_overrides, registry.move_sources())
};
let all_feature = resolve_with_overrides.features(resolve_with_overrides.root());

let mut invalid_spec = vec![];
let pkgids = if spec.len() > 0 {
Expand Down Expand Up @@ -182,7 +183,7 @@ pub fn compile_pkg<'a>(root_package: &Package,
Some(args) => {
if to_builds.len() == 1 {
let targets = try!(generate_targets(to_builds[0], profiles,
mode, filter, release));
mode, filter, all_feature, release));
Copy link
Member

Choose a reason for hiding this comment

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

Like below, it may be best to load the all_feature variable just above this using to_builds[0] to ensure it's a contained borrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

What should be done here? to_builds[0].summary().features() returns a &HashMap<String, Vec<String>>, which is a collection of features from the [features] section of Cargo.toml, but if I understand correctly all_feature should have a list of enabled features for the crate that's going to be built. I need some pointers, because I'm a bit lost.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah the resolve.features(pkgid) is how the set of activated features is learned about. So by moving the all_feature variable down here it'll use the right package to learn the set of activated features.

if targets.len() == 1 {
let (target, profile) = targets[0];
let mut profile = profile.clone();
Expand All @@ -203,7 +204,7 @@ pub fn compile_pkg<'a>(root_package: &Package,
None => {
for &to_build in to_builds.iter() {
let targets = try!(generate_targets(to_build, profiles, mode,
filter, release));
filter, all_feature, release));
Copy link
Member

Choose a reason for hiding this comment

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

The list of all features should actually come from the features enabled for the to_build package rather than the root package in this case.

package_targets.push((to_build, targets));
}
}
Expand Down Expand Up @@ -280,6 +281,7 @@ fn generate_targets<'a>(pkg: &'a Package,
profiles: &'a Profiles,
mode: CompileMode,
filter: &CompileFilter,
features: Option<&HashSet<String>>,
release: bool)
-> CargoResult<Vec<(&'a Target, &'a Profile)>> {
let build = if release {&profiles.release} else {&profiles.dev};
Expand All @@ -290,7 +292,7 @@ fn generate_targets<'a>(pkg: &'a Package,
CompileMode::Build => build,
CompileMode::Doc { .. } => &profiles.doc,
};
return match *filter {
let targets = match *filter {
CompileFilter::Everything => {
match mode {
CompileMode::Bench => {
Expand Down Expand Up @@ -361,6 +363,16 @@ fn generate_targets<'a>(pkg: &'a Package,
Ok(targets)
}
};

targets.map(|x| x.iter().filter_map(|&(t, p)| {
Copy link
Member

Choose a reason for hiding this comment

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

Hm perhaps this changed from before? It looks like the map part of filter_map isn't being used, so this may be able to just be a filter.

It may also be more clear here to explicitly use if/else instead of a match for the logic, the multi-line guard for one of the arms was a little jarring.

Copy link
Member

Choose a reason for hiding this comment

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

Also, instead of having targets be of the type CargoResult<Vec<_>> it's probably best to have it just be Vec<_> (e.g. remove all the Ok above) and just wrap it in Ok after the filtering is done.

Ah and as a final point I think this can use retain instead of iter + filter + collect

match (t.is_lib(), t.features()) {
(true, _) | (false, None) => Some((t, p)),
(false, Some(v)) if v.iter().filter(|&f| {
features.map_or(true, |x| !x.contains(f))
}).count() == 0 => Some((t, p)),
_ => None
}
}).collect())
}

/// Read the `paths` configuration variable to discover all path overrides that
Expand Down
12 changes: 8 additions & 4 deletions src/cargo/util/toml.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,7 @@ struct TomlTarget {
doc: Option<bool>,
plugin: Option<bool>,
harness: Option<bool>,
features: Option<Vec<String>>,
}

#[derive(RustcDecodable, Clone)]
Expand Down Expand Up @@ -721,6 +722,7 @@ impl TomlTarget {
doc: None,
plugin: None,
harness: None,
features: None
}
}

Expand Down Expand Up @@ -807,7 +809,7 @@ fn normalize(lib: &Option<TomlLibTarget>,
PathValue::Path(default(bin))
});
let mut target = Target::bin_target(&bin.name(), &path.to_path(),
None);
None, bin.features.clone());
configure(bin, &mut target);
dst.push(target);
}
Expand All @@ -828,7 +830,8 @@ fn normalize(lib: &Option<TomlLibTarget>,
PathValue::Path(default(ex))
});

let mut target = Target::example_target(&ex.name(), &path.to_path());
let mut target = Target::example_target(&ex.name(), &path.to_path(),
ex.features.clone());
configure(ex, &mut target);
dst.push(target);
}
Expand All @@ -847,7 +850,7 @@ fn normalize(lib: &Option<TomlLibTarget>,
metadata.mix(&format!("test-{}", test.name()));

let mut target = Target::test_target(&test.name(), &path.to_path(),
metadata);
metadata, test.features.clone());
configure(test, &mut target);
dst.push(target);
}
Expand All @@ -867,7 +870,8 @@ fn normalize(lib: &Option<TomlLibTarget>,

let mut target = Target::bench_target(&bench.name(),
&path.to_path(),
metadata);
metadata,
bench.features.clone());
configure(bench, &mut target);
dst.push(target);
}
Expand Down
104 changes: 104 additions & 0 deletions tests/test_cargo_compile_feature_deps.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
use support::{project, execs};
use hamcrest::{assert_that, existing_file, not};

fn setup() {
}

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

[features]
default = ["a"]
a = []

[[bin]]
name = "foo"
features = ["a"]
"#)
.file("src/main.rs", "fn main() {}");

assert_that(p.cargo_process("build"),
execs().with_status(0));

assert_that(&p.bin("foo"), existing_file());

assert_that(p.cargo_process("build").arg("--no-default-features"),
execs().with_status(0));

assert_that(&p.bin("foo"), not(existing_file()));
});

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

[features]
a = []

[[bin]]
name = "foo"
features = ["a"]
"#)
.file("src/main.rs", "fn main() {}");

assert_that(p.cargo_process("build").arg("--features").arg("a"),
execs().with_status(0));

assert_that(&p.bin("foo"), existing_file());
});

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

[features]
default = ["a", "b"]
a = []
b = ["a"]
c = []

[[bin]]
name = "foo_1"
path = "src/foo_1.rs"
features = ["b", "c"]

[[bin]]
name = "foo_2"
path = "src/foo_2.rs"
features = ["a"]
"#)
.file("src/foo_1.rs", "fn main() {}")
.file("src/foo_2.rs", "fn main() {}");

assert_that(p.cargo_process("build"),
execs().with_status(0));

assert_that(&p.bin("foo_1"), not(existing_file()));
assert_that(&p.bin("foo_2"), existing_file());

assert_that(p.cargo_process("build").arg("--features").arg("c"),
execs().with_status(0));

assert_that(&p.bin("foo_1"), existing_file());
assert_that(&p.bin("foo_2"), existing_file());

assert_that(p.cargo_process("build").arg("--no-default-features"),
execs().with_status(0));

assert_that(&p.bin("foo_1"), not(existing_file()));
assert_that(&p.bin("foo_2"), not(existing_file()));
});

1 change: 1 addition & 0 deletions tests/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ mod test_cargo_build_lib;
mod test_cargo_clean;
mod test_cargo_compile;
mod test_cargo_compile_custom_build;
mod test_cargo_compile_feature_deps;
mod test_cargo_compile_git_deps;
mod test_cargo_compile_path_deps;
mod test_cargo_compile_plugins;
Expand Down