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

[experiment] patch with patch files (re-resolve) #14055

Closed
wants to merge 14 commits into from

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Jun 13, 2024

What does this PR try to resolve?

This is an alternative of #13779.

Most parts of the design are the same. The differences are:

  • patch entry no longer requires an exact version requirement, e.g. =1.2.3.
    Instead, only after a few rounds of dependency resolutions can a patch entry know which package version it is going to patch.
  • One patch is able to match multiple versions, if applicable.
  • Patching package.version field to a SemVer incompat version is allowed but kinda useless. The metadata of patched package, for example v2.0.0-to-v1.0.0, is different from the metadata of the real v1.0.0. Thus rustc things they are different versions.

How should we test and review this PR?

This is built upon #13779 with two new commits.

While this works, I don't think the code change is in a good state to merge. The design (hacking on a new SourceKind) could be inherently wrong.

The ugliest part is code that handles the reuse of existing patched in lockfile edd3172. There are rooms for cleanup but I don't believe it'll be significantly better.

Additional information

Doc hasn't got updated.

@rustbot
Copy link
Collaborator

rustbot commented Jun 13, 2024

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

@rustbot rustbot added A-configuration Area: cargo config files and env vars A-manifest Area: Cargo.toml issues A-registries Area: registries A-testing-cargo-itself Area: cargo's tests A-unstable Area: nightly unstable support A-workspaces Area: workspaces S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2024
@weihanglo weihanglo force-pushed the unidiff-reresolve branch 3 times, most recently from edd3172 to 8015563 Compare June 17, 2024 21:22
@bors
Copy link
Collaborator

bors commented Jul 3, 2024

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

@bors
Copy link
Collaborator

bors commented Jul 11, 2024

☔ The latest upstream changes (presumably #14234) 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.
Only after a few dependency resolution happen, can a `[patch]` entry
with patch files know which version it is going to patch. Hence we need
to "delay" the patch process until at least one resolution is done, and
re-resolves the graph is any patch become applicable with the newly
resolved dependency graph.
We've tracked patched depencencies in Cargo.lock with the `patched+`
protocol. However, we don't really make use of it when rebuilding from a
lockfile. This PR fixes that. You can see the test cases now won't
update registry becaseu it starts resuing locked patch dependencies.
@bors
Copy link
Collaborator

bors commented Jul 26, 2024

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

@weihanglo
Copy link
Member Author

See #13779 (comment).

@weihanglo weihanglo closed this Aug 9, 2024
@weihanglo weihanglo deleted the unidiff-reresolve branch August 9, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-manifest Area: Cargo.toml issues A-registries Area: registries A-testing-cargo-itself Area: cargo's tests A-unstable Area: nightly unstable support A-workspaces Area: workspaces S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants