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.

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

Fix a couple issues with cyclic features and dev-dependencies #10103

Merged
merged 2 commits into from
Nov 22, 2021
Merged

Fix a couple issues with cyclic features and dev-dependencies #10103

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
d7ab4a6
`cargo tree`: Fix stack overflow on cyclic features.
ehuss Nov 20, 2021
2eee644
Fix resolve error with cyclic dev-dependency features.
ehuss Nov 20, 2021
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
10 changes: 6 additions & 4 deletions src/cargo/core/resolver/dep_cache.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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();
}