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.

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

fix(toml): Warn, rather than fail publish, if a target is excluded #13713

Merged
merged 6 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 15 additions & 9 deletions crates/cargo-util-schemas/src/manifest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,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 Expand Up @@ -243,15 +252,12 @@ impl TomlPackage {
}

pub fn resolved_readme(&self) -> Result<Option<&String>, UnresolvedError> {
self.readme
.as_ref()
.map(|v| {
v.resolved().and_then(|sb| match sb {
StringOrBool::Bool(_) => Err(UnresolvedError),
StringOrBool::String(value) => Ok(value),
})
})
.transpose()
let readme = self.readme.as_ref().ok_or(UnresolvedError)?;
readme.resolved().and_then(|sb| match sb {
StringOrBool::Bool(false) => Ok(None),
StringOrBool::Bool(true) => Err(UnresolvedError),
StringOrBool::String(value) => Ok(Some(value)),
})
}

pub fn resolved_keywords(&self) -> Result<Option<&Vec<String>>, UnresolvedError> {
Expand Down
6 changes: 6 additions & 0 deletions src/cargo/core/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ impl Manifest {
pub fn contents(&self) -> &str {
self.contents.as_str()
}
/// See [`Manifest::resolved_toml`] for what "resolved" means
pub fn to_resolved_contents(&self) -> CargoResult<String> {
let toml = toml::to_string_pretty(self.resolved_toml())?;
Ok(format!("{}\n{}", MANIFEST_PREAMBLE, toml))
Expand All @@ -496,6 +497,11 @@ impl Manifest {
&self.original_toml
}
/// The [`TomlManifest`] with all fields expanded
///
/// This is the intersection of what fields need resolving for cargo-publish that also are
/// useful for the operation of cargo, including
/// - workspace inheritance
/// - target discovery
pub fn resolved_toml(&self) -> &TomlManifest {
&self.resolved_toml
}
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 @@ -689,7 +689,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
Copy link
Member

@weihanglo weihanglo Apr 22, 2024

Choose a reason for hiding this comment

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

nit: Should this be a set instead of an array?

.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
148 changes: 114 additions & 34 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ fn to_workspace_root_config(
ws_root_config
}

/// See [`Manifest::resolved_toml`] for more details
#[tracing::instrument(skip_all)]
fn resolve_toml(
original_toml: &manifest::TomlManifest,
Expand All @@ -264,7 +265,7 @@ fn resolve_toml(
manifest_file: &Path,
gctx: &GlobalContext,
warnings: &mut Vec<String>,
_errors: &mut Vec<String>,
errors: &mut Vec<String>,
) -> CargoResult<manifest::TomlManifest> {
if let Some(workspace) = &original_toml.workspace {
if workspace.resolver.as_deref() == Some("3") {
Expand All @@ -277,11 +278,11 @@ fn resolve_toml(
package: None,
project: None,
profile: original_toml.profile.clone(),
lib: original_toml.lib.clone(),
bin: original_toml.bin.clone(),
example: original_toml.example.clone(),
test: original_toml.test.clone(),
bench: original_toml.bench.clone(),
lib: None,
bin: None,
example: None,
test: None,
bench: None,
dependencies: None,
dev_dependencies: None,
dev_dependencies2: None,
Expand Down Expand Up @@ -318,6 +319,47 @@ fn resolve_toml(
});
resolved_toml.package = Some(resolved_package);

resolved_toml.lib = targets::resolve_lib(
original_toml.lib.as_ref(),
package_root,
&original_package.name,
edition,
warnings,
)?;
resolved_toml.bin = Some(targets::resolve_bins(
original_toml.bin.as_ref(),
package_root,
&original_package.name,
edition,
original_package.autobins,
warnings,
resolved_toml.lib.is_some(),
)?);
resolved_toml.example = Some(targets::resolve_examples(
original_toml.example.as_ref(),
package_root,
edition,
original_package.autoexamples,
warnings,
errors,
)?);
resolved_toml.test = Some(targets::resolve_tests(
original_toml.test.as_ref(),
package_root,
edition,
original_package.autotests,
warnings,
errors,
)?);
resolved_toml.bench = Some(targets::resolve_benches(
original_toml.bench.as_ref(),
package_root,
edition,
original_package.autobenches,
warnings,
errors,
)?);

let activated_opt_deps = resolved_toml
.features
.as_ref()
Expand Down Expand Up @@ -494,7 +536,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: targets::resolve_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 All @@ -519,10 +561,10 @@ fn resolve_package_toml<'a>(
.map(manifest::InheritableField::Value),
workspace: original_package.workspace.clone(),
im_a_teapot: original_package.im_a_teapot.clone(),
autobins: original_package.autobins.clone(),
autoexamples: original_package.autoexamples.clone(),
autotests: original_package.autotests.clone(),
autobenches: original_package.autobenches.clone(),
autobins: Some(false),
autoexamples: Some(false),
autotests: Some(false),
autobenches: Some(false),
default_run: original_package.default_run.clone(),
description: original_package
.description
Expand Down Expand Up @@ -553,7 +595,10 @@ fn resolve_package_toml<'a>(
.transpose()?
.as_ref(),
)
.map(|s| manifest::InheritableField::Value(StringOrBool::String(s))),
.map(|s| manifest::InheritableField::Value(StringOrBool::String(s)))
.or(Some(manifest::InheritableField::Value(StringOrBool::Bool(
false,
)))),
keywords: original_package
.keywords
.clone()
Expand Down Expand Up @@ -1146,11 +1191,10 @@ fn to_real_manifest(
// If we have a lib with no path, use the inferred lib or else the package name.
let targets = to_targets(
&features,
&original_toml,
&resolved_toml,
package_name,
package_root,
edition,
&resolved_package.build,
&resolved_package.metabuild,
warnings,
errors,
Expand Down Expand Up @@ -2357,10 +2401,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 @@ -2392,6 +2441,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 @@ -2408,11 +2458,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 Expand Up @@ -2502,14 +2562,14 @@ fn prepare_toml_for_publish(
}

let lib = if let Some(target) = &me.lib {
Some(prepare_target_for_publish(target, "library")?)
prepare_target_for_publish(target, included, "library", ws.gctx())?
} else {
None
};
let bin = prepare_targets_for_publish(me.bin.as_ref(), "binary")?;
let example = prepare_targets_for_publish(me.example.as_ref(), "example")?;
let test = prepare_targets_for_publish(me.test.as_ref(), "test")?;
let bench = prepare_targets_for_publish(me.bench.as_ref(), "benchmark")?;
let bin = prepare_targets_for_publish(me.bin.as_ref(), included, "binary", ws.gctx())?;
let example = prepare_targets_for_publish(me.example.as_ref(), included, "example", ws.gctx())?;
let test = prepare_targets_for_publish(me.test.as_ref(), included, "test", ws.gctx())?;
let bench = prepare_targets_for_publish(me.bench.as_ref(), included, "benchmark", ws.gctx())?;

let all = |_d: &manifest::TomlDependency| true;
let mut manifest = manifest::TomlManifest {
Expand Down Expand Up @@ -2667,31 +2727,51 @@ fn prepare_toml_for_publish(

fn prepare_targets_for_publish(
targets: Option<&Vec<manifest::TomlTarget>>,
included: &[PathBuf],
context: &str,
gctx: &GlobalContext,
) -> CargoResult<Option<Vec<manifest::TomlTarget>>> {
let Some(targets) = targets else {
return Ok(None);
};

let mut prepared = Vec::with_capacity(targets.len());
for target in targets {
let target = prepare_target_for_publish(target, context)?;
let Some(target) = prepare_target_for_publish(target, included, context, gctx)? else {
continue;
};
prepared.push(target);
}

Ok(Some(prepared))
if prepared.is_empty() {
Ok(None)
} else {
Ok(Some(prepared))
}
}

fn prepare_target_for_publish(
target: &manifest::TomlTarget,
included: &[PathBuf],
context: &str,
) -> CargoResult<manifest::TomlTarget> {
let mut target = target.clone();
if let Some(path) = target.path {
let path = normalize_path(&path.0);
target.path = Some(manifest::PathValue(normalize_path_sep(path, context)?));
gctx: &GlobalContext,
) -> CargoResult<Option<manifest::TomlTarget>> {
let path = target.path.as_ref().expect("previously resolved");
let path = normalize_path(&path.0);
if !included.contains(&path) {
let name = target.name.as_ref().expect("previously resolved");
gctx.shell().warn(format!(
"ignoring {context} `{name}` as `{}` is not included in the published package",
path.display()
))?;
return Ok(None);
}
Ok(target)

let mut target = target.clone();
let path = normalize_path_sep(path, context)?;
target.path = Some(manifest::PathValue(path.into()));

Ok(Some(target))
}

fn normalize_path_sep(path: PathBuf, context: &str) -> CargoResult<PathBuf> {
Expand Down