[experiment] `patch` with patch files #13779

weihanglo · 2024-04-18T21:50:22Z

What does this PR try to resolve?

An unstable -Zpatch-files flag is added to support patching your dependencies with patch files, with the patchtool you configure in [patchtool] config table.

This is one experiment based on rust-lang/rfcs#3177 that the team maybe be interested in.
See also #4648.

How should we test and review this PR?

Apparently commit by commit. You could start from test to understand the current user experience of it.

Some design decisions in this implementation:

Every patch is settled and applied before the dependency resolution. This implies
- No re-resolve.
- Cargo need to fetch a certain version of package beforehand.
  This is currently done by requiring an exact version requirement =,
  so Cargo knows what to download.
- Due to the nature of patches applied before dependency resolution,
  we could patch whatever we want including dependencies and package.version.
- The Summary registered to PacakgeRegistry is the source after patching.
{ patches = [<path>] } field is only available for dependencies under [patch] table,
and only allows patching registry dependencies for now.
A new source PatchedSource that fetches and applies patches to the original source code during block_until_ready.
- Introduce a new protocol patched+, which compose the SourceId to patched.
- Tracked what was patched in URL query string, for example patching serde@1.0.0 is like:
```
patched+registry+https://github.com/rust-lang/crates.io-index?name=serde&version=1.0.0&patch=/path/to/my-bugfix.patch
```
  This SourceId representation also appears in Cargo.lock and Package ID Spec.
[patchtool] table in Config to control.
I imagine it would be something like git mergetool for registering tools,
though at this moment it only accepts one via path.
Cache of patched source resides in $CARGO_HOME/patched-src/<hash-of-original-source>/<pkg>-<version>/<cksum-of-patches>.
Patched source rebuilds whenever content of patch files or the underlying source change.

You can find some more documentation under src/cargo/sources/patched.rs.

Additional information

I don't think patching before dependency resolution is ergonomic.
Your patched source code would be stuck in time and you need to eagerly update and rebuild them.
Some patches may be safely applied across major versions.
However with the current design you need to specify the same patch twice.

We may like to revisit patching during the resolution,
and reconsider the work of re-resolving dependencies.
That is a more user-friendly way for patching with patch files.

rustbot · 2024-04-18T21:50:27Z

r? @ehuss

rustbot has assigned @ehuss.
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

weihanglo · 2024-04-19T01:22:57Z

src/cargo/sources/patched.rs

+/// A file indicates that if present, the patched source is ready to use.
+const READY_LOCK: &str = ".cargo-ok";
+
+/// `PatchedSource` is a source that, when fetching, it patches a paticular


See this doc comment for some design decisions.

epage · 2024-04-19T01:47:05Z

@weihanglo how would you like this reviewer?

As this is unstable and we previously agreed to the idea of experiments, in theory the bar is fairly low.

I will admit some hesitance on this experiment. In adding a new source kind, I'm assuming this can be a bit invasive (to be fair, I've not yet looked at the code). I'm also a bit surprised at the design direction described in the PR description.

So how much design discussion should we work out vs get in, try it out, and then iterate on alternative design ideas as we see needed?

weihanglo · 2024-04-19T02:10:49Z

@epage, This is not intended to merge as-is. It's more like a proof of concept that this approach does works but with a not okay user experience.
To be honest, I lean toward a resolve-then-patch solution but this one is not. I am tempted to write another one that might introduce re-resolve.

About reviewing the pull request, I would recommend starting from tests to get an idea of how it looks like. I would suggest not getting into details until we feel better with the UI.

We could probably open a Zulip topic on t-cargo for design discussions.

bors · 2024-04-19T22:25:07Z

