fix: Ensure dep/feature activates the dependency on 2024 by epage · Pull Request #14221 · rust-lang/cargo

epage · 2024-07-09T19:02:22Z

What does this PR try to resolve?

How should we test and review this PR?

This doesn't revert the last commit of #14018 so that it works properly
on Editions 2021 and 2024.

Additional information

This doesn't revert the last commit of rust-lang#14018 so that it works properly on Editions 2021 and 2024. Fixes rust-lang#14016

rustbot · 2024-07-09T19:02:27Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

This is a follow up to rust-lang#14181 which broke CI in rust-lang#14221 ``` ---- progress::always_shows_progress stdout ---- running `/Users/runner/work/cargo/cargo/target/debug/cargo check` thread 'progress::always_shows_progress' panicked at tests/testsuite/progress.rs:128:10: ---- expected: tests/testsuite/progress.rs:116:13 ++++ actual: stderr 1 1 | [DOWNLOADING] 1 crate 2 2 | [DOWNLOADING] 2 crates 3 3 | [DOWNLOADING] 3 crates 4 4 | [DOWNLOADED] 3 crates ([..]KB) in [..]s 5 - [BUILDING] [..] 0/4: [..] 6 5 | [BUILDING] [..] 3/4: foo 7 6 | [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s 8 7 | ...∅ Update with SNAPSHOTS=overwrite ```

epage · 2024-07-09T20:39:41Z

For CI failure, see #14223

This is a follow up to rust-lang#14181 which broke CI in rust-lang#14221 ``` ---- progress::always_shows_progress stdout ---- running `/Users/runner/work/cargo/cargo/target/debug/cargo check` thread 'progress::always_shows_progress' panicked at tests/testsuite/progress.rs:128:10: ---- expected: tests/testsuite/progress.rs:116:13 ++++ actual: stderr 1 1 | [DOWNLOADING] 1 crate 2 2 | [DOWNLOADING] 2 crates 3 3 | [DOWNLOADING] 3 crates 4 4 | [DOWNLOADED] 3 crates ([..]KB) in [..]s 5 - [BUILDING] [..] 0/4: [..] 6 5 | [BUILDING] [..] 3/4: foo 7 6 | [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s 8 7 | ...∅ Update with SNAPSHOTS=overwrite ```

test(progress): Resolve flakiness This is a follow up to #14181 which broke CI in #14221 ``` ---- progress::always_shows_progress stdout ---- running `/Users/runner/work/cargo/cargo/target/debug/cargo check` thread 'progress::always_shows_progress' panicked at tests/testsuite/progress.rs:128:10: ---- expected: tests/testsuite/progress.rs:116:13 ++++ actual: stderr 1 1 | [DOWNLOADING] 1 crate 2 2 | [DOWNLOADING] 2 crates 3 3 | [DOWNLOADING] 3 crates 4 4 | [DOWNLOADED] 3 crates ([..]KB) in [..]s 5 - [BUILDING] [..] 0/4: [..] 6 5 | [BUILDING] [..] 3/4: foo 7 6 | [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s 8 7 | ...∅ Update with SNAPSHOTS=overwrite ```

weihanglo · 2024-07-10T01:33:53Z

src/cargo/util/toml/mod.rs

        return Ok(None);
    };

+    if Edition::Edition2024 <= edition {


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

Anyway, this is not a blocker.

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 >=)

weihanglo · 2024-07-10T01:55:17Z

src/cargo/util/toml/mod.rs

+        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));


So, this PR chose the approach:

Implicitly create dep:serde if serde/derive is used

I forgot if we had a conclusion somewhere.

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.

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

cargo/src/cargo/util/toml/mod.rs

Lines 1442 to 1459 in 471287f

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(),

);

We want everything to be normalized on resolved_toml, right?

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)

Implicitly create dep:serde if serde/derive is used

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

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

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

weihanglo · 2024-07-10T18:22:41Z

@bors r+

Thanks!

bors · 2024-07-10T18:22:44Z

📌 Commit 99fae91 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-07-10T18:23:52Z

⌛ Testing commit 99fae91 with merge fd2b60e...

bors · 2024-07-10T18:53:01Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing fd2b60e to master...

