Skip to content

Commit

Permalink
New semantics for --features flag
Browse files Browse the repository at this point in the history 
Historically, feature-related flags like `--all-features`,
`--no-default-features` and `--features` operated on the *current*
package. That is, `cargo --package foo --feature feat` would activate
`feat` for the package at the current working directory, and not for the
`foo` package. `-Z package-features` flag implements the more obvious
semantics for this combination of flags. This changes behavior, and that
is why we want to start with an unstable opt-in. The changes are:

* `--feature` flag affects the selected package. It is an error to
  specify `--feature` with more than a single `-p`, or with `-p` outside
  workspace (the latter could work in theory, but would be hard to
  implement).

* `--all-features` and `--no-default-features` affect all selected
  packages, and not the one at cwd.

* The package in `cwd` is not implicitly enabled when doing feature
  selection. That is, `cargo build -Z package-features -p foo` could
  select *less* features for various packages than `cargo build -p foo`.
  • Loading branch information
@matklad
matklad committed Apr 13, 2018
1 parent 8afcf45 commit 8d0b31b
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 41 deletions.
2 changes: 2 additions & 0 deletions src/cargo/core/features.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ pub struct CliUnstable {
pub no_index_update: bool,
pub avoid_dev_deps: bool,
pub minimal_versions: bool,
pub package_features: bool,
}

