Skip to content

backtrack if can not activate #5000

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

Merged
bors merged 5 commits into from
Mar 1, 2018
Merged

backtrack if can not activate #5000

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8348469
add a test for cargo/issues/4347
Eh2406 Feb 27, 2018
3300728
WIP: make it recurs if activation fails
Eh2406 Feb 27, 2018
5d4402c
fix the todo's
Eh2406 Feb 27, 2018
6218584
One more todo to fix
Eh2406 Mar 1, 2018
2cbd1dd
fix cause and the error messages
Eh2406 Mar 1, 2018
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
220 changes: 146 additions & 74 deletions src/cargo/core/resolver/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ fn activate(cx: &mut Context,
parent: Option<&Summary>,
candidate: Candidate,
method: &Method)
-> CargoResult<Option<(DepsFrame, Duration)>> {
-> ActivateResult<Option<(DepsFrame, Duration)>> {
if let Some(parent) = parent {
cx.resolve_graph.push(GraphNode::Link(parent.package_id().clone(),
candidate.summary.package_id().clone()));
Expand Down Expand Up @@ -536,14 +536,40 @@ impl Ord for DepsFrame {
enum ConflictReason {
Semver,
Links(String),
MissingFeatures(String),
}

enum ActivateError {
Error(::failure::Error),
Conflict(PackageId, ConflictReason),
}
type ActivateResult<T> = Result<T, ActivateError>;

impl From<::failure::Error> for ActivateError {
fn from(t: ::failure::Error) -> Self {
ActivateError::Error(t)
}
}

impl From<(PackageId, ConflictReason)> for ActivateError {
fn from(t: (PackageId, ConflictReason)) -> Self {
ActivateError::Conflict(t.0, t.1)
}
}

impl ConflictReason {
fn is_links(&self) -> bool {
match *self {
ConflictReason::Semver => false,
ConflictReason::Links(_) => true,
if let ConflictReason::Links(_) = *self {
return true;
}
false
}

fn is_missing_features(&self) -> bool {
if let ConflictReason::MissingFeatures(_) = *self {
return true;
}
false
}
}

Expand All @@ -556,6 +582,7 @@ struct BacktrackFrame<'a> {
parent: Summary,
dep: Dependency,
features: Rc<Vec<String>>,
conflicting_activations: HashMap<PackageId, ConflictReason>,
}

#[derive(Clone)]
Expand Down Expand Up @@ -652,9 +679,17 @@ fn activate_deps_loop<'a>(
summary: summary.clone(),
replace: None,
};
let res = activate(&mut cx, registry, None, candidate, method)?;
if let Some((frame, _)) = res {
remaining_deps.push(frame);
let res = activate(&mut cx, registry, None, candidate, method);
match res {
Ok(Some((frame, _))) => remaining_deps.push(frame),
Ok(None) => (),
Err(ActivateError::Error(e)) => return Err(e),
Err(ActivateError::Conflict(id, reason)) => {
match reason {
ConflictReason::MissingFeatures(features) => bail!("Package `{}` does not have these features: `{}`", id, features),
_ => panic!("bad error from activate"),
}
}
}
}

Expand Down Expand Up @@ -710,74 +745,96 @@ fn activate_deps_loop<'a>(

trace!("{}[{}]>{} {} candidates", parent.name(), cur, dep.name(), candidates.len());
trace!("{}[{}]>{} {} prev activations", parent.name(), cur, dep.name(), cx.prev_active(&dep).len());
let mut remaining_candidates = RemainingCandidates::new(&candidates);
let next = remaining_candidates.next(cx.prev_active(&dep), &cx.links);

// Alright, for each candidate that's gotten this far, it meets the
// following requirements:
//
// 1. The version matches the dependency requirement listed for this
// package
// 2. There are no activated versions for this package which are
// semver/links-compatible, or there's an activated version which is
// precisely equal to `candidate`.
//
// This means that we're going to attempt to activate each candidate in
// turn. We could possibly fail to activate each candidate, so we try
// each one in turn.
let (candidate, has_another) = next.or_else(|mut conflicting| {
// This dependency has no valid candidate. Backtrack until we
// find a dependency that does have a candidate to try, and try
// to activate that one. This resets the `remaining_deps` to
// their state at the found level of the `backtrack_stack`.
trace!("{}[{}]>{} -- no candidates", parent.name(), cur, dep.name());
find_candidate(
&mut backtrack_stack,
&mut cx,
&mut remaining_deps,
&mut parent,
&mut cur,
&mut dep,
&mut features,
&mut remaining_candidates,
&mut conflicting,
).ok_or_else(|| {
activation_error(
&cx,
registry,
&parent,
&dep,
&conflicting,
&candidates,
config,
)
})
})?;
let mut remaining_candidates = RemainingCandidates::new(&candidates);
let mut successfully_activated = false;
let mut conflicting_activations = HashMap::new();

while !successfully_activated {
let next = remaining_candidates.next(cx.prev_active(&dep), &cx.links);

// Alright, for each candidate that's gotten this far, it meets the
// following requirements:
//
// 1. The version matches the dependency requirement listed for this
// package
// 2. There are no activated versions for this package which are
// semver/links-compatible, or there's an activated version which is
// precisely equal to `candidate`.
//
// This means that we're going to attempt to activate each candidate in
// turn. We could possibly fail to activate each candidate, so we try
// each one in turn.
let (candidate, has_another) = next.or_else(|conflicting| {
conflicting_activations.extend(conflicting);
// This dependency has no valid candidate. Backtrack until we
// find a dependency that does have a candidate to try, and try
// to activate that one. This resets the `remaining_deps` to
// their state at the found level of the `backtrack_stack`.
trace!("{}[{}]>{} -- no candidates", parent.name(), cur, dep.name());
find_candidate(
&mut backtrack_stack,
&mut cx,
&mut remaining_deps,
&mut parent,
&mut cur,
&mut dep,
&mut features,
&mut remaining_candidates,
&mut conflicting_activations,
).ok_or_else(|| {
// if we hit an activation error and we are out of other combinations
// then just report that error
activation_error(
&cx,
registry,
&parent,
&dep,
&conflicting_activations,
&candidates,
config,
)
})
})?;

// We have a candidate. Add an entry to the `backtrack_stack` so
// we can try the next one if this one fails.
if has_another {
backtrack_stack.push(BacktrackFrame {
// We have a candidate. Clone a `BacktrackFrame`
// so we can add it to the `backtrack_stack` if activation succeeds.
// We clone now in case `activate` changes `cx` and then fails.
let backtrack = BacktrackFrame {
cur,
context_backup: Context::clone(&cx),
deps_backup: <BinaryHeap<DepsFrame>>::clone(&remaining_deps),
remaining_candidates,
remaining_candidates: remaining_candidates.clone(),
parent: Summary::clone(&parent),
dep: Dependency::clone(&dep),
features: Rc::clone(&features),
});
}
conflicting_activations: conflicting_activations.clone(),
};

let method = Method::Required {
dev_deps: false,
features: &features,
uses_default_features: dep.uses_default_features(),
};
trace!("{}[{}]>{} trying {}", parent.name(), cur, dep.name(), candidate.summary.version());
let res = activate(&mut cx, registry, Some(&parent), candidate, &method)?;
if let Some((frame, dur)) = res {
remaining_deps.push(frame);
deps_time += dur;
let method = Method::Required {
dev_deps: false,
features: &features,
uses_default_features: dep.uses_default_features(),
};
trace!("{}[{}]>{} trying {}", parent.name(), cur, dep.name(), candidate.summary.version());
let res = activate(&mut cx, registry, Some(&parent), candidate, &method);
successfully_activated = res.is_ok();

match res {
Ok(Some((frame, dur))) => {
remaining_deps.push(frame);
deps_time += dur;
}
Ok(None) => (),
Err(ActivateError::Error(e)) => return Err(e),
Err(ActivateError::Conflict(id, reason)) => { conflicting_activations.insert(id, reason); },
}

// Add an entry to the `backtrack_stack` so
// we can try the next one if this one fails.
if has_another && successfully_activated {
backtrack_stack.push(backtrack);
}
}
}

Expand Down Expand Up @@ -824,6 +881,7 @@ fn find_candidate<'a>(
*dep = frame.dep;
*features = frame.features;
*remaining_candidates = frame.remaining_candidates;
*conflicting_activations = frame.conflicting_activations;
return Some((candidate, has_another));
}
}
Expand Down Expand Up @@ -865,9 +923,9 @@ fn activation_error(cx: &Context,

let mut conflicting_activations: Vec<_> = conflicting_activations.iter().collect();
conflicting_activations.sort_unstable();
let (links_errors, other_errors): (Vec<_>, Vec<_>) = conflicting_activations.drain(..).rev().partition(|&(_, r)| r.is_links());
let (links_errors, mut other_errors): (Vec<_>, Vec<_>) = conflicting_activations.drain(..).rev().partition(|&(_, r)| r.is_links());

for &(p, r) in &links_errors {
for &(p, r) in links_errors.iter() {
if let ConflictReason::Links(ref link) = *r {
msg.push_str("\n\nthe package `");
msg.push_str(dep.name());
Expand All @@ -880,12 +938,27 @@ fn activation_error(cx: &Context,
msg.push_str(&describe_path(p));
}

let (features_errors, other_errors): (Vec<_>, Vec<_>) = other_errors.drain(..).partition(|&(_, r)| r.is_missing_features());

for &(p, r) in features_errors.iter() {
if let ConflictReason::MissingFeatures(ref features) = *r {
msg.push_str("\n\nthe package `");
msg.push_str(dep.name());
msg.push_str("` depends on `");
msg.push_str(p.name());
msg.push_str("`, with features: `");
msg.push_str(features);
msg.push_str("` but it does not have these features.\n");
}
msg.push_str(&describe_path(p));
}

if links_errors.is_empty() {
msg.push_str("\n\nall possible versions conflict with \
previously selected packages.");
}

for &(p, _) in &other_errors {
for &(p, _) in other_errors.iter() {
msg.push_str("\n\n previously selected ");
msg.push_str(&describe_path(p));
}
Expand Down Expand Up @@ -1148,7 +1221,7 @@ impl<'a> Context<'a> {
fn build_deps(&mut self,
registry: &mut Registry,
candidate: &Summary,
method: &Method) -> CargoResult<Vec<DepInfo>> {
method: &Method) -> ActivateResult<Vec<DepInfo>> {
// First, figure out our set of dependencies based on the requested set
// of features. This also calculates what features we're going to enable
// for our own dependencies.
Expand Down Expand Up @@ -1265,7 +1338,7 @@ impl<'a> Context<'a> {
fn resolve_features<'b>(&mut self,
s: &'b Summary,
method: &'b Method)
-> CargoResult<Vec<(Dependency, Vec<String>)>> {
-> ActivateResult<Vec<(Dependency, Vec<String>)>> {
let dev_deps = match *method {
Method::Everything => true,
Method::Required { dev_deps, .. } => dev_deps,
Expand Down Expand Up @@ -1300,7 +1373,7 @@ impl<'a> Context<'a> {
base.extend(dep.features().iter().cloned());
for feature in base.iter() {
if feature.contains('/') {
bail!("feature names may not contain slashes: `{}`", feature);
return Err(format_err!("feature names may not contain slashes: `{}`", feature).into());
}
}
ret.push((dep.clone(), base));
Expand All @@ -1314,8 +1387,7 @@ impl<'a> Context<'a> {
.map(|s| &s[..])
.collect::<Vec<&str>>();
let features = unknown.join(", ");
bail!("Package `{}` does not have these features: `{}`",
s.package_id(), features)
return Err((s.package_id().clone(), ConflictReason::MissingFeatures(features)))?;
}

// Record what list of features is active for this package.
Expand Down
19 changes: 13 additions & 6 deletions tests/testsuite/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,17 @@ fn invalid4() {

assert_that(p.cargo("build"),
execs().with_status(101).with_stderr("\
[ERROR] Package `bar v0.0.1 ([..])` does not have these features: `bar`
"));
error: failed to select a version for `bar`.
... required by package `foo v0.0.1 ([..])`
versions that meet the requirements `*` are: 0.0.1

the package `bar` depends on `bar`, with features: `bar` but it does not have these features.
Copy link
Member

@alexcrichton alexcrichton Mar 1, 2018

Choose a reason for hiding this comment

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

Hm shouldn't this read "the package foo" instead of "the package bar"?

Copy link
Contributor Author

@Eh2406 Eh2406 Mar 1, 2018

Choose a reason for hiding this comment

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

Yes, and it is fixed. Thanks. (Sorry about all the force pushing.)

package `bar v0.0.1 ([..])`
... which is depended on by `foo v0.0.1 ([..])`

all possible versions conflict with previously selected packages.
Copy link
Member

@alexcrichton alexcrichton Mar 1, 2018

Choose a reason for hiding this comment

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

Would this clause be possible to remove in this scenario? I think it's more applicable to version conflicts than feature conflicts, right?

Copy link
Contributor Author

@Eh2406 Eh2406 Mar 1, 2018

Choose a reason for hiding this comment

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

Removed


failed to select a version for `bar` which could resolve this conflict"));

p.change_file("Cargo.toml", r#"
[project]
Expand All @@ -121,8 +130,7 @@ fn invalid4() {

assert_that(p.cargo("build").arg("--features").arg("test"),
execs().with_status(101).with_stderr("\
[ERROR] Package `foo v0.0.1 ([..])` does not have these features: `test`
"));
error: Package `foo v0.0.1 ([..])` does not have these features: `test`"));
}

#[test]
Expand Down Expand Up @@ -1052,8 +1060,7 @@ fn dep_feature_in_cmd_line() {
// Trying to enable features of transitive dependencies is an error
assert_that(p.cargo("build").arg("--features").arg("bar/some-feat"),
execs().with_status(101).with_stderr("\
[ERROR] Package `foo v0.0.1 ([..])` does not have these features: `bar`
"));
error: Package `foo v0.0.1 ([..])` does not have these features: `bar`"));

// Hierarchical feature specification should still be disallowed
assert_that(p.cargo("build").arg("--features").arg("derived/bar/some-feat"),
Expand Down
Loading