Skip to content

Commit

Permalink
Auto merge of #10103 - ehuss:tree-cycle, r=alexcrichton
Browse files Browse the repository at this point in the history 
Fix a couple issues with cyclic features and dev-dependencies

This fixes two issues with cyclic features and dev-dependencies:

* `cargo tree` would enter an infinite loop for cyclic features.
* The resolver would return a confusing error if a cyclic dev-dependency attempted to enable a feature on its parent that resulted in a cycle.  This fixes it to resolve correctly.

Fixes #10101
  • Loading branch information
@bors
bors committed Nov 22, 2021
2 parents df8cda0 + 2eee644 commit a2c6ec7
Show file tree
Hide file tree
Showing 3 changed files with 258 additions and 30 deletions.
10 changes: 6 additions & 4 deletions src/cargo/core/resolver/dep_cache.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,10 +280,12 @@ pub fn resolve_features<'b>(
// dep_name/feat_name` where `dep_name` does not exist. All other
// validation is done either in `build_requirements` or
// `build_feature_map`.
for dep_name in reqs.deps.keys() {
if !valid_dep_names.contains(dep_name) {
let e = RequirementError::MissingDependency(*dep_name);
return Err(e.into_activate_error(parent, s));
if parent.is_none() {
for dep_name in reqs.deps.keys() {
if !valid_dep_names.contains(dep_name) {
let e = RequirementError::MissingDependency(*dep_name);
return Err(e.into_activate_error(parent, s));
}
}
}

Expand Down
62 changes: 36 additions & 26 deletions src/cargo/ops/tree/graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,28 +427,32 @@ fn add_pkg(
/// ```text
/// from -Edge-> featname -Edge::Feature-> to
/// ```
///
/// Returns a tuple `(missing, index)`.
/// `missing` is true if this feature edge was already added.
/// `index` is the index of the index in the graph of the `Feature` node.
fn add_feature(
graph: &mut Graph<'_>,
name: InternedString,
from: Option<usize>,
to: usize,
kind: EdgeKind,
) -> usize {
) -> (bool, usize) {
// `to` *must* point to a package node.
assert!(matches! {graph.nodes[to], Node::Package{..}});
let node = Node::Feature {
node_index: to,
name,
};
let node_index = match graph.index.get(&node) {
Some(idx) => *idx,
None => graph.add_node(node),
let (missing, node_index) = match graph.index.get(&node) {
Some(idx) => (false, *idx),
None => (true, graph.add_node(node)),
};
if let Some(from) = from {
graph.edges[from].add_edge(kind, node_index);
}
graph.edges[node_index].add_edge(EdgeKind::Feature, to);
node_index
(missing, node_index)
}

/// Adds nodes for features requested on the command-line for the given member.
Expand Down Expand Up @@ -480,7 +484,7 @@ fn add_cli_features(
for fv in to_add {
match fv {
FeatureValue::Feature(feature) => {
let index = add_feature(graph, feature, None, package_index, EdgeKind::Feature);
let index = add_feature(graph, feature, None, package_index, EdgeKind::Feature).1;
graph.cli_features.insert(index);
}
// This is enforced by CliFeatures.
Expand Down Expand Up @@ -511,10 +515,11 @@ fn add_cli_features(
if is_optional {
// Activate the optional dep on self.
let index =
add_feature(graph, dep_name, None, package_index, EdgeKind::Feature);
add_feature(graph, dep_name, None, package_index, EdgeKind::Feature).1;
graph.cli_features.insert(index);
}
let index = add_feature(graph, dep_feature, None, dep_index, EdgeKind::Feature);
let index =
add_feature(graph, dep_feature, None, dep_index, EdgeKind::Feature).1;
graph.cli_features.insert(index);
}
}
Expand Down Expand Up @@ -571,21 +576,24 @@ fn add_feature_rec(
for fv in fvs {
match fv {
FeatureValue::Feature(dep_name) => {
let feat_index = add_feature(
let (missing, feat_index) = add_feature(
graph,
*dep_name,
Some(from),
package_index,
EdgeKind::Feature,
);
add_feature_rec(
graph,
resolve,
*dep_name,
package_id,
feat_index,
package_index,
);
// Don't recursive if the edge already exists to deal with cycles.
if missing {
add_feature_rec(
graph,
resolve,
*dep_name,
package_id,
feat_index,
package_index,
);
}
}
// Dependencies are already shown in the graph as dep edges. I'm
// uncertain whether or not this might be confusing in some cases
Expand Down Expand Up @@ -628,21 +636,23 @@ fn add_feature_rec(
EdgeKind::Feature,
);
}
let feat_index = add_feature(
let (missing, feat_index) = add_feature(
graph,
*dep_feature,
Some(from),
dep_index,
EdgeKind::Feature,
);
add_feature_rec(
graph,
resolve,
*dep_feature,
dep_pkg_id,
feat_index,
dep_index,
);
if missing {
add_feature_rec(
graph,
resolve,
*dep_feature,
dep_pkg_id,
feat_index,
dep_index,
);
}
}
}
}
Expand Down
216 changes: 216 additions & 0 deletions tests/testsuite/tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1831,3 +1831,219 @@ foo v0.1.0 ([..]/foo)
.with_status(101)
.run();
}

#[cargo_test]
fn cyclic_features() {
// Check for stack overflow with cyclic features (oops!).
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "1.0.0"
[features]
a = ["b"]
b = ["a"]
default = ["a"]
"#,
)
.file("src/lib.rs", "")
.build();

p.cargo("tree -e features")
.with_stdout("foo v1.0.0 ([ROOT]/foo)")
.run();

p.cargo("tree -e features -i foo")
.with_stdout(
"\
foo v1.0.0 ([ROOT]/foo)
├── foo feature \"a\"
│ ├── foo feature \"b\"
│ │ └── foo feature \"a\" (*)
│ └── foo feature \"default\" (command-line)
├── foo feature \"b\" (*)
└── foo feature \"default\" (command-line)
",
)
.run();
}

#[cargo_test]
fn dev_dep_cycle_with_feature() {
// Cycle with features and a dev-dependency.
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "1.0.0"
[dev-dependencies]
bar = { path = "bar" }
[features]
a = ["bar/feat1"]
"#,
)
.file("src/lib.rs", "")
.file(
"bar/Cargo.toml",
r#"
[package]
name = "bar"
version = "1.0.0"
[dependencies]
foo = { path = ".." }
[features]
feat1 = ["foo/a"]
"#,
)
.file("bar/src/lib.rs", "")
.build();

p.cargo("tree -e features --features a")
.with_stdout(
"\
foo v1.0.0 ([ROOT]/foo)
[dev-dependencies]
└── bar feature \"default\"
└── bar v1.0.0 ([ROOT]/foo/bar)
└── foo feature \"default\" (command-line)
└── foo v1.0.0 ([ROOT]/foo) (*)
",
)
.run();

p.cargo("tree -e features --features a -i foo")
.with_stdout(
"\
foo v1.0.0 ([ROOT]/foo)
├── foo feature \"a\" (command-line)
│ └── bar feature \"feat1\"
│ └── foo feature \"a\" (command-line) (*)
└── foo feature \"default\" (command-line)
└── bar v1.0.0 ([ROOT]/foo/bar)
├── bar feature \"default\"
│ [dev-dependencies]
│ └── foo v1.0.0 ([ROOT]/foo) (*)
└── bar feature \"feat1\" (*)
",
)
.run();
}

#[cargo_test]
fn dev_dep_cycle_with_feature_nested() {
// Checks for an issue where a cyclic dev dependency tries to activate a
// feature on its parent that tries to activate the feature back on the
// dev-dependency.
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "1.0.0"
[dev-dependencies]
bar = { path = "bar" }
[features]
a = ["bar/feat1"]
b = ["a"]
"#,
)
.file("src/lib.rs", "")
.file(
"bar/Cargo.toml",
r#"
[package]
name = "bar"
version = "1.0.0"
[dependencies]
foo = { path = ".." }
[features]
feat1 = ["foo/b"]
"#,
)
.file("bar/src/lib.rs", "")
.build();

p.cargo("tree -e features")
.with_stdout(
"\
foo v1.0.0 ([ROOT]/foo)
[dev-dependencies]
└── bar feature \"default\"
└── bar v1.0.0 ([ROOT]/foo/bar)
└── foo feature \"default\" (command-line)
└── foo v1.0.0 ([ROOT]/foo) (*)
",
)
.run();

p.cargo("tree -e features --features a -i foo")
.with_stdout(
"\
foo v1.0.0 ([ROOT]/foo)
├── foo feature \"a\" (command-line)
│ └── foo feature \"b\"
│ └── bar feature \"feat1\"
│ └── foo feature \"a\" (command-line) (*)
├── foo feature \"b\" (*)
└── foo feature \"default\" (command-line)
└── bar v1.0.0 ([ROOT]/foo/bar)
├── bar feature \"default\"
│ [dev-dependencies]
│ └── foo v1.0.0 ([ROOT]/foo) (*)
└── bar feature \"feat1\" (*)
",
)
.run();

p.cargo("tree -e features --features b -i foo")
.with_stdout(
"\
foo v1.0.0 ([ROOT]/foo)
├── foo feature \"a\"
│ └── foo feature \"b\" (command-line)
│ └── bar feature \"feat1\"
│ └── foo feature \"a\" (*)
├── foo feature \"b\" (command-line) (*)
└── foo feature \"default\" (command-line)
└── bar v1.0.0 ([ROOT]/foo/bar)
├── bar feature \"default\"
│ [dev-dependencies]
│ └── foo v1.0.0 ([ROOT]/foo) (*)
└── bar feature \"feat1\" (*)
",
)
.run();

p.cargo("tree -e features --features bar/feat1 -i foo")
.with_stdout(
"\
foo v1.0.0 ([ROOT]/foo)
├── foo feature \"a\"
│ └── foo feature \"b\"
│ └── bar feature \"feat1\" (command-line)
│ └── foo feature \"a\" (*)
├── foo feature \"b\" (*)
└── foo feature \"default\" (command-line)
└── bar v1.0.0 ([ROOT]/foo/bar)
├── bar feature \"default\"
│ [dev-dependencies]
│ └── foo v1.0.0 ([ROOT]/foo) (*)
└── bar feature \"feat1\" (command-line) (*)
",
)
.run();
}

0 comments on commit a2c6ec7

Please sign in to comment.