From c5369e1459858dcd9704f7ce6f6cd54b7d08b052 Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Fri, 17 Nov 2023 15:04:35 +0800 Subject: [PATCH 1/5] Add warning for invalid custom build file --- src/cargo/ops/cargo_package.rs | 41 ++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 6ac09dc771d..df562e36985 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -7,6 +7,7 @@ use std::sync::Arc; use std::task::Poll; use crate::core::compiler::{BuildConfig, CompileMode, DefaultExecutor, Executor}; +use crate::core::manifest::Target; use crate::core::resolver::CliFeatures; use crate::core::{registry::PackageRegistry, resolver::HasDevUnits}; use crate::core::{Feature, Shell, Verbosity, Workspace}; @@ -331,6 +332,29 @@ fn build_ar_list( warn_on_nonexistent_file(&pkg, &readme_path, "readme", &ws)?; } } + + for t in pkg + .manifest() + .targets() + .iter() + .filter(|t| t.is_custom_build()) + { + if let Some(custome_build_path) = t.src_path().path() { + let abs_custome_build_path = + paths::normalize_path(&pkg.root().join(custome_build_path)); + if abs_custome_build_path.is_file() { + if !abs_custome_build_path + .ancestors() + .any(|ancestor| ancestor == pkg.root()) + { + warn_custom_build_file_not_in_package(pkg, &abs_custome_build_path, t, &ws)? + } + } else { + warn_on_nonexistent_file(&pkg, &custome_build_path, "build", &ws)? + } + } + } + result.sort_unstable_by(|a, b| a.rel_path.cmp(&b.rel_path)); Ok(result) @@ -405,6 +429,23 @@ fn warn_on_nonexistent_file( )) } +fn warn_custom_build_file_not_in_package( + pkg: &Package, + path: &Path, + target: &Target, + ws: &Workspace<'_>, +) -> CargoResult<()> { + let msg = format!( + "the source file of {:?} target `{}` doesn't appear to be a path inside of the package.\n\ + It is at {}, whereas the root the package is {}.\n\ + This may cause issue during packaging, as modules resolution and resources included via macros are often relative to the path of source files.\n\ + Please update the `build` setting in the manifest at `{}` and point to a path inside the root of the package.\n", + target.kind(), target.name(), path.display(), pkg.root().display(), pkg.manifest_path().display() + ); + + ws.config().shell().warn(&msg) +} + /// Construct `Cargo.lock` for the package to be published. fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult { let config = ws.config(); From ac1e66d947ec274181926345df0a3fe8b22a52b3 Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Fri, 17 Nov 2023 16:26:44 +0800 Subject: [PATCH 2/5] Add testcase for custom build file warning --- src/cargo/ops/cargo_package.rs | 2 +- tests/testsuite/package.rs | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index df562e36985..7c964ad3d7b 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -439,7 +439,7 @@ fn warn_custom_build_file_not_in_package( "the source file of {:?} target `{}` doesn't appear to be a path inside of the package.\n\ It is at {}, whereas the root the package is {}.\n\ This may cause issue during packaging, as modules resolution and resources included via macros are often relative to the path of source files.\n\ - Please update the `build` setting in the manifest at `{}` and point to a path inside the root of the package.\n", + Please update the `build` setting in the manifest at `{}` and point to a path inside the root of the package.", target.kind(), target.name(), path.display(), pkg.root().display(), pkg.manifest_path().display() ); diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index a3b90323b16..3a62a3086e5 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -3304,3 +3304,57 @@ fn init_and_add_inner_target(p: ProjectBuilder) -> ProjectBuilder { .file("derp/target/foo.txt", "") .file("derp/not_target/foo.txt", "") } + +#[cargo_test] +fn custom_build_warning() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + license = "MIT" + description = "foo" + authors = [] + build = "../t_custom_build/custom_build.rs" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + p.cargo("package -l") + .with_stderr(format!( + "\ +warning: manifest has no documentation, homepage or repository. +See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. +warning: build `{}/../t_custom_build/custom_build.rs` does not appear to exist. +Please update the build setting in the manifest at `{}/Cargo.toml` +This may become a hard error in the future. +", + p.root().display(), + p.root().display() + )) + .run(); + + // crate custom_build.rs outside the package root + let custom_build_root = p.root().parent().unwrap().join("t_custom_build"); + _ = fs::create_dir(&custom_build_root).unwrap(); + _ = fs::write(&custom_build_root.join("custom_build.rs"), "fn main() {}"); + + p.cargo("package -l") + .with_stderr(format!( + "\ +warning: manifest has no documentation, homepage or repository. +See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. +warning: the source file of \"custom-build\" target `build-script-custom_build` doesn't appear to be a path inside of the package. +It is at {}/t_custom_build/custom_build.rs, whereas the root the package is {}. +This may cause issue during packaging, as modules resolution and resources included via macros are often relative to the path of source files. +Please update the `build` setting in the manifest at `{}/Cargo.toml` and point to a path inside the root of the package. +", +p.root().parent().unwrap().display(), + p.root().display(), + p.root().display() + )) + .run(); + _ = fs::remove_dir_all(&custom_build_root).unwrap(); +} From 92ce5a2b27e3da3fb33d15833253105153bef7fd Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Mon, 20 Nov 2023 12:53:16 +0800 Subject: [PATCH 3/5] Change the logging level and other improvement. --- src/cargo/ops/cargo_package.rs | 36 ++++++++++++--------- tests/testsuite/package.rs | 58 ++++++++++++++-------------------- 2 files changed, 45 insertions(+), 49 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 7c964ad3d7b..150cc89c026 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -342,15 +342,9 @@ fn build_ar_list( if let Some(custome_build_path) = t.src_path().path() { let abs_custome_build_path = paths::normalize_path(&pkg.root().join(custome_build_path)); - if abs_custome_build_path.is_file() { - if !abs_custome_build_path - .ancestors() - .any(|ancestor| ancestor == pkg.root()) - { - warn_custom_build_file_not_in_package(pkg, &abs_custome_build_path, t, &ws)? - } - } else { - warn_on_nonexistent_file(&pkg, &custome_build_path, "build", &ws)? + if !abs_custome_build_path.is_file() || !abs_custome_build_path.starts_with(pkg.root()) + { + error_custom_build_file_not_in_package(pkg, &abs_custome_build_path, t, &ws)? } } } @@ -429,21 +423,33 @@ fn warn_on_nonexistent_file( )) } -fn warn_custom_build_file_not_in_package( +fn error_custom_build_file_not_in_package( pkg: &Package, path: &Path, target: &Target, ws: &Workspace<'_>, ) -> CargoResult<()> { + let tip = { + if path.is_file() { + format!("the source file of {:?} target `{}` doesn't appear to be a path inside of the package.\n\ + It is at `{}`, whereas the root the package is `{}`.\n", + target.kind(), target.name(), path.display(), pkg.root().display() + ) + } else { + format!( + "the source file of {:?} target `{}` doesn't appear to exist.\n", + target.kind(), + target.name() + ) + } + }; let msg = format!( - "the source file of {:?} target `{}` doesn't appear to be a path inside of the package.\n\ - It is at {}, whereas the root the package is {}.\n\ + "{}\ This may cause issue during packaging, as modules resolution and resources included via macros are often relative to the path of source files.\n\ Please update the `build` setting in the manifest at `{}` and point to a path inside the root of the package.", - target.kind(), target.name(), path.display(), pkg.root().display(), pkg.manifest_path().display() + tip, pkg.manifest_path().display() ); - - ws.config().shell().warn(&msg) + ws.config().shell().error(&msg) } /// Construct `Cargo.lock` for the package to be published. diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 3a62a3086e5..1a15fbd92a0 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -3306,55 +3306,45 @@ fn init_and_add_inner_target(p: ProjectBuilder) -> ProjectBuilder { } #[cargo_test] -fn custom_build_warning() { +fn build_script_outside_pkg_root() { let p = project() .file( "Cargo.toml", r#" - [package] - name = "foo" - version = "0.0.1" - license = "MIT" - description = "foo" - authors = [] - build = "../t_custom_build/custom_build.rs" - "#, + [package] + name = "foo" + version = "0.0.1" + license = "MIT" + description = "foo" + authors = [] + build = "../t_custom_build/custom_build.rs" + "#, ) .file("src/main.rs", "fn main() {}") .build(); - p.cargo("package -l") - .with_stderr(format!( - "\ + let mut expect_msg = String::from("\ warning: manifest has no documentation, homepage or repository. See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. -warning: build `{}/../t_custom_build/custom_build.rs` does not appear to exist. -Please update the build setting in the manifest at `{}/Cargo.toml` -This may become a hard error in the future. -", - p.root().display(), - p.root().display() - )) - .run(); +error: the source file of \"custom-build\" target `build-script-custom_build` doesn't appear to exist. +This may cause issue during packaging, as modules resolution and resources included via macros are often relative to the path of source files. +Please update the `build` setting in the manifest at `[CWD]/Cargo.toml` and point to a path inside the root of the package. +"); + // custom_build.rs does not exist + p.cargo("package -l").with_stderr(&expect_msg).run(); - // crate custom_build.rs outside the package root + // custom_build.rs outside the package root let custom_build_root = p.root().parent().unwrap().join("t_custom_build"); _ = fs::create_dir(&custom_build_root).unwrap(); _ = fs::write(&custom_build_root.join("custom_build.rs"), "fn main() {}"); - - p.cargo("package -l") - .with_stderr(format!( - "\ + expect_msg = format!( + "\ warning: manifest has no documentation, homepage or repository. See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. -warning: the source file of \"custom-build\" target `build-script-custom_build` doesn't appear to be a path inside of the package. -It is at {}/t_custom_build/custom_build.rs, whereas the root the package is {}. +error: the source file of \"custom-build\" target `build-script-custom_build` doesn't appear to be a path inside of the package. +It is at `{}/t_custom_build/custom_build.rs`, whereas the root the package is `[CWD]`. This may cause issue during packaging, as modules resolution and resources included via macros are often relative to the path of source files. -Please update the `build` setting in the manifest at `{}/Cargo.toml` and point to a path inside the root of the package. -", -p.root().parent().unwrap().display(), - p.root().display(), - p.root().display() - )) - .run(); +Please update the `build` setting in the manifest at `[CWD]/Cargo.toml` and point to a path inside the root of the package. +", p.root().parent().unwrap().display()); + p.cargo("package -l").with_stderr(&expect_msg).run(); _ = fs::remove_dir_all(&custom_build_root).unwrap(); } From edfbcf0a6cec2e9d2be56a035873bed0cbc3596c Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Tue, 21 Nov 2023 11:10:46 +0800 Subject: [PATCH 4/5] Return a hard error when custom build outside package --- src/cargo/ops/cargo_package.rs | 18 +++++++----------- tests/testsuite/package.rs | 19 ++++++++++++------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 150cc89c026..6587f1b317c 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -344,7 +344,7 @@ fn build_ar_list( paths::normalize_path(&pkg.root().join(custome_build_path)); if !abs_custome_build_path.is_file() || !abs_custome_build_path.starts_with(pkg.root()) { - error_custom_build_file_not_in_package(pkg, &abs_custome_build_path, t, &ws)? + error_custom_build_file_not_in_package(pkg, &abs_custome_build_path, t)?; } } } @@ -427,20 +427,16 @@ fn error_custom_build_file_not_in_package( pkg: &Package, path: &Path, target: &Target, - ws: &Workspace<'_>, -) -> CargoResult<()> { +) -> CargoResult> { let tip = { + let description_name = target.description_named(); if path.is_file() { - format!("the source file of {:?} target `{}` doesn't appear to be a path inside of the package.\n\ + format!("the source file of `{description_name}` doesn't appear to be a path inside of the package.\n\ It is at `{}`, whereas the root the package is `{}`.\n", - target.kind(), target.name(), path.display(), pkg.root().display() + path.display(), pkg.root().display() ) } else { - format!( - "the source file of {:?} target `{}` doesn't appear to exist.\n", - target.kind(), - target.name() - ) + format!("the source file of `{description_name}` doesn't appear to exist.\n",) } }; let msg = format!( @@ -449,7 +445,7 @@ fn error_custom_build_file_not_in_package( Please update the `build` setting in the manifest at `{}` and point to a path inside the root of the package.", tip, pkg.manifest_path().display() ); - ws.config().shell().error(&msg) + anyhow::bail!(msg) } /// Construct `Cargo.lock` for the package to be published. diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 1a15fbd92a0..eff68dae925 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -3325,26 +3325,31 @@ fn build_script_outside_pkg_root() { let mut expect_msg = String::from("\ warning: manifest has no documentation, homepage or repository. See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. -error: the source file of \"custom-build\" target `build-script-custom_build` doesn't appear to exist. +error: the source file of `build script` doesn't appear to exist. This may cause issue during packaging, as modules resolution and resources included via macros are often relative to the path of source files. Please update the `build` setting in the manifest at `[CWD]/Cargo.toml` and point to a path inside the root of the package. "); // custom_build.rs does not exist - p.cargo("package -l").with_stderr(&expect_msg).run(); + p.cargo("package -l") + .with_status(101) + .with_stderr(&expect_msg) + .run(); // custom_build.rs outside the package root - let custom_build_root = p.root().parent().unwrap().join("t_custom_build"); + let custom_build_root = paths::root().join("t_custom_build"); _ = fs::create_dir(&custom_build_root).unwrap(); _ = fs::write(&custom_build_root.join("custom_build.rs"), "fn main() {}"); expect_msg = format!( "\ warning: manifest has no documentation, homepage or repository. See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. -error: the source file of \"custom-build\" target `build-script-custom_build` doesn't appear to be a path inside of the package. +error: the source file of `build script` doesn't appear to be a path inside of the package. It is at `{}/t_custom_build/custom_build.rs`, whereas the root the package is `[CWD]`. This may cause issue during packaging, as modules resolution and resources included via macros are often relative to the path of source files. Please update the `build` setting in the manifest at `[CWD]/Cargo.toml` and point to a path inside the root of the package. -", p.root().parent().unwrap().display()); - p.cargo("package -l").with_stderr(&expect_msg).run(); - _ = fs::remove_dir_all(&custom_build_root).unwrap(); +", paths::root().display()); + p.cargo("package -l") + .with_status(101) + .with_stderr(&expect_msg) + .run(); } From 75aaa40c786e31364c6a5bf7bb6288e07b1b0383 Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Wed, 22 Nov 2023 11:09:55 +0800 Subject: [PATCH 5/5] Remove the unnecessary backticks --- src/cargo/ops/cargo_package.rs | 4 ++-- tests/testsuite/package.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 6587f1b317c..d837aacdc75 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -431,12 +431,12 @@ fn error_custom_build_file_not_in_package( let tip = { let description_name = target.description_named(); if path.is_file() { - format!("the source file of `{description_name}` doesn't appear to be a path inside of the package.\n\ + format!("the source file of {description_name} doesn't appear to be a path inside of the package.\n\ It is at `{}`, whereas the root the package is `{}`.\n", path.display(), pkg.root().display() ) } else { - format!("the source file of `{description_name}` doesn't appear to exist.\n",) + format!("the source file of {description_name} doesn't appear to exist.\n",) } }; let msg = format!( diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index eff68dae925..371157e4efa 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -3325,7 +3325,7 @@ fn build_script_outside_pkg_root() { let mut expect_msg = String::from("\ warning: manifest has no documentation, homepage or repository. See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. -error: the source file of `build script` doesn't appear to exist. +error: the source file of build script doesn't appear to exist. This may cause issue during packaging, as modules resolution and resources included via macros are often relative to the path of source files. Please update the `build` setting in the manifest at `[CWD]/Cargo.toml` and point to a path inside the root of the package. "); @@ -3343,7 +3343,7 @@ Please update the `build` setting in the manifest at `[CWD]/Cargo.toml` and poin "\ warning: manifest has no documentation, homepage or repository. See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info. -error: the source file of `build script` doesn't appear to be a path inside of the package. +error: the source file of build script doesn't appear to be a path inside of the package. It is at `{}/t_custom_build/custom_build.rs`, whereas the root the package is `[CWD]`. This may cause issue during packaging, as modules resolution and resources included via macros are often relative to the path of source files. Please update the `build` setting in the manifest at `[CWD]/Cargo.toml` and point to a path inside the root of the package.