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.

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

Try to improve "version not found" error #6112

Merged
merged 1 commit into from
Oct 3, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions src/cargo/core/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ pub trait Registry {
self.query(dep, &mut |s| ret.push(s), fuzzy)?;
Ok(ret)
}

fn describe_source(&self, source: &SourceId) -> String;
fn is_replaced(&self, source: &SourceId) -> bool;
}

/// This structure represents a registry of known packages. It internally
Expand Down Expand Up @@ -528,6 +531,20 @@ impl<'cfg> Registry for PackageRegistry<'cfg> {
f(self.lock(override_summary));
Ok(())
}

fn describe_source(&self, id: &SourceId) -> String {
match self.sources.get(id) {
Some(src) => src.describe(),
None => id.to_string(),
}
}

fn is_replaced(&self, id: &SourceId) -> bool {
match self.sources.get(id) {
Some(src) => src.is_replaced(),
None => false,
}
}
}

fn lock(locked: &LockedMap, patches: &HashMap<Url, Vec<PackageId>>, summary: Summary) -> Summary {
Expand Down
16 changes: 10 additions & 6 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -933,13 +933,13 @@ fn activation_error(
};

let mut msg = format!(
"no matching version `{}` found for package `{}`\n\
location searched: {}\n\
versions found: {}\n",
dep.version_req(),
"failed to select a version for the requirement `{} = \"{}\"`\n \
candidate versions found which didn't match: {}\n \
location searched: {}\n",
dep.package_name(),
dep.source_id(),
versions
dep.version_req(),
versions,
registry.describe_source(dep.source_id()),
);
msg.push_str("required by ");
msg.push_str(&describe_path(&graph.path_to_top(parent.package_id())));
Expand All @@ -954,6 +954,10 @@ fn activation_error(
);
}

if registry.is_replaced(dep.source_id()) {
msg.push_str("\nperhaps a crate was updated and forgotten to be re-vendored?");
}

msg
} else {
// Maybe the user mistyped the name? Like `dep-thing` when `Dep_Thing`
Expand Down
25 changes: 25 additions & 0 deletions src/cargo/core/source/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,15 @@ pub trait Source {
fn verify(&self, _pkg: &PackageId) -> CargoResult<()> {
Ok(())
}

/// Describes this source in a human readable fashion, used for display in
/// resolver error messages currently.
fn describe(&self) -> String;

/// Returns whether a source is being replaced by another here
fn is_replaced(&self) -> bool {
false
}
}

pub enum MaybePackage {
Expand Down Expand Up @@ -139,6 +148,14 @@ impl<'a, T: Source + ?Sized + 'a> Source for Box<T> {
fn verify(&self, pkg: &PackageId) -> CargoResult<()> {
(**self).verify(pkg)
}

fn describe(&self) -> String {
(**self).describe()
}

fn is_replaced(&self) -> bool {
(**self).is_replaced()
}
}

impl<'a, T: Source + ?Sized + 'a> Source for &'a mut T {
Expand Down Expand Up @@ -185,6 +202,14 @@ impl<'a, T: Source + ?Sized + 'a> Source for &'a mut T {
fn verify(&self, pkg: &PackageId) -> CargoResult<()> {
(**self).verify(pkg)
}

fn describe(&self) -> String {
(**self).describe()
}

fn is_replaced(&self) -> bool {
(**self).is_replaced()
}
}

/// A `HashMap` of `SourceId` -> `Box<Source>`
Expand Down
4 changes: 4 additions & 0 deletions src/cargo/sources/directory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,4 +212,8 @@ impl<'cfg> Source for DirectorySource<'cfg> {

Ok(())
}

fn describe(&self) -> String {
format!("directory source `{}`", self.root.display())
}
}
4 changes: 4 additions & 0 deletions src/cargo/sources/git/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ impl<'cfg> Source for GitSource<'cfg> {
fn fingerprint(&self, _pkg: &Package) -> CargoResult<String> {
Ok(self.rev.as_ref().unwrap().to_string())
}

fn describe(&self) -> String {
format!("git repository {}", self.source_id)
}
}

#[cfg(test)]
Expand Down
7 changes: 7 additions & 0 deletions src/cargo/sources/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -558,4 +558,11 @@ impl<'cfg> Source for PathSource<'cfg> {
let (max, max_path) = self.last_modified_file(pkg)?;
Ok(format!("{} ({})", max, max_path.display()))
}

fn describe(&self) -> String {
match self.source_id.url().to_file_path() {
Ok(path) => path.display().to_string(),
Err(_) => self.source_id.to_string(),
}
}
}
4 changes: 4 additions & 0 deletions src/cargo/sources/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -585,4 +585,8 @@ impl<'cfg> Source for RegistrySource<'cfg> {
fn fingerprint(&self, pkg: &Package) -> CargoResult<String> {
Ok(pkg.package_id().version().to_string())
}

fn describe(&self) -> String {
self.source_id.display_registry()
}
}
8 changes: 8 additions & 0 deletions src/cargo/sources/replaced.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,12 @@ impl<'cfg> Source for ReplacedSource<'cfg> {
let id = id.with_source_id(&self.replace_with);
self.inner.verify(&id)
}

