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

refactor(toml): Split out an explicit step to resolve Cargo.toml #13693

Merged
merged 34 commits into from
Apr 3, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Apr 2, 2024

What does this PR try to resolve?

This builds on #13664 and #13666. Currently, once we have deserialized Cargo.toml, we pass it to a large machinery (to_real_manifest, to_virtual_manifest) so that

  • Cargo.toml is resolved
  • Summary is created
  • Manifest is created

This splits out the resolving of Cargo.toml which is mostly workspace inheritance today.

While splitting logic conjoined like this can be a bit messy in the short term, the hope is that overall this makes the logic easier to follow (more condensed, focused sections to view; more explicit inputs/outputs).

In particular, I hope that this will make it clearer and easier to shift more logic into the resolving step, specifically the inferring of build targets for #13456.

How should we test and review this PR?

This is broken up into very small steps in the hope that it makes it easier to analyze a step.

Additional information

epage added 30 commits March 28, 2024 13:33
This also removes duplicated inheritance and one of them specifying the
wrong field.
Returning a `&String` is unusual but this keeps things easier on both
sides.
This will make it easier to evaluate what needs to be resolved in the
future.
We can't have validation depend on `TomlManifest::resolved_lints` yet
because we need to pull out the resolving of deps first.
Normally, I prefer directly constructing something but I feel this works
better in this case so I can limit a lot of work that is coupled to a
`package` being present.

Since we still directly construct, just with `None`, I think this still
works.
I'm somewhat tempted to flatten the function call.
The parallel between the package an virtual-manifest cases would help to
keep them in sync.
@rustbot
Copy link
Collaborator

rustbot commented Apr 2, 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-manifest Area: Cargo.toml issues A-workspaces Area: workspaces S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 2, 2024
@rustbot rustbot added the A-unstable Area: nightly unstable support label Apr 2, 2024
Copy link
Member

@Muscraft Muscraft left a comment

Choose a reason for hiding this comment

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

Thanks!

This makes stuff much easier to understand

@Muscraft
Copy link
Member

Muscraft commented Apr 3, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 3, 2024

📌 Commit 58b6501 has been approved by Muscraft

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

bors commented Apr 3, 2024

⌛ Testing commit 58b6501 with merge 09d5e96...

@bors
Copy link
Contributor

bors commented Apr 3, 2024

☀️ Test successful - checks-actions
Approved by: Muscraft
Pushing 09d5e96 to master...

@bors bors merged commit 09d5e96 into rust-lang:master Apr 3, 2024
21 checks passed
@epage epage deleted the resolve-toml branch April 4, 2024 21:49
bors added a commit that referenced this pull request Apr 5, 2024
refactor(toml): Decouple target discovery from Target creation

### What does this PR try to resolve?

This builds on #13693 so that the resolving of targets is easier to pull out into `resolve_toml` in prep for fixing #13456

### How should we test and review this PR?

### Additional information
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2024
Update cargo

9 commits in 0637083df5bbdcc951845f0d2eff6999cdb6d30a..28e7b2bc0a812f90126be30f48a00a4ada990eaa
2024-04-02 23:55:05 +0000 to 2024-04-05 19:31:01 +0000
- refactor(toml): Decouple target discovery from Target creation (rust-lang/cargo#13701)
- Don't depend on `?` affecting type inference in weird ways (rust-lang/cargo#13706)
- test(metadata): Show behavior with TOML-specific types (rust-lang/cargo#13703)
- fix: adjust tracing verbosity in list_files_git (rust-lang/cargo#13704)
- doc: comments on `PackageRegistry` (rust-lang/cargo#13698)
- Switch to using gitoxide by default for listing files (rust-lang/cargo#13696)
- Allow precise update to prerelease. (rust-lang/cargo#13626)
- refactor(toml): Split out an explicit step to resolve `Cargo.toml` (rust-lang/cargo#13693)
- chore(deps): update rust crate base64 to 0.22.0 (rust-lang/cargo#13675)

r? ghost
@rustbot rustbot added this to the 1.79.0 milestone Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues A-unstable Area: nightly unstable support A-workspaces Area: workspaces S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants