Relax requirement that [replace] overrides have the same version number
Closed this issue · 19 comments
Top-level overrides with [replace] as implemented in #2385 require that the replacement has the exact same version number in its Cargo.toml as the package being replaced. As others have discussed in the PR I think this restriction gets in the way more than it helps.
Here is a workflow I just had. This happens more often than not when I use [replace].
- I use some package maintained by someone else. In this case xml-rs.
- I realize I need some functionality to be added in that package. Maybe it just belongs there better, or like here I can’t do it in my own crates because of privacy or trait coherence rules.
- I write and send a pull request: netvl/xml-rs#121 I include a version number increment in the PR to signal to the maintainer that I’d like them to publish this change on crates.io.
- Back in my crate I add a
[replace]override, which doesn’t work because versions don’t match. I have to revert the version number locally. If I want some else to use my override, I need to push a second branch of the dependency with the same changes but without the version number increment. - Cargo doesn’t let me "update" my dependency (not the replacement) to a version that isn’t published yet.
Maybe the requirement could be that the replacement’s version is SemVer-compatible with the thing being replaced, instead of exactly the same?
IMO you shouldn't have included a version number increment, that would have solved everything?
Yes, but that’s not the point.
As a maintainer, if someone sending me a PR expects the change to published on crates.io soon after merging, I like if they include the version number increment in the PR. That’s one fewer step for me to do (if the change is small I might take a couple minutes to review/merge/publish it while I’m in the middle of doing something else), and I’m less likely to forget.
Even if it’s not a workflow you prefer personally as a maintainer, I don’t think there’s anything wrong with it or a good reason for Cargo to make it more difficult.
That is the point. Including a version bump is an antipattern, that's up to the maintainer (which you can of course request in comments)
Including a version bump is an antipattern
I disagree, but at this point we’re going in circles.
But even if it is an antipattern, it’s not a "you’re absolutely gonna shoot yourself in the foot" mistake. I don’t think Cargo should be that opinionated about workflows.
For what it’s worth, in servo/* repositories the maintainers never push to master. Anyone (including maintainers) who wants to land any change makes a PR, has it reviewed by someone else who then tells homu to send it to CI and land it if tests pass. Waiting for Travis-CI to schedule a build and run tests takes a few to many minutes.
When a PR has changes we know we want on crates.io, if we have to wait for it land to make another PR and do this whole dance all over again just to increment the version number, that’s a lot of overhead.
Fun fact: @nikomatsakis himself bumps his own projects to N+1 the moment he releases N. This means his projects cannot be replaced with master.
This [replace] feature with such version requirements is useless as such, IMO.
(I'm not really sure what's being discussed in this issue, but I certainly wouldn't take my usage of cargo to be "best practice".)
That is the point. Including a version bump is an antipattern, that's up to the maintainer (which you can of course request in comments)
What exactly do you expect us to do when our bumps are that involved? Do twice as many PRs in two batches, the first without the bumps and the second with just them? That doesn't scale, whatever were your comments on #2385.
I've been talking a lot with @SimonSapin, @nox, @wycats, and @alexcrichton about this issue. I'm not going to rehash everything here, but I wanted to float a proposal I've been thinking about and have talked to @wycats and @alexcrichton about in some detail, and they are basically on board with. If this looks like it will meet everyone's needs, I'll write up an RFC.
The basic idea was inspired by @nox's idea of a "staging index": basically, provide a way to layer a "pre-publication" version of a crate on top of an existing source like crates.io. The key point is that the pre-publication version works with Cargo's dependency resolution as if it had been published to crates.io. But the pre-publication version can come from a different location, such as a git repository.
Let's take the xml-rs example from this issue; let's say it's currently at version 0.3.1 on crates.io. Here's what the workflow would look like:
-
Fork xml-rs
-
Make change to xml-rs in your fork, including version bump to 0.3.2
-
Update your Cargo.toml
[prepublish] xml-rs = { git = "https://github.com/SimonSapin/xml-rs", branch = "io-error" } [dependencies] xml-rs = "0.3.2"
-
Do integration testing
-
Send PR to xml-rs, requesting publication
-
xml-rs author reviews
- waits for CI
- merges
- publishes
-
Remove
[prepublish], but keep version bump -
Make PR for Servo
The idea here is that you set up the version dependencies of your Cargo.toml as they should be once all publication has taken placee. While you're working prior to publication, you are getting the new version from a git repository. Cargo, however, will resolve dependencies as if that new version had been published.
This is crucially different from [replace]; whereas [replace] globally changes the source for an existing version of a crate, [prepublish] adds a new version of a crate. This works very nicely with Cargo's dependency resolution:
-
If the new version is a minor bump, Cargo will automatically coalesce it with existing dependencies. That is, if other crates in the graph depend on
xml-rs = "0.3.1", Cargo will resolve the dependency to the newer minor version, 0.3.2, which it will then draw from the git repository. -
If the new version is a major bump, Cargo will not coalesce it with existing dependencies, nor should it -- updating crates to work with the new version will require changes and testing. However, it's now possible to fork intermediate crates in the dependency graph, do the upgrade, bump their Cargo.toml to use a new serde and bump their own major version, and compile them using
[prepublish]. So, for example, when moving from serde 0.8 to 0.9, you should be able to set up a version of Servo in which all necessary crates are updated to serde 0.9 and with new version numbers -- all of the Cargo.toml files can be set up to have their "final form" modulo[prepublish]. You can then do integration testing, before landing any of the changes to these crates.
Stepping back, this [prepublish] feature essentially lets you simulate making an "atomic transaction" that spans several repositories. While you won't actually be able to land those changes atomically, you can set up the final state you want to achieve on a branch, and then work to land the pieces in some appropriate order, removing [prepublish] entries as publications occur. We could layer Cargo subcommands that fork a dependency, bump its version, and add it to [prepublish] automatically.
If you have a crate in [prepublish] and the version being pre-published is also found in the normal crate index, you'll get a warning; Cargo will prefer the version in the index, unless your lockfile already ties it to the pre-published version (in which case it won't break your build).
Why is this better than just letting [replace] span versions? The biggest difference is around major version bumps. Allowing you to replace one major version with another is a recipe for disaster; at best, you'll get compiler errors, but you could easily get silent behavioral changes from stemming from uses of the crate you didn't even realize were there. Actually pushing through the upgrade to the new version, crate-by-crate with unit testing, is essential.
More generally, in terms of trying to model an "atomic transaction" across repos, working with the eventual published version and Cargo's usual resolution gives a much more principled and clear story about what's going on than you get with [replace].
One of the big fears about letting [replace] change version numbers is that, if something is wrong with the new version of a dependency, a developer unaware of the replacement may well file bugs against that dependency, or otherwise be confused about what's going on (for example, because when they compile their own library directly, which doesn't use [replace], they don't see the problem). Servo's rule of not checking [replace] into master goes a long distance toward mitigating this problem, since only the developers working on a "transaction" ever see the intermediate state, and they should be aware of the active replacements. But [prepublish] lends additional clarity to the state by explicitly working with a new version, which should also help avoid confusion.
Even if we do add something like [prepublish], the [replace] feature still has a role to play for cases like emergency security fixes or other narrowly tailored replacements.
@SimonSapin, @nox, @larsbergstrom: what do you think?
@aturon This looks great! Thank you for putting this together.
I think that a key insight is that the way [replace] work currently is only a hindrance for Servo for crate versions that are not published yet, while the scenarios that @alexcrichton and @wycats are concerned about involved published crate versions.
Would [prepublish] like [crate] only be used in the top-level Cargo.toml file, or would it be in each crate that has a corresponding dependency? The former sounds more convenient, since we sometimes have a shared dependency used in many crates.
(Unimportant correction: in your workflow description, "Make PR for Servo" comes after "Do integration testing". But running all Servo tests on a laptop tends to take a very long time so we usually do that on CI servers, which involves opening a PR earlier in this workflow.)
@SimonSapin it definitely doesn't need to be in every crate. I'm not sure if we'd need to force it at crate root, or whether it could make sense to accumulate prepublications anywhere they appear.
Sounds good to me, thanks for the writeup @aturon! In terms of the nitty-gritty details we've often wanted the ability to augment sources in Cargo.toml for purposes such as:
- Adding a new registry (e.g. not crates.io) for the purposes of, for example, company internal packages.
- Changing the default registry from crates.io to something else (e.g. an internal registry)
I have a hunch that this sort of [prepublish] would want to integrate with this or otherwise be along these lines. All that basically to say that literally as written here:
[prepublish]
foo = { git = '..' }I think may not quite suffice because there's an implicit registry that's being modified, the crates.io registry. It may work by saying that this uses the "default" registry and we end up with the same syntax, but something to be considered at least (minor details)
Some discussion of [replace] and cargo vendor’s checksumming in Firefox:
- https://groups.google.com/forum/#!topic/mozilla.dev.platform/TSRH1EU-JVo
- https://bugzilla.mozilla.org/show_bug.cgi?id=1342815
In particular, there’s a proposal to systematically add a [replace] entry for every vendored crate, to work around checksumming and make local editing easier.
@SimonSapin I believe the preferred solution there is what's on the tin of https://bugzilla.mozilla.org/show_bug.cgi?id=1342815, no? That is, the real problem is that it's not trivial to fork code locally to do local edits when testing things out, it's a few steps.
Beyond that though it doesn't sound related to this particular issue at all?
@alexcrichton I think @SimonSapin was just giving us a heads up; this issue is where we've been logging the overall issues around [replace].
I do suspect that using [replace] is indeed the correct thing for what Gecko is trying to do in @SimonSapin's links, rather than prepublish.
Yes, I wanted to point to these threads to show other pain points around [replace]. I agree that [replace], not [prepublish], is probably the right thing for these particular scenarios.
(Though I also think that [prepublish] can’t become real soon enough ;))
This would be a nice feature to have. I've just come across a problem where changes to nightly have broken old versions of the cocoa crate, such as 0.3.3 and 0.5.2. These versions are required by the dependencies of my project, but I know from testing that they work with the latest version.
One solution would be to fork all the dependencies that I rely on that specify a cocoa version and then change their cargo.toml files, but a much better solution would be able to specify version upgrades like "cocoa:0.3.3" = "0.9", and then let me be responsible if something breaks.
Closing now that [patch] (formerly [prepublish]) exists: #4123