fn describe(&self) -> String {
format!("{} (which is replacing {})", self.inner.describe(), self.to_replace)
}

fn is_replaced(&self) -> bool {
true
}
}
42 changes: 42 additions & 0 deletions tests/testsuite/directory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -683,3 +683,45 @@ fn workspace_different_locations() {
",
).run();
}

#[test]
fn version_missing() {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general the error messages are not nearly adequately tested. I'd like one day to be sure we have all the code branches tested. Thank you for adding this test!

setup();

VendorPackage::new("foo")
.file("src/lib.rs", "pub fn foo() {}")
.build();

VendorPackage::new("bar")
.file(
"Cargo.toml",
r#"
[package]
name = "bar"
version = "0.1.0"
authors = []

[dependencies]
foo = "2"
"#,
)
.file("src/main.rs", "fn main() {}")
.build();

cargo_process("install bar")
.with_stderr(
"\
[INSTALLING] bar v0.1.0
error: failed to compile [..]

Caused by:
failed to select a version for the requirement `foo = \"^2\"`
candidate versions found which didn't match: 0.0.1
location searched: directory source `[..] (which is replacing registry `[..]`)
required by package `bar v0.1.0`
perhaps a crate was updated and forgotten to be re-vendored?
",
)
.with_status(101)
.run();
}
19 changes: 10 additions & 9 deletions tests/testsuite/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,9 @@ fn wrong_version() {
.with_status(101)
.with_stderr_contains(
"\
error: no matching version `>= 1.0.0` found for package `foo`
location searched: registry [..]
versions found: 0.0.2, 0.0.1
error: failed to select a version for the requirement `foo = \">= 1.0.0\"`
candidate versions found which didn't match: 0.0.2, 0.0.1
location searched: `[..]` index (which is replacing registry `[..]`)
required by package `foo v0.0.1 ([..])`
",
).run();
Expand All @@ -233,9 +233,9 @@ required by package `foo v0.0.1 ([..])`
.with_status(101)
.with_stderr_contains(
"\
error: no matching version `>= 1.0.0` found for package `foo`
location searched: registry [..]
versions found: 0.0.4, 0.0.3, 0.0.2, ...
error: failed to select a version for the requirement `foo = \">= 1.0.0\"`
candidate versions found which didn't match: 0.0.4, 0.0.3, 0.0.2, ...
location searched: `[..]` index (which is replacing registry `[..]`)
required by package `foo v0.0.1 ([..])`
",
).run();
Expand Down Expand Up @@ -520,10 +520,11 @@ fn relying_on_a_yank_is_bad() {
.with_status(101)
.with_stderr_contains(
"\
error: no matching version `= 0.0.2` found for package `baz`
location searched: registry `[..]`
versions found: 0.0.1
error: failed to select a version for the requirement `baz = \"= 0.0.2\"`
candidate versions found which didn't match: 0.0.1
location searched: `[..]` index (which is replacing registry `[..]`)
required by package `bar v0.0.1`
... which is depended on by `foo [..]`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change? I am not objecting, just mechanically, what changed making us print which is depended on by?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I just expanded the error message a bit here, only a subset of the output was previously tested with with_stderr_contains (vs with_stderr)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. 👍

",
).run();
}
Expand Down
12 changes: 6 additions & 6 deletions tests/testsuite/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ use proptest::prelude::*;

proptest! {
#![proptest_config(ProptestConfig {
cases:
if env::var("CI").is_ok() {
256 // we have a lot of builds in CI so one or another of them will find problems
} else {
1024 // but locally try and find it in the one build
},
// Note that this is a little low in terms of cases we'd like to test,
// but this number affects how long this function takes. It can be
// increased locally to execute more tests and try to find more bugs,
// but for now it's semi-low to run in a small-ish amount of time on CI
// and locally.
cases: 256,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you choose to change this?

I assume it was running to slowly locally. If that is correct can we atleast add back in a comment on how running repeatedly or increasing this number will increase the chance of finding a bug at the expense of time?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah I found this was taking ~4m or so on my local machine which made a full test run sort of unbearable unfortunately :(. And sure I'll add a comment!

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. It is new and we are still experimenting with how it fits into the workflow. I am ok with running fewer iterations, for the better ergonomics of running the test.

max_shrink_iters:
if env::var("CI").is_ok() {
// This attempts to make sure that CI will fail fast,
Expand Down
8 changes: 8 additions & 0 deletions tests/testsuite/support/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ pub fn resolve_with_config(
}
Ok(())
}

fn describe_source(&self, _src: &SourceId) -> String {
String::new()
}

fn is_replaced(&self, _src: &SourceId) -> bool {
false
}
}
let mut registry = MyRegistry(registry);
let summary = Summary::new(
Expand Down