Skip to content

Commit

Permalink
Auto merge of #14325 - epage:14321-master, r=weihanglo
Browse files Browse the repository at this point in the history 
fix(publish): Don't strip non-dev features

### What does this PR try to resolve?

First, we added support for stripping of local-only dev-dependencies.
This was dual-implemented for `Cargo.toml` and `Summary`.

This left off stripping of `dep/feature` that reference dev-dependencies
(enabling features within dev-dependencies).
When we fixed this, we again dual-implemented it.

The `Cargo.toml` version was correct but the `Summary` version was
instead stripping too many features,
particularly features that reference renamed dependencies.
We didn't have tests for this case and it wasn't caught earlier because
crates.io re-generates the `Summary` from `Cargo.toml`, ignoring what we
post.
That makes this only show up with custom registries that trust what
Cargo posts.

Rather than fixing the `Summary` generation, I remove the
dual-implementation and instead generate the `Summary` from the
published `Cargo.toml`.

Fixes #14321

### How should we test and review this PR?

### Additional information

Unfortunately, we don't have access directly to the packaged
`Cargo.toml`.
It could be passed around and I originally did so, hoping to remove use
of the local `Package`.
However, the local `Package` is needed for things like reading the
`README`.
So I scaled back and isolate the change to only what needs it.
This also makes it easier for `prepare_transmit` callers.
Fully open to someone exploring removing this extra `prepare_for_publish` in the future.
  • Loading branch information
@bors
bors committed Jul 31, 2024
2 parents 257b72b + 7c17862 commit d3e84f0
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 165 deletions.
13 changes: 9 additions & 4 deletions src/cargo/ops/cargo_package.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult<Vec<Fi
} else {
let tarball = create_package(ws, &pkg, ar_files, local_reg.as_ref())?;
if let Some(local_reg) = local_reg.as_mut() {
local_reg.add_package(&pkg, &tarball)?;
local_reg.add_package(ws, &pkg, &tarball)?;
}
outputs.push((pkg, opts, tarball));
}
Expand Down Expand Up @@ -893,7 +893,7 @@ fn tar(
.iter()
.map(|ar_file| ar_file.rel_path.clone())
.collect::<Vec<_>>();
let publish_pkg = prepare_for_publish(pkg, ws, &included)?;
let publish_pkg = prepare_for_publish(pkg, ws, Some(&included))?;

let mut uncompressed_size = 0;
for ar_file in ar_files {
Expand Down Expand Up @@ -1299,7 +1299,12 @@ impl<'a> TmpRegistry<'a> {
self.root.join("index")
}

fn add_package(&mut self, package: &Package, tar: &FileLock) -> CargoResult<()> {
fn add_package(
&mut self,
ws: &Workspace<'_>,
package: &Package,
tar: &FileLock,
) -> CargoResult<()> {
debug!(
"adding package {}@{} to local overlay at {}",
package.name(),
Expand All @@ -1317,7 +1322,7 @@ impl<'a> TmpRegistry<'a> {
tar_copy.flush()?;
}

let new_crate = super::registry::prepare_transmit(self.gctx, package, self.upstream)?;
let new_crate = super::registry::prepare_transmit(self.gctx, ws, package, self.upstream)?;

tar.file().seek(SeekFrom::Start(0))?;
let cksum = cargo_util::Sha256::new()
Expand Down
60 changes: 23 additions & 37 deletions src/cargo/ops/registry/publish.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
//! [1]: https://doc.rust-lang.org/nightly/cargo/reference/registry-web-api.html#publish

use std::collections::BTreeMap;
use std::collections::BTreeSet;
use std::collections::HashSet;
use std::fs::File;
use std::time::Duration;
Expand All @@ -21,7 +20,6 @@ use crate::core::dependency::DepKind;
use crate::core::manifest::ManifestMetadata;
use crate::core::resolver::CliFeatures;
use crate::core::Dependency;
use crate::core::FeatureValue;
use crate::core::Package;
use crate::core::PackageIdSpecQuery;
use crate::core::SourceId;
Expand All @@ -35,7 +33,7 @@ use crate::sources::CRATES_IO_REGISTRY;
use crate::util::auth;
use crate::util::cache_lock::CacheLockMode;
use crate::util::context::JobsConfig;
use crate::util::interning::InternedString;
use crate::util::toml::prepare_for_publish;
use crate::util::Progress;
use crate::util::ProgressStyle;
use crate::CargoResult;
Expand Down Expand Up @@ -185,6 +183,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
.status("Uploading", pkg.package_id().to_string())?;
transmit(
opts.gctx,
ws,
pkg,
tarball.file(),
&mut registry,
Expand Down Expand Up @@ -324,16 +323,16 @@ fn verify_dependencies(

pub(crate) fn prepare_transmit(
gctx: &GlobalContext,
pkg: &Package,
ws: &Workspace<'_>,
local_pkg: &Package,
registry_id: SourceId,
) -> CargoResult<NewCrate> {
let deps = pkg
let included = None; // don't filter build-targets
let publish_pkg = prepare_for_publish(local_pkg, ws, included)?;

let deps = publish_pkg
.dependencies()
.iter()
.filter(|dep| {
// Skip dev-dependency without version.
dep.is_transitive() || dep.specified_req()
})
.map(|dep| {
// If the dependency is from a different registry, then include the
// registry in the dependency.
Expand Down Expand Up @@ -378,7 +377,7 @@ pub(crate) fn prepare_transmit(
})
})
.collect::<CargoResult<Vec<NewCrateDependency>>>()?;
let manifest = pkg.manifest();
let manifest = publish_pkg.manifest();
let ManifestMetadata {
ref authors,
ref description,
Expand All @@ -395,53 +394,39 @@ pub(crate) fn prepare_transmit(
ref rust_version,
} = *manifest.metadata();
let rust_version = rust_version.as_ref().map(ToString::to_string);
let readme_content = readme
let readme_content = local_pkg
.manifest()
.metadata()
.readme
.as_ref()
.map(|readme| {
paths::read(&pkg.root().join(readme))
.with_context(|| format!("failed to read `readme` file for package `{}`", pkg))
paths::read(&local_pkg.root().join(readme)).with_context(|| {
format!("failed to read `readme` file for package `{}`", local_pkg)
})
})
.transpose()?;
if let Some(ref file) = *license_file {
if !pkg.root().join(file).exists() {
if let Some(ref file) = local_pkg.manifest().metadata().license_file {
if !local_pkg.root().join(file).exists() {
bail!("the license file `{}` does not exist", file)
}
}

let deps_set = deps
.iter()
.map(|dep| dep.name.clone())
.collect::<BTreeSet<String>>();

let string_features = match manifest.resolved_toml().features() {
Some(features) => features
.iter()
.map(|(feat, values)| {
(
feat.to_string(),
values
.iter()
.filter(|fv| {
let feature_value = FeatureValue::new(InternedString::new(fv));
match feature_value {
FeatureValue::Dep { dep_name }
| FeatureValue::DepFeature { dep_name, .. } => {
deps_set.contains(&dep_name.to_string())
}
_ => true,
}
})
.map(|fv| fv.to_string())
.collect(),
values.iter().map(|fv| fv.to_string()).collect(),
)
})
.collect::<BTreeMap<String, Vec<String>>>(),
None => BTreeMap::new(),
};

Ok(NewCrate {
name: pkg.name().to_string(),
vers: pkg.version().to_string(),
name: publish_pkg.name().to_string(),
vers: publish_pkg.version().to_string(),
deps,
features: string_features,
authors: authors.clone(),
Expand All @@ -463,13 +448,14 @@ pub(crate) fn prepare_transmit(

fn transmit(
gctx: &GlobalContext,
ws: &Workspace<'_>,
pkg: &Package,
tarball: &File,
registry: &mut Registry,
registry_id: SourceId,
dry_run: bool,
) -> CargoResult<()> {
let new_crate = prepare_transmit(gctx, pkg, registry_id)?;
let new_crate = prepare_transmit(gctx, ws, pkg, registry_id)?;

// Do not upload if performing a dry run
if dry_run {
Expand Down
45 changes: 26 additions & 19 deletions src/cargo/util/toml/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2551,15 +2551,16 @@ fn unused_dep_keys(
}
}

/// Make the [`Package`] self-contained so its ready for packaging
pub fn prepare_for_publish(
me: &Package,
ws: &Workspace<'_>,
included: &[PathBuf],
packaged_files: Option<&[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(), included)?;
prepare_toml_for_publish(me.manifest().resolved_toml(), ws, me.root(), packaged_files)?;
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 @@ -2591,7 +2592,7 @@ fn prepare_toml_for_publish(
me: &manifest::TomlManifest,
ws: &Workspace<'_>,
package_root: &Path,
included: &[PathBuf],
packaged_files: Option<&[PathBuf]>,
) -> CargoResult<manifest::TomlManifest> {
let gctx = ws.gctx();

Expand All @@ -2608,7 +2609,8 @@ 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 build = if included.contains(&path) {
let included = packaged_files.map(|i| i.contains(&path)).unwrap_or(true);
let build = if included {
let path = path
.into_os_string()
.into_string()
Expand Down Expand Up @@ -2712,14 +2714,16 @@ fn prepare_toml_for_publish(
}

let lib = if let Some(target) = &me.lib {
prepare_target_for_publish(target, included, "library", ws.gctx())?
prepare_target_for_publish(target, packaged_files, "library", ws.gctx())?
} else {
None
};
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 bin = prepare_targets_for_publish(me.bin.as_ref(), packaged_files, "binary", ws.gctx())?;
let example =
prepare_targets_for_publish(me.example.as_ref(), packaged_files, "example", ws.gctx())?;
let test = prepare_targets_for_publish(me.test.as_ref(), packaged_files, "test", ws.gctx())?;
let bench =
prepare_targets_for_publish(me.bench.as_ref(), packaged_files, "benchmark", ws.gctx())?;

let all = |_d: &manifest::TomlDependency| true;
let mut manifest = manifest::TomlManifest {
Expand Down Expand Up @@ -2877,7 +2881,7 @@ fn prepare_toml_for_publish(

fn prepare_targets_for_publish(
targets: Option<&Vec<manifest::TomlTarget>>,
included: &[PathBuf],
packaged_files: Option<&[PathBuf]>,
context: &str,
gctx: &GlobalContext,
) -> CargoResult<Option<Vec<manifest::TomlTarget>>> {
Expand All @@ -2887,7 +2891,8 @@ fn prepare_targets_for_publish(

let mut prepared = Vec::with_capacity(targets.len());
for target in targets {
let Some(target) = prepare_target_for_publish(target, included, context, gctx)? else {
let Some(target) = prepare_target_for_publish(target, packaged_files, context, gctx)?
else {
continue;
};
prepared.push(target);
Expand All @@ -2902,19 +2907,21 @@ fn prepare_targets_for_publish(

fn prepare_target_for_publish(
target: &manifest::TomlTarget,
included: &[PathBuf],
packaged_files: Option<&[PathBuf]>,
context: &str,
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);
if let Some(packaged_files) = packaged_files {
if !packaged_files.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);
}
}

let mut target = target.clone();
Expand Down
4 changes: 2 additions & 2 deletions tests/testsuite/inheritable_workspace_fields.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -752,11 +752,11 @@ You may press ctrl-c to skip waiting; the crate should be available shortly.
"homepage": "https://www.rust-lang.org",
"keywords": ["cli"],
"license": "MIT",
"license_file": "../LICENSE",
"license_file": "LICENSE",
"links": null,
"name": "bar",
"readme": "README.md",
"readme_file": "../README.md",
"readme_file": "README.md",
"repository": "https://github.com/example/example",
"rust_version": "1.60",
"vers": "1.2.3"
Expand Down
Loading

0 comments on commit d3e84f0

Please sign in to comment.