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

Support [patch] in .cargo/config files #9204

Merged
merged 7 commits into from
Mar 15, 2021
Merged

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Feb 24, 2021

This patch adds support for [patch] sections in .cargo/config.toml
files. Patches from config files defer to [patch] in Cargo.toml if
both provide a patch for the same crate.

The current implementation merge config patches into the workspace
manifest patches. It's unclear if that's the right long-term plan, or
whether these patches should be stored separately (though likely still
in the manifest). Regardless, they should likely continue to be
parsed when the manifest is parsed so that errors and such occur in the
same place regardless of where a patch is specified.

Fixes #5539.

This patch adds support for `[patch]` sections in `.cargo/config.toml`
files. Patches from config files defer to `[patch]` in `Cargo.toml` if
both provide a patch for the same crate.

The current implementation merge config patches into the workspace
manifest patches. It's unclear if that's the right long-term plan, or
whether these patches should be stored separately (though likely still
in the manifest). Regardless, they _should_ likely continue to be
parsed when the manifest is parsed so that errors and such occur in the
same place regardless of where a patch is specified.

Fixes rust-lang#5539.
@rust-highfive
Copy link

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 24, 2021
@alexcrichton
Copy link
Member

I'm a bit wary about modifying the patch tables so deep in the manifest, it seems like this would emulate a [patch] in all crates.io dependencies decoded as well I think? Would it be possible to refactor so that when the [patch] entries are registered with the registry that's where the config-based patches are registered?

@jonhoo
Copy link
Contributor Author

jonhoo commented Feb 26, 2021

Good point! I placed the parsing there because I wanted to re-use to_dependency, which takes a Context, which is only constructed in that scope. I've re-organized slightly in 140a770 so that the parsing still happens in the same place (where we have Config), but the actual merging of dependencies only happens on resolve. What do you think?

I'm also a little unsure about

for dep in &mut *deps {
if let Some(i) = left.iter().position(|cdep| {
// XXX: should this also take into account version numbers?
dep.name_in_toml() == cdep.name_in_toml()
}) {

The patch tables are stored as BTreeMap<String, BTreeMap<String, TomlDependency>> which implies that each key in the patch table has to be unique, but I'm concerned about what happens if someone tries to patch, say, two versions of the same package using different patch names:

# Cargo.toml
[patch.crates-io]
nom5 = { ..., version = "5" }
nom6 = { ..., version = "6" }
# .cargo/config
[patch.crates-io]
nom = { ..., version = "5" }

Can that even happen? What's the expected outcome in this case?

@bors
Copy link
Contributor

bors commented Mar 3, 2021

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

src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Mar 5, 2021

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

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay, but this version looks good to me! Ia've got some minor thoughts inline, but otherwise r=me

src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 15, 2021

@alexcrichton Excellent, all good comments — I've pushed fixes in b50cde8. I don't think bors picked up your r= though?

@alexcrichton
Copy link
Member

@bors: r+

Oh that's not an actual feature of bors, it's more of "I won't take XX days to review this next time, I'll write r+ soon after I see the email it was updated"

@bors
Copy link
Contributor

bors commented Mar 15, 2021

📌 Commit b50cde8 has been approved by alexcrichton

@bors 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 Mar 15, 2021
@bors
Copy link
Contributor

bors commented Mar 15, 2021

⌛ Testing commit b50cde8 with merge 695465c...

@bors
Copy link
Contributor

bors commented Mar 15, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 695465c to master...

@bors bors merged commit 695465c into rust-lang:master Mar 15, 2021
@jonhoo jonhoo deleted the patch-in-config branch March 15, 2021 19:14
@ehuss
Copy link
Contributor

ehuss commented Mar 15, 2021

@jonhoo Can you follow up with a PR to add this to the unstable documentation (here). Unstable features should always be documented there.

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 15, 2021

Will do! Should we leave this open as a tracking issue? What do you envision as the stabilization path for this feature?

@ehuss
Copy link
Contributor

ehuss commented Mar 15, 2021

I think it's fine to open a new issue, it doesn't matter too much. The "new issue" button should have a template for a tracking issue.

@ehuss
Copy link
Contributor

ehuss commented Mar 15, 2021

Oh, and I'm not sure exactly what needs to happen to stabilize. We don't have a clear process for that, since it doesn't happen too often. 😜 Normally we let it bake on nightly for a while, to let people to try it out and see if there are any issues, and to gain more experience with it. This also gives some time to consider if it is the right design and something that we actually want to commit to.

@jonhoo
Copy link
Contributor Author

jonhoo commented Mar 15, 2021

Documentation PR: #9270

Tracking issue: #9269

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 17, 2021
Update cargo

8 commits in 32da9eaa5de5be241cf8096ca6b749a157194f77..90691f2bfe9a50291a98983b1ed2feab51d5ca55
2021-03-13 01:18:40 +0000 to 2021-03-16 21:36:55 +0000
- Add report if `cargo fix --edition` changes features. (rust-lang/cargo#9268)
- Fix --feature pkg/feat for V1 resolver for non-member. (rust-lang/cargo#9275)
- Fix doc duplicate removal of root units. (rust-lang/cargo#9276)
- Add CLI help text for patch-in-config (rust-lang/cargo#9271)
- Document `-Zpatch-in-config` (rust-lang/cargo#9270)
- Support [patch] in .cargo/config files (rust-lang/cargo#9204)
- Add `--future-incompat-report` support to `cargo test` (rust-lang/cargo#9264)
- 🍱 Crop favicon (rust-lang/cargo#9262)
@ehuss ehuss added this to the 1.52.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow [patch] (and [replace]) sections in Cargo config.
6 participants