From eddf7b7b7b173b9022209c8794ee174c0f7123b7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 30 Jul 2024 14:54:37 -0500 Subject: [PATCH] fix(publish): Don't strip non-dev features 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`. 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. Fixes #14321 --- src/cargo/ops/cargo_package.rs | 11 ++-- src/cargo/ops/registry/publish.rs | 51 +++++++------------ .../testsuite/inheritable_workspace_fields.rs | 4 +- tests/testsuite/publish.rs | 3 +- 4 files changed, 30 insertions(+), 39 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index cc4e517c624..459e780f32f 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -288,7 +288,7 @@ pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult 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(), @@ -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() diff --git a/src/cargo/ops/registry/publish.rs b/src/cargo/ops/registry/publish.rs index e58e0a5164d..592a1cb0765 100644 --- a/src/cargo/ops/registry/publish.rs +++ b/src/cargo/ops/registry/publish.rs @@ -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; @@ -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; @@ -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; @@ -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, @@ -324,16 +323,16 @@ fn verify_dependencies( pub(crate) fn prepare_transmit( gctx: &GlobalContext, + ws: &Workspace<'_>, local_pkg: &Package, registry_id: SourceId, ) -> CargoResult { - let deps = local_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. @@ -378,7 +377,7 @@ pub(crate) fn prepare_transmit( }) }) .collect::>>()?; - let manifest = local_pkg.manifest(); + let manifest = publish_pkg.manifest(); let ManifestMetadata { ref authors, ref description, @@ -395,7 +394,10 @@ 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(&local_pkg.root().join(readme)).with_context(|| { @@ -403,37 +405,19 @@ pub(crate) fn prepare_transmit( }) }) .transpose()?; - if let Some(ref file) = *license_file { + 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::>(); - 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::>>(), @@ -441,8 +425,8 @@ pub(crate) fn prepare_transmit( }; Ok(NewCrate { - name: local_pkg.name().to_string(), - vers: local_pkg.version().to_string(), + name: publish_pkg.name().to_string(), + vers: publish_pkg.version().to_string(), deps, features: string_features, authors: authors.clone(), @@ -464,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 { diff --git a/tests/testsuite/inheritable_workspace_fields.rs b/tests/testsuite/inheritable_workspace_fields.rs index 70fb61197f8..664e25602c1 100644 --- a/tests/testsuite/inheritable_workspace_fields.rs +++ b/tests/testsuite/inheritable_workspace_fields.rs @@ -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" diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 753d40101f2..cb97657040d 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -1865,7 +1865,8 @@ You may press ctrl-c to skip waiting; the crate should be available shortly. "target-build-only/cat", "target-normal-and-dev/cat", "optional-dep-feature/cat", - "dep:optional-namespaced" + "dep:optional-namespaced", + "dep:optional-renamed-namespaced10" ] }, "homepage": "foo",