Update cargo 31 commits in 154fdac39ae9629954e19e9986fd2cf2cdd8d964..a2b58c3dad4d554ba01ed6c45c41ff85390560f2 2024-07-07 01:28:23 +0000 to 2024-07-16 00:52:02 +0000 - chore(ci): bump CI tools (rust-lang/cargo#14257) - test: migrate fetch and list_availables to snapbox (rust-lang/cargo#14214) - chore: downgrade to jobserver@0.1.28 (rust-lang/cargo#14254) - perf(source): Don't `du` on every git source load (rust-lang/cargo#14252) - fix(source): Don't warn about unreferenced duplicate packages (rust-lang/cargo#14239) - feat(test): Add cargo_test to test-support prelude (rust-lang/cargo#14243) - Add workflow to publish Cargo automatically (rust-lang/cargo#14202) - test: migrate implicit_features to snapbox (rust-lang/cargo#14245) - test: migrate build-std/main to snapbox (rust-lang/cargo#14241) - test: migrate check_cfg to snapbox (rust-lang/cargo#14235) - refactor(source): More RecursivePathSource clean up (rust-lang/cargo#14231) - Add more profiling traces (rust-lang/cargo#14238) - fix(overrides): Don't warn on duplicate packages from using '..' (rust-lang/cargo#14234) - fix(test): Redact elapsed time in the minutes time frame (rust-lang/cargo#14233) - test: Migrate lto tests to snapbox (rust-lang/cargo#14209) - fix: Ensure dep/feature activates the dependency on 2024 (rust-lang/cargo#14221) - chore(docs): update index of reference (rust-lang/cargo#14228) - test: migrate test to snapbox (rust-lang/cargo#14226) - chore: remove duplicate words (rust-lang/cargo#14229) - docs(contrib): Document things I look for in RFCs (rust-lang/cargo#14222) - docs(ref): Note MSRV for features in the docs (rust-lang/cargo#14224) - test(progress): Resolve flakiness (rust-lang/cargo#14223) - fix(test): Reduce over-prescription to the caller (rust-lang/cargo#14217) - refactor: move get_source_id out of registry (rust-lang/cargo#14218) - fix: rename to `rustdoc::broken_intra_doc_links` (rust-lang/cargo#14215) - test: migrate member_errors, multitarget and new to snapbox (rust-lang/cargo#14210) - test: migrate generate_lockfile and glob_targets to snapbox (rust-lang/cargo#14200) - test: Ensure --list test works with cargo-bloat (rust-lang/cargo#14213) - dont make new constant InternedString in hot path (rust-lang/cargo#14211) - Fix compatible_with_older_cargo test. (rust-lang/cargo#14212) - test: migrate metabuild, metadata and net_config to snapbox (rust-lang/cargo#14162)

epage added 3 commits July 8, 2024 16:49

refactor(toml): Make room for resolving features

6db2864

test: Show bad error for dep_name/feature_name on 2024

85cc994

fix: Ensure dep/feature activates the dependency on 2024

99fae91

This doesn't revert the last commit of rust-lang#14018 so that it works properly on Editions 2021 and 2024. Fixes rust-lang#14016

rustbot assigned weihanglo Jul 9, 2024

rustbot added A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2024

epage mentioned this pull request Jul 9, 2024

test(progress): Resolve flakiness #14223

Merged

epage force-pushed the implicit branch from cce530b to 336e1cf Compare July 10, 2024 00:47

weihanglo reviewed Jul 10, 2024

View reviewed changes

epage force-pushed the implicit branch from 336e1cf to 99fae91 Compare July 10, 2024 14:53

weihanglo approved these changes Jul 10, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 10, 2024

bors merged commit fd2b60e into rust-lang:master Jul 10, 2024

Skgland mentioned this pull request Jul 10, 2024

Lint when > or >= is used i.e. compared values are not listed in (number line) order rust-lang/rust-clippy#13082

Open

epage deleted the implicit branch July 10, 2024 20:07

weihanglo mentioned this pull request Jul 17, 2024

Update cargo rust-lang/rust#127838

Merged

rustbot added this to the 1.81.0 milestone Jul 17, 2024

cassaundra mentioned this pull request Jul 22, 2024

Activation of features of non-optional dependencies breaks manifest for 2024 edition #14283

Closed

ehuss mentioned this pull request Sep 11, 2024

Tracking Issue for remove-implicit-feature RFC 3491 #12826

Open

epage mentioned this pull request Oct 1, 2024

Bad error message when a strong dep features's depedency is an unused optional dependency #14016

Open

	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(),
	);

Conversation

epage commented Jul 9, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Uh oh!

rustbot commented Jul 9, 2024

Uh oh!

epage commented Jul 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo commented Jul 10, 2024

Uh oh!

bors commented Jul 10, 2024

Uh oh!

bors commented Jul 10, 2024

Uh oh!

bors commented Jul 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants