From f05a5f8d6af09535df683b717e997f6e153a6b3b Mon Sep 17 00:00:00 2001 From: AudaciousAxiom <179637270+AudaciousAxiom@users.noreply.github.com> Date: Sun, 12 Jan 2025 09:38:15 +0100 Subject: [PATCH 1/2] test(feature-metadata): test valid and invalid feature metadata --- tests/testsuite/features.rs | 208 ++++++++++++++++++++++++++++++++++++ 1 file changed, 208 insertions(+) diff --git a/tests/testsuite/features.rs b/tests/testsuite/features.rs index eb2bd6f0012..2a6189c86c4 100644 --- a/tests/testsuite/features.rs +++ b/tests/testsuite/features.rs @@ -2458,3 +2458,211 @@ required-features = ["feat"] p.cargo(&format!("check --target={host} --examples --frozen")) .run(); } + +#[cargo_test] +fn feature_metadata() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["feature-metadata"] + + [package] + name = "foo" + edition = "2015" + + [features] + a = [] + b = [] + c = { enables = ["a", "b"] } + "#, + ) + .file( + "src/main.rs", + r#" + fn main() { + #[cfg(not(all(feature = "a", feature = "b")))] + compile_error!("Cargo features `a` and `b` must be enabled"); + } + "#, + ) + .build(); + + p.cargo("check --features c") + .masquerade_as_nightly_cargo(&["feature-metadata"]) + .with_status(101) + .with_stderr_data(str![[r#" +[ERROR] invalid type: map, expected a sequence + --> Cargo.toml:11:21 + | +11 | c = { enables = ["a", "b"] } + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +"#]]) + .run(); +} + +#[cargo_test] +fn feature_metadata_is_unstable() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + edition = "2015" + + [features] + a = { enables = [] } + "#, + ) + .file("src/main.rs", "") + .build(); + + p.cargo("check --features a") + .with_status(101) + .with_stderr_data(str![[r#" +[ERROR] invalid type: map, expected a sequence + --> Cargo.toml:7:21 + | +7 | a = { enables = [] } + | ^^^^^^^^^^^^^^^^ + +"#]]) + .run(); +} + +#[cargo_test] +fn feature_metadata_missing_enables() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["feature-metadata"] + + [package] + name = "foo" + edition = "2015" + + [features] + foo = {} + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .masquerade_as_nightly_cargo(&["feature-metadata"]) + .with_status(101) + .with_stderr_data(str![[r#" +[ERROR] invalid type: map, expected a sequence + --> Cargo.toml:9:23 + | +9 | foo = {} + | ^^ + +"#]]) + .run(); +} + +#[cargo_test] +fn feature_metadata_empty_enables() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["feature-metadata"] + + [package] + name = "foo" + edition = "2015" + + [features] + foo = { enables = [] } + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .masquerade_as_nightly_cargo(&["feature-metadata"]) + .with_status(101) + .with_stderr_data(str![[r#" +[ERROR] invalid type: map, expected a sequence + --> Cargo.toml:9:23 + | +9 | foo = { enables = [] } + | ^^^^^^^^^^^^^^^^ + +"#]]) + .run(); +} + +#[cargo_test] +fn unused_keys_in_feature_metadata() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["feature-metadata"] + + [package] + name = "foo" + edition = "2015" + + [features] + foo = { enables = ["bar"], a = false, b = true } + bar = [] + "#, + ) + .file("src/lib.rs", "") + .build(); + + p.cargo("check") + .masquerade_as_nightly_cargo(&["feature-metadata"]) + .with_status(101) + .with_stderr_data(str![[r#" +[ERROR] invalid type: map, expected a sequence + --> Cargo.toml:9:23 + | +9 | foo = { enables = ["bar"], a = false, b = true } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +"#]]) + .run(); +} + +#[cargo_test] +fn normalize_feature_metadata() { + let p = project() + .file( + "Cargo.toml", + r#" + cargo-features = ["feature-metadata"] + + [package] + name = "foo" + edition = "2015" + + [features] + a = [] + b = [] + c = { enables = ["a", "b"] } + "#, + ) + .file("src/main.rs", "") + .build(); + + p.cargo("package --no-verify") + .masquerade_as_nightly_cargo(&["feature-metadata"]) + .with_status(101) + .with_stderr_data(str![[r#" +[ERROR] invalid type: map, expected a sequence + --> Cargo.toml:11:21 + | +11 | c = { enables = ["a", "b"] } + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +"#]]) + .run(); +} From 5110662cff7c2d443da863591b01281a859bce3d Mon Sep 17 00:00:00 2001 From: AudaciousAxiom <179637270+AudaciousAxiom@users.noreply.github.com> Date: Sun, 12 Jan 2025 09:51:18 +0100 Subject: [PATCH 2/2] feat(manifest)!: implement feature-metadata RFC3416 --- .../cargo-util-schemas/manifest.schema.json | 37 ++++++++- crates/cargo-util-schemas/src/manifest/mod.rs | 61 ++++++++++++++- doc/book/src/reference/unstable.md | 16 ++++ src/ops/registry/cargo_publish.rs | 2 +- src/workspace/features.rs | 3 + src/workspace/parser/mod.rs | 61 +++++++++++---- tests/testsuite/features.rs | 75 +++++++++++-------- 7 files changed, 204 insertions(+), 51 deletions(-) diff --git a/crates/cargo-util-schemas/manifest.schema.json b/crates/cargo-util-schemas/manifest.schema.json index 219ff8a9793..c190bf8d784 100644 --- a/crates/cargo-util-schemas/manifest.schema.json +++ b/crates/cargo-util-schemas/manifest.schema.json @@ -51,10 +51,7 @@ "null" ], "additionalProperties": { - "type": "array", - "items": { - "type": "string" - } + "$ref": "#/$defs/FeatureDefinition" } }, "lib": { @@ -626,6 +623,38 @@ ] }, "TomlValue": true, + "FeatureDefinition": { + "description": "Definition of a feature.", + "anyOf": [ + { + "description": "Features that this feature enables.", + "type": "array", + "items": { + "type": "string" + } + }, + { + "description": "Unstable feature `feature-metadata`. Metadata of this feature.", + "$ref": "#/$defs/FeatureMetadata" + } + ] + }, + "FeatureMetadata": { + "description": "Unstable feature `feature-metadata`. Metadata of a feature.", + "type": "object", + "properties": { + "enables": { + "description": "Features that this feature enables.", + "type": "array", + "items": { + "type": "string" + } + } + }, + "required": [ + "enables" + ] + }, "TomlTarget": { "type": "object", "properties": { diff --git a/crates/cargo-util-schemas/src/manifest/mod.rs b/crates/cargo-util-schemas/src/manifest/mod.rs index 2b0ca539b0b..72eb7ee81ea 100644 --- a/crates/cargo-util-schemas/src/manifest/mod.rs +++ b/crates/cargo-util-schemas/src/manifest/mod.rs @@ -41,7 +41,7 @@ pub struct TomlManifest { pub package: Option>, pub project: Option>, pub badges: Option>>, - pub features: Option>>, + pub features: Option>, pub lib: Option, pub bin: Option>, pub example: Option>, @@ -112,7 +112,7 @@ impl TomlManifest { .or(self.build_dependencies2.as_ref()) } - pub fn features(&self) -> Option<&BTreeMap>> { + pub fn features(&self) -> Option<&BTreeMap> { self.features.as_ref() } @@ -1559,6 +1559,63 @@ impl TomlPlatform { } } +/// Definition of a feature. +#[derive(Clone, Debug, Serialize)] +#[cfg_attr(feature = "unstable-schema", derive(schemars::JsonSchema))] +#[serde(untagged)] +pub enum FeatureDefinition { + /// Features that this feature enables. + Array(Vec), + /// Unstable feature `feature-metadata`. Metadata of this feature. + Metadata(FeatureMetadata), +} + +// Implementing `Deserialize` manually allows for a better error message when the `enables` key is +// missing. +impl<'de> de::Deserialize<'de> for FeatureDefinition { + fn deserialize(d: D) -> Result + where + D: de::Deserializer<'de>, + { + UntaggedEnumVisitor::new() + .seq(|seq| { + seq.deserialize::>() + .map(FeatureDefinition::Array) + }) + .map(|seq| { + seq.deserialize::() + .map(FeatureDefinition::Metadata) + }) + .deserialize(d) + } +} + +impl FeatureDefinition { + /// Returns the features that this feature enables. + pub fn enables(&self) -> &[String] { + match self { + Self::Array(features) => features, + Self::Metadata(FeatureMetadata { + enables: features, .. + }) => features, + } + } +} + +/// Unstable feature `feature-metadata`. Metadata of a feature. +#[derive(Clone, Debug, Deserialize, Serialize)] +#[cfg_attr(feature = "unstable-schema", derive(schemars::JsonSchema))] +pub struct FeatureMetadata { + /// Features that this feature enables. + pub enables: Vec, + + /// This is here to provide a way to see the "unused manifest keys" when deserializing + #[serde(skip_serializing)] + #[serde(flatten)] + #[cfg_attr(feature = "unstable-schema", schemars(skip))] + pub _unused_keys: BTreeMap, +} + #[derive(Serialize, Debug, Clone)] #[cfg_attr(feature = "unstable-schema", derive(schemars::JsonSchema))] pub struct InheritableLints { diff --git a/doc/book/src/reference/unstable.md b/doc/book/src/reference/unstable.md index 2a74a601a4d..d739c38fbb3 100644 --- a/doc/book/src/reference/unstable.md +++ b/doc/book/src/reference/unstable.md @@ -115,6 +115,7 @@ Each new feature described below should explain how to use it. * [artifact dependencies](#artifact-dependencies) --- Allow build artifacts to be included into other build artifacts and build them for different targets. * [Profile `trim-paths` option](#profile-trim-paths-option) --- Control the sanitization of file paths in build outputs. * [path bases](#path-bases) --- Named base directories for path dependencies. + * [feature-metadata](#feature-metadata) --- Table syntax for feature definitions. * [`unstable-editions`](#unstable-editions) --- Allows use of editions that are not yet stable. * Information and metadata * [unit-graph](#unit-graph) --- Emits JSON for Cargo's internal graph structure. @@ -2444,6 +2445,21 @@ See the [`include` config documentation](config.md#include) for more. The `pubtime` index field has been stabilized in Rust 1.94.0. +## feature-metadata + +* Tracking Issue: [#14157](https://github.com/rust-lang/cargo/issues/14157) + +This allows to use a table when defining features, with a required `enables` key: + +```toml +[features] +# same as `foo = []` +foo = { enables = [] } +``` + +This is equivalent to the array-of-strings syntax. +Support for other keys should be added later. + ## lockfile-path Support for `resolver.lockfile-path` config field has been stabilized in Rust 1.97.0. diff --git a/src/ops/registry/cargo_publish.rs b/src/ops/registry/cargo_publish.rs index 3ec04b1a0dc..46e20e31280 100644 --- a/src/ops/registry/cargo_publish.rs +++ b/src/ops/registry/cargo_publish.rs @@ -617,7 +617,7 @@ pub(crate) fn prepare_transmit( .map(|(feat, values)| { ( feat.to_string(), - values.iter().map(|fv| fv.to_string()).collect(), + values.enables().iter().map(|fv| fv.to_string()).collect(), ) }) .collect::>>(), diff --git a/src/workspace/features.rs b/src/workspace/features.rs index 94043e76292..3b77edae581 100644 --- a/src/workspace/features.rs +++ b/src/workspace/features.rs @@ -583,6 +583,9 @@ features! { /// Allows use of panic="immediate-abort". (unstable, panic_immediate_abort, "", "reference/unstable.html#panic-immediate-abort"), + + /// Allow to use a table for defining features. + (unstable, feature_metadata, "", "reference/unstable.html#feature_metadata"), } /// Status and metadata for a single unstable feature. diff --git a/src/workspace/parser/mod.rs b/src/workspace/parser/mod.rs index 23665eb1c84..13c6f974317 100644 --- a/src/workspace/parser/mod.rs +++ b/src/workspace/parser/mod.rs @@ -15,8 +15,8 @@ use anyhow::{Context as _, anyhow, bail}; use cargo_platform::Platform; use cargo_util::paths; use cargo_util_schemas::manifest::{ - self, PackageName, PathBaseName, TomlDependency, TomlDetailedDependency, TomlManifest, - TomlPackageBuild, TomlWorkspace, + self, FeatureDefinition, FeatureMetadata, FeatureName, PackageName, PathBaseName, + TomlDependency, TomlDetailedDependency, TomlManifest, TomlPackageBuild, TomlWorkspace, }; use cargo_util_schemas::manifest::{RustVersion, StringOrBool}; use itertools::Itertools; @@ -877,8 +877,8 @@ pub(crate) fn default_readme_from_package_root(package_root: &Path) -> Option>>, -) -> CargoResult>>> { + original_features: Option<&BTreeMap>, +) -> CargoResult>> { let Some(normalized_features) = original_features.cloned() else { return Ok(None); }; @@ -1563,6 +1563,8 @@ pub fn to_real_manifest( } } + validate_feature_definitions(&features, original_toml.features.as_ref(), warnings)?; + validate_dependencies(original_toml.dependencies.as_ref(), None, None, warnings)?; validate_dependencies( original_toml.dev_dependencies(), @@ -1765,7 +1767,7 @@ pub fn to_real_manifest( .map(|(k, v)| { ( k.to_string().into(), - v.iter().map(InternedString::from).collect(), + v.enables().iter().map(InternedString::from).collect(), ) }) .collect(), @@ -2049,6 +2051,30 @@ fn to_virtual_manifest( Ok(manifest) } +fn validate_feature_definitions( + cargo_features: &Features, + features: Option<&BTreeMap>, + warnings: &mut Vec, +) -> CargoResult<()> { + let Some(features) = features else { + return Ok(()); + }; + for (feature, feature_definition) in features { + match feature_definition { + FeatureDefinition::Array(..) => {} + FeatureDefinition::Metadata(FeatureMetadata { _unused_keys, .. }) => { + cargo_features.require(Feature::feature_metadata())?; + warnings.extend( + _unused_keys + .keys() + .map(|k| format!("unused manifest key: `features.{feature}.{k}`")), + ); + } + } + } + Ok(()) +} + #[tracing::instrument(skip_all)] fn validate_dependencies( original_deps: Option<&BTreeMap>, @@ -3185,16 +3211,23 @@ fn prepare_toml_for_publish( }; features.values_mut().for_each(|feature_deps| { - feature_deps.retain(|feature_dep| { - let feature_value = FeatureValue::new(feature_dep.into()); - match feature_value { - FeatureValue::Dep { dep_name } | FeatureValue::DepFeature { dep_name, .. } => { - let k = &manifest::PackageName::new(dep_name.to_string()).unwrap(); - dep_name_set.contains(k) + let feature_array = feature_deps + .enables() + .iter() + .filter(|feature_dep| { + let feature_value = FeatureValue::new((*feature_dep).into()); + match feature_value { + FeatureValue::Dep { dep_name } + | FeatureValue::DepFeature { dep_name, .. } => { + let k = &manifest::PackageName::new(dep_name.to_string()).unwrap(); + dep_name_set.contains(k) + } + _ => true, } - _ => true, - } - }); + }) + .cloned() + .collect(); + *feature_deps = FeatureDefinition::Array(feature_array); }); } diff --git a/tests/testsuite/features.rs b/tests/testsuite/features.rs index 2a6189c86c4..ee57f1260ce 100644 --- a/tests/testsuite/features.rs +++ b/tests/testsuite/features.rs @@ -1,6 +1,9 @@ //! Tests for `[features]` table. +use std::fs::File; + use crate::prelude::*; +use cargo_test_support::publish::validate_crate_contents; use cargo_test_support::registry::{Dependency, Package}; use cargo_test_support::{basic_manifest, project}; use cargo_test_support::{rustc_host, str}; @@ -2490,13 +2493,9 @@ fn feature_metadata() { p.cargo("check --features c") .masquerade_as_nightly_cargo(&["feature-metadata"]) - .with_status(101) .with_stderr_data(str![[r#" -[ERROR] invalid type: map, expected a sequence - --> Cargo.toml:11:21 - | -11 | c = { enables = ["a", "b"] } - | ^^^^^^^^^^^^^^^^^^^^^^^^ +[CHECKING] foo v0.0.0 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) .run(); @@ -2522,11 +2521,14 @@ fn feature_metadata_is_unstable() { p.cargo("check --features a") .with_status(101) .with_stderr_data(str![[r#" -[ERROR] invalid type: map, expected a sequence - --> Cargo.toml:7:21 - | -7 | a = { enables = [] } - | ^^^^^^^^^^^^^^^^ +[ERROR] failed to parse manifest at `[ROOT]/foo/Cargo.toml` + +Caused by: + feature `feature-metadata` is required + + The package requires the Cargo feature called `feature-metadata`, but that feature is not stabilized in this version of Cargo ([..]). + Consider trying a newer version of Cargo (this may require the nightly release). + See https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#feature_metadata for more information about the status of this feature. "#]]) .run(); @@ -2555,7 +2557,7 @@ fn feature_metadata_missing_enables() { .masquerade_as_nightly_cargo(&["feature-metadata"]) .with_status(101) .with_stderr_data(str![[r#" -[ERROR] invalid type: map, expected a sequence +[ERROR] missing field `enables` --> Cargo.toml:9:23 | 9 | foo = {} @@ -2586,13 +2588,9 @@ fn feature_metadata_empty_enables() { p.cargo("check") .masquerade_as_nightly_cargo(&["feature-metadata"]) - .with_status(101) .with_stderr_data(str![[r#" -[ERROR] invalid type: map, expected a sequence - --> Cargo.toml:9:23 - | -9 | foo = { enables = [] } - | ^^^^^^^^^^^^^^^^ +[CHECKING] foo v0.0.0 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) .run(); @@ -2620,13 +2618,12 @@ fn unused_keys_in_feature_metadata() { p.cargo("check") .masquerade_as_nightly_cargo(&["feature-metadata"]) - .with_status(101) .with_stderr_data(str![[r#" -[ERROR] invalid type: map, expected a sequence - --> Cargo.toml:9:23 - | -9 | foo = { enables = ["bar"], a = false, b = true } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +[WARNING] Cargo.toml: unused manifest key: `features.foo.a` +[WARNING] Cargo.toml: unused manifest key: `features.foo.b` +[WARNING] `foo` (manifest) generated 2 warnings +[CHECKING] foo v0.0.0 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s "#]]) .run(); @@ -2655,14 +2652,32 @@ fn normalize_feature_metadata() { p.cargo("package --no-verify") .masquerade_as_nightly_cargo(&["feature-metadata"]) - .with_status(101) .with_stderr_data(str![[r#" -[ERROR] invalid type: map, expected a sequence - --> Cargo.toml:11:21 - | -11 | c = { enables = ["a", "b"] } - | ^^^^^^^^^^^^^^^^^^^^^^^^ +[WARNING] manifest has no description, license, license-file, documentation, homepage or repository + | + = [NOTE] see https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for more info +[PACKAGING] foo v0.0.0 ([ROOT]/foo) +[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed) "#]]) .run(); + let f = File::open(&p.root().join("target/package/foo-0.0.0.crate")).unwrap(); + let normalized_manifest = str![[r#" +... +[features] +a = [] +b = [] +c = [ + "a", + "b", +] + +... +"#]]; + validate_crate_contents( + f, + "foo-0.0.0.crate", + &["Cargo.lock", "Cargo.toml", "Cargo.toml.orig", "src/main.rs"], + [("Cargo.toml", normalized_manifest)], + ); }