impl CliUnstable {
Expand Down Expand Up @@ -325,6 +326,7 @@ impl CliUnstable {
"no-index-update" => self.no_index_update = true,
"avoid-dev-deps" => self.avoid_dev_deps = true,
"minimal-versions" => self.minimal_versions = true,
"package-features" => self.package_features = true,
_ => bail!("unknown `-Z` flag specified: {}", k),
}

Expand Down
115 changes: 74 additions & 41 deletions src/cargo/ops/resolve.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,53 +213,86 @@ pub fn resolve_with_previous<'a, 'cfg>(
registry.lock_patches();
}

let mut summaries = Vec::new();

for member in ws.members() {
registry.add_sources(&[member.package_id().source_id().clone()])?;
let method_to_resolve = match method {
// When everything for a workspace we want to be sure to resolve all
// members in the workspace, so propagate the `Method::Everything`.
Method::Everything => Method::Everything,

// If we're not resolving everything though then we're constructing the
// exact crate graph we're going to build. Here we don't necessarily
// want to keep around all workspace crates as they may not all be
// built/tested.
//
// Additionally, the `method` specified represents command line
// flags, which really only matters for the current package
// (determined by the cwd). If other packages are specified (via
// `-p`) then the command line flags like features don't apply to
// them.
//
// As a result, if this `member` is the current member of the
// workspace, then we use `method` specified. Otherwise we use a
// base method with no features specified but using default features
// for any other packages specified with `-p`.
Method::Required { dev_deps, .. } => {
let base = Method::Required {
dev_deps,
features: &[],
all_features: false,
uses_default_features: true,
};
let member_id = member.package_id();
match ws.current_opt() {
Some(current) if member_id == current.package_id() => method,
_ => {
if specs.iter().any(|spec| spec.matches(member_id)) {
base
} else {
continue;
}
}

let mut summaries = Vec::new();
if ws.config().cli_unstable().package_features {
let mut members = Vec::new();
match method {
Method::Everything => members.extend(ws.members()),
Method::Required { features, all_features, uses_default_features, .. } => {
if specs.len() > 1 && !features.is_empty() {
bail!("cannot specify features for more than one package");
}
members.extend(
ws.members().filter(|m| specs.iter().any(|spec| spec.matches(m.package_id())))
);
// Edge case: running `cargo build -p foo`, where `foo` is not a member
// of current workspace. Add all packages from workspace to get `foo`
// into the resolution graph.
if members.is_empty() {
if !(features.is_empty() && !all_features && uses_default_features) {
bail!("cannot specify features for packages outside of workspace");
}
members.extend(ws.members());
}
}
};
}
for member in members {
let summary = registry.lock(member.summary().clone());
summaries.push((summary, method))
}
} else {
for member in ws.members() {
let method_to_resolve = match method {
// When everything for a workspace we want to be sure to resolve all
// members in the workspace, so propagate the `Method::Everything`.
Method::Everything => Method::Everything,

// If we're not resolving everything though then we're constructing the
// exact crate graph we're going to build. Here we don't necessarily
// want to keep around all workspace crates as they may not all be
// built/tested.
//
// Additionally, the `method` specified represents command line
// flags, which really only matters for the current package
// (determined by the cwd). If other packages are specified (via
// `-p`) then the command line flags like features don't apply to
// them.
//
// As a result, if this `member` is the current member of the
// workspace, then we use `method` specified. Otherwise we use a
// base method with no features specified but using default features
// for any other packages specified with `-p`.
Method::Required { dev_deps, .. } => {
let base = Method::Required {
dev_deps,
features: &[],
all_features: false,
uses_default_features: true,
};
let member_id = member.package_id();
match ws.current_opt() {
Some(current) if member_id == current.package_id() => method,
_ => {
if specs.iter().any(|spec| spec.matches(member_id)) {
base
} else {
continue;
}
}
}
}
};

let summary = registry.lock(member.summary().clone());
summaries.push((summary, method_to_resolve));
}
};

let summary = registry.lock(member.summary().clone());
summaries.push((summary, method_to_resolve));
}

let root_replace = ws.root_replace();

Expand Down
82 changes: 82 additions & 0 deletions tests/testsuite/features.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ use std::io::prelude::*;

use cargotest::support::paths::CargoPathExt;
use cargotest::support::{execs, project};
use cargotest::ChannelChanger;
use hamcrest::assert_that;
use cargotest::support::registry::Package;

#[test]
fn invalid1() {
Expand Down Expand Up @@ -1629,3 +1631,83 @@ fn many_cli_features_comma_and_space_delimited() {
)),
);
}

#[test]
fn combining_features_and_package() {
Package::new("dep", "1.0.0").publish();

let p = project("ws")
.file(
"Cargo.toml",
r#"
[project]
name = "ws"
version = "0.0.1"
authors = []
[workspace]
members = ["foo"]
[dependencies]
dep = "1"
"#,
)
.file("src/lib.rs", "")
.file(
"foo/Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
authors = []
[features]
main = []
"#,
)
.file("foo/src/main.rs", r#"
#[cfg(feature = "main")]
fn main() {}
"#)
.build();

assert_that(
p.cargo("build -Z package-features --all --features main")
.masquerade_as_nightly_cargo(),
execs().with_status(101).with_stderr_contains("\
[ERROR] cannot specify features for more than one package"
),
);

assert_that(
p.cargo("build -Z package-features --package dep --features main")
.masquerade_as_nightly_cargo(),
execs().with_status(101).with_stderr_contains("\
[ERROR] cannot specify features for packages outside of workspace"
),
);
assert_that(
p.cargo("build -Z package-features --package dep --all-features")
.masquerade_as_nightly_cargo(),
execs().with_status(101).with_stderr_contains("\
[ERROR] cannot specify features for packages outside of workspace"
),
);
assert_that(
p.cargo("build -Z package-features --package dep --no-default-features")
.masquerade_as_nightly_cargo(),
execs().with_status(101).with_stderr_contains("\
[ERROR] cannot specify features for packages outside of workspace"
),
);

assert_that(
p.cargo("build -Z package-features --all --all-features")
.masquerade_as_nightly_cargo(),
execs().with_status(0),
);
assert_that(
p.cargo("run -Z package-features --package foo --features main")
.masquerade_as_nightly_cargo(),
execs().with_status(0),
);
}

0 comments on commit 8d0b31b

Please sign in to comment.