fix(publish): Don't strip non-dev features

epage · epage · commit eddf7b7b7b17 · 2024-07-30T14:54:37.000-05:00
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
diff --git 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<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));
         }
@@ -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(),
@@ -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
@@ -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<NewCrate> {
-    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::<CargoResult<Vec<NewCrateDependency>>>()?;
-    let manifest = local_pkg.manifest();
+    let manifest = publish_pkg.manifest();
     let ManifestMetadata {
         ref authors,
         ref description,
@@ -395,54 +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(&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 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: 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
@@ -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
@@ -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",
