From 305f1f711d4146a866e14e91555ecb9c9be777d4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 3 Apr 2024 14:39:23 -0500 Subject: [PATCH] fix(toml): Warn, rather than fail publish, if build.rs is excluded This could offer a minor performance gain when reading this manifest since the target doesn't need to be discovered. --- crates/cargo-util-schemas/src/manifest/mod.rs | 9 +++ src/cargo/ops/cargo_package.rs | 6 +- src/cargo/util/toml/mod.rs | 56 +++++++++++++++--- src/cargo/util/toml/targets.rs | 28 ++------- tests/testsuite/artifact_dep.rs | 1 + tests/testsuite/features2.rs | 1 + tests/testsuite/features_namespaced.rs | 2 + .../testsuite/inheritable_workspace_fields.rs | 5 ++ tests/testsuite/package.rs | 59 +++++++++++++++++-- tests/testsuite/publish.rs | 3 + tests/testsuite/weak_dep_features.rs | 1 + 11 files changed, 132 insertions(+), 39 deletions(-) diff --git a/crates/cargo-util-schemas/src/manifest/mod.rs b/crates/cargo-util-schemas/src/manifest/mod.rs index 92eea753e446..7b89529b8958 100644 --- a/crates/cargo-util-schemas/src/manifest/mod.rs +++ b/crates/cargo-util-schemas/src/manifest/mod.rs @@ -221,6 +221,15 @@ impl TomlPackage { self.authors.as_ref().map(|v| v.resolved()).transpose() } + pub fn resolved_build(&self) -> Result, UnresolvedError> { + let readme = self.build.as_ref().ok_or(UnresolvedError)?; + match readme { + StringOrBool::Bool(false) => Ok(None), + StringOrBool::Bool(true) => Err(UnresolvedError), + StringOrBool::String(value) => Ok(Some(value)), + } + } + pub fn resolved_exclude(&self) -> Result>, UnresolvedError> { self.exclude.as_ref().map(|v| v.resolved()).transpose() } diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 479e9b2d639a..f54683c8b0cf 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -684,7 +684,11 @@ fn tar( let base_name = format!("{}-{}", pkg.name(), pkg.version()); let base_path = Path::new(&base_name); - let publish_pkg = prepare_for_publish(pkg, ws)?; + let included = ar_files + .iter() + .map(|ar_file| ar_file.rel_path.clone()) + .collect::>(); + let publish_pkg = prepare_for_publish(pkg, ws, &included)?; let mut uncompressed_size = 0; for ar_file in ar_files { diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 5444a0ea815c..e6ad52d19746 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -428,7 +428,7 @@ fn resolve_package_toml<'a>( .map(|value| field_inherit_with(value, "authors", || inherit()?.authors())) .transpose()? .map(manifest::InheritableField::Value), - build: original_package.build.clone(), + build: resolve_package_build(original_package.build.as_ref(), package_root), metabuild: original_package.metabuild.clone(), default_target: original_package.default_target.clone(), forced_target: original_package.forced_target.clone(), @@ -532,6 +532,29 @@ fn resolve_package_toml<'a>( Ok(Box::new(resolved_package)) } +/// Returns the path to the build script if one exists for this crate. +fn resolve_package_build( + build: Option<&StringOrBool>, + package_root: &Path, +) -> Option { + const BUILD_RS: &str = "build.rs"; + match build { + None => { + // If there is a `build.rs` file next to the `Cargo.toml`, assume it is + // a build script. + let build_rs = package_root.join(BUILD_RS); + if build_rs.is_file() { + Some(StringOrBool::String(BUILD_RS.to_owned())) + } else { + Some(StringOrBool::Bool(false)) + } + } + // Explicitly no build script. + Some(StringOrBool::Bool(false)) | Some(StringOrBool::String(_)) => build.cloned(), + Some(StringOrBool::Bool(true)) => Some(StringOrBool::String(BUILD_RS.to_owned())), + } +} + /// Returns the name of the README file for a [`manifest::TomlPackage`]. fn resolve_package_readme( package_root: &Path, @@ -1074,7 +1097,6 @@ fn to_real_manifest( package_name, package_root, edition, - &resolved_package.build, &resolved_package.metabuild, warnings, errors, @@ -2290,10 +2312,15 @@ fn unused_dep_keys( } } -pub fn prepare_for_publish(me: &Package, ws: &Workspace<'_>) -> CargoResult { +pub fn prepare_for_publish( + me: &Package, + ws: &Workspace<'_>, + included: &[PathBuf], +) -> CargoResult { let contents = me.manifest().contents(); let document = me.manifest().document(); - let original_toml = prepare_toml_for_publish(me.manifest().resolved_toml(), ws, me.root())?; + let original_toml = + prepare_toml_for_publish(me.manifest().resolved_toml(), ws, me.root(), included)?; let resolved_toml = original_toml.clone(); let features = me.manifest().unstable_features().clone(); let workspace_config = me.manifest().workspace_config().clone(); @@ -2325,6 +2352,7 @@ fn prepare_toml_for_publish( me: &manifest::TomlManifest, ws: &Workspace<'_>, package_root: &Path, + included: &[PathBuf], ) -> CargoResult { let gctx = ws.gctx(); @@ -2341,11 +2369,21 @@ fn prepare_toml_for_publish( package.workspace = None; if let Some(StringOrBool::String(path)) = &package.build { let path = paths::normalize_path(Path::new(path)); - let path = path - .into_os_string() - .into_string() - .map_err(|_err| anyhow::format_err!("non-UTF8 `package.build`"))?; - package.build = Some(StringOrBool::String(normalize_path_string_sep(path))); + let build = if included.contains(&path) { + let path = path + .into_os_string() + .into_string() + .map_err(|_err| anyhow::format_err!("non-UTF8 `package.build`"))?; + let path = normalize_path_string_sep(path); + StringOrBool::String(path) + } else { + ws.gctx().shell().warn(format!( + "ignoring `package.build` as `{}` is not included in the published package", + path.display() + ))?; + StringOrBool::Bool(false) + }; + package.build = Some(build); } let current_resolver = package .resolver diff --git a/src/cargo/util/toml/targets.rs b/src/cargo/util/toml/targets.rs index 937177c95838..0c7f617be3e2 100644 --- a/src/cargo/util/toml/targets.rs +++ b/src/cargo/util/toml/targets.rs @@ -16,8 +16,8 @@ use std::path::{Path, PathBuf}; use anyhow::Context as _; use cargo_util_schemas::manifest::{ - PathValue, StringOrBool, StringOrVec, TomlBenchTarget, TomlBinTarget, TomlExampleTarget, - TomlLibTarget, TomlManifest, TomlTarget, TomlTestTarget, + PathValue, StringOrVec, TomlBenchTarget, TomlBinTarget, TomlExampleTarget, TomlLibTarget, + TomlManifest, TomlTarget, TomlTestTarget, }; use crate::core::compiler::rustdoc::RustdocScrapeExamples; @@ -38,7 +38,6 @@ pub(super) fn to_targets( package_name: &str, package_root: &Path, edition: Edition, - custom_build: &Option, metabuild: &Option, warnings: &mut Vec, errors: &mut Vec, @@ -125,10 +124,11 @@ pub(super) fn to_targets( targets.extend(to_bench_targets(&toml_benches, package_root, edition)?); // processing the custom build script - if let Some(custom_build) = maybe_custom_build(custom_build, package_root) { + if let Some(custom_build) = package.resolved_build().expect("should be resolved") { if metabuild.is_some() { anyhow::bail!("cannot specify both `metabuild` and `build`"); } + let custom_build = Path::new(custom_build); let name = format!( "build-script-{}", custom_build @@ -1074,26 +1074,6 @@ Cargo doesn't know which to use because multiple target files found at `{}` and } } -/// Returns the path to the build script if one exists for this crate. -fn maybe_custom_build(build: &Option, package_root: &Path) -> Option { - let build_rs = package_root.join("build.rs"); - match *build { - // Explicitly no build script. - Some(StringOrBool::Bool(false)) => None, - Some(StringOrBool::Bool(true)) => Some(build_rs), - Some(StringOrBool::String(ref s)) => Some(PathBuf::from(s)), - None => { - // If there is a `build.rs` file next to the `Cargo.toml`, assume it is - // a build script. - if build_rs.is_file() { - Some(build_rs) - } else { - None - } - } - } -} - fn name_or_panic(target: &TomlTarget) -> &str { target .name diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs index d08264007f3b..2a5361f42a3c 100644 --- a/tests/testsuite/artifact_dep.rs +++ b/tests/testsuite/artifact_dep.rs @@ -2164,6 +2164,7 @@ edition = "2015" name = "foo" version = "0.1.0" authors = [] +build = false description = "foo" homepage = "foo" documentation = "foo" diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs index a219d8a3c27f..30ccab787ffa 100644 --- a/tests/testsuite/features2.rs +++ b/tests/testsuite/features2.rs @@ -1703,6 +1703,7 @@ edition = "2015" name = "a" version = "0.1.0" authors = ["Zzz"] +build = false description = "foo" homepage = "https://example.com/" readme = false diff --git a/tests/testsuite/features_namespaced.rs b/tests/testsuite/features_namespaced.rs index 59014d3ca57b..f24aa8fa8c6e 100644 --- a/tests/testsuite/features_namespaced.rs +++ b/tests/testsuite/features_namespaced.rs @@ -985,6 +985,7 @@ You may press ctrl-c [..] edition = "2015" name = "foo" version = "0.1.0" +build = false description = "foo" homepage = "https://example.com/" readme = false @@ -1104,6 +1105,7 @@ You may press ctrl-c [..] edition = "2015" name = "foo" version = "0.1.0" +build = false description = "foo" homepage = "https://example.com/" readme = false diff --git a/tests/testsuite/inheritable_workspace_fields.rs b/tests/testsuite/inheritable_workspace_fields.rs index 3448ce477fa7..64304026eea0 100644 --- a/tests/testsuite/inheritable_workspace_fields.rs +++ b/tests/testsuite/inheritable_workspace_fields.rs @@ -226,6 +226,7 @@ rust-version = "1.60" name = "foo" version = "1.2.3" authors = ["Rustaceans"] +build = false exclude = ["foo.txt"] include = [ "bar.txt", @@ -397,6 +398,7 @@ edition = "2015" name = "bar" version = "0.2.0" authors = [] +build = false readme = false [dependencies.dep] @@ -529,6 +531,7 @@ edition = "2015" name = "bar" version = "0.2.0" authors = [] +build = false readme = false [dependencies.dep] @@ -776,6 +779,7 @@ rust-version = "1.60" name = "bar" version = "1.2.3" authors = ["Rustaceans"] +build = false exclude = ["foo.txt"] include = [ "bar.txt", @@ -952,6 +956,7 @@ edition = "2015" name = "bar" version = "0.2.0" authors = [] +build = false readme = false [dependencies.dep] diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 4485ca36f263..daba88b41186 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -1217,6 +1217,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = false exclude = ["*.txt"] description = "foo" readme = false @@ -1294,6 +1295,7 @@ edition = "2015" name = "bar" version = "0.1.0" authors = [] +build = false readme = false "#, cargo::core::manifest::MANIFEST_PREAMBLE @@ -1362,6 +1364,7 @@ fn package_public_dep() { edition = "2015" name = "foo" version = "0.0.1" +build = false readme = false [dependencies.bar] @@ -1381,6 +1384,7 @@ version = "1.0.0" edition = "2015" name = "foo" version = "0.0.1" +build = false readme = false [dependencies.bar] @@ -2810,6 +2814,7 @@ fn workspace_overrides_resolver() { edition = "2021" name = "bar" version = "0.1.0" +build = false readme = false resolver = "1" "#, @@ -2830,6 +2835,7 @@ resolver = "1" edition = "2015" name = "baz" version = "0.1.0" +build = false readme = false "#, cargo::core::manifest::MANIFEST_PREAMBLE @@ -2892,6 +2898,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = false exclude = ["*.txt"] description = "foo" homepage = "https://example.com/" @@ -2988,6 +2995,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = false description = "foo" documentation = "https://example.com/" readme = false @@ -3096,6 +3104,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = false description = "foo" homepage = "https://example.com/" readme = false @@ -3733,6 +3742,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = "build.rs" include = [ "src/lib.rs", "build.rs", @@ -3772,6 +3782,7 @@ fn discovery_inferred_build_rs_excluded() { .with_stderr( "\ [PACKAGING] foo v0.0.1 ([CWD]) +[WARNING] ignoring `package.build` as `build.rs` is not included in the published package [VERIFYING] foo v0.0.1 ([CWD]) [COMPILING] foo v0.0.1 ([CWD][..]) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..] @@ -3803,6 +3814,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = false include = ["src/lib.rs"] description = "foo" documentation = "docs.rs/foo" @@ -3908,20 +3920,51 @@ fn discovery_explicit_build_rs_excluded() { .build(); p.cargo("package") - .with_status(101) .with_stdout("") .with_stderr( "\ [PACKAGING] foo v0.0.1 ([CWD]) +[WARNING] ignoring `package.build` as `build.rs` is not included in the published package [VERIFYING] foo v0.0.1 ([CWD]) [COMPILING] foo v0.0.1 ([CWD][..]) -[ERROR] couldn't read build.rs: [..] - -[ERROR] could not compile `foo` (build script) due to 1 previous error -[ERROR] failed to verify package tarball +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..] +[PACKAGED] 3 files, [..] ([..] compressed) ", ) .run(); + + let f = File::open(&p.root().join("target/package/foo-0.0.1.crate")).unwrap(); + validate_crate_contents( + f, + "foo-0.0.1.crate", + &["Cargo.toml", "Cargo.toml.orig", "src/lib.rs"], + &[( + "Cargo.toml", + r#"# THIS FILE IS AUTOMATICALLY GENERATED BY CARGO +# +# When uploading crates to the registry Cargo will automatically +# "normalize" Cargo.toml files for maximal compatibility +# with all versions of Cargo and also rewrite `path` dependencies +# to registry (e.g., crates.io) dependencies. +# +# If you are reading this file be aware that the original Cargo.toml +# will likely look very different (and much more reasonable). +# See Cargo.toml.orig for the original contents. + +[package] +edition = "2015" +name = "foo" +version = "0.0.1" +authors = [] +build = false +include = ["src/lib.rs"] +description = "foo" +documentation = "docs.rs/foo" +readme = false +license = "MIT" +"#, + )], + ); } #[cargo_test] @@ -3987,6 +4030,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = false include = [ "src/main.rs", "src/lib.rs", @@ -4057,6 +4101,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = false include = ["src/main.rs"] description = "foo" documentation = "docs.rs/foo" @@ -4133,6 +4178,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = false include = [ "src/main.rs", "src/lib.rs", @@ -4259,6 +4305,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = false include = [ "src/lib.rs", "src/bin/foo/main.rs", @@ -4335,6 +4382,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = false include = ["src/lib.rs"] description = "foo" documentation = "docs.rs/foo" @@ -4426,6 +4474,7 @@ edition = "2015" name = "foo" version = "0.0.1" authors = [] +build = false include = [ "src/lib.rs", "src/bin/foo/main.rs", diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 8eab3c2b4074..6e219a3d2d30 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -1560,6 +1560,7 @@ You may press ctrl-c [..] name = \"foo\"\n\ version = \"0.1.0\"\n\ authors = []\n\ + build = false\n\ description = \"foo\"\n\ readme = false\n\ license = \"MIT\"\n\ @@ -1673,6 +1674,7 @@ edition = "2015" name = "foo" version = "0.1.0" authors = [] +build = false description = "foo" homepage = "foo" documentation = "foo" @@ -1933,6 +1935,7 @@ edition = "2015" name = "foo" version = "0.1.0" authors = [] +build = false description = "foo" homepage = "foo" documentation = "foo" diff --git a/tests/testsuite/weak_dep_features.rs b/tests/testsuite/weak_dep_features.rs index c2810c76fc82..f360946fab9b 100644 --- a/tests/testsuite/weak_dep_features.rs +++ b/tests/testsuite/weak_dep_features.rs @@ -630,6 +630,7 @@ You may press ctrl-c to skip waiting; the crate should be available shortly. edition = "2015" name = "foo" version = "0.1.0" +build = false description = "foo" homepage = "https://example.com/" readme = false