Skip to content
Draft
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
161 changes: 154 additions & 7 deletions src/cargo/core/resolver/errors.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use std::fmt;
use std::fmt::Write as _;
use std::path::{Path, PathBuf};
use std::task::Poll;

use crate::core::{Dependency, PackageId, Registry, Summary};
use crate::sources::IndexSummary;
use crate::core::{Dependency, PackageId, Registry, Shell, SourceId, Summary};
use crate::sources::source::QueryKind;
use crate::sources::{IndexSummary, PathSource, RecursivePathSource};
use crate::util::edit_distance::{closest, edit_distance};
use crate::util::errors::CargoResult;
use crate::util::{GlobalContext, OptVersionReq, VersionExt};
Expand Down Expand Up @@ -392,11 +393,79 @@ pub(super) fn activation_error(
});
let _ = writeln!(&mut msg, "perhaps you meant: {suggestions}");
} else {
let _ = writeln!(
&mut msg,
"no matching package named `{}` found",
dep.package_name()
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This was moved from out of the else block, into it.

if dep.source_id().is_path() {
let path = dep
.source_id()
.url()
.to_file_path()
.expect("[ERROR]: failed to get the path");
Copy link
Contributor

Choose a reason for hiding this comment

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

expects are an assert and should name the condition that is upholding this ("I expect foo to be true")


let global_context = match gctx {
Some(context) => context,
None => &GlobalContext::new(Shell::new(), PathBuf::new(), PathBuf::new()),
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems suspicious for us to create global contexts on the fly

};
let sid = dep.source_id();

let root_package =
inspect_root_package(&mut msg, Path::new(&path), global_context, sid);

let requested = dep.package_name().as_str();
let recursive_packages = inspect_recursive_packages(
&mut msg,
Path::new(&path),
global_context,
sid,
requested,
);

if let Some(name) = root_package {
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if a root package is not found?

let _ = writeln!(
&mut msg,
"no matching package named `{}` found at `{}`",
name.name,
dep.package_name()
Comment on lines +414 to +417
Copy link
Contributor

Choose a reason for hiding this comment

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

Grammatically, this reads in an odd way.

"at" in a situation like this implies a location which would likely to be assumed to be a path but to say "package foo not found in package bar" is odd because they are both packages and a package cannot have packages inside of it.

I would instead recommend having a note naming the package that does exist at a specific path

);

let _ = writeln!(
&mut msg,
"note: required by {}",
describe_path_in_context(resolver_ctx, &parent.package_id()),
);

let _ = writeln!(
&mut msg,
"help: package `{}` exists at `{}`",
name.name, name.name
Comment on lines +426 to +429
Copy link
Contributor

Choose a reason for hiding this comment

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

The at is giving the package name

);
} else if let Some(name) = recursive_packages {
let _ = writeln!(
&mut msg,
"no matching package named `{}` found at `{}`",
Copy link
Contributor

Choose a reason for hiding this comment

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

We sure are repeating this a lot

dep.package_name(),
path.display()
);

let _ = writeln!(
&mut msg,
"note: required by {}",
describe_path_in_context(resolver_ctx, &parent.package_id()),
);

let _ = writeln!(
&mut msg,
"help: package `{}` exists at `{}`",
name.name, name.name
Comment on lines +454 to +457
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

);
} else {
let _ = writeln!(&mut msg, "no root or recursive package name is found");
}
} else {
let _ = writeln!(
&mut msg,
"no matching package named `{}` found",
dep.package_name()
);
}
}

let mut location_searched_msg = registry.describe_source(dep.source_id());
Expand Down Expand Up @@ -575,3 +644,81 @@ pub(crate) fn describe_path<'a>(

String::new()
}

#[derive(Debug)]
struct RootPackageInfo {
pub name: String,
}

fn inspect_root_package(
msg: &mut String,
path: &Path,
gctx: &GlobalContext,
sid: SourceId,
) -> Option<RootPackageInfo> {
Comment on lines +624 to +628
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit unusual to have a function like this also modify msg

let mut ps = PathSource::new(path, sid, gctx);

ps.root_package().expect("failed to get the root");
Copy link
Contributor

Choose a reason for hiding this comment

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

expects are an assert and should name the condition that is upholding this ("I expect foo to be true")


if let Err(e) = ps.load() {
msg.push_str(&e.to_string());
msg.push('\n');
}

let pkg = ps
.read_package()
.expect("failed to read the package in root");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we getting the root package, throwing it away, then calling read_package?


let package_info = RootPackageInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a struct for this?

name: pkg.name().to_string(),
};

return Some(package_info);
}

#[derive(Debug)]
struct RecursivePackageInfo {
name: String,
_path: PathBuf,
}

fn inspect_recursive_packages(
Copy link
Contributor

Choose a reason for hiding this comment

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

this name doesn't say what it is doing

msg: &mut String,
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit unusual to have a function like this also modify msg

path: &Path,
gctx: &GlobalContext,
sid: SourceId,
requested: &str,
) -> Option<RecursivePackageInfo> {
let mut rps = RecursivePathSource::new(path, sid, gctx);

if let Err(e) = rps.load() {
msg.push_str(&e.to_string());
msg.push('\n');
}

let pkgs = rps
.read_packages()
.expect("failed to read the packages recursively");

for pkg in pkgs {
if pkg.name() == requested {
let manifest = pkg.manifest_path();
let pkg_dir = manifest
.parent()
.expect("failed to take the parent path")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto about expect

.to_path_buf();

let package_info = RecursivePackageInfo {
name: pkg.name().to_string(),
_path: pkg_dir,
};
Comment on lines +711 to +714
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a struct?


return Some(package_info);
// } else {
// let list = rps.list_files(&pkg).unwrap();
// println!("{:?}", list);
Comment on lines +717 to +719
Copy link
Contributor

Choose a reason for hiding this comment

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

dead code

}
}

return None;
}
2 changes: 1 addition & 1 deletion src/cargo/sources/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ impl<'gctx> PathSource<'gctx> {
Ok(())
}

fn read_package(&self) -> CargoResult<Package> {
pub fn read_package(&self) -> CargoResult<Package> {
let path = self.path.join("Cargo.toml");
let pkg = ops::read_package(&path, self.source_id, self.gctx)?;
Ok(pkg)
Expand Down
4 changes: 3 additions & 1 deletion tests/testsuite/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1277,7 +1277,9 @@ fn cargo_compile_with_dep_name_mismatch() {
p.cargo("build")
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] no matching package named `notquitebar` found
[ERROR] no matching package named `bar` found at `notquitebar`
[NOTE] required by package `foo v0.0.1 ([ROOT]/foo)`
[HELP] package `bar` exists at `bar`
location searched: [ROOT]/foo/bar
required by package `foo v0.0.1 ([ROOT]/foo)`

Expand Down
92 changes: 92 additions & 0 deletions tests/testsuite/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1919,3 +1919,95 @@ foo v1.0.0 ([ROOT]/foo)
"#]])
.run();
}

#[cargo_test]
fn invalid_package_name_in_path() {
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.5.0"
edition = "2015"
authors = []

[workspace]
Copy link
Member

Choose a reason for hiding this comment

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

Is [workspace] here essential? If not maybe we should remove.


[dependencies]
definitely_not_bar = { path = "crates/bar" }
"#,
)
.file("src/lib.rs", "")
.file(
"crates/bar/Cargo.toml",
r#"
[package]
name = "bar"
version = "0.5.0"
edition = "2015"
authors = []
"#,
)
.file("crates/bar/src/lib.rs", "")
.build();

p.cargo("generate-lockfile")
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] no matching package named `bar` found at `definitely_not_bar`
[NOTE] required by package `foo v0.5.0 ([ROOT]/foo)`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this test is failing. Tests should be atomic which includes tests passing. This should be showing the existing behavior so that the following commit then changes that behavior so we can see the new behavior.

[HELP] package `bar` exists at `bar`
location searched: [ROOT]/foo/crates/bar
required by package `foo v0.5.0 ([ROOT]/foo)`

"#]])
.run();
}

#[cargo_test]
fn invalid_package_in_subdirectory() {
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.5.0"
edition = "2015"
authors = []

[workspace]

[dependencies]
definitely_not_bar = { path = "crates/bar" }
"#,
)
.file("src/lib.rs", "")
.file(
"crates/bar/definitely_not_bar/Cargo.toml",
r#"
[package]
name = "definitely_not_bar"
version = "0.5.0"
edition = "2015"
authors = []
"#,
)
.file("crates/bar/definitely_not_bar/src/lib.rs", "")
.build();

p.cargo("generate-lockfile")
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] failed to load manifest for dependency `definitely_not_bar`

Caused by:
failed to read `[ROOT]/foo/crates/bar/Cargo.toml`

Caused by:
[NOT_FOUND]

"#]])
.run();
}