Skip to content

Feature dependencies #2325

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 2 commits into from
Closed

Feature dependencies #2325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08a88b5
add feature dependency to all targets but lib
tsurai Oct 10, 2015
2244960
Fix issues
JanLikar Jan 28, 2016
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 @@ -133,6 +133,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 @@ -211,6 +212,7 @@ impl Target {
name: String::new(),
src_path: PathBuf::new(),
metadata: None,
features: None,
doc: false,
doctest: false,
harness: true,
Expand All @@ -235,12 +237,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 @@ -261,35 +265,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 @@ -299,6 +309,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() }
Copy link
Member

Choose a reason for hiding this comment

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

Could this change to returning Option<&[T]> instead of Option<&Vec<T>>? (generally a little more ergonomic)

pub fn kind(&self) -> &TargetKind { &self.kind }
pub fn tested(&self) -> bool { self.tested }
pub fn harness(&self) -> bool { self.harness }
Expand Down
47 changes: 33 additions & 14 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 @@ -155,6 +155,11 @@ pub fn compile_pkg<'a>(root_package: &Package,
s.split(' ')
}).map(|s| s.to_string()).collect::<Vec<String>>();

if spec.len() > 0 && (no_default_features || features.len() > 0) {
bail!("features cannot be modified when the main package \
is not being built");
}
Copy link
Member

Choose a reason for hiding this comment

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

Ah this was removed in 3332c91, so perhaps a rebase brought it back in by accident?


if jobs == Some(0) {
bail!("jobs must be at least 1")
}
Expand Down Expand Up @@ -194,8 +199,9 @@ pub fn compile_pkg<'a>(root_package: &Package,
panic!("`rustc` and `rustdoc` should not accept multiple `-p` flags")
}
(Some(args), _) => {
let all_features = resolve_with_overrides.features(to_builds[0].package_id());
let targets = try!(generate_targets(to_builds[0], profiles,
mode, filter, release));
mode, filter, all_features, release));
if targets.len() == 1 {
let (target, profile) = targets[0];
let mut profile = profile.clone();
Expand All @@ -208,8 +214,9 @@ pub fn compile_pkg<'a>(root_package: &Package,
}
}
(None, Some(args)) => {
let all_features = resolve_with_overrides.features(to_builds[0].package_id());
let targets = try!(generate_targets(to_builds[0], profiles,
mode, filter, release));
mode, filter, all_features, release));
if targets.len() == 1 {
let (target, profile) = targets[0];
let mut profile = profile.clone();
Expand All @@ -223,8 +230,9 @@ pub fn compile_pkg<'a>(root_package: &Package,
}
(None, None) => {
for &to_build in to_builds.iter() {
let all_features = resolve_with_overrides.features(to_build.package_id());
let targets = try!(generate_targets(to_build, profiles, mode,
filter, release));
filter, all_features, release));
package_targets.push((to_build, targets));
}
}
Expand Down Expand Up @@ -301,6 +309,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 @@ -311,13 +320,13 @@ fn generate_targets<'a>(pkg: &'a Package,
CompileMode::Build => build,
CompileMode::Doc { .. } => &profiles.doc,
};
match *filter {
let mut targets = match *filter {
CompileFilter::Everything => {
match mode {
CompileMode::Bench => {
Ok(pkg.targets().iter().filter(|t| t.benched()).map(|t| {
pkg.targets().iter().filter(|t| t.benched()).map(|t| {
(t, profile)
}).collect::<Vec<_>>())
}).collect::<Vec<_>>()
}
CompileMode::Test => {
let mut base = pkg.targets().iter().filter(|t| {
Expand All @@ -333,16 +342,16 @@ fn generate_targets<'a>(pkg: &'a Package,
base.push((t, build));
}
}
Ok(base)
base
}
CompileMode::Build => {
Ok(pkg.targets().iter().filter(|t| {
pkg.targets().iter().filter(|t| {
t.is_bin() || t.is_lib()
}).map(|t| (t, profile)).collect())
}).map(|t| (t, profile)).collect()
}
CompileMode::Doc { .. } => {
Ok(pkg.targets().iter().filter(|t| t.documented())
.map(|t| (t, profile)).collect())
pkg.targets().iter().filter(|t| t.documented())
.map(|t| (t, profile)).collect()
}
}
}
Expand Down Expand Up @@ -377,9 +386,19 @@ fn generate_targets<'a>(pkg: &'a Package,
try!(find(tests, "test", TargetKind::Test, test));
try!(find(benches, "bench", TargetKind::Bench, &profiles.bench));
}
Ok(targets)
targets
}
}
};

targets.retain(|&(t, _)| {
t.is_lib() ||
t.features().is_none() ||
t.features().unwrap().iter().filter(|&f| {
features.map_or(true, |x| !x.contains(f))
}).count() == 0
Copy link
Member

Choose a reason for hiding this comment

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

It probably doesn't make sense to have lib targets with features, so can that be prevented during decoding of a manifest? That means this could look like:

match t.features() {
    Some(f) => f.iter().any(|f| features.contains(f))
    None => true,
}

(or something like that).

In general I'd recommend avoiding patterns like:

  • foo.is_some() || foo.unwrap().bar() -- try to do the match only once to avoid unwrap()
  • iter().filter(..).count() == 0 is the same as leveraging any() or all() I believe
  • map_or -- currently in Cargo I tend to prefer .map().unwrap_or() as the ordering of the arguments is a bit more natural

});

Ok(targets)
}

/// 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 @@ -737,6 +737,7 @@ struct TomlTarget {
doc: Option<bool>,
plugin: Option<bool>,
harness: Option<bool>,
features: Option<Vec<String>>,
}

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

Expand Down Expand Up @@ -851,7 +853,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 @@ -872,7 +874,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 @@ -891,7 +894,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 @@ -911,7 +914,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"),
Copy link
Member

Choose a reason for hiding this comment

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

Currently the cargo_process helper will blow away the directory and rebuild it, but some platforms (notably windows) don't like blowing away directories right after Cargo uses them, so could this change to just p.cargo? That'll avoid blowing away the directory and rebuilding, but should do basically the same thing.

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 @@ -40,6 +40,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