Skip to content

Commit

Permalink
fix(toml): Warn, rather than fail publish, if build.rs is excluded
Browse files Browse the repository at this point in the history
This could offer a minor performance gain when reading this manifest
since the target doesn't need to be discovered.
  • Loading branch information
epage committed Apr 10, 2024
1 parent 956bd34 commit 305f1f7
Show file tree
Hide file tree
Showing 11 changed files with 132 additions and 39 deletions.
9 changes: 9 additions & 0 deletions crates/cargo-util-schemas/src/manifest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,15 @@ impl TomlPackage {
self.authors.as_ref().map(|v| v.resolved()).transpose()
}

pub fn resolved_build(&self) -> Result<Option<&String>, 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<Option<&Vec<String>>, UnresolvedError> {
self.exclude.as_ref().map(|v| v.resolved()).transpose()
}
Expand Down
6 changes: 5 additions & 1 deletion src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Vec<_>>();
let publish_pkg = prepare_for_publish(pkg, ws, &included)?;

let mut uncompressed_size = 0;
for ar_file in ar_files {
Expand Down
56 changes: 47 additions & 9 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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<StringOrBool> {
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,
Expand Down Expand Up @@ -1074,7 +1097,6 @@ fn to_real_manifest(
package_name,
package_root,
edition,
&resolved_package.build,
&resolved_package.metabuild,
warnings,
errors,
Expand Down Expand Up @@ -2290,10 +2312,15 @@ fn unused_dep_keys(
}
}

pub fn prepare_for_publish(me: &Package, ws: &Workspace<'_>) -> CargoResult<Package> {
pub fn prepare_for_publish(
me: &Package,
ws: &Workspace<'_>,
included: &[PathBuf],
) -> CargoResult<Package> {
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();
Expand Down Expand Up @@ -2325,6 +2352,7 @@ fn prepare_toml_for_publish(
me: &manifest::TomlManifest,
ws: &Workspace<'_>,
package_root: &Path,
included: &[PathBuf],
) -> CargoResult<manifest::TomlManifest> {
let gctx = ws.gctx();

Expand All @@ -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
Expand Down
28 changes: 4 additions & 24 deletions src/cargo/util/toml/targets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -38,7 +38,6 @@ pub(super) fn to_targets(
package_name: &str,
package_root: &Path,
edition: Edition,
custom_build: &Option<StringOrBool>,
metabuild: &Option<StringOrVec>,
warnings: &mut Vec<String>,
errors: &mut Vec<String>,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<StringOrBool>, package_root: &Path) -> Option<PathBuf> {
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
Expand Down
1 change: 1 addition & 0 deletions tests/testsuite/artifact_dep.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2164,6 +2164,7 @@ edition = "2015"
name = "foo"
version = "0.1.0"
authors = []
build = false
description = "foo"
homepage = "foo"
documentation = "foo"
Expand Down
1 change: 1 addition & 0 deletions tests/testsuite/features2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1703,6 +1703,7 @@ edition = "2015"
name = "a"
version = "0.1.0"
authors = ["Zzz"]
build = false
description = "foo"
homepage = "https://example.com/"
readme = false
Expand Down
2 changes: 2 additions & 0 deletions tests/testsuite/features_namespaced.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions tests/testsuite/inheritable_workspace_fields.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ rust-version = "1.60"
name = "foo"
version = "1.2.3"
authors = ["Rustaceans"]
build = false
exclude = ["foo.txt"]
include = [
"bar.txt",
Expand Down Expand Up @@ -397,6 +398,7 @@ edition = "2015"
name = "bar"
version = "0.2.0"
authors = []
build = false
readme = false
[dependencies.dep]
Expand Down Expand Up @@ -529,6 +531,7 @@ edition = "2015"
name = "bar"
version = "0.2.0"
authors = []
build = false
readme = false
[dependencies.dep]
Expand Down Expand Up @@ -776,6 +779,7 @@ rust-version = "1.60"
name = "bar"
version = "1.2.3"
authors = ["Rustaceans"]
build = false
exclude = ["foo.txt"]
include = [
"bar.txt",
Expand Down Expand Up @@ -952,6 +956,7 @@ edition = "2015"
name = "bar"
version = "0.2.0"
authors = []
build = false
readme = false
[dependencies.dep]
Expand Down
Loading

0 comments on commit 305f1f7

Please sign in to comment.