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

fix: Ensure dep/feature activates the dependency on 2024 #14221

Merged
merged 3 commits into from
Jul 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ fn resolve_toml(
dev_dependencies2: None,
build_dependencies: None,
build_dependencies2: None,
features: original_toml.features.clone(),
features: None,
target: None,
replace: original_toml.replace.clone(),
patch: original_toml.patch.clone(),
Expand Down Expand Up @@ -322,6 +322,8 @@ fn resolve_toml(
});
resolved_toml.package = Some(resolved_package);

resolved_toml.features = resolve_features(original_toml.features.as_ref(), edition)?;

resolved_toml.lib = targets::resolve_lib(
original_toml.lib.as_ref(),
package_root,
Expand Down Expand Up @@ -686,6 +688,40 @@ fn default_readme_from_package_root(package_root: &Path) -> Option<String> {
None
}

#[tracing::instrument(skip_all)]
fn resolve_features(
original_features: Option<&BTreeMap<manifest::FeatureName, Vec<String>>>,
edition: Edition,
) -> CargoResult<Option<BTreeMap<manifest::FeatureName, Vec<String>>>> {
let Some(mut resolved_features) = original_features.cloned() else {
return Ok(None);
};

if Edition::Edition2024 <= edition {
Copy link
Member

Choose a reason for hiding this comment

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

I understand you like Yoda style more, but forgot the reason.

Anyway, this is not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually really dislike Yoda style for equality checks. For these checks, I prefer number line order (the lower value is always on the left, so < and <= are only ever used and not > or >=)

for activations in resolved_features.values_mut() {
let mut deps = Vec::new();
for feature_value in activations.iter() {
let feature_value = FeatureValue::new(InternedString::new(feature_value));
Copy link
Member

Choose a reason for hiding this comment

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

So, this PR chose the approach:

Implicitly create dep:serde if serde/derive is used

I forgot if we had a conclusion somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #14016 (comment)

We discussed this in today's team meeting and lean towards "dep_name/feature_name" implying "dep:dep_name" in Edition 2024 with it being normalized in the published Cargo.toml file.

Copy link
Member

Choose a reason for hiding this comment

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

Why did we choose to do it here instead of in Summary?

let summary = Summary::new(
pkgid,
deps,
&resolved_toml
.features
.as_ref()
.unwrap_or(&Default::default())
.iter()
.map(|(k, v)| {
(
InternedString::new(k),
v.iter().map(InternedString::from).collect(),
)
})
.collect(),
resolved_package.links.as_deref(),
rust_version.clone(),
);

Copy link
Member

Choose a reason for hiding this comment

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

We want everything to be normalized on resolved_toml, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By doing it here, we get cargo publish normalization "for free" and all data-structures are consistent for less chance of bugs (which also makes it easier to introspect for debugging)

Copy link
Member

Choose a reason for hiding this comment

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

Implicitly create dep:serde if serde/derive is used

Doesn't this fail if serde is not an optional dependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this fail if serde is not an optional dependency?

Ran into this as well, see #14283. :)

let FeatureValue::DepFeature {
dep_name,
dep_feature: _,
weak: false,
} = feature_value
else {
continue;
};
let dep = FeatureValue::Dep { dep_name }.to_string();
if !activations.contains(&dep) {
deps.push(dep);
}
}
activations.extend(deps);
}
}

Ok(Some(resolved_features))
}

#[tracing::instrument(skip_all)]
fn resolve_dependencies<'a>(
gctx: &GlobalContext,
Expand Down
67 changes: 67 additions & 0 deletions tests/testsuite/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1847,6 +1847,73 @@ fn features_option_given_twice() {
p.cargo("check --features a --features b").run();
}

#[cargo_test(nightly, reason = "edition2024 is not stable")]
fn strong_dep_feature_edition2024() {
let p = project()
.file(
"Cargo.toml",
r#"
cargo-features = ["edition2024"]
[package]
name = "foo"
version = "0.1.0"
edition = "2024"

[features]
optional_dep = ["optional_dep/foo"]

[dependencies]
optional_dep = { path = "optional_dep", optional = true }
"#,
)
.file(
"src/main.rs",
r#"
fn main() {}
"#,
)
.file(
"optional_dep/Cargo.toml",
r#"
[package]
name = "optional_dep"
[features]
foo = []
"#,
)
.file(
"optional_dep/src/lib.rs",
r#"
"#,
)
.build();

p.cargo("metadata")
.masquerade_as_nightly_cargo(&["edition2024"])
.with_stdout_data(
str![[r#"
{
"metadata": null,
"packages": [
{
"features": {
"optional_dep": [
"optional_dep/foo",
"dep:optional_dep"
]
},
"name": "foo",
"...": "{...}"
}
],
"...": "{...}"
}
"#]]
.json(),
)
.run();
}

#[cargo_test]
fn multi_multi_features() {
let p = project()
Expand Down