☔ The latest upstream changes (presumably #13778) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-04-22T16:59:35Z

☔ The latest upstream changes (presumably #13783) made this pull request unmergeable. Please resolve the merge conflicts.

Eh2406 · 2024-04-26T20:05:29Z

Having look this over it seems pretty reasonable. If you would prefer to experiment with a "resolution loop" I'd be happy to compare cost-benefits, but I suspect this is simpler.

Cargo need to fetch a certain version of package beforehand. This is currently done by requiring an exact version requirement =, so Cargo knows what to download.

I don't understand why the equals requirement is needed. Say the patch was for >2, why couldn't we ask of the underlying source for a list of versions and then patch each one as they turn out to be needed? Well, I guess will we might then have to patch them all to see if the version numbers change as a result of the patching process. But that could be avoided by banning changing version numbers.

I'm not remembering the API at the moment, it may be that we need to return them all up front instead of "as they turn out to be needed"? If that's the problem, we could think about making a lazier API. (PubGrub would be happy with the lazy API, I'm not sure how much churn would be involved in moving the current resolver.)

I don't think patching before dependency resolution is ergonomic.
Your patched source code would be stuck in time and you need to eagerly update and rebuild them.

I don't think I follow, what ends up stuck in time?

Some patches may be safely applied across major versions.
However with the current design you need to specify the same patch twice.

This seems to be an important point. Especially as it's across "different versions" with the current = limitation.

epage · 2024-04-26T20:20:53Z

Oh, I should capture some office hour thoughts

In my mind, no matter the type of patch, it should all act the same. It sounds like this patches before resolution, rather than during, and allows your patches to affect resolution unlike regular patches (iiuc). If this is the case for both kinds of patches, then that feels like a barrier to move forward.

In my mind, I see this PR introducing two things:

Registry [patch] entries
patches key for any kind of [patch] entry

I think we should decouple these conversations so we can make sure we are getting the right semantics for each or, at minimum, defer non-patches registry [patch] entries without backing ourselves into a corner on its design.

A thought of mine that didn't come up during office hours is me worrying that pluggable patch tools might be generalizing too much. I'd want to see a better explanation of why that extra complexity is needed and what the alternatives are.

Eh2406 · 2024-04-26T20:39:41Z

unlike regular patches (iiuc).

Regular patches can also affect resolution. It is replace that can not.

weihanglo · 2024-04-26T22:01:06Z

Reply to #13779 (comment):

I don't understand why the equals requirement is needed.
...it may be that we need to return them all up front instead of "as they turn out to be needed"? If that's the problem, we could think about making a lazier API.

= requirement is needed so that we know what to patch before resolution. Otherwise yeah we need to patch every version in that version range.

We could have lazy mechanism, but I would like to avoid patching “during resolution” at this moment. Patching before or after resolution is less complicated I believe.

I don't think patching before dependency resolution is ergonomic.
Your patched source code would be stuck in time and you need to eagerly update and rebuild them.

I don't think I follow, what ends up stuck in time?

The exact requirement issue.

Reply to #13779 (comment):

In my mind, I see this PR introducing two things:

Registry [patch] entries

patches key for any kind of [patch] entry

Actually this PR intentionally touches nothing in ops::resolve or core::resolver. The core idea is having a new source and prepare it for resolution. You can consider it as a new kind of VCS source. Hence, it is not really that controversial.

The one major thing to explore is the semantic of the new patched source kind when being in [patch].

Just talked to Jacob earlier. There are at least three timing for patching:

Registry [patch] before resolution
- This is how today's [patch] work, as well as this PR.
- Need to know every summary upfront.
Register [patch] on demand during resolution
- This might be hard. [patch] can be added or removed depending on application of other [patch], so more complicated backtraces might be needed. (I didn't look into this route deep though)
Register [patch] after resolution, and re-resolve if needed
- Re-resolve happens if any summary has changed due to patch, and is currently in use in resolve graph
- If we go this route, we could first ban any change to package.{name,version}. By doing so [patch] should have the same semantic with the current patch kinds (name@version must match).
- This is the next thing I would like to experiment on.

(By registering [patch] I mean making [patch] entry available for dependency resolver to select/query.)

bors · 2024-04-30T19:00:56Z

☔ The latest upstream changes (presumably #13813) made this pull request unmergeable. Please resolve the merge conflicts.

Since `[patch]` section exists also in config, so have it inboth cargo-features and -Z flag.

`SourceKind::Patched` represents a source patched by local patch files.

One thing left out here is a consistent/stable hash for patches, The are absolute local paths that might introduces unnecessary changes in lockfile. To mitigate this, patches under the current workspace root will be relative.

rustbot assigned ehuss Apr 18, 2024

weihanglo force-pushed the unidiff branch 2 times, most recently from b8bb3e5 to 74c61c6 Compare April 19, 2024 01:08

weihanglo commented Apr 19, 2024

View reviewed changes

weihanglo mentioned this pull request Apr 19, 2024

Allow patching dependencies with patch files #4648

Open

weihanglo force-pushed the unidiff branch 2 times, most recently from b77a914 to 2b15782 Compare April 19, 2024 01:38

weihanglo force-pushed the unidiff branch 2 times, most recently from a46ced8 to ef73e26 Compare April 19, 2024 02:01

weihanglo force-pushed the unidiff branch from ef73e26 to ca6fa7e Compare April 20, 2024 03:36

weihanglo mentioned this pull request Apr 20, 2024

allow overriding dependency major version #5640

Open

weihanglo mentioned this pull request Apr 25, 2024

refactor(toml): extract dependency-to-source-id to function #13802

Merged

weihanglo force-pushed the unidiff branch from ca6fa7e to 1d6c52e Compare April 25, 2024 18:38

feat(unstable): new unstable feature patch-files

3754e87

Since `[patch]` section exists also in config, so have it inboth cargo-features and -Z flag.

weihanglo force-pushed the unidiff branch from 1d6c52e to 3829cc0 Compare May 10, 2024 13:50

weihanglo added 11 commits May 10, 2024 10:03

feat(schemas): dep.patches field (unstable)

eecf2f3

feat(schemas): SourceKind::Patched (unstable)

9afc52d

`SourceKind::Patched` represents a source patched by local patch files.

feat: support SourceKind::Patched in SourceId

e3555c4

One thing left out here is a consistent/stable hash for patches, The are absolute local paths that might introduces unnecessary changes in lockfile. To mitigate this, patches under the current workspace root will be relative.

feat: dont canonicalize patched+ to https for github

d69cb28

feat: new table [patchtool] in config.toml

b235dfe

feat: new source PatchedSource

7f86abb

feat: parse dep.patches to construct patched source

337bd95

test(patch-files): verify non-blocking gates and warnings

3bcf776

test(patch-files): verify invalid manifest

ba219a8

test(patch-files): verify patch works

d201363

test(patch-files): verify pkgid sourceid representation

368fe57

weihanglo force-pushed the unidiff branch from 3829cc0 to 368fe57 Compare May 10, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[experiment] `patch` with patch files #13779

[experiment] `patch` with patch files #13779

weihanglo commented Apr 18, 2024 •

edited

rustbot commented Apr 18, 2024

weihanglo Apr 19, 2024

epage commented Apr 19, 2024

weihanglo commented Apr 19, 2024

bors commented Apr 19, 2024

bors commented Apr 22, 2024

Eh2406 commented Apr 26, 2024 •

edited

epage commented Apr 26, 2024

Eh2406 commented Apr 26, 2024

weihanglo commented Apr 26, 2024 •

edited

bors commented Apr 30, 2024

[experiment] patch with patch files #13779

Are you sure you want to change the base?

[experiment] patch with patch files #13779

Conversation

weihanglo commented Apr 18, 2024 • edited

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Apr 18, 2024

weihanglo Apr 19, 2024

Choose a reason for hiding this comment

epage commented Apr 19, 2024

weihanglo commented Apr 19, 2024

bors commented Apr 19, 2024

bors commented Apr 22, 2024

Eh2406 commented Apr 26, 2024 • edited

epage commented Apr 26, 2024

Eh2406 commented Apr 26, 2024

weihanglo commented Apr 26, 2024 • edited

bors commented Apr 30, 2024

[experiment] `patch` with patch files #13779

[experiment] `patch` with patch files #13779

weihanglo commented Apr 18, 2024 •

edited

Eh2406 commented Apr 26, 2024 •

edited

weihanglo commented Apr 26, 2024 •

edited