From 4e5af28150543b388334de720d1b93c1fb712031 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Dec 2024 10:31:53 -0600 Subject: [PATCH 1/6] refactor(schema): Group TomlManifest fields to clarify requires_package I found a bug in the manifest parser and figured this would help make it more obvious. Since I was already changing the order, I figure I'm make things a little more logical (user-facing first, implementtion details later) --- crates/cargo-util-schemas/src/manifest/mod.rs | 20 +++++++------ src/cargo/util/toml/mod.rs | 26 ++++++++--------- tests/testsuite/features_namespaced.rs | 16 +++++------ tests/testsuite/publish.rs | 28 +++++++++---------- tests/testsuite/weak_dep_features.rs | 8 +++--- 5 files changed, 50 insertions(+), 48 deletions(-) diff --git a/crates/cargo-util-schemas/src/manifest/mod.rs b/crates/cargo-util-schemas/src/manifest/mod.rs index efb71d8ceac..bfb23495c3e 100644 --- a/crates/cargo-util-schemas/src/manifest/mod.rs +++ b/crates/cargo-util-schemas/src/manifest/mod.rs @@ -35,11 +35,13 @@ use crate::schema::TomlValueWrapper; #[serde(rename_all = "kebab-case")] #[cfg_attr(feature = "unstable-schema", derive(schemars::JsonSchema))] pub struct TomlManifest { - // when adding new fields, be sure to check whether `requires_package` should disallow them pub cargo_features: Option>, + + // Update `requires_package` when adding new package-specific fields pub package: Option>, pub project: Option>, - pub profile: Option, + pub badges: Option>>, + pub features: Option>>, pub lib: Option, pub bin: Option>, pub example: Option>, @@ -52,14 +54,14 @@ pub struct TomlManifest { pub build_dependencies: Option>, #[serde(rename = "build_dependencies")] pub build_dependencies2: Option>, - pub features: Option>>, pub target: Option>, - pub replace: Option>, - pub patch: Option>>, - pub workspace: Option, - pub badges: Option>>, pub lints: Option, + pub workspace: Option, + pub profile: Option, + pub patch: Option>>, + pub replace: Option>, + /// Report unused keys (see also nested `_unused_keys`) /// Note: this is populated by the caller, rather than automatically #[serde(skip)] @@ -69,6 +71,8 @@ pub struct TomlManifest { impl TomlManifest { pub fn requires_package(&self) -> impl Iterator { [ + self.badges.as_ref().map(|_| "badges"), + self.features.as_ref().map(|_| "features"), self.lib.as_ref().map(|_| "lib"), self.bin.as_ref().map(|_| "bin"), self.example.as_ref().map(|_| "example"), @@ -79,9 +83,7 @@ impl TomlManifest { self.build_dependencies() .as_ref() .map(|_| "build-dependencies"), - self.features.as_ref().map(|_| "features"), self.target.as_ref().map(|_| "target"), - self.badges.as_ref().map(|_| "badges"), self.lints.as_ref().map(|_| "lints"), ] .into_iter() diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index ab96cdd03b0..0a52c4f0c78 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -279,7 +279,8 @@ fn normalize_toml( cargo_features: original_toml.cargo_features.clone(), package: None, project: None, - profile: original_toml.profile.clone(), + badges: None, + features: None, lib: None, bin: None, example: None, @@ -290,13 +291,12 @@ fn normalize_toml( dev_dependencies2: None, build_dependencies: None, build_dependencies2: None, - features: None, target: None, - replace: original_toml.replace.clone(), - patch: None, - workspace: original_toml.workspace.clone(), - badges: None, lints: None, + workspace: original_toml.workspace.clone(), + profile: original_toml.profile.clone(), + patch: None, + replace: original_toml.replace.clone(), _unused_keys: Default::default(), }; @@ -2820,9 +2820,11 @@ fn prepare_toml_for_publish( let all = |_d: &manifest::TomlDependency| true; let mut manifest = manifest::TomlManifest { + cargo_features: me.cargo_features.clone(), package: Some(package), project: None, - profile: me.profile.clone(), + badges: me.badges.clone(), + features: me.features.clone(), lib, bin, example, @@ -2837,7 +2839,6 @@ fn prepare_toml_for_publish( dev_dependencies2: None, build_dependencies: map_deps(gctx, me.build_dependencies(), all)?, build_dependencies2: None, - features: me.features.clone(), target: match me.target.as_ref().map(|target_map| { target_map .iter() @@ -2863,12 +2864,11 @@ fn prepare_toml_for_publish( Some(Err(e)) => return Err(e), None => None, }, - replace: None, - patch: None, - workspace: None, - badges: me.badges.clone(), - cargo_features: me.cargo_features.clone(), lints: me.lints.clone(), + workspace: None, + profile: me.profile.clone(), + patch: None, + replace: None, _unused_keys: Default::default(), }; strip_features(&mut manifest); diff --git a/tests/testsuite/features_namespaced.rs b/tests/testsuite/features_namespaced.rs index 796a31edc70..d04625a0705 100644 --- a/tests/testsuite/features_namespaced.rs +++ b/tests/testsuite/features_namespaced.rs @@ -1006,6 +1006,9 @@ homepage = "https://example.com/" readme = false license = "MIT" +[features] +feat = ["opt-dep1"] + [lib] name = "foo" path = "src/lib.rs" @@ -1018,9 +1021,6 @@ optional = true version = "1.0" optional = true -[features] -feat = ["opt-dep1"] - "##]], )], ); @@ -1143,6 +1143,11 @@ homepage = "https://example.com/" readme = false license = "MIT" +[features] +feat1 = [] +feat2 = ["dep:bar"] +feat3 = ["feat2"] + [lib] name = "foo" path = "src/lib.rs" @@ -1151,11 +1156,6 @@ path = "src/lib.rs" version = "1.0" optional = true -[features] -feat1 = [] -feat2 = ["dep:bar"] -feat3 = ["feat2"] - "##]], )], ); diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 51ab6cfdaf7..4bcb9194416 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -2015,6 +2015,20 @@ readme = false license = "MIT" repository = "foo" +[features] +foo_feature = [ + "normal-only/cat", + "build-only/cat", + "normal-and-dev/cat", + "target-normal-only/cat", + "target-build-only/cat", + "target-normal-and-dev/cat", + "optional-dep-feature/cat", + "dep:optional-namespaced", + "optional-renamed-dep-feature10/cat", + "dep:optional-renamed-namespaced10", +] + [[bin]] name = "foo" path = "src/main.rs" @@ -2057,20 +2071,6 @@ features = ["cat"] version = "1.0" features = ["cat"] -[features] -foo_feature = [ - "normal-only/cat", - "build-only/cat", - "normal-and-dev/cat", - "target-normal-only/cat", - "target-build-only/cat", - "target-normal-and-dev/cat", - "optional-dep-feature/cat", - "dep:optional-namespaced", - "optional-renamed-dep-feature10/cat", - "dep:optional-renamed-namespaced10", -] - [target."cfg(unix)".dependencies.target-normal-and-dev] version = "1.0" features = ["cat"] diff --git a/tests/testsuite/weak_dep_features.rs b/tests/testsuite/weak_dep_features.rs index b0997414cfe..583f67c8b53 100644 --- a/tests/testsuite/weak_dep_features.rs +++ b/tests/testsuite/weak_dep_features.rs @@ -650,6 +650,10 @@ homepage = "https://example.com/" readme = false license = "MIT" +[features] +feat1 = [] +feat2 = ["bar?/feat"] + [lib] name = "foo" path = "src/lib.rs" @@ -658,10 +662,6 @@ path = "src/lib.rs" version = "1.0" optional = true -[features] -feat1 = [] -feat2 = ["bar?/feat"] - "##]], )], ); From aef2f6d1ae6f01f0b659717943b1bc30ac5ca3a8 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Dec 2024 10:48:31 -0600 Subject: [PATCH 2/6] test(patch): Ensure the patch actuall gets built --- tests/testsuite/patch.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index c919e1184ec..825dbea5937 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -1479,8 +1479,17 @@ fn patch_in_virtual() { .file("foo/src/lib.rs", r#""#) .build(); - p.cargo("check").run(); - p.cargo("check") + p.cargo("check -p foo") + .with_stderr_data(str![[r#" +[UPDATING] `dummy-registry` index +[LOCKING] 1 package to latest compatible version +[CHECKING] bar v0.1.0 ([ROOT]/foo/bar) +[CHECKING] foo v0.1.0 ([ROOT]/foo/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]) + .run(); + p.cargo("check -p foo") .with_stderr_data(str![[r#" [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s From cb30ab61d15f1233cb2c961bb048cdd079b2358c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Dec 2024 10:50:55 -0600 Subject: [PATCH 3/6] refactor(toml): Initialize fields before everything else --- src/cargo/util/toml/mod.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 0a52c4f0c78..81155915191 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -275,6 +275,15 @@ fn normalize_toml( warnings: &mut Vec, errors: &mut Vec, ) -> CargoResult { + let package_root = manifest_file.parent().unwrap(); + + let inherit_cell: LazyCell = LazyCell::new(); + let inherit = || { + inherit_cell + .try_borrow_with(|| load_inheritable_fields(gctx, manifest_file, &workspace_config)) + }; + let workspace_root = || inherit().map(|fields| fields.ws_root().as_path()); + let mut normalized_toml = manifest::TomlManifest { cargo_features: original_toml.cargo_features.clone(), package: None, @@ -300,15 +309,6 @@ fn normalize_toml( _unused_keys: Default::default(), }; - let package_root = manifest_file.parent().unwrap(); - - let inherit_cell: LazyCell = LazyCell::new(); - let inherit = || { - inherit_cell - .try_borrow_with(|| load_inheritable_fields(gctx, manifest_file, &workspace_config)) - }; - let workspace_root = || inherit().map(|fields| fields.ws_root().as_path()); - if let Some(original_package) = original_toml.package() { let package_name = &original_package.name; From e83dc0405ba0f14c166c4a0954fb119b961001da Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Dec 2024 10:55:20 -0600 Subject: [PATCH 4/6] test(patch): Speed up path-bases test --- tests/testsuite/patch.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index 825dbea5937..e3e14b90587 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -3078,7 +3078,12 @@ fn patch_with_base() { .file("src/lib.rs", "use bar::hello as _;") .build(); - p.cargo("build -v") + p.cargo("tree") .masquerade_as_nightly_cargo(&["path-bases"]) + .with_stdout_data(str![[r#" +foo v0.5.0 ([ROOT]/foo) +└── bar v0.5.0 ([ROOT]/bar) + +"#]]) .run(); } From 2d23b94a7f5f86bdf74fd5da2617e82ffad1f222 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Dec 2024 11:00:04 -0600 Subject: [PATCH 5/6] test(base): Verify bases in patches in virtual manifests --- tests/testsuite/patch.rs | 69 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index e3e14b90587..eb5efd4c33b 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -3038,7 +3038,7 @@ foo v0.0.0 ([ROOT]/foo) } #[cargo_test] -fn patch_with_base() { +fn patch_in_real_with_base() { let bar = project() .at("bar") .file("Cargo.toml", &basic_manifest("bar", "0.5.0")) @@ -3087,3 +3087,70 @@ foo v0.5.0 ([ROOT]/foo) "#]]) .run(); } + +#[cargo_test] +fn patch_in_virtual_with_base() { + let bar = project() + .at("bar") + .file("Cargo.toml", &basic_manifest("bar", "0.5.0")) + .file("src/lib.rs", "pub fn hello() {}") + .build(); + Package::new("bar", "0.5.0").publish(); + + let p = project() + .file( + ".cargo/config.toml", + &format!( + r#" + [path-bases] + test = '{}' + "#, + bar.root().parent().unwrap().display() + ), + ) + .file( + "Cargo.toml", + r#" + cargo-features = ["path-bases"] + + [workspace] + members = ["foo"] + + [patch.crates-io] + bar = { base = 'test', path = 'bar' } + "#, + ) + .file( + "foo/Cargo.toml", + r#" + [package] + name = "foo" + version = "0.5.0" + authors = ["wycats@example.com"] + edition = "2018" + + [dependencies] + bar = "0.5.0" + "#, + ) + .file("foo/src/lib.rs", "use bar::hello as _;") + .build(); + + p.cargo("tree") + .masquerade_as_nightly_cargo(&["path-bases"]) + .with_status(101) + .with_stderr_data(str![[r#" +[ERROR] failed to load source for dependency `bar` + +Caused by: + Unable to update [ROOT]/foo/bar + +Caused by: + failed to read `[ROOT]/foo/bar/Cargo.toml` + +Caused by: + [NOT_FOUND] + +"#]]) + .run(); +} From 5b8b2ac248dca8727c9a10cb8fd6376d9e4f0051 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Dec 2024 11:00:57 -0600 Subject: [PATCH 6/6] fix(base): Support bases in patches in virtual manifests This bug has been there since #14360 --- src/cargo/util/toml/mod.rs | 22 ++++++++++------------ tests/testsuite/patch.rs | 15 +++------------ 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 81155915191..bc80097b705 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -304,7 +304,12 @@ fn normalize_toml( lints: None, workspace: original_toml.workspace.clone(), profile: original_toml.profile.clone(), - patch: None, + patch: normalize_patch( + gctx, + original_toml.patch.as_ref(), + &workspace_root, + features, + )?, replace: original_toml.replace.clone(), _unused_keys: Default::default(), }; @@ -483,13 +488,6 @@ fn normalize_toml( } normalized_toml.target = (!normalized_target.is_empty()).then_some(normalized_target); - normalized_toml.patch = normalize_patch( - gctx, - original_toml.patch.as_ref(), - &workspace_root, - features, - )?; - let normalized_lints = original_toml .lints .clone() @@ -1733,14 +1731,14 @@ fn to_virtual_manifest( root, }; ( - replace(&original_toml, &mut manifest_ctx)?, - patch(&original_toml, &mut manifest_ctx)?, + replace(&normalized_toml, &mut manifest_ctx)?, + patch(&normalized_toml, &mut manifest_ctx)?, ) }; - if let Some(profiles) = &original_toml.profile { + if let Some(profiles) = &normalized_toml.profile { validate_profiles(profiles, gctx.cli_unstable(), &features, warnings)?; } - let resolve_behavior = original_toml + let resolve_behavior = normalized_toml .workspace .as_ref() .and_then(|ws| ws.resolver.as_deref()) diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index eb5efd4c33b..ff46b324091 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -3138,18 +3138,9 @@ fn patch_in_virtual_with_base() { p.cargo("tree") .masquerade_as_nightly_cargo(&["path-bases"]) - .with_status(101) - .with_stderr_data(str![[r#" -[ERROR] failed to load source for dependency `bar` - -Caused by: - Unable to update [ROOT]/foo/bar - -Caused by: - failed to read `[ROOT]/foo/bar/Cargo.toml` - -Caused by: - [NOT_FOUND] + .with_stdout_data(str![[r#" +foo v0.5.0 ([ROOT]/foo/foo) +└── bar v0.5.0 ([ROOT]/bar) "#]]) .run();