support for [patch] override from .cargo/config

[jchlapinski]

We have exactly the same scenario as described in #5539. We would like to be able to override [patch] for some crates WITHOUT the need to modify their individual Cargo.toml manifests.

This PR is a draft implementation of this use case, where all entries from [patch] section defined in .cargo/config are picked up for every manifest.
Please comment if such a change is viable for cargo. Some potential problems/design decisions that should be solved:

1. Should [patch] entries be merged from both .cargo/config and Cargo.toml, or simply overridden entirely from .cargo/config if present.
2. What about [replace] section? I have seen a few open issues with discussion whether this section should be deprecated/removed?
3. If [patch] is overridden for several nested crates, from .cargo/config, probably warnings about unused patch dependency for each crate should then be silenced?

As a more detailed explanation why we want this feature - we develop a large project consisting of a few crates that are regular [workspace] members, as well as a few crates that are self-contained libraries, published independently on crates.io (however some depend on others).
In order to be able to work on a project as a whole, we nested those self-contained libraries in main repository as git submodules, and patched workspace members to use local versions of those libraries, instead of downloading them from crates.io. This makes developement much easier, since simultaneous changes in dependent libraries can be tested, before releasing them on crates.io.
However since, those libraries are independent from the main project, they had to be excluded from the workspace, and as such, their dependencies are not patched. This creates a problem, since when testing, each library is build using all of its dependencies from crates.io (skipping locally made changes in other libs).

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Thanks for the PR and sorry for the delay in getting to this!

This has been a feature we've been hesitant to add in the past because [patch] affects Cargo.lock and the generation of Cargo.lock has always been system-independent in the sense that that the same lock file is generated across systems. This feature, however, would break that invariant because it's possible to generate a Cargo.lock on a system which can't be done on another system unless some extra configuration is added.

There is however a feature where you can replace with .cargo/config, but due to its restrictions of not being able to update the lock file it's necessarily less powerful. Would the paths configuration key work for your use case?

[jchlapinski]

There is however a feature where you can replace with .cargo/config, but due to its restrictions of not being able to update the lock file it's necessarily less powerful. Would the paths configuration key work for your use case?

@alexcrichton That actually works exactly as we need, however it prints out this nerve wrecking long warning, as described in #5539 (comment).
My impression from this warning was that paths is being deprecated and soon to be removed from cargo, therefore I was investigating another way of solving the problem

[alexcrichton]

The paths feature isn't necessarily going away any time soon, but that warning can be thought of as a configuration error. When you override a package via paths it needs to have the exact same dependency list as the package that it's overriding. Are you able to fix that warning from being emitted?

[jchlapinski]

Hmm, that same dependency requirement, which I was unaware of, makes paths feature less usefull in our case... It is quite often that when working on a "patched" crate we add/remove dependencies to it as needed. Also some of the "patched" crates in our code depend on other crates, which are also locally "patched".

[kuviman]

@alexcrichton Can the issue with Cargo.lock be solved if it was possible to opt in or out using the [patch] section from .cargo/config?

So, for local testing with all the patched crates we would be using something like cargo build --use-local-patches, then Cargo.lock would be modified but should not be published.

Then, if everything is ok we would push/publish dependencies in correct order using regular cargo build to get the needed Cargo.lock which will now be reproducible on other systems.

This would solve adding/removing [patch] section to Cargo.toml problem at least, which also modifies Cargo.lock in non-reproducible ways.

[alexcrichton]

Ah ok, thanks for checking @jchlapinski!

@kuviman that's a possibility yeah! I mostly mean to point out that the sort of silent usage of [patch] from .cargo/config I don't think lines up well with Cargo's current goals, but I haven't thought too hard about how we might support this otherwise. What you mention sounds plausible!

[jchlapinski]

In order for [patch] feature to work well in projects with multiple crates, there needs to be a mechanism to define the [patch] section somewhere up the folder tree from the given Cargo.toml manifest. For this purpose .cargo/config works well.

However, @alexcrichton does not like the idea that basically in case .cargo/config is not included in the project files, it can lead to different results on systems that are missing it.

Perhaps the solution would be one of the following:

1. In Cargo.lock in [[package]] section for every local crate also include it's relative to Cargo.lock path in the filesystem. That way even, if .cargo/config is missing, but the overall project structure is OK (meaning all local crates are at expected paths), the project should compile fine.
   For example

[[package]]
name = "local-lib"
source =  "../local-lib" # currently for local crates this field is not defined at all
version = "0.2.4"
dependencies = []

2. Include the [patch] section into Cargo.lock in order for cargo to be able to verify its correctness with the current config when compiling.

I am happy to put this feature into code, after it is established how should it work.

[alexcrichton]

@jchlapinski hm I wonder if you can clarify your problem statement a bit? You say:

In order for [patch] feature to work well in projects with multiple crates, there needs to be a mechanism to define the [patch] section somewhere up the folder tree from the given Cargo.toml manifest.

FWIW a [workspace] would solve this issue where all crates in the same workspace share the same [patch]. I'd presume though there's a reason y'all aren't using workspaces?

---

For your possible solutions, those are plausible at least I think! I haven't given this a huge amount of thought as to what a solution would look like, I think I still need to do work to understand the use case motivating this.

[kuviman]

Hm, actually, virtual [workspace] with [patch] section works great, didn't think of that before. This also makes the target directory common for the projects, so it's better than .cargo/config for my usecase at least. The only issues are:

• Cargo.lock file is now located near the workspace root, so it would be needed to delete workspace's Cargo.toml temporarily to update project's Cargo.lock

• Doesn't work if one of the projects already uses workspaces - since nested workspaces don't work. If they did, it would be great (Nested workspaces #5042)

Anyway, works for me, thanks for the tip!

[alexcrichton]

@jchlapinski would the virtual manifest idea work for you as well? If so maybe we could close this in favor of the nested workspaces issue?

[jchlapinski]

@alexcrichton unfortunately no, we are already using virtual manifests and workspaces, but that does not solve our issues completely. Please allow me a day or two, I will write up a working example of what we are trying to achieve and what is not fully working right now for us, so that we will avoid confusion.

Nested workspaces are also something that we would love to see supported, BTW.

[bors]

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

[alexcrichton]

Ok I'm gonna go ahead and close this since this has gone quiet for quite some time. I think it'd be great to continue the discussion on nested workspaces to figure out a design and/or manpower to implement, and if there are other targeted issues here to take care of it'd be much appreciated to have a follow-up issue to discuss!