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 resolve Method #7186

Merged
merged 2 commits into from
Jul 29, 2019
Merged

Refactor resolve Method #7186

merged 2 commits into from
Jul 29, 2019

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jul 27, 2019

This changes the enum Method to a struct called ResolveOpts. The intent is to try to make it a little easier to read and understand.

If this doesn't actually look better to anyone, I'm fine with discarding this (I can resubmit just with documentation). It only seems marginally better, but I have had difficulty with understanding the subtleties of Method for a while. I wasn't able to measure any performance difference.

This also has a few other changes:

  • Adds some documentation.
  • Removes resolve_ws_precisely, cutting down the number of resolve functions from 4 to 3.

@rust-highfive
Copy link

r? @Eh2406

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2019
metadata: Metadata,
/// `[patch]` entries that did not match anything, preserved in
/// `Cargo.lock` as the `[[patch.unused]]` table array.
/// TODO: *Why* is this kept in `Cargo.lock`? Removing it doesn't seem to
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 couldn't figure out the purpose of unused_patches, any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it was added in #4123 if that helps...

Copy link
Member

Choose a reason for hiding this comment

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

One of the features of [patch] is that it ends up needing to resolve all the dependencies all the time. This means that if we didn't record unused patch entries in Cargo.lock then every time you type cargo build Cargo would "Updating registry ..." and take a lot longer on each build. (it's not guaranteed this would happen but it's very likely this would happen)

By recording unused [patch] entries in the lock file we can ensure that all subsequent cargo build have a guaranteed resolution of what's in the manifest (so long as it hasn't changed) so we won't ever have to re-update the registry and we can get nice, quick, deterministic builds.

You can sort of think of it like adding an entry to [dependencies] but you don't actually use it, just in this case we know whether it's used or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that makes sense! Thanks for the explanation.

@Eh2406
Copy link
Contributor

Eh2406 commented Jul 27, 2019

Thank you very much! This definitely makes things clearer! Having skimmed it it all looks good.

@ehuss
Copy link
Contributor Author

ehuss commented Jul 29, 2019

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Jul 29, 2019

📌 Commit abf2bb4 has been approved by alexcrichton

@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 Jul 29, 2019
bors added a commit that referenced this pull request Jul 29, 2019
Refactor resolve `Method`

This changes the enum `Method` to a struct called `ResolveOpts`. The intent is to try to make it a little easier to read and understand.

If this doesn't actually look better to anyone, I'm fine with discarding this (I can resubmit just with documentation). It only seems marginally better, but I have had difficulty with understanding the subtleties of `Method` for a while. I wasn't able to measure any performance difference.

This also has a few other changes:
- Adds some documentation.
- Removes `resolve_ws_precisely`, cutting down the number of resolve functions from 4 to 3.
@bors
Copy link
Collaborator

bors commented Jul 29, 2019

⌛ Testing commit abf2bb4 with merge 1f74bdf...

@bors
Copy link
Collaborator

bors commented Jul 29, 2019

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 1f74bdf to master...

@bors bors merged commit abf2bb4 into rust-lang:master Jul 29, 2019
bors added a commit to rust-lang/rust that referenced this pull request Aug 1, 2019
Update cargo, rls

## cargo

12 commits in d0f828419d6ce6be21a90866964f58eb2c07cd56..26092da337b948719549cd5ed3d1051fd847afd7
2019-07-23 21:58:59 +0000 to 2019-07-31 23:24:32 +0000
- tests: Enable features to fix unstabilized `#[bench]` (rust-lang/cargo#7198)
- Fix excluding target dirs from backups on OSX (rust-lang/cargo#7192)
- Handle symlinks to directories (rust-lang/cargo#6817)
- Enable pipelined compilation by default (rust-lang/cargo#7143)
- Refactor resolve `Method` (rust-lang/cargo#7186)
- Update `cargo_compile` module doc. (rust-lang/cargo#7187)
- Clean up TargetInfo (rust-lang/cargo#7185)
- Fix some issues with absolute paths in dep-info files. (rust-lang/cargo#7137)
- Update the `url` crate to 2.0 (rust-lang/cargo#7175)
- Tighten requirements for git2 crates (rust-lang/cargo#7176)
- Fix a deadlocking test with master libgit2 (rust-lang/cargo#7179)
- Fix detection of cyclic dependencies through `[patch]` (rust-lang/cargo#7174)

## rls

1 commits in 70347b5d4dfe78eeb9e6f6db85f773c8d43cd22b..93d9538c6000fcf6c8da763ef4ce7a8d407b7d24
2019-07-30 12:56:38 +0200 to 2019-07-31 21:42:49 +0200
- Update cargo (rust-lang/rls#1529)
bors added a commit that referenced this pull request Aug 5, 2019
Remove debug panic in package-features.

I accidentally left a debug panic in #7186. This adds a test to exercise this code path.
pietroalbini added a commit to rust-lang/docs.rs that referenced this pull request Sep 4, 2019
This also updates the docs.rs codebase to account for this cargo change:
rust-lang/cargo#7186
@ehuss ehuss added this to the 1.38.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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