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

Make required dependency as future an error, remove RcList #6860

Merged
merged 9 commits into from
Apr 25, 2019
Merged

Make required dependency as future an error, remove RcList #6860

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f0f8565
remove the resolver warnings
Eh2406 Apr 16, 2019
4590e74
calculate features the hard way
Eh2406 Apr 17, 2019
0131d09
move to a function
Eh2406 Apr 17, 2019
df62a57
list used used_replacements after we figure out what is used
Eh2406 Apr 17, 2019
097dbdf
invert parents to get graph
Eh2406 Apr 17, 2019
8cd9b0c
remove dead code
Eh2406 Apr 17, 2019
d0c80ec
address nits
Eh2406 Apr 17, 2019
a473716
fix replacements
Eh2406 Apr 19, 2019
35ff555
just give up and make it an error
Eh2406 Apr 22, 2019
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
Prev Previous commit
Next Next commit
move to a function
  • Loading branch information
@Eh2406
Eh2406 committed Apr 18, 2019
commit 0131d09dd46d6ad4606384b0dbc4b9ef8170c852
88 changes: 49 additions & 39 deletions src/cargo/core/resolver/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,45 +159,7 @@ pub fn resolve(

// If we have a shell, emit warnings about required deps used as feature.
if print_warnings && config.is_some() {
let mut new_cx = cx.clone();
new_cx.resolve_features = im_rc::HashMap::new();
let mut features_from_dep = HashMap::new();
for (summery, method) in summaries {
for (dep, features) in new_cx
.resolve_features(None, summery, &method, config)
.expect("can not resolve_features for a required summery")
{
features_from_dep.insert((summery.package_id(), dep), features);
}
}
for summery in resolve.sort().iter().rev().map(|id| {
cx.activations
.get(&id.as_activations_key())
.expect("id in dependency graph but not in activations")
.0
.clone()
}) {
for (parent, deps) in cx.parents.edges(&summery.package_id()) {
for dep in deps.iter() {
let features = features_from_dep
.remove(&(*parent, dep.clone()))
.expect("fulfilled a dep that was not needed");
let method = Method::Required {
dev_deps: false,
features: &features,
all_features: false,
uses_default_features: dep.uses_default_features(),
};
for (dep, features) in new_cx
.resolve_features(None, &summery, &method, config)
.expect("can not resolve_features for a used dep")
{
features_from_dep.insert((summery.package_id(), dep), features);
}
}
}
}
assert_eq!(cx.resolve_features, new_cx.resolve_features);
emit_warnings(&cx, &resolve, summaries, config)
}

Ok(resolve)
Expand Down Expand Up @@ -1129,3 +1091,51 @@ fn check_duplicate_pkgs_in_lockfile(resolve: &Resolve) -> CargoResult<()> {
}
Ok(())
}

/// re-run all used resolve_features so it can print warnings
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
fn emit_warnings(
cx: &Context,
resolve: &Resolve,
summaries: &[(Summary, Method<'_>)],
config: Option<&Config>,
) {
let mut new_cx = cx.clone();
new_cx.resolve_features = im_rc::HashMap::new();
let mut features_from_dep = HashMap::new();
for (summery, method) in summaries {
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
for (dep, features) in new_cx
.resolve_features(None, summery, &method, config)
.expect("can not resolve_features for a required summery")
{
features_from_dep.insert((summery.package_id(), dep), features);
}
}
for summery in resolve.sort().iter().rev().map(|id| {
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
cx.activations
.get(&id.as_activations_key())
.expect("id in dependency graph but not in activations")
.0
.clone()
}) {
for (parent, deps) in cx.parents.edges(&summery.package_id()) {
for dep in deps.iter() {
let features = features_from_dep
.remove(&(*parent, dep.clone()))
.expect("fulfilled a dep that was not needed");
let method = Method::Required {
dev_deps: false,
features: &features,
all_features: false,
uses_default_features: dep.uses_default_features(),
};
for (dep, features) in new_cx
.resolve_features(None, &summery, &method, config)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to perhaps avoid re-calling resolve_features here? I found this somewhat complicated to follow, and figured it might be simpler if we simply walk over the graph after resolution, check the list of activated features for each package, and then present a warning if the feature enables a required dependency.

We may produce a slightly worse error message since we don't have why a feature is activated, but that seems reasonable given the age of this warning perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original commit f0f8565, did that. It was a lot cleaner. But it did not work. resolve_features is only adds a canonicalized feature to cx.resolve_features. So I did not find a way to reconstruct the warnings from that. (The hard cases are "invalid9", "dep_feature_in_cmd_line") Suggestions are welcome.

I also don't think we can extend that strategy to only the features transitively enabled by some deps. But I would love to be proven wrong.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could build up a side table or something like that during resolution to facilitate this strategy of emitting warnings? I suppose that's sort of like what happens today, but I think it'd be best to decouple the warning emission as much as we can from the exact state of the resolver today, because it may make future changes to resolve_features more complicated in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, this new code that redos the work of the resolver is going to be a pane to keep in sink.

I gave up on making it work, and the future opportunities of filtering dependencies, and just made this case a resolver bactrackable error.

.expect("can not resolve_features for a used dep")
{
features_from_dep.insert((summery.package_id(), dep), features);
}
}
}
}
assert_eq!(cx.resolve_features, new_cx.resolve_features);
}