Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(manifest)!: implement feature-metadata RFC3416 #15056

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
37 changes: 33 additions & 4 deletions crates/cargo-util-schemas/manifest.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,7 @@
"null"
],
"additionalProperties": {
"type": "array",
"items": {
"type": "string"
}
"$ref": "#/$defs/FeatureDefinition"
}
},
"lib": {
Expand Down Expand Up @@ -597,6 +594,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": {
Expand Down
61 changes: 59 additions & 2 deletions crates/cargo-util-schemas/src/manifest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub struct TomlManifest {
pub package: Option<Box<TomlPackage>>,
pub project: Option<Box<TomlPackage>>,
pub badges: Option<BTreeMap<String, BTreeMap<String, String>>>,
pub features: Option<BTreeMap<FeatureName, Vec<String>>>,
pub features: Option<BTreeMap<FeatureName, FeatureDefinition>>,
pub lib: Option<TomlLibTarget>,
pub bin: Option<Vec<TomlBinTarget>>,
pub example: Option<Vec<TomlExampleTarget>>,
Expand Down Expand Up @@ -110,7 +110,7 @@ impl TomlManifest {
.or(self.build_dependencies2.as_ref())
}

pub fn features(&self) -> Option<&BTreeMap<FeatureName, Vec<String>>> {
pub fn features(&self) -> Option<&BTreeMap<FeatureName, FeatureDefinition>> {
self.features.as_ref()
}

Expand Down Expand Up @@ -1521,6 +1521,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<String>),
/// 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: D) -> Result<FeatureDefinition, D::Error>
where
D: de::Deserializer<'de>,
{
UntaggedEnumVisitor::new()
.seq(|seq| {
seq.deserialize::<Vec<String>>()
.map(FeatureDefinition::Array)
})
.map(|seq| {
seq.deserialize::<FeatureMetadata>()
.map(FeatureDefinition::Metadata)
})
.deserialize(d)
}
}

impl FeatureDefinition {
/// Returns the features that this feature enables.
pub fn enables(&self) -> &[String] {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method could also return an impl Iterator<Item = String> if preferred.

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<String>,

/// 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<String, toml::Value>,
}

#[derive(Serialize, Debug, Clone)]
#[cfg_attr(feature = "unstable-schema", derive(schemars::JsonSchema))]
pub struct InheritableLints {
Expand Down
3 changes: 3 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,9 @@ features! {

/// Allow paths that resolve relatively to a base specified in the config.
(unstable, path_bases, "", "reference/unstable.html#path-bases"),

/// Allow to use a table for defining features.
(unstable, feature_metadata, "", "reference/unstable.html#feature_metadata"),
}

/// Status and metadata for a single unstable feature.
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/registry/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,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::<BTreeMap<String, Vec<String>>>(),
Expand Down
62 changes: 49 additions & 13 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ use anyhow::{anyhow, bail, Context as _};
use cargo_platform::Platform;
use cargo_util::paths;
use cargo_util_schemas::manifest::{
self, PackageName, PathBaseName, TomlDependency, TomlDetailedDependency, TomlManifest,
self, FeatureDefinition, FeatureMetadata, FeatureName, PackageName, PathBaseName,
TomlDependency, TomlDetailedDependency, TomlManifest,
};
use cargo_util_schemas::manifest::{RustVersion, StringOrBool};
use itertools::Itertools;
Expand Down Expand Up @@ -708,8 +709,8 @@ fn default_readme_from_package_root(package_root: &Path) -> Option<String> {

#[tracing::instrument(skip_all)]
fn normalize_features(
original_features: Option<&BTreeMap<manifest::FeatureName, Vec<String>>>,
) -> CargoResult<Option<BTreeMap<manifest::FeatureName, Vec<String>>>> {
original_features: Option<&BTreeMap<manifest::FeatureName, FeatureDefinition>>,
) -> CargoResult<Option<BTreeMap<manifest::FeatureName, FeatureDefinition>>> {
let Some(normalized_features) = original_features.cloned() else {
return Ok(None);
};
Expand Down Expand Up @@ -1296,6 +1297,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(),
Expand Down Expand Up @@ -1497,7 +1500,7 @@ pub fn to_real_manifest(
.map(|(k, v)| {
(
InternedString::new(k),
v.iter().map(InternedString::from).collect(),
v.enables().iter().map(InternedString::from).collect(),
)
})
.collect(),
Expand Down Expand Up @@ -1764,6 +1767,32 @@ fn to_virtual_manifest(
Ok(manifest)
}

fn validate_feature_definitions(
cargo_features: &Features,
features: Option<&BTreeMap<FeatureName, FeatureDefinition>>,
warnings: &mut Vec<String>,
) -> CargoResult<()> {
let Some(features) = features else {
return Ok(());
};

for (feature, feature_definition) in features {
match feature_definition {
FeatureDefinition::Array(..) => {}
FeatureDefinition::Metadata(FeatureMetadata { _unused_keys, .. }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haven't got time into full review, though I think the meta field should be behind a nightly feature flag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Should there be a Cargo unstable feature for this change?

Yes. Probably behind a cargo-feature "feature-metadata".

  • This PR of course introduces a breaking change in the cargo-util-schemas crate, is there anything to do regarding this in this PR?

Could add a doc comment on relevant field/variant indicating it is unstable/nightly only.

/// Unstable feature `-Ztrim-paths`.
pub trim_paths: Option<TomlTrimPaths>,

There is a CI job checking if a member crate needs a version bump. It didn't warn you so I assume it has already been bumped in this release cycle. You do not need to do anything.

  • Should this PR also attempt to update core::Summary or should this be left to future implementations of RFCs providing other keys (e.g., doc)?

Summary is more like a thing for dependency resolution. I think we revisit it in the future. Regardless, see epage's comment #14157 (comment) that the feature itself is not particularly useful until other RFC gets merged. Anyway, thanks for the contribution!

cargo_features.require(Feature::feature_metadata())?;
warnings.extend(
_unused_keys
.keys()
.map(|k| format!("unused manifest key: `features.{feature}.{k}`")),
AudaciousAxiom marked this conversation as resolved.
Show resolved Hide resolved
);
}
}
}

Ok(())
}

#[tracing::instrument(skip_all)]
fn validate_dependencies(
original_deps: Option<&BTreeMap<manifest::PackageName, manifest::InheritableDependency>>,
Expand Down Expand Up @@ -2902,16 +2931,23 @@ fn prepare_toml_for_publish(
};

features.values_mut().for_each(|feature_deps| {
feature_deps.retain(|feature_dep| {
let feature_value = FeatureValue::new(InternedString::new(feature_dep));
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(InternedString::new(feature_dep));
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);
Comment on lines +2934 to +2950
Copy link
Author

@AudaciousAxiom AudaciousAxiom Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For compatibility, this uses the array syntax for generating the normalized manifest, even when the table syntax is used by authors (see the corresponding integration test).

});
}

Expand Down
16 changes: 16 additions & 0 deletions src/doc/src/reference/unstable.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ Each new feature described below should explain how to use it.
* [Profile `trim-paths` option](#profile-trim-paths-option) --- Control the sanitization of file paths in build outputs.
* [`[lints.cargo]`](#lintscargo) --- Allows configuring lints for Cargo.
* [path bases](#path-bases) --- Named base directories for path dependencies.
* [feature-metadata](#feature-metadata) --- Table syntax for feature definitions.
* Information and metadata
* [Build-plan](#build-plan) --- Emits JSON information on which commands will be run.
* [unit-graph](#unit-graph) --- Emits JSON for Cargo's internal graph structure.
Expand Down Expand Up @@ -1619,6 +1620,21 @@ will prefer the value in the configuration. The allows Cargo to add new built-in
path bases without compatibility issues (as existing uses will shadow the
built-in name).

## 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
* Original Issue: [#5707](https://github.com/rust-lang/cargo/issues/5707)
* Tracking Issue: [#14421](https://github.com/rust-lang/cargo/issues/14421)
Expand Down
Loading
Loading