rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.53k stars 2.37k forks source link

Allow specifying global replacements in .cargo/config, not just crate Cargo.toml #3308

Closed vvuk closed 7 years ago

vvuk commented 7 years ago

the .cargo/config paths mechanism seems to be somewhat deprecated, or at least unmaintained -- e.g. if any of those crates have optional dependencies that aren't normally selected (e.g. clippy), a warning is printed that a dependency was added on the optional ones.

By contrast, [replace] works well, but it can only be specified in a crate's Cargo.toml, making it difficult to work with when the upstream is changing (requires rebasing your replacements to Cargo.toml, and making sure they don't get checked in accidentally).

We should be able to specify global replacements in .cargo/config.

alexcrichton commented 7 years ago

The tl;dr; here is unfortunately this is not possible.

To explain a bit more in detail, this all boils down to how the two features interact with Cargo.lock. When you use [replace] in a manifest it takes this into account when creating Cargo.lock. So much so that replacements are themselves recorded in Cargo.lock. This has many benefits:

Note that this is all possible because [replace] is located in Cargo.toml, meaning it's able to modify Cargo.lock.

Now paths, on the other hand, is a very historical feature in Cargo. It was basically literally the first thing implemented, and we just never got around to fixing it before Cargo's release. The paths feature is placed in .cargo/config, which crucially means that it cannot affect Cargo.lock resolution. As a result, it has few of the benefits mentioned above.

The main point is that if you can't modify Cargo.lock, then an override cannot restructure the crate graph. If the graph were restructured then there's no way we could restructure it deterministically over time, creating an endless stream of bug reports about spurious recompiles, spurious registry updates, etc. Note that this is precisely the bug in the paths implementation. It allows for restructuring the crate graph, which implies that nondeterministic restructuring happens, causing endless bugs in Cargo.

So with all that in mind, we've got two use case for replacing code with something locally:

The first is [replace], stable, and works well (as you've noted). The latter has historically been paths but doesn't work well and the "more powerful" features of it are deprecated. Note, though, that the feature itself is not deprecated nor going away as it has a powerful and strong use case! In the future paths will just start to have a hard constraint that a replacement cannot restructure the dependency graph. In other words, if you don't get a warning today, then you're fine.

Does that all make sense? With that in mind we can't literally add [replace] to global configuration, but paths will always be around for overriding. In that sense this may boil down to https://github.com/rust-lang/cargo/issues/736 where we should provide the ability to perhaps have more targeted path overrides (like [replace]) where you can specify both a source and a version range that needs to be replaced.

vvuk commented 7 years ago

The main point is that if you can't modify Cargo.lock

Okay, but -- why can't you modify Cargo.lock? Or rather: if I have some kind of global replace list, why can't it behave as if every single Cargo.toml that cargo read had those replacements specified? It should still certainly record their presence in Cargo.lock, and if they ever go away, the behaviour should be the same as if the Cargo.toml was changed to remove them.

I agree that such global configuration can lead to confusion, but there's already precedent, such as specifying rustflags in a .cargo/config. There are valid use cases for it (for example: I work on the webrender crate. I want to always use my local version of it, assuming versions match, without needing to modify Cargo.toml in anything that might depend on it, and without needing to carry that modification through my git tree. I can't use path overrides because it causes problems if I ever change deps.).

In the future paths will just start to have a hard constraint that a replacement cannot restructure the dependency graph. In other words, if you don't get a warning today, then you're fine.

Hm, then I think there's a bug here. I have a crate that has serde_derive listed as an optional dependency. Default features specify codegen, which does serde_codegen. If I specify a path override to that crate, which is literally exactly the same as the one on crates.io, I get a warning about a dep on serde_derive being either added or modified to not match. This is the same behaviour on other crates on any optional deps (e.g. any crate that has an optional dep on clippy).

alexcrichton commented 7 years ago

The purpose of Cargo.lock is to provide reproducible builds at the crate graph level, but if we start reading global configuration which modifies Cargo.lock then that starts to erode. You could now forget there's a global replace and commit the Cargo.lock into the repo, and then it can be very difficult to reconstruct how to get the same crate graph.

The confusion part is basically the lynchpin of why this currently isn't allowed, basically.

Hm, then I think there's a bug here.

Oh dear! Can you file a bug?

vvuk commented 7 years ago

Yep, the confusion would be bad -- but wouldn't it be mitigated by recording the replacements in Cargo.lock and reporting if they change? e.g. Cargo.lock was generated with replacements that are now missing! Build may not be what you expect. or somesuch.

Oh dear! Can you file a bug?

Filed https://github.com/rust-lang/cargo/issues/3313 :)

alexcrichton commented 7 years ago

Yeah we could provide a warning, but it wouldn't solve the problem of figuring out how to regenerate Cargo.lock (which is done on every build). In that sense we'd have to record everything but we also need to balance changes to Cargo.lock as having is oscillate too much can be bad for checking into repositories.

Thanks for filing the issue!

larsbergstrom commented 7 years ago

@wycats This discussion captures a lot of what we were talking about a few weeks ago around the need to only use [replace] for suitable situations (e.g., handling a CVE) and you identified both some work that should happen around paths and possibly another new feature. Do you have a writeup / RFC for that?

jethrogb commented 7 years ago

I was unpleasantly surprised by these warnings popping up after a recent upgrade.

I use git dependencies a lot. For development across git repositories, I have to use path overrides. I don't think I should have to make temporary changes to Cargo.toml that are never going to be checked in.

carols10cents commented 7 years ago

The tl;dr; here is unfortunately this is not possible.

I'm closing this issue based on this comment; please reopen and update the title and description if I've misunderstood.

snuk182 commented 6 years ago

Another usecase for .cargo/config paths is the usage of Qt bindings generator. While pre-generated bindings exist at crates.io, thus can be referenced as a default stub, there is a variety of environments that contain Qt with variety of Qt versions, against which the pre-generated bingings are useless and should be generated in place. And in order to use them the dependant libraries should somehow reach them. Via config paths this is done transparently (I also like the dependency remap warning on cargo build). Via [patch] for the libraries I don't own this is impossible.