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(toml): Error on [project] in Edition 2024 #13747

Merged
merged 10 commits into from
Apr 16, 2024
Merged

Conversation

epage
Copy link
Contributor

@epage epage commented Apr 12, 2024

What does this PR try to resolve?

[package] was added in 86b2a2a in the pre-1.0 days but [project] was never removed nor did we warn on its use until recently in #11135. So likely we can't remove it completely but we can remove it in Edition 2024+.

This includes cargo fix support which is hard coded directly into the cargo fix command.

This is part of

While we haven't signed off on everything in #13629, I figured this (and a couple other changes) are not very controversial.

How should we test and review this PR?

Per commit. Tests are added to show the behavior changes, including in cargo fix.

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Apr 12, 2024

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

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. A-manifest Area: Cargo.toml issues Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2024
tests/testsuite/check.rs Show resolved Hide resolved
src/bin/cargo/commands/fix.rs Show resolved Hide resolved
tests/testsuite/fix.rs Show resolved Hide resolved
@rustbot rustbot added the A-workspaces Area: workspaces label Apr 12, 2024
@weihanglo weihanglo added the T-cargo Team: Cargo label Apr 12, 2024
@weihanglo
Copy link
Member

This only errors out on edition 2024, though for hard errors it's better to get a consensus from the team.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 12, 2024

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Apr 12, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 13, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@bors
Copy link
Contributor

bors commented Apr 15, 2024

☔ The latest upstream changes (presumably #13754) made this pull request unmergeable. Please resolve the merge conflicts.

@weihanglo
Copy link
Member

The cargo team went to the proposed deprecations in #13629 today, and we are fine with this erroring on Edition 2024. No need to wait for a full FCP.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 16, 2024

📌 Commit cbd9def has been approved by weihanglo

It is now in the queue for this repository.

@bors 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 Apr 16, 2024
@bors
Copy link
Contributor

bors commented Apr 16, 2024

⌛ Testing commit cbd9def with merge 6f06fe9...

@bors
Copy link
Contributor

bors commented Apr 16, 2024

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

@bors bors merged commit 6f06fe9 into rust-lang:master Apr 16, 2024
21 checks passed
@epage epage deleted the deprecated branch April 16, 2024 20:25
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2024
Update cargo

11 commits in 48eca1b164695022295ce466b64b44e4e0228b08..6f06fe908a5ee0f415c187f868ea627e82efe07d
2024-04-12 21:16:36 +0000 to 2024-04-16 18:47:44 +0000
- fix(toml): Error on `[project]` in Edition 2024 (rust-lang/cargo#13747)
- feat(update): Include a Locking message (rust-lang/cargo#13759)
- chore(deps): update rust crate gix to 0.62.0 [security] (rust-lang/cargo#13760)
- test(schemas): Ensure tests cover the correct case (rust-lang/cargo#13761)
- feat(resolve): Tell the user the style of resovle done (rust-lang/cargo#13754)
- Make sure to also wrap the initial `-vV` invocation (rust-lang/cargo#13659)
- docs: update `checkout` GitHub action version (rust-lang/cargo#13757)
- Recategorize cargo test's `--doc` flag under "Target Selection" (rust-lang/cargo#13756)
- Reword sentence describing workspace toml for clarity (rust-lang/cargo#13753)
- docs(ref): Update unstable docs for msrv-policy (rust-lang/cargo#13751)
- refactor(config): Consistently use kebab-case (rust-lang/cargo#13748)

r? ghost
@rustbot rustbot added this to the 1.79.0 milestone Apr 17, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Apr 17, 2024
Update cargo

11 commits in 48eca1b164695022295ce466b64b44e4e0228b08..6f06fe908a5ee0f415c187f868ea627e82efe07d
2024-04-12 21:16:36 +0000 to 2024-04-16 18:47:44 +0000
- fix(toml): Error on `[project]` in Edition 2024 (rust-lang/cargo#13747)
- feat(update): Include a Locking message (rust-lang/cargo#13759)
- chore(deps): update rust crate gix to 0.62.0 [security] (rust-lang/cargo#13760)
- test(schemas): Ensure tests cover the correct case (rust-lang/cargo#13761)
- feat(resolve): Tell the user the style of resovle done (rust-lang/cargo#13754)
- Make sure to also wrap the initial `-vV` invocation (rust-lang/cargo#13659)
- docs: update `checkout` GitHub action version (rust-lang/cargo#13757)
- Recategorize cargo test's `--doc` flag under "Target Selection" (rust-lang/cargo#13756)
- Reword sentence describing workspace toml for clarity (rust-lang/cargo#13753)
- docs(ref): Update unstable docs for msrv-policy (rust-lang/cargo#13751)
- refactor(config): Consistently use kebab-case (rust-lang/cargo#13748)

r? ghost
if opts.edition {
check_resolver_change(ws, opts)?;
let specs = opts.compile_opts.spec.to_package_id_specs(&original_ws)?;
let members: Vec<&Package> = original_ws
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, there seems to be some problem here, it doesn't fetch all members of the workspace. If the members contain [project], then cargo fix --edition won't complete the migration either.

Is this intentional, or am I misinterpreting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do filter by the --package flag which is us just doing what the user told us. I'm not seeing how presence of [proiject] causes a problem. We also have a test showing that it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't seem to have been clear.

What I meant was that if the members in the workspace use [project] instead of [package], then executing the command cargo fix --edition in the workspace does not modify the members.

I'm wondering if the changes should be done on all members at once.

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 think I'm still missing the problem case.

Could you create a test case to demonstrate it?

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is a workspace ws containing the member hello.

which:

# [ws]/Cargo.toml
[workspace]
members = ["hello"]

[project]     # Pay attention here 
name = "ws"
version = "0.1.0"
edition = "2021"
# [ws]/hello/Cargo.toml
[project]     # Pay attention here 
name = "hello"
version = "0.1.0"
edition = "2021"

After running cargo fix --edition --allow-no-vcs in the workspace. The cargo.toml of ws completes the modification, but the cargo.toml of member hello does not.

I think both should be modified to [package] here.

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've written that up as a test case which is passing.

Yes, we aren't migrating hello's Cargo.toml but we also aren't migrating hellos src/lib.rs. Manifest and rust migration should be done hand-in-hand. The user has to opt-in to migrating hello with the --workspace flag. cargo fix --edition --workspace will migrate both packages, manifest and rust.

#[cargo_test]
fn migrate_project_to_package_ws() {
    let p = project()
        .file(
            "Cargo.toml",
            r#"
cargo-features = ["edition2024"]

[workspace]
members = ["hello"]

# Before project
[ project ] # After project header
# After project header line
name = "foo"
edition = "2021"
# After project table
"#,
        )
        .file("src/lib.rs", "")
        .file(
            "hello/Cargo.toml",
            r#"
cargo-features = ["edition2024"]

# Before project
[ project ] # After project header
# After project header line
name = "hello"
edition = "2021"
# After project table
"#,
        )
        .file("hello/src/lib.rs", "")
        .build();

    p.cargo("fix --edition --allow-no-vcs")
        .masquerade_as_nightly_cargo(&["edition2024"])
        .with_stderr(
            "\
[MIGRATING] Cargo.toml from 2021 edition to 2024
[FIXED] Cargo.toml (1 fix)
[WARNING] [CWD]/hello/Cargo.toml: `[project]` is deprecated in favor of `[package]`
[LOCKING] 2 packages to latest compatible versions
[CHECKING] foo v0.0.0 ([CWD])
[MIGRATING] src/lib.rs from 2021 edition to 2024
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..]s
",
        )
        .run();
    assert_eq!(
        p.read_file("Cargo.toml"),
        r#"
cargo-features = ["edition2024"]

[workspace]
members = ["hello"]

# Before project
[ package ] # After project header
# After project header line
name = "foo"
edition = "2021"
# After project table
"#
    );
    assert_eq!(
        p.read_file("hello/Cargo.toml"),
        r#"
cargo-features = ["edition2024"]

# Before project
[ project ] # After project header
# After project header line
name = "hello"
edition = "2021"
# After project table
"#
    );
}

@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. A-manifest Area: Cargo.toml issues A-workspaces Area: workspaces Command-fix disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants