-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
b414a91
af99add
338281e
1cdcd4a
d67aee5
567a104
d61ffb5
2faf5e0
905820c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
|
@@ -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); | ||||||
}; | ||||||
|
@@ -1296,6 +1297,8 @@ pub fn to_real_manifest( | |||||
} | ||||||
} | ||||||
|
||||||
validate_feature_definitions(original_toml.features.as_ref(), warnings)?; | ||||||
|
||||||
validate_dependencies(original_toml.dependencies.as_ref(), None, None, warnings)?; | ||||||
validate_dependencies( | ||||||
original_toml.dev_dependencies(), | ||||||
|
@@ -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(), | ||||||
|
@@ -1764,6 +1767,30 @@ fn to_virtual_manifest( | |||||
Ok(manifest) | ||||||
} | ||||||
|
||||||
fn validate_feature_definitions( | ||||||
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, .. }) => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. Probably behind a cargo-feature "feature-metadata".
Could add a doc comment on relevant field/variant indicating it is unstable/nightly only. cargo/crates/cargo-util-schemas/src/manifest/mod.rs Lines 936 to 937 in f15df8f
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.
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! |
||||||
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>>, | ||||||
|
@@ -2902,16 +2929,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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||||||
}); | ||||||
} | ||||||
|
||||||
|
There was a problem hiding this comment.
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.