Automatically update `patch`, and provide better errors if an update is not possible. #8248

ehuss · 2020-05-16T20:51:57Z

This attempts to provide more user-friendly error messages for some situations with a bad patch. This is a follow-up to #8243.

I think this more or less covers all the issues from #4678. I imagine there are other corner cases, but those will need to wait for another day. The main one I can think of is when the patch location is missing required features. Today you get a "blah was not used in the crate graph." warning, with some suggestions added in #6470, but it doesn't actually check if there is a feature mismatch.

Closes #4678

rust-highfive · 2020-05-16T20:52:00Z

r? @alexcrichton

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

alexcrichton · 2020-05-18T14:38:50Z

Thanks for working on this!

I think this is good to give some more useful information here regardless, but I don't think this is the final state we want to end up at. Ideally Cargo would automatically update/manage the lock file, even in the face of stale lock files and updated patches, avoiding the need to print an error asking the user to update.

One of the things I run into is when I've got a [patch] pointing to the local repository (for unrelated reasons). If the crates are all bumped then nothing works afterwards. For example the wasm-bindgen repository you can run rustc publish.rs && ./publish bump, after which cargo build gives you weird #4678 error messages. Cargo still gives an error (a better one) with this PR, but ideally the lock file would simply automatically get updated.

I think though it may be a small enough change to, if there's one candidate, treat it as if there's no locked version if the previously locked version doesn't match?

ehuss · 2020-05-18T19:40:39Z

Yea, I agree it would be better to be automatic. I'm uncertain how to do that.

The issue is that the old version is "locked" in the PackageRegistry, so adding a patch to a new version won't work (it will still ultimately want to use the old version). If the patch function just returns the new patch, it ends up in the "unused" section, and Cargo remains locked to the old version.

Do you have any advice or ideas how to handle that? "Unlocking" the original entry doesn't sound like the right way to go, so maybe making it part of the "avoid" list early on might work? register_previous_locks is pretty gnarly, and I haven't yet understood everything it does. It may need to register patches before the call to register_previous_locks, and I'm uncertain if that's possible. If you don't have any ideas, I'll try to spend some time untangling it.

alexcrichton · 2020-05-18T20:13:35Z

Hm I'll try to dig in for a bit and see what I can come up with.

alexcrichton · 2020-05-18T20:23:45Z

Ok so I think the general problem is that the keep filter used here is not aggressive enough and fails to account for this comment. We can't keep a [patch] to a path dependency if the path dependency has changed on the filesystem. I think we can fix the issue I've run into historically with using the updated keep function.

I've got an ugly patch which I believe handles the case I run into with wasm-bindgen at least.

I think this should work in general to hook into the register_previous_locks function a bit more. That could perhaps return the "keep" set or maybe even a new closure to keep things with. I haven't looked too much into this though. I'd always left #4678 as a "I should investigate this one day" issue where I wanted to dig into cc'd issues, reproduce, and verify the fix. Ideally that #4678 never shows up in error messages at all and we shouldn't need this PR beyond the better error messages it returns in the default case.

WDYT about trying to hook into register_previous_locks a bit more, updating the error message to indicate a new Cargo issue should be opened if it seems suspicious, and landing these improvements as well?

ehuss · 2020-05-20T01:20:07Z

OK, I took a stab at making patches update automatically. In the process, I changed it so that if there are multiple possible patches, it picks the one with the greatest version. That might be dangerous, but seems to be decent behavior when patching with an alt registry.

There are a million edge cases, and I'm a bit tired of chasing them. I imagine there are still a lot of rough spots. At this point, my hope is that user reports will help guide anything that needs to be fixed. However, if you can think of some other tests to try, I'll give them a shot. One area that I'm uncomfortable with is transitive dependency updates. Somehow they seem to work, but the fact that I didn't make changes to register_previous_locks makes me uneasy.

From a high level, the main change is to update the filter ("avoid" set) to include patches when the patch is locked, but the locked value is no longer found. This should make it so that cargo build will automatically update Cargo.lock to the new value. There are two things that are added to the avoid set: the patch itself, and the thing the patch is patching.

