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 for "Running cargo update without a Cargo.lock ignores arguments" #6872 #6904

Merged
merged 4 commits into from May 15, 2019

Conversation

Projects
None yet
4 participants
@fluffysquirrels
Copy link
Contributor

commented May 3, 2019

No description provided.

Alex Helfet added some commits May 3, 2019

Alex Helfet
@rust-highfive

This comment has been minimized.

Copy link

commented May 3, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@fluffysquirrels

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Closes #6872.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Oops sorry I missed this! Could this perhaps be structured a way internally that doesn't involve recursing and redoing a lot of work? Perhaps the branch could simply return an empty resolve as the previous resolve since one technically doesn't exist?

@fluffysquirrels

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

I can see how to remove the recursion and skip saving and reloading Cargo.lock.

The current code queries previous_resolve (loaded from the Cargo.lock), looking for the dependencies' PackageId to do the update. I'll try and work out another way to query this, it looks like PackageRegistry might work.

@fluffysquirrels

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

This updated code avoids recursion and an unnecessary save and load of the lock file.

The second new PackageRegistry is required to avoid a failed assertion assert!(!self.patches_locked) in PackageRegistry::lock_patches.

To avoid using a first resolve_with_previous at all, I'm looking to use PackageRegistry::query (provides a set of Summary structs) to replace Resolve::query (returns a single PackageId struct) in the update logic. I'm not sure how I'd go about finding a unique PackageId from this set. I can filter by the version string provided to update on the command line, but it's not clear to me that will provide a unique answer.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Hm so if you update this branch to simply return an empty Resolve (aka Resolve::default(), adding that if it doesn't already exist), I think that would also pass tests and exhibit the desired behavior?

@fluffysquirrels

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

There is no Resolve::default()currently, and if I wrote a trivial one with e.g. an empty .graph field, then Resolve::query() would just query the empty graph and return nothing, hence in cargo update we wouldn't find any of the dependencies we were trying to update.

I can pass None instead of Some(resolve) to ops::resolve_with_previous later in cargo update, but then I still need SourceIds for each package we are updating, and I don't know how to get them.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Oh I think I see. I thought that the --precise information was carried through some other side channel that didn't require the previous resolve, but I see now how it needs the previous resolve. This feels pretty bad but that's not your fault at all, it's largely just how badly designed the method of passing --precise down is! (blame me for that)

In light of that this looks good to me!

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

📌 Commit 36160ed has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

⌛️ Testing commit 36160ed with merge 1b4fab3...

bors added a commit that referenced this pull request May 15, 2019

Auto merge of #6904 - fluffysquirrels:first-update-precise, r=alexcri…
…chton

Fix for "Running cargo update without a Cargo.lock ignores arguments" #6872
@bors

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 1b4fab3 to master...

@bors bors merged commit 36160ed into rust-lang:master May 15, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@fluffysquirrels fluffysquirrels deleted the fluffysquirrels:first-update-precise branch May 15, 2019

@ehuss ehuss referenced this pull request May 16, 2019

Merged

Update cargo #60874

bors added a commit to rust-lang/rust that referenced this pull request May 16, 2019

Auto merge of #60874 - ehuss:update-cargo, r=alexcrichton
Update cargo

17 commits in 759b6161a328db1d4863139e90875308ecd25a75..c4fcfb725b4be00c72eb9cf30c7d8b095577c280
2019-05-06 20:47:49 +0000 to 2019-05-15 19:48:47 +0000
- tests: registry: revert readonly permission after running tests. (rust-lang/cargo#6947)
- Remove Candidate (rust-lang/cargo#6946)
- Fix for "Running cargo update without a Cargo.lock ignores arguments" rust-lang/cargo#6872 (rust-lang/cargo#6904)
- Fix a minor mistake in the changelog. (rust-lang/cargo#6944)
- Give a better error message when crates.io requests time out (rust-lang/cargo#6936)
- Re-enable compatibility with readonly CARGO_HOME (rust-lang/cargo#6940)
- Fix version of `ignore`. (rust-lang/cargo#6938)
- Stabilize offline mode. (rust-lang/cargo#6934)
- zsh: Add doc options to include non-public items documentation (rust-lang/cargo#6929)
- zsh: Suggest --lib option as binary template now the default (rust-lang/cargo#6926)
- Migrate package include/exclude to gitignore patterns. (rust-lang/cargo#6924)
- Implement the Cargo half of pipelined compilation (take 2) (rust-lang/cargo#6883)
- Always include `Cargo.toml` when packaging. (rust-lang/cargo#6925)
- Remove unnecessary calls to masquerade_as_nightly_cargo. (rust-lang/cargo#6923)
- download: fix "Downloaded 1 crates" message (crates -> crate) (rust-lang/cargo#6920)
- Changed RUST_LOG usage to CARGO_LOG to avoid confusion. (rust-lang/cargo#6918)
- crate download: don't print that a crate was the largest download if it was the only download (rust-lang/cargo#6916)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.