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

Improve error messages for wrong target paths #9116

Closed
wants to merge 2 commits into from
Closed

Improve error messages for wrong target paths #9116

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
9ae5239
Improve error messages for wrong target paths
CPerezz Jan 31, 2021
5ba1412
Update & Add tests for wrong target paths
CPerezz Jan 31, 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
12 changes: 10 additions & 2 deletions src/cargo/util/toml/targets.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,10 +826,18 @@ fn target_path(
return Ok(path);
}
}
let target_folder_name = match target_kind {
"test" => "tests",
"bench" => "benches",
"example" => "examples",
"bins" => "bin",
_ => target_kind,
};
Err(format!(
"can't find `{name}` {target_kind}, specify {target_kind}.path",
"can't find `{name}` {target_kind} at `{target_folder_name}/{name}`, specify {target_kind}.path if you want to use a non-default path",
Copy link
Member

Choose a reason for hiding this comment

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

Could this perhaps use tthe inferred list at the top to generate a precise list of candidates attempted? Otherwise this misses out that either benches/foo.rs or benches/foo/main.rs could exist. (e.g. benches/foo could exist when this says it doesn't b/c you happened to be missing benches/foo/main.rs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @alexcrichton Thanks for the catch!! I didn't thought about that!
There's only one problem. This slice is constructed here

And as you see, infer_from_directory you send the benches dir to the fn. And obviously, that resolves into an empty array because the bench is located under bench/ and not benches.

I don't really see how to tackle that. Since we need to "guess" which is the wrong path. And if we parse from package_root looking for similar names to benches, we might end up in undesired places.

Which is your suggestion?

If you want to clarify even more, We can go for a suggestion message below the err saying something like:

Please, make sure to name your target folder correctly. \
For {target_kind} the folder should be called `{target_folder_name}/..`

Copy link
Member

Choose a reason for hiding this comment

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

Ah that's a good point. I think there will need to be some refactoring for this in any case though. The error message is already slightly incorrect in that if we found two candidates it says that something wasn't found when actually we found two things. Similarly I'm worried about the catch-all _ => target_kind here since that may produce incorrect messages trying to look in a folder like lib/foo for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't IIUC. The catch all is precisely what handles lib to be matched to lib itself. The others are the ones which can contain wrong folder names. But maybe I'm missing something.

We could parse the top level of the workspace looking for example, test, bins or bench folders. If that's the case, we try to extract inferred from there and we will be able to formulate a better error message. Otherways. I really don;t know what we can do. Since literally the path could be whatever.

Do you have any similar situations in other places of the crate? Do you have any suggestions on what to go for?

Copy link
Member

Choose a reason for hiding this comment

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

There is a set of paths that Cargo probes for for targets which don't list a path. That set I think should be refactored to be able to be passed in here explicitly so this doesn't have to reconstruct or re-infer what the missing paths were. I think that refactoring would solve all of my concerns here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation Alex. Could you point where Cargo probes for for targets which don't list a path? I've been looking across the repo for something similar to a Layout structure where these targets without listed path are treated or stored and haven't seen it.

I checked from the creation of the Workspace till the execution of the benches and nothing seemed to me like was cheking this "missed" targets.

That would be really helpful for me so I can make another PR to refactor this and then, use that so that the error message gets improved.
I can also include the refactor here, whatever you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the probe logic is in the functions in this module (inferred_bins, inferred_lib, infer_from_directory). I think the refactoring would be to change those to return a list of potential candidates (essentially, remove the checks for path.exists()), and then the code that creates the targets (toml_targets_and_inferred I think) will need to check for the first path in that candidate list that exists (something like inferred.iter().find(|p| p.exists())). Then, you can pass that list of candidates to target_path so that it can include them in the error message.

It may need to be careful since those paths are absolute. I'm not sure how that should be displayed, but maybe strip the current directory from the prefix?

name = name,
target_kind = target_kind
target_kind = target_kind,
target_folder_name = target_folder_name,
))
}
(None, Some(_)) => unreachable!(),
Expand Down
51 changes: 51 additions & 0 deletions tests/testsuite/bench.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1755,3 +1755,54 @@ fn json_artifact_includes_executable_for_benchmark() {
)
.run();
}

#[cargo_test]
fn external_bench_warns_wrong_path() {
if !is_nightly() {
return;
}

let p = project()
.file(
"Cargo.toml",
r#"
[project]
name = "foo"
version = "0.0.1"
authors = []

[[bench]]
name = "external"
"#,
)
.file(
"src/lib.rs",
r#"
#![feature(test)]
#[cfg(test)]
extern crate test;

pub fn get_hello() -> &'static str { "Hello" }
"#,
)
.file(
"./bench/external.rs",
r#"
#![feature(test)]
#[allow(unused_extern_crates)]
extern crate foo;
extern crate test;

#[bench]
fn external_bench(_b: &mut test::Bencher) {()}
"#,
)
.build();

p.cargo("bench")
.with_status(101)
.with_stderr_contains(
"[..]can't find `[..]` bench at `[..]`, specify [..] if you want to use a non-default path",
)
.run();
}
34 changes: 29 additions & 5 deletions tests/testsuite/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1849,7 +1849,7 @@ fn non_existing_example() {
[ERROR] failed to parse manifest at `[..]`

Caused by:
can't find `hello` example, specify example.path",
can't find `[..]` example at `[..]`, specify [..] if you want to use a non-default path",
)
.run();
}
Expand All @@ -1869,7 +1869,7 @@ fn non_existing_binary() {
[ERROR] failed to parse manifest at `[..]`

Caused by:
can't find `foo` bin, specify bin.path",
can't find `[..]` bin at `[..]`, specify [..] if you want to use a non-default path",
)
.run();
}
Expand Down Expand Up @@ -4220,7 +4220,7 @@ fn no_bin_in_src_with_lib() {
[ERROR] failed to parse manifest at `[..]`

Caused by:
can't find `foo` bin, specify bin.path",
can't find `[..]` bin at `[..]`, specify [..] if you want to use a non-default path",
)
.run();
}
Expand Down Expand Up @@ -4397,7 +4397,7 @@ fn same_metadata_different_directory() {
}

#[cargo_test]
fn building_a_dependent_crate_witout_bin_should_fail() {
fn building_a_dependent_crate_without_bin_should_fail() {
Package::new("testless", "0.1.0")
.file(
"Cargo.toml",
Expand Down Expand Up @@ -4430,7 +4430,7 @@ fn building_a_dependent_crate_witout_bin_should_fail() {

p.cargo("build")
.with_status(101)
.with_stderr_contains("[..]can't find `a_bin` bin, specify bin.path")
.with_stderr_contains("[..]can't find `[..]` bin at `[..]`, specify [..] if you want to use a non-default path")
.run();
}

Expand Down Expand Up @@ -5438,3 +5438,27 @@ fn primary_package_env_var() {

foo.cargo("test").run();
}

#[cargo_test]
fn using_bins_instead_of_bin_is_warned_in_err() {
let p = project()
.file(
"Cargo.toml",
r#"
[project]
name = "foo"
version = "0.1.0"

[[bin]]
name = "a_bin"
"#,
)
.file("src/lib.rs", "")
.file("bins/a_bin.rs", "fn main { () }")
.build();

p.cargo("build")
.with_status(101)
.with_stderr_contains("[..]can't find `[..]` bin at `[..]`, specify [..] if you want to use a non-default path")
.run();
}