Prioritize [patch] versions the way we do for versions in the lockfile

[Eh2406]

Describe the problem you are trying to solve

Often when [patch] does not work for a user it is because there are newer versions available from the canonical source. Fixing this requires ether temporarily modifying the version of the replaced package or the requirement of the depending package. In ether case this probably needs to be un-done before upstreaming the fix.

Describe the solution you'd like

If a version is from the [patch] section try it before versions from the canonical source, even if the patched package has a lower version. This is already what happens for versions that were in a lockfile, so it should not be hard to do.

Notes

We thought of this in a Cargo Team meeting, and I wanted to wright it down before we forgot.

[djmitche]

I'd like to give this a try, if I can.

It sounds like the suggested fix changes existing behavior. In particular, perhaps a crate has old [patch] hanging around that are unused but accidentally not deleted from the manifest. With the suggested fix, those patches would suddenly start applying.

Alternatives might be to warn or fail on an otherwise-ignored patch.

I'll start figuring out how the internals work to see where this change might be made.

[djmitche]

In fact, it looks like such a warning is in place:

cargo/src/cargo/ops/resolve.rs

Lines 435 to 452 in d1c0a9d

	if register_patches {
	// It would be good if this warning was more targeted and helpful
	// (such as showing close candidates that failed to match). However,
	// that's not terribly easy to do, so just show a general help
	// message.
	let warnings: Vec<String> = resolved
	.unused_patches()
	.iter()
	.map(|pkgid| format!("Patch `{}` was not used in the crate graph.", pkgid))
	.collect();
	if !warnings.is_empty() {
	ws.config().shell().warn(format!(
	"{}\n{}",
	warnings.join("\n"),
	UNUSED_PATCH_WARNING
	))?;
	}
	}

Maybe that fixes this issue? Or maybe it means that the changes to existing behavior are OK, since those changes are in cases that currently generate a warning.

[Eh2406]

I think the latter. It is ok to change the behavior as it has a warning.

[djmitche]

@rustbot claim

[djmitche]

I think this will be the critical passage:

cargo/src/cargo/core/registry.rs

Lines 553 to 566 in 7cdf534

	// A crucial feature of the `[patch]` feature is that we *don't*
	// query the actual registry if we have a "locked" dependency. A
	// locked dep basically just means a version constraint of `=a.b.c`,
	// and because patches take priority over the actual source then if
	// we have a candidate we're done.
	if patches.len() == 1 && dep.is_locked() {
	let patch = patches.remove(0);
	match override_summary {
	Some(summary) => (summary, 1, Some(patch)),
	None => {
	f(patch);
	return Ok(());
	}
	}

here, patches.len() == 1 means that there's exactly one patch for this name, and if the conditional is taken then the patch is used regardless of any other versions in the registry. I think we just need to remove the dep.is_locked() here, and always use a patch if one (and exactly one) is found, treating it as a lock. Does that sound about right?

[Eh2406]

I think that would lead to the more radical "don't look at other versions if patch exists", witch is a more significant change then we wanted to do (at this time).

The section of code I had in mind was try_to_use witch would get the patched versions added to it. The code for converting the patch descriptions in Cargo.toml in to PID's is just above, and is a bit of a mess. I think that the avoid_patch_ids , may be just what is need.

[djmitche]

I've read through both resolve modules and still not clear how it all fits together. In particular, I'm confused about "avoid" -- why does ops::resolve "avoid" the patch ids? Does it make sense to both "avoid" and "try_to_use" the same package id? In particular, the keep callable specifically returns false for "avoided" things (to_avoid and avoid_patch_ids), and that callback is used to construct try_to_use. So it seems that a significant chunk of the code in ops::resolve::resolve_with_previous is dedicated to calculating avoid_patch_ids and omitting those package ids from the try_to_use list. As I understand your suggestion, it would be to render that calculation unused!

If you can help me understand a bit more about how all of this fits together, that'd be great.

[alexcrichton]

The "avoid" bits were originally added for cargo update. When you run cargo update -p foo it adds foo to the "avoid" list, and with the --aggressive flag it adds foo and all of its transitive dependencies to the "avoid" list. The idea was that we would avoid locking those dependencies and allow them to freely float and get resolved via some other means internally (probably by updating them).

I think we since adapted the "avoid" to also include entries where if the Cargo.toml version requirement doesn't match what's in the lock file then we add that to "avoid" as well (and maybe transitive deps, it's been awhile since I looked at this). The purpose of that check was to continue to support builds where you edit Cargo.toml with a more restrictive version requirement than before.

I agree though that it's all very complicated, it's unfortunately not seen a ton of love of the years and I haven't done a great job of keeping it readable.

[djmitche]

Thanks for that - the history is helpful!

I'm still at a loss here, though -- anything I do seems to upset some delicate balance. For example, avoid_patch_ids seems to be more than just the PackageIds of the [patch] entries, but also versions of those packages from previous.

I definitely don't understand this, and I think that a guess-and-check method of making changes until the tests pass is not a sufficiently robust way to modify code this critical to Rust users. I'll keep trying things, but I will need a great deal of attention to the review! The 90s build time is making for slow going, too :(

My current project is to try to generate a list of preferred_patch_ids containing PatchIds of the patches. I'm struggling to convert a Dependency into a PackageId, though, since in general a Dependency may not have an exact version -- and even if that's successful, I'm not sure it's the right approach!

[djmitche]

Hm, it seems that the Dependency for a [patch] is not, in fact, exact. So, I think I'm stumped.

[djmitche]

Here is what I've got so far.

[Eh2406]

In the original Cargo meeting where we thought of this idea we were discussing how [patch] was tangled with so many complicated parts of the code. We were excited to find this one small way to make things better without much change. It is entirely possible that there is some complication that we missed.

[Eh2406]

So this is going to be trickier than I thought. Sorry for leading you astray. As you pointed out repeatedly the complication is that when making try_to_use patches are Dependencys not PackageIds. Then when they become PackageId in cargo/src/cargo/core/registry.rs we are at to low an abstraction level.

Plan B: is to smuggle patches all the way down to where try_to_use gets used. Then this becomes something like let a_in_previous = self.try_to_use.contains(&a.package_id()) || self.patches.iter().any(|d| d.matches(&a.package_id()));

[djmitche]

I can give that a shot.. but, is that tying things into even further knots?

Apologies for the delay -- I was away for the long weekend.

[Eh2406]

You are volunteering to do work, there is no need to apologize!

If you find that it makes for more problems, then it may be to knotty to be worth it. Otherwise it is a judgment call the most complicated code getting a little harder vs patch being easier to use.

[djmitche]

It looks like Plan B wasn't bad, after all. Maybe try_to_use should be renamed to prefer_locked_ids?

[djmitche]

Thanks for your help!

[Eh2406]

If you have renames or refactors you think would help, please do!

[djmitche]

Thanks for the reminder -- I created #9703.