alexcrichton

Thanks for all the tests! This is looking good to me, and I think the general idea should work out.

FWIW it's taken us years to get the resolver right and it still falls over in some obscure cases. It's unfortunately a really tricky problem and we can't really forsee all the issues all the time. That being said it's worked out pretty well having a solid core implementation where we fix bugs over time, so I think that's fine with [patch] here to not sweat about "did we think of everything?!". In the limit of time we'll think of everything :)

alexcrichton · 2020-05-20T14:29:38Z

src/cargo/core/registry.rs

+        summaries.sort_by(|a, b| a.version().cmp(b.version()));
+        summaries.pop().unwrap()
+    };
+    if summaries.len() > 1 {


This makes me a bit hesitant here trying to handle multiple candidates being returned. When [patch] was originally implemented we didn't have custom registries yet, so the only real usage of [patch] was pointing to git and path, neither of which can possibly have multiple versions of a package. With custom registries, though, that's now possible!

My main worry is that the candidates here don't participate in the normal version resolution of everything else. The biggest version may not always be the right one to pick (with other constraints like links and such).

Could this continue to restrict to only one package? I think it's fine to allow more candidates, but I think what should be done is to add all the candidates to "these can be searched through with version resolution". Version resolution would then do its normal thing to pick which is the best.

I'm not sure if I'm understanding correctly. If you're saying that this function should return multiple summaries, the problem is that any of the entries that aren't selected during version resolution end up in the unused patch list which triggers a warning.

I guess it could group related summaries together, so that the "unused patch" logic could only mark them unused if none in the group were used. However, that would require changes in lots of places (which I'd rather not do right now).

Unless you meant something else, should I just switch it back to an error (maybe with a suggestion to use an = version requirement)?

Oh sorry, let me clarify. Primarily this is a change in behavior from today which I'm worried about, so I would prefer to switch it back to make multiple candidates an error.

Other than that I was thinking that we probably could support this, but not by picking a version here and instead letting version resolution later pick a version. The theoretical idea behind [patch] was "take this crate and assume it comes from that source". That same idea works for "take this set of crates and assume they all come from that source", but the code here just isn't structured to do that. Ideally what we should do here is that when there's an unlocked (the only way to return multiple versions) dependency we make all matching candidates available for version resolution from the resolver, and the resolver picks one we'd then lock. If it ends up being entirely unused then we could pick the biggest one to lock and it wouldn't be an issue.

Does that make sense?

alexcrichton · 2020-05-20T14:37:57Z

src/cargo/core/registry.rs

+            // version that matches "0.1.2". It is unusual to explicitly write a
+            // version in the patch.
+            anyhow::bail!(
+                "The patch is locked to {} in Cargo.lock, but the version in the \


I think technically the mention of Cargo.lock here may be superfluous? While true it may not necessarily help that previously you had a successful build and have since modified the version to be too restrictive.

I also think it'd be nice, if we're specifically calling out versions, to list the versions in play here.

Could this error case fall-through to the error handling code below to get a similar error message? It'd be nice if the existence of a lock file only guided which packages remained unlocked afterwards.

alexcrichton · 2020-05-20T14:41:14Z

src/cargo/core/registry.rs

+                locked_patch.version_req(),
+            );
+        }
+        let summary = best_summary(&mut orig_matches);


It might be best to restructure this so it's maybe recursive or otherwise shares logic with the above. I'm a bit wary of having two instances of "pick the summary out of this list of summaries" which differ slightly

alexcrichton · 2020-05-20T14:42:26Z

src/cargo/core/registry.rs

+                    name_only_dep,
+                    e
+                );
+                "".to_string()


This might be a bit simpler to explicitly ignore it with:

let found = source.query_vec(...).unwrap_or_else(|| { warn!(...); Vec::new() });

alexcrichton · 2020-05-20T14:42:55Z

src/cargo/core/registry.rs

+        };
+        if found.is_empty() {
+            anyhow::bail!(
+                "The patch location does not appear to contain any packages \


Could the patch location be mentioned here and below? (e.g. rendering in source_id() and such)

…ies.

ehuss · 2020-05-20T17:20:18Z

I think I covered most of your feedback.

alexcrichton · 2020-05-21T14:29:45Z

@bors: r+

👍

bors · 2020-05-21T14:29:47Z

📌 Commit 4f2bae9 has been approved by alexcrichton

bors · 2020-05-21T14:29:55Z

⌛ Testing commit 4f2bae9 with merge d662f25...

bors · 2020-05-21T14:53:19Z

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

Update cargo 7 commits in 500b2bd01c958f5a33b6aa3f080bea015877b83c..9fcb8c1d20c17f51054f7aa4e08ff28d381fe096 2020-05-18 17:12:54 +0000 to 2020-05-25 16:25:36 +0000 - Bump to semver 0.10 for `VersionReq::is_exact` (rust-lang/cargo#8279) - Fix panic with `cargo tree --target=all -Zfeatures=all` (rust-lang/cargo#8269) - Fix nightly tests with llvm-tools. (rust-lang/cargo#8272) - Provide better error messages for a bad `patch`. (rust-lang/cargo#8248) - Try installing exact versions before updating (rust-lang/cargo#8022) - Document unstable `strip` profile feature (rust-lang/cargo#8262) - Add option to strip binaries (rust-lang/cargo#8246)

Provide better error messages for a bad patch.

b78cb37

rust-highfive assigned alexcrichton May 16, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2020

Make patches automatically update if updated.

afa3ace

alexcrichton reviewed May 20, 2020

View reviewed changes

ehuss added 4 commits May 20, 2020 08:36

Simplify error handling.

2c0cd97

Don't include a special-case error for a locked patch matching 0 entr…

6fd11a7

…ies.

Show patch location in error message.

5dde9cc

Revert change to automatically select the greatest patch match.

4f2bae9

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 May 21, 2020

bors merged commit d662f25 into rust-lang:master May 21, 2020

ehuss changed the title ~~Provide better error messages for a bad patch.~~ Automatically update patch, and provide better errors if an update is not possible. May 21, 2020

ehuss mentioned this pull request May 26, 2020

Update cargo rust-lang/rust#72596

Merged

ehuss mentioned this pull request Sep 8, 2020

patch was not used message does not adequately describe reason #8690

Closed

ehuss added this to the 1.45.0 milestone Feb 6, 2022

ldm0 mentioned this pull request May 4, 2024

Don't squash path dependencies with same package name #13863

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically update `patch`, and provide better errors if an update is not possible. #8248

Automatically update `patch`, and provide better errors if an update is not possible. #8248

ehuss commented May 16, 2020

rust-highfive commented May 16, 2020

alexcrichton commented May 18, 2020

ehuss commented May 18, 2020

alexcrichton commented May 18, 2020

alexcrichton commented May 18, 2020

ehuss commented May 20, 2020

alexcrichton left a comment

alexcrichton May 20, 2020

ehuss May 20, 2020

alexcrichton May 20, 2020

alexcrichton May 20, 2020

alexcrichton May 20, 2020

alexcrichton May 20, 2020

alexcrichton May 20, 2020

alexcrichton May 20, 2020

ehuss commented May 20, 2020

alexcrichton commented May 21, 2020

bors commented May 21, 2020

bors commented May 21, 2020

bors commented May 21, 2020

Automatically update patch, and provide better errors if an update is not possible. #8248

Automatically update patch, and provide better errors if an update is not possible. #8248

Conversation

ehuss commented May 16, 2020

rust-highfive commented May 16, 2020

alexcrichton commented May 18, 2020

ehuss commented May 18, 2020

alexcrichton commented May 18, 2020

alexcrichton commented May 18, 2020

ehuss commented May 20, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented May 20, 2020

alexcrichton commented May 21, 2020

bors commented May 21, 2020

bors commented May 21, 2020

bors commented May 21, 2020

Automatically update `patch`, and provide better errors if an update is not possible. #8248

Automatically update `patch`, and provide better errors if an update is not possible. #8248