From 74390a4ad711414449a331e0fe88c5eb864470da Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 17 Nov 2021 16:58:39 +0800 Subject: [PATCH 1/2] Enhance error message for target auto-discovery Enhance for following scenarios: 1. Target without `path` specified and cannot be found. 2. Target without `path` specified and cannot be found, but a file exists at the commonly wrong path, e.g. `example/a.rs`, `bench/b.rs`. 3. Found multiple candidate files and cannot infer which to use. --- src/cargo/util/toml/targets.rs | 126 ++++++++++++++++++++++++++++++--- 1 file changed, 118 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/toml/targets.rs b/src/cargo/util/toml/targets.rs index 51f367f1ed2..473994b285d 100644 --- a/src/cargo/util/toml/targets.rs +++ b/src/cargo/util/toml/targets.rs @@ -25,6 +25,11 @@ use crate::util::restricted_names; use anyhow::Context as _; +const DEFAULT_TEST_DIR_NAME: &'static str = "tests"; +const DEFAULT_BENCH_DIR_NAME: &'static str = "benches"; +const DEFAULT_EXAMPLE_DIR_NAME: &'static str = "examples"; +const DEFAULT_BIN_DIR_NAME: &'static str = "bin"; + pub fn targets( features: &Features, manifest: &TomlManifest, @@ -353,7 +358,10 @@ fn clean_bins( return Some(path); } - let path = package_root.join("src").join("bin").join("main.rs"); + let path = package_root + .join("src") + .join(DEFAULT_BIN_DIR_NAME) + .join("main.rs"); if path.exists() { return Some(path); } @@ -370,7 +378,7 @@ fn clean_examples( warnings: &mut Vec, errors: &mut Vec, ) -> CargoResult> { - let inferred = infer_from_directory(&package_root.join("examples")); + let inferred = infer_from_directory(&package_root.join(DEFAULT_EXAMPLE_DIR_NAME)); let targets = clean_targets( "example", @@ -415,7 +423,7 @@ fn clean_tests( warnings: &mut Vec, errors: &mut Vec, ) -> CargoResult> { - let inferred = infer_from_directory(&package_root.join("tests")); + let inferred = infer_from_directory(&package_root.join(DEFAULT_TEST_DIR_NAME)); let targets = clean_targets( "test", @@ -590,7 +598,9 @@ fn inferred_bins(package_root: &Path, package_name: &str) -> Vec<(String, PathBu if main.exists() { result.push((package_name.to_string(), main)); } - result.extend(infer_from_directory(&package_root.join("src").join("bin"))); + result.extend(infer_from_directory( + &package_root.join("src").join(DEFAULT_BIN_DIR_NAME), + )); result } @@ -812,6 +822,90 @@ fn configure(features: &Features, toml: &TomlTarget, target: &mut Target) -> Car Ok(()) } +/// Build an error message for a target path that cannot be determined either +/// by auto-discovery or specifiying. +/// +/// This function tries to detect commonly wrong paths for targets: +/// +/// test -> tests/*.rs, tests/*/main.rs +/// bench -> benches/*.rs, benches/*/main.rs +/// example -> examples/*.rs, examples/*/main.rs +/// bin -> src/bin/*.rs, src/bin/*/main.rs +/// +/// Note that the logic need to sync with [`infer_from_directory`] if changes. +fn target_path_not_found_error_message( + package_root: &Path, + target: &TomlTarget, + target_kind: &str, +) -> String { + fn possible_target_paths(name: &str, kind: &str, commonly_wrong: bool) -> [PathBuf; 2] { + let mut target_path = PathBuf::new(); + match (kind, commonly_wrong) { + // commonly wrong paths + ("test" | "bench" | "example", true) => target_path.push(kind), + ("bin", true) => { + target_path.push("src"); + target_path.push("bins"); + } + // default inferred paths + ("test", false) => target_path.push(DEFAULT_TEST_DIR_NAME), + ("bench", false) => target_path.push(DEFAULT_BENCH_DIR_NAME), + ("example", false) => target_path.push(DEFAULT_EXAMPLE_DIR_NAME), + ("bin", false) => { + target_path.push("src"); + target_path.push(DEFAULT_BIN_DIR_NAME); + } + _ => unreachable!("invalid target kind: {}", kind), + } + target_path.push(name); + + let target_path_file = { + let mut path = target_path.clone(); + path.set_extension("rs"); + path + }; + let target_path_subdir = { + target_path.push("main.rs"); + target_path + }; + return [target_path_file, target_path_subdir]; + } + + let target_name = target.name(); + let commonly_wrong_paths = possible_target_paths(&target_name, target_kind, true); + let possible_paths = possible_target_paths(&target_name, target_kind, false); + let existing_wrong_path_index = match ( + package_root.join(&commonly_wrong_paths[0]).exists(), + package_root.join(&commonly_wrong_paths[1]).exists(), + ) { + (true, _) => Some(0), + (_, true) => Some(1), + _ => None, + }; + + if let Some(i) = existing_wrong_path_index { + return format!( + "\ +can't find `{name}` {kind} at default paths, but found a file at `{wrong_path}`. +Perhaps rename the file to `{possible_path}` for target auto-discovery, \ +or specify {kind}.path if you want to use a non-default path.", + name = target_name, + kind = target_kind, + wrong_path = commonly_wrong_paths[i].display(), + possible_path = possible_paths[i].display(), + ); + } + + format!( + "can't find `{name}` {kind} at `{path_file}` or `{path_dir}`. \ + Please specify {kind}.path if you want to use a non-default path.", + name = target_name, + kind = target_kind, + path_file = possible_paths[0].display(), + path_dir = possible_paths[1].display(), + ) +} + fn target_path( target: &TomlTarget, inferred: &[(String, PathBuf)], @@ -835,16 +929,32 @@ fn target_path( let second = matching.next(); match (first, second) { (Some(path), None) => Ok(path), - (None, None) | (Some(_), Some(_)) => { + (None, None) => { + if edition == Edition::Edition2015 { + if let Some(path) = legacy_path(target) { + return Ok(path); + } + } + Err(target_path_not_found_error_message( + package_root, + target, + target_kind, + )) + } + (Some(p0), Some(p1)) => { if edition == Edition::Edition2015 { if let Some(path) = legacy_path(target) { return Ok(path); } } Err(format!( - "can't find `{name}` {target_kind}, specify {target_kind}.path", - name = name, - target_kind = target_kind + "\ +cannot infer path for `{}` {} +Cargo doesn't know which to use because multiple target files found at `{}` and `{}`.", + target.name(), + target_kind, + p0.strip_prefix(package_root).unwrap_or(&p0).display(), + p1.strip_prefix(package_root).unwrap_or(&p1).display(), )) } (None, Some(_)) => unreachable!(), From 2698bc6a42b7212a2549a8b05b536c1a49e53ced Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Wed, 17 Nov 2021 17:16:13 +0800 Subject: [PATCH 2/2] Test for target auto-discovery error enhancement --- tests/testsuite/build.rs | 259 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 253 insertions(+), 6 deletions(-) diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 8aff41bb96b..7646b72e3a8 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -1899,6 +1899,40 @@ fn explicit_examples() { .run(); } +#[cargo_test] +fn non_existing_test() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "1.0.0" + + [lib] + name = "foo" + path = "src/lib.rs" + + [[test]] + name = "hello" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("build --tests -v") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + can't find `hello` test at `tests/hello.rs` or `tests/hello/main.rs`. \ + Please specify test.path if you want to use a non-default path.", + ) + .run(); +} + #[cargo_test] fn non_existing_example() { let p = project() @@ -1908,7 +1942,6 @@ fn non_existing_example() { [package] name = "foo" version = "1.0.0" - authors = [] [lib] name = "foo" @@ -1921,14 +1954,49 @@ fn non_existing_example() { .file("src/lib.rs", "") .build(); - p.cargo("test -v") + p.cargo("build --examples -v") .with_status(101) .with_stderr( "\ [ERROR] failed to parse manifest at `[..]` Caused by: - can't find `hello` example, specify example.path", + can't find `hello` example at `examples/hello.rs` or `examples/hello/main.rs`. \ + Please specify example.path if you want to use a non-default path.", + ) + .run(); +} + +#[cargo_test] +fn non_existing_benchmark() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "1.0.0" + + [lib] + name = "foo" + path = "src/lib.rs" + + [[bench]] + name = "hello" + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("build --benches -v") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + can't find `hello` bench at `benches/hello.rs` or `benches/hello/main.rs`. \ + Please specify bench.path if you want to use a non-default path.", ) .run(); } @@ -1948,7 +2016,184 @@ fn non_existing_binary() { [ERROR] failed to parse manifest at `[..]` Caused by: - can't find `foo` bin, specify bin.path", + can't find `foo` bin at `src/bin/foo.rs` or `src/bin/foo/main.rs`. \ + Please specify bin.path if you want to use a non-default path.", + ) + .run(); +} + +#[cargo_test] +fn commonly_wrong_path_of_test() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "1.0.0" + + [lib] + name = "foo" + path = "src/lib.rs" + + [[test]] + name = "foo" + "#, + ) + .file("src/lib.rs", "") + .file("test/foo.rs", "") + .build(); + + p.cargo("build --tests -v") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + can't find `foo` test at default paths, but found a file at `test/foo.rs`. + Perhaps rename the file to `tests/foo.rs` for target auto-discovery, \ + or specify test.path if you want to use a non-default path.", + ) + .run(); +} + +#[cargo_test] +fn commonly_wrong_path_of_example() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "1.0.0" + + [lib] + name = "foo" + path = "src/lib.rs" + + [[example]] + name = "foo" + "#, + ) + .file("src/lib.rs", "") + .file("example/foo.rs", "") + .build(); + + p.cargo("build --examples -v") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + can't find `foo` example at default paths, but found a file at `example/foo.rs`. + Perhaps rename the file to `examples/foo.rs` for target auto-discovery, \ + or specify example.path if you want to use a non-default path.", + ) + .run(); +} + +#[cargo_test] +fn commonly_wrong_path_of_benchmark() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "1.0.0" + + [lib] + name = "foo" + path = "src/lib.rs" + + [[bench]] + name = "foo" + "#, + ) + .file("src/lib.rs", "") + .file("bench/foo.rs", "") + .build(); + + p.cargo("build --benches -v") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + can't find `foo` bench at default paths, but found a file at `bench/foo.rs`. + Perhaps rename the file to `benches/foo.rs` for target auto-discovery, \ + or specify bench.path if you want to use a non-default path.", + ) + .run(); +} + +#[cargo_test] +fn commonly_wrong_path_binary() { + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file("src/lib.rs", "") + .file("src/bins/foo.rs", "") + .build(); + + p.cargo("build -v") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + can't find `foo` bin at default paths, but found a file at `src/bins/foo.rs`. + Perhaps rename the file to `src/bin/foo.rs` for target auto-discovery, \ + or specify bin.path if you want to use a non-default path.", + ) + .run(); +} + +#[cargo_test] +fn commonly_wrong_path_subdir_binary() { + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file("src/lib.rs", "") + .file("src/bins/foo/main.rs", "") + .build(); + + p.cargo("build -v") + .with_status(101) + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + can't find `foo` bin at default paths, but found a file at `src/bins/foo/main.rs`. + Perhaps rename the file to `src/bin/foo/main.rs` for target auto-discovery, \ + or specify bin.path if you want to use a non-default path.", + ) + .run(); +} + +#[cargo_test] +fn found_multiple_target_files() { + let p = project() + .file("Cargo.toml", &basic_bin_manifest("foo")) + .file("src/lib.rs", "") + .file("src/bin/foo.rs", "") + .file("src/bin/foo/main.rs", "") + .build(); + + p.cargo("build -v") + .with_status(101) + // Don't assert the inferred pathes since the order is non-deterministic. + .with_stderr( + "\ +[ERROR] failed to parse manifest at `[..]` + +Caused by: + cannot infer path for `foo` bin + Cargo doesn't know which to use because multiple target files found \ + at `src/bin/foo[..].rs` and `src/bin/foo[..].rs`.", ) .run(); } @@ -4319,7 +4564,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 `foo` bin at `src/bin/foo.rs` or `src/bin/foo/main.rs`. [..]", ) .run(); } @@ -4529,7 +4774,9 @@ 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 `a_bin` bin at `src/bin/a_bin.rs` or `src/bin/a_bin/main.rs`[..]", + ) .run(); }