From ad7d6502ae903f787ba82fd3ea0da6be88c6722d Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Mon, 22 Jul 2024 15:58:25 +0800 Subject: [PATCH 1/7] test: The latest rustc version should not appear in the test output --- tests/testsuite/lints/implicit_features.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testsuite/lints/implicit_features.rs b/tests/testsuite/lints/implicit_features.rs index 37a12ba69d9..174bc1d4a94 100644 --- a/tests/testsuite/lints/implicit_features.rs +++ b/tests/testsuite/lints/implicit_features.rs @@ -127,7 +127,7 @@ unused_optional_dependency = "allow" .masquerade_as_nightly_cargo(&["cargo-lints", "edition2024"]) .with_stderr_data(str![[r#" [UPDATING] `dummy-registry` index -[LOCKING] 2 packages to latest Rust 1.81.0-nightly compatible versions +[LOCKING] 2 packages to latest Rust [..] compatible versions [CHECKING] foo v0.1.0 ([ROOT]/foo) [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s From 3eb2986c98c8879bdcea38ce0212b87849ed433f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 30 Jul 2024 13:22:16 -0500 Subject: [PATCH 2/7] test(publish): Consolidate dev-dep stripping tests --- tests/testsuite/publish.rs | 104 +------------------------------------ 1 file changed, 2 insertions(+), 102 deletions(-) diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index c03e726d6da..79da7647b47 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -1598,108 +1598,7 @@ You may press ctrl-c to skip waiting; the crate should be available shortly. } #[cargo_test] -fn publish_dev_dep_no_version() { - let registry = RegistryBuilder::new().http_api().http_index().build(); - - let p = project() - .file( - "Cargo.toml", - r#" - [package] - name = "foo" - version = "0.1.0" - edition = "2015" - authors = [] - license = "MIT" - description = "foo" - documentation = "foo" - homepage = "foo" - repository = "foo" - - [dev-dependencies] - bar = { path = "bar" } - "#, - ) - .file("src/lib.rs", "") - .file("bar/Cargo.toml", &basic_manifest("bar", "0.0.1")) - .file("bar/src/lib.rs", "") - .build(); - - p.cargo("publish --no-verify") - .replace_crates_io(registry.index_url()) - .with_stderr_data(str![[r#" -[UPDATING] crates.io index -[PACKAGING] foo v0.1.0 ([ROOT]/foo) -[PACKAGED] 3 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) -[UPLOADING] foo v0.1.0 ([ROOT]/foo) -[UPLOADED] foo v0.1.0 to registry `crates-io` -[NOTE] waiting for `foo v0.1.0` to be available at registry `crates-io`. -You may press ctrl-c to skip waiting; the crate should be available shortly. -[PUBLISHED] foo v0.1.0 at registry `crates-io` - -"#]]) - .run(); - - publish::validate_upload_with_contents( - r#" - { - "authors": [], - "badges": {}, - "categories": [], - "deps": [], - "description": "foo", - "documentation": "foo", - "features": {}, - "homepage": "foo", - "keywords": [], - "license": "MIT", - "license_file": null, - "links": null, - "name": "foo", - "readme": null, - "readme_file": null, - "repository": "foo", - "rust_version": null, - "vers": "0.1.0" - } - "#, - "foo-0.1.0.crate", - &["Cargo.toml", "Cargo.toml.orig", "src/lib.rs"], - &[( - "Cargo.toml", - &format!( - r#"{} -[package] -edition = "2015" -name = "foo" -version = "0.1.0" -authors = [] -build = false -autobins = false -autoexamples = false -autotests = false -autobenches = false -description = "foo" -homepage = "foo" -documentation = "foo" -readme = false -license = "MIT" -repository = "foo" - -[lib] -name = "foo" -path = "src/lib.rs" - -[dev-dependencies] -"#, - cargo::core::manifest::MANIFEST_PREAMBLE - ), - )], - ); -} - -#[cargo_test] -fn publish_with_feature_point_diff_kinds_dep() { +fn publish_dev_dep_stripping() { let registry = RegistryBuilder::new().http_api().http_index().build(); Package::new("normal-only", "1.0.0") .feature("cat", &[]) @@ -2005,6 +1904,7 @@ features = ["cat"] )], ); } + #[cargo_test] fn credentials_ambiguous_filename() { // `publish` generally requires a remote registry From 68147de37e1c6a6ddebc427268b729b0cbdb8ffa Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 30 Jul 2024 13:36:28 -0500 Subject: [PATCH 3/7] test(publish): Add more dev-dep stripping regression cases --- tests/testsuite/publish.rs | 48 +++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 79da7647b47..9c4eabad597 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -1603,6 +1603,12 @@ fn publish_dev_dep_stripping() { Package::new("normal-only", "1.0.0") .feature("cat", &[]) .publish(); + Package::new("optional-dep-feature", "1.0.0") + .feature("cat", &[]) + .publish(); + Package::new("optional-namespaced", "1.0.0") + .feature("cat", &[]) + .publish(); Package::new("build-only", "1.0.0") .feature("cat", &[]) .publish(); @@ -1644,11 +1650,15 @@ fn publish_dev_dep_stripping() { "target-build-only/cat", "target-dev-only/cat", "target-normal-and-dev/cat", + "optional-dep-feature/cat", + "dep:optional-namespaced", ] [dependencies] normal-only = { version = "1.0", features = ["cat"] } normal-and-dev = { version = "1.0", features = ["cat"] } + optional-dep-feature = { version = "1.0", features = ["cat"], optional = true } + optional-namespaced = { version = "1.0", features = ["cat"], optional = true } [build-dependencies] build-only = { version = "1.0", features = ["cat"] } @@ -1738,6 +1748,28 @@ You may press ctrl-c to skip waiting; the crate should be available shortly. "target": null, "version_req": "^1.0" }, + { + "default_features": true, + "features": [ + "cat" + ], + "kind": "normal", + "name": "optional-dep-feature", + "optional": true, + "target": null, + "version_req": "^1.0" + }, + { + "default_features": true, + "features": [ + "cat" + ], + "kind": "normal", + "name": "optional-namespaced", + "optional": true, + "target": null, + "version_req": "^1.0" + }, { "default_features": true, "features": [ @@ -1814,7 +1846,9 @@ You may press ctrl-c to skip waiting; the crate should be available shortly. "normal-and-dev/cat", "target-normal-only/cat", "target-build-only/cat", - "target-normal-and-dev/cat" + "target-normal-and-dev/cat", + "optional-dep-feature/cat", + "dep:optional-namespaced" ] }, "homepage": "foo", @@ -1865,6 +1899,16 @@ features = ["cat"] version = "1.0" features = ["cat"] +[dependencies.optional-dep-feature] +version = "1.0" +features = ["cat"] +optional = true + +[dependencies.optional-namespaced] +version = "1.0" +features = ["cat"] +optional = true + [dev-dependencies.normal-and-dev] version = "1.0" features = ["cat"] @@ -1881,6 +1925,8 @@ foo_feature = [ "target-normal-only/cat", "target-build-only/cat", "target-normal-and-dev/cat", + "optional-dep-feature/cat", + "dep:optional-namespaced", ] [target."cfg(unix)".dependencies.target-normal-and-dev] From a4b1143f80e98ab1947ed411450caf6d3d42e4a9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 30 Jul 2024 13:46:58 -0500 Subject: [PATCH 4/7] test(publish): Show bad dev-dep stripping See #14321 --- tests/testsuite/publish.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 9c4eabad597..753d40101f2 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -1609,6 +1609,9 @@ fn publish_dev_dep_stripping() { Package::new("optional-namespaced", "1.0.0") .feature("cat", &[]) .publish(); + Package::new("optional-renamed-namespaced", "1.0.0") + .feature("cat", &[]) + .publish(); Package::new("build-only", "1.0.0") .feature("cat", &[]) .publish(); @@ -1652,6 +1655,7 @@ fn publish_dev_dep_stripping() { "target-normal-and-dev/cat", "optional-dep-feature/cat", "dep:optional-namespaced", + "dep:optional-renamed-namespaced10", ] [dependencies] @@ -1659,6 +1663,7 @@ fn publish_dev_dep_stripping() { normal-and-dev = { version = "1.0", features = ["cat"] } optional-dep-feature = { version = "1.0", features = ["cat"], optional = true } optional-namespaced = { version = "1.0", features = ["cat"], optional = true } + optional-renamed-namespaced10 = { version = "1.0", features = ["cat"], optional = true, package = "optional-renamed-namespaced" } [build-dependencies] build-only = { version = "1.0", features = ["cat"] } @@ -1770,6 +1775,18 @@ You may press ctrl-c to skip waiting; the crate should be available shortly. "target": null, "version_req": "^1.0" }, + { + "default_features": true, + "explicit_name_in_toml": "optional-renamed-namespaced10", + "features": [ + "cat" + ], + "kind": "normal", + "name": "optional-renamed-namespaced", + "optional": true, + "target": null, + "version_req": "^1.0" + }, { "default_features": true, "features": [ @@ -1909,6 +1926,12 @@ version = "1.0" features = ["cat"] optional = true +[dependencies.optional-renamed-namespaced10] +version = "1.0" +features = ["cat"] +optional = true +package = "optional-renamed-namespaced" + [dev-dependencies.normal-and-dev] version = "1.0" features = ["cat"] @@ -1927,6 +1950,7 @@ foo_feature = [ "target-normal-and-dev/cat", "optional-dep-feature/cat", "dep:optional-namespaced", + "dep:optional-renamed-namespaced10", ] [target."cfg(unix)".dependencies.target-normal-and-dev] From 09eb536749179cf7c324d8c192b6767674945eed Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 30 Jul 2024 14:37:54 -0500 Subject: [PATCH 5/7] refactor(package): Make target filtering optional We'll be doing a second `prepare_for_publish` call that doesn't need the filtering, we don't have access to the `included`, and we don't want the warnings duplicated. --- src/cargo/ops/cargo_package.rs | 2 +- src/cargo/util/toml/mod.rs | 27 +++++++++++++++------------ 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index b32f2ed20d0..b33383422cd 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -732,7 +732,7 @@ fn tar( .iter() .map(|ar_file| ar_file.rel_path.clone()) .collect::>(); - 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 { diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 5bd6f79fe48..ef66abf24d5 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2575,7 +2575,7 @@ fn unused_dep_keys( pub fn prepare_for_publish( me: &Package, ws: &Workspace<'_>, - included: &[PathBuf], + included: Option<&[PathBuf]>, ) -> CargoResult { let contents = me.manifest().contents(); let document = me.manifest().document(); @@ -2612,7 +2612,7 @@ fn prepare_toml_for_publish( me: &manifest::TomlManifest, ws: &Workspace<'_>, package_root: &Path, - included: &[PathBuf], + included: Option<&[PathBuf]>, ) -> CargoResult { let gctx = ws.gctx(); @@ -2629,7 +2629,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 = included.map(|i| i.contains(&path)).unwrap_or(true); + let build = if included { let path = path .into_os_string() .into_string() @@ -2898,7 +2899,7 @@ fn prepare_toml_for_publish( fn prepare_targets_for_publish( targets: Option<&Vec>, - included: &[PathBuf], + included: Option<&[PathBuf]>, context: &str, gctx: &GlobalContext, ) -> CargoResult>> { @@ -2923,19 +2924,21 @@ fn prepare_targets_for_publish( fn prepare_target_for_publish( target: &manifest::TomlTarget, - included: &[PathBuf], + included: Option<&[PathBuf]>, context: &str, gctx: &GlobalContext, ) -> CargoResult> { 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(included) = included { + 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); + } } let mut target = target.clone(); From aa68ea788d5968f0c45829764443aed6275b5991 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 30 Jul 2024 14:39:55 -0500 Subject: [PATCH 6/7] refactor(publish): Clarify we prepare_transmit using the local package --- src/cargo/ops/registry/publish.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/cargo/ops/registry/publish.rs b/src/cargo/ops/registry/publish.rs index 4ce5e36a50a..dad6ff5e7be 100644 --- a/src/cargo/ops/registry/publish.rs +++ b/src/cargo/ops/registry/publish.rs @@ -323,13 +323,13 @@ fn verify_dependencies( fn transmit( gctx: &GlobalContext, - pkg: &Package, + local_pkg: &Package, tarball: &File, registry: &mut Registry, registry_id: SourceId, dry_run: bool, ) -> CargoResult<()> { - let deps = pkg + let deps = local_pkg .dependencies() .iter() .filter(|dep| { @@ -380,7 +380,7 @@ fn transmit( }) }) .collect::>>()?; - let manifest = pkg.manifest(); + let manifest = local_pkg.manifest(); let ManifestMetadata { ref authors, ref description, @@ -400,12 +400,13 @@ fn transmit( let readme_content = 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 !local_pkg.root().join(file).exists() { bail!("the license file `{}` does not exist", file) } } @@ -450,8 +451,8 @@ fn transmit( let warnings = registry .publish( &NewCrate { - name: pkg.name().to_string(), - vers: pkg.version().to_string(), + name: local_pkg.name().to_string(), + vers: local_pkg.version().to_string(), deps, features: string_features, authors: authors.clone(), From 25ecfce902c9bffce868c5283237d95b9c697cf3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 30 Jul 2024 14:54:37 -0500 Subject: [PATCH 7/7] 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/registry/publish.rs | 44 ++++++------------- .../testsuite/inheritable_workspace_fields.rs | 4 +- tests/testsuite/publish.rs | 3 +- 3 files changed, 18 insertions(+), 33 deletions(-) diff --git a/src/cargo/ops/registry/publish.rs b/src/cargo/ops/registry/publish.rs index dad6ff5e7be..d00485355ca 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; @@ -184,6 +182,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, @@ -323,19 +322,19 @@ fn verify_dependencies( fn transmit( gctx: &GlobalContext, + ws: &Workspace<'_>, local_pkg: &Package, tarball: &File, registry: &mut Registry, registry_id: SourceId, dry_run: bool, ) -> 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. @@ -380,7 +379,7 @@ fn transmit( }) }) .collect::>>()?; - let manifest = local_pkg.manifest(); + let manifest = publish_pkg.manifest(); let ManifestMetadata { ref authors, ref description, @@ -397,7 +396,10 @@ fn 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(|| { @@ -405,7 +407,7 @@ fn 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) } @@ -417,31 +419,13 @@ fn transmit( return Ok(()); } - 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::>>(), 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",