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

Implement top-level overrides #2385

Merged
merged 1 commit into from
Apr 7, 2016

Conversation

alexcrichton
Copy link
Member

This commit is an implementation of top-level overrides to be encoded into the
manifest itself directly. This style of override is distinct from the existing
paths support in .cargo/config in two important ways:

  • Top level overrides are intended to be checked in and shared amongst all
    developers of a project.
  • Top level overrides are reflected in Cargo.lock.

The second point is crucially important here as it will ensure that an override
on one machine behaves the same as an override on another machine. This solves
many long-standing problems with paths-based overrides which suffer from some
level of nondeterminism as they're not encoded.

From a syntactical point of view, an override looks like:

[replace]
"libc:0.2.0" = { git = 'https://github.com/my-username/libc';, branch = '0.2-fork' }

This declaration indicates that whenever resolution would otherwise encounter
the libc package version 0.2.0 from crates.io, it should instead replace it
with the custom git dependency on a specific branch.

The key "libc:0.2.0" here is actually a package id specification which will
allow selecting various components of a graph. For example the same named
package coming from two distinct locations can be selected against, as well as
multiple versions of one crate in a dependency graph. The replacement dependency
has the same syntax as the [dependencies] section of Cargo.toml.

One of the major uses of this syntax will be, for example, using a temporary
fork of a crate while the changes are pushed upstream to the original repo. This
will avoid the need to change the intermediate projects immediately, and over
time once fixes have landed upstream the [replace] section in a Cargo.toml
can be removed.

There are also two crucial restrictions on overrides.

  • A crate with the name foo can only get overridden with packages also of
    the name foo.
  • A crate can only get overridden with a crate of the exact same version.

A consequence of these restrictions is that crates.io cannot be used to replace
anything from crates.io. There's only one version of something on crates.io, so
there's nothing else to replace it with (name/version are a unique key).

Closes #942

@rust-highfive
Copy link

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

r? @wycats

@bors
Copy link
Contributor

bors commented Feb 16, 2016

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

@SimonSapin
Copy link
Contributor

[replace]
libc = { git = "https://github.com/alexcrichton/libc" }

Is it possible to specify a git branch or commit here?

@alexcrichton
Copy link
Member Author

@SimonSapin yes, as mentioned in the description the syntax in [replace] is the same as that of [dependencies]

@larsbergstrom
Copy link

After our conversation on #2375, it sounds like this is going to be fantastic for us! Thanks a ton, @alexcrichton. I think it's also going to make some of our really massive efforts (like impactful rust upgrades & adding new platforms) significantly easier to collaborate on across multiple people.

@codyps
Copy link
Contributor

codyps commented Feb 22, 2016

If at some point we wanted to allow additional constraints on the thing being replaced (specifying more exactly it's dependency location or version) what would that end up looking like? Is this scheme extensible for that purpose?

Is there a plan to allow the restricted replacement provided by [replace] be used in .cargo/config files for those people who have local changes for experimentation that aren't intended to be committed? Or are they expected to modify but avoid committing changes to cargo.toml?

@johntyree
Copy link

How does this compare to cabal's allow-newer option? Is something like that possible with this scheme, or is this just for forcing a particular single version of a package?

https://www.haskell.org/cabal/users-guide/installing-packages.html#miscellaneous-options

@alexcrichton
Copy link
Member Author

@jmesmon

The syntax in [replace] is the same as in [dependencies] so you can be as precise as you like in terms of (a) what you're replacing and (b) what you're replacing with. For example git repositories can specify exact SHAs and crates.io crates can specify exact versions if necessary.

Also yeah it seems like we may want to expand this to also be applicable in .cargo/config, although I'm somewhat hesitant to do so as it currently suffers from the fundamental bug of not generating a lock file. This causes spurious updates and rebuilds when using overrides from .cargo/config, and I'd want to fix that problem before introducing more functionality in that kind of configuration.

@johntyree

I believe this provides something very similar to that functionality. You'd basically be saying that "when you see this package with this version, instead you can use this newer version"

@johntyree
Copy link

@alexcrichton the way allow-newer works is basically to say "Use the dependencies you have, but ignore the upper bounds on them." It seems like this method is more like "replace whatever you have with exactly this."

It seems like it helps more with packages whose specifications are not updated quickly enough, rather than the case we're talking about here of simply wanting to force a particular version for development purposes.

@codyps
Copy link
Contributor

codyps commented Feb 22, 2016

@alexcrichton

My point about more specific overrides is that in contrast with [dependencies], [replace] affects many instances of a particular dependency in the tree of dependencies. The proposed method here takes an all-or-nothing approach, which does't immediately allow someone to, for example, only replace libc where libc matches a certain version requirement. Or matches some other requirement (dependency of a particular dependency, etc).

Essentially, how does consideration of #736 play with the proposed syntax here? Is it natural to extend it (at some point in the future, I'd rather like to see this PR merged sooner) to allow greater matching control, or is that something that would need to be added via a separate Cargo.toml structure/section?

@codyps
Copy link
Contributor

codyps commented Feb 22, 2016

@johntyree this also would be a package configuration option, not a user configuration option/flag (like allow-newer). Users would need to manually edit the existing Cargo.toml file. Not quite as convenient for that use case.

@alexcrichton
Copy link
Member Author

@johntyree

Ah yeah that's true that this doesn't support "use whatever was listed except the higher version". This sort of mechanism, however, is intended to be a transiently-committed artifact of Cargo.toml rather than a long term component, so perhaps it'd work out? I think that "replace with this dependency" is also more general because you can redirect to a git fork or a locally checked out copy.

@jmesmon

which does't immediately allow someone to, for example, only replace libc where libc matches a certain version requirement

Oh sorry, to clarify, the override can look like:

"libc:0.2.4" = "0.3"

This will only override libc 0.2.4 to the version 0.3. In general the key here is a package id specification, which can be used to select any node within a graph. In other words, this should handle #736 for overrides encoded into Cargo.toml.

Is that what you were thinking of?

@codyps
Copy link
Contributor

codyps commented Feb 23, 2016

@alexcrichton

Ah, that's very nice. For those wondering, the docs for package id specifications appears to be here: http://doc.crates.io/pkgid-spec.html

It appears to cover some of the use cases, but doesn't quite cover all of them: One can't specify that a CrateA depended on by CrateB should be replaced regardless of the version, but no other replacement of CrateA should occur.

In practice, the need to do this might not occur too often, but it isn't clear to me how easy add it after the fact would be. I suppose it could be possible to (later on) expand the package ID spec grammar if people end up needing it?

@alexcrichton
Copy link
Member Author

Yeah it's a good point that you can only target nodes in the dependency graph, not necessarily edges. We may be able to invent a syntax for that, but perhaps this would be good enough for now?

@codyps
Copy link
Contributor

codyps commented Feb 23, 2016

@alexcrichton yep, this seems like a good step in the right direction (and solves actual issues I have today). +1

@alexcrichton
Copy link
Member Author

I got a chance to discuss this with @wycats today and the conclusion was that he's a little hesitant to land this just yet.

@wycats can probably articulate better than I can, but the gist of it is that this feature has the ability to propagate the idea throughout the Rust ecosystem that versions don't matter so much. A failure mode of this feature is:

  • Core dependency X stops being maintained
  • Dependencies of X release new versions, need X to update as well
  • Instructions for using X now include "you must have [replace] in Cargo.toml" to work

This feature can also obviously create very confusing bug reports for crate authors. You could perhaps forget that a [replace] is in play locally, report a bug upstream about a compilation or runtime error, and the original author has no idea how to reproduce because they're not using the replaced dependency.

Finally, this can hide problems such as X not updating its dependency on Y for a subtle but intentional reason. Applications may then blindly replace Y with a more updated version and everything could appear OK until later when the "subtle bug" was encountered.


Overall, @wycats was going to disucss some more with others to get some more insight on this. We discussed how this solves many workflow issues with Cargo today (like cascading updates) and how we can possibly be vigilant to ward off the concerns mentioned above.

@alexcrichton
Copy link
Member Author

Oh and another point brought up is that this does not implement platform-specific replacements, only global replacements. One could imagine a package with an OSX-specific fix but you don't want to replace it on other platforms.

@SimonSapin
Copy link
Contributor

You could perhaps forget that a [replace] is in play locally, report a bug upstream about a compilation or runtime error, and the original author has no idea how to reproduce because they're not using the replaced dependency.

Forgetting you have overrides in place already happens with "old style" path overrides and causes momentary confusion, but one generally figures it out fairly quickly (before filing bugs upstream) since the error messages from rustc include source file names with paths to your git clones rather than the usual $CARGO_HOME/registry/src/github.com-88ac128001ac3a9a/...

I don’t think it’s been a major problem so far.

this does not implement platform-specific replacements

That could be done in the same style as #2328 : [target.'cfg(unix)'.replace]

@codyps
Copy link
Contributor

codyps commented Feb 26, 2016

Right now people who need to distribute code that requires something like [replace] are just going to use submodules & include a .cargo/config with path overrides in the project (even if we'd really like to not do that). If we don't have a "right" way to do this, I'd expect folks to continue to use the "wrong" way.

As for making it clear what is happening to the users building code: making sure the output is distinct (as it is when using cargo config path overrides) seems like a straight forward solution.

@larsbergstrom
Copy link

If the concern is that folks might not notice that the [replace] is still there, you could block publishing to crates.io if one is in place in the Cargo.toml file. This would more than likely prevent people from being able to publish a library with such an override and would make any of those scenarios ("you must have [replace] in Cargo.toml") mean that you either have a publishable Cargo.toml in your GH repo but it fails TravisCI or you can pass TravisCI builds but can't publish.

For toplevel apps, it seems like it wouldn't matter as much, if the concern is about libraries needing an override for some dependency of theirs. After all, toplevel apps probably have a Cargo.lock checked in, too, and that can have all sorts of wonky versions in there (see: Servo ).

@bors
Copy link
Contributor

bors commented Feb 29, 2016

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

@wycats
Copy link
Contributor

wycats commented Mar 11, 2016

I had a somewhat long chat with @alexcrichton about the tension here between maintaining a meaningful concept of versions (if it's easy to say "2.4 really means 2.5", then that becomes an acceptable solution to version mistakes rather than fixing them upstream) and the transient needs you have during upgrading (you really need "libc 0.3" to unify with "libc 1.0" because of transitive dependencies).

I want to think about it a little bit more and talk with @aturon (and others) about it, but I'll try to have an opinion I feel confident about by the end of next week.

It's definitely a tough problem; sorry this is taking a while to sort out.

@wycats
Copy link
Contributor

wycats commented Apr 5, 2016

I believe @wycats can articulate more, however.

I shall try 😄

The basic idea behind this feature, and the use-case it's most directly targeting is:

  • I have an app that has a Cargo.toml and a Cargo.lock.
  • For some (emergency, high-urgency) reason, I need to quickly apply a patch to some indirect dependency and redeploy my app. I need to do this quickly even though the underlying package has not yet published a new version to crates.io.
    • One of the most common cases of this: some work has happened on master of a package to fix a bug, but it hasn't yet been deployed, and I really need the fix.
  • I would like to make a change that has the lowest impact on the entire dependency graph, and get the fix out ASAP.

The intended workflow for this use-case is:

  1. If the patch you care about is already in the upstream github repository, and its tests pass, [replace] the crates.io package with a git crate, using the appropriate branch (master, incoming, and bugfix-1234 are all reasonably common).
  2. If the patch you care about has been submitted as a PR, but hasn't yet been merged, create your own fork of the repository, merge the patch, run the tests, and [replace] the creates.io package with your fork. (don't rely on the PR's fork/branch, because PR branches usually get deleted when they're merged).
  3. If the patch you care about hasn't been submitted yet, do the work yourself, [replace] the package in your Cargo.toml, and once you've confirmed it's working for you and you've deployed it, submit a PR to the upstream from your fork so that the next person with the problem can use option (1) or (2) instead of doing the work manually.

The fact that only a handful of people need to do (3) above is a feature of this workflow. It gives the community a way to rally around in-progress patches and use them as they land, rather than having everyone do the work themselves.

If the patch needs to be applied to a prior release, and master is ahead of the release, the same workflow above applies, but the person who writes the original patch bases it on the release they care about. It's a feature of this workflow that there is a lightweight way for the community to quickly create patched versions of any release tag, and for everyone on that release tag to share the work.

Also, applying the patch in this way does not introduce a full-on cascade; since the replaced package has the same name and version as the original release tag, so as far as the dependency resolver is concerned, nothing has changed.


What about version bumps?

The idea of the current approach is that version bumps are a special case of the "short-term patch" workflow.

In particular, if there are a number of packages in your dependency graph that depend on servo-url, and you want to upgrade to a new version of url, the way to accomplish that is to:

  1. Update master (or incoming) of the packages that depend on url to depend on the new version. Do not bump the version of these packages yet.
  2. Attempt to compile and run the tests for each package. If the package fails to compile or the tests fail, fix the code to be compatible with the new version of url.
  3. [replace] the crates.io (or git) versions of these packages with the updated branch.
  4. Run cargo build and confirm that everything compiles.
  5. You are now able to ship.
  6. Over time, ship those crates to crates.io, bumping the versions of those crates as you do.
  7. Remove the [replace] lines, and repeat the process for the next layer of dependencies.

Step (2) above is critical. While it may occasionally work to whole-hog replace a version of a dependency with another version throughout the dependency graph, it quite often will not. The step of confirming that direct dependents of the update works by attempting to compile the crate before moving on helps to identify the precise reason a particular update isn't working, and gives a clear workflow for fixing it.

Better yet, once that process is done, any other application that depends on that package can use the branch, knowing that the upgrade will work, and that any necessary fixes have already been incorporated.

This approach does not require a big-bang cascading update to the entire dependency graph; instead, it focuses the update effort where it belongs: to confirm that packages that claim to support url 0.5.7 actually do work with 0.5.8.


Our goal right now is to monotonically improve the experience of updating dependencies carefully but steadily.

As we learn more about the pain points that are still left once we ship the first set of features, we will continue to improve Cargo to directly support workflows that continue to have too much pain, but it's hard to speculate about precisely what those solutions should be when such a disproportionate amount of the pain is focused on things we know how to improve conservatively.

@johntyree
Copy link

since the replaced package has the same name and version as the original release tag, so as far as the dependency resolver is concerned, nothing has changed.

This is a misfeature in most organizations that care about versioning, I think. There is assumed to be a one-way mapping from versions to code.

What version of libfoo are you using?

0.2.3

Bob's 0.2.3?

No. Bob's patch broke this other thing. I'm using Jane's 0.2.3.

Remind me why we have version numbers again?

I don't know. They seem pretty arbitrary.


It gives the community a way to rally around in-progress patches...

To me it seems like you're approaching this backwards. Github has always provided a way to share patches before they hit upstream. What this does is prevent everything else.

I understand that you're trying to establish what you feel are best practices, I just don't agree that this is the right way to go about it. Rather than having the resolver choose a new, non-broken version of a package, it seems like you're proposing that we each locally rewrite history and pretend that the broken version was never released. How is that better from a complexity point of view?

@wycats
Copy link
Contributor

wycats commented Apr 6, 2016

Remind me why we have version numbers again?

I don't know. They seem pretty arbitrary.

It sounds like we have a strong agreement that we want to have, as a core constraint, that the community has a strong sense of what versions mean.

What alternative do you have in mind for replacements?

@wycats
Copy link
Contributor

wycats commented Apr 6, 2016

I wanted to elaborate a little more on something that was kind of implicit in my original comment.

While my comment explicitly talked about the workflows that I thought would work well with the current status of the design, I was implicitly talking about a possible enhancement: allowing [replace] section to change the version of any package in the dependency graph, not just the source.

I have some concerns about that direction (more in a sec), and I also feel that the more minimal, conservative version of this feature will improve the status quo and give us a clearer view of how to address the pain points that remain.

When I talk about 'the version-changing feature' below, I am talking about an enhancement to this PR that would also allow you to replace any pkgid with any other pkgid, regardless of version. As it stands, this PR allows you to replace a pkgid with another (libc 0.3 from crates.io with libc 0.3 from github), as long as it shares the same name and version.

The most important concern I have about the version-changing feature is similar to @johntyree's concern: I think it's important that we retain as much community consensus as possible about the meaning of versions.

Since applications almost always use dependencies that they don't manage directly, the need to upgrade non-atomically ("I need to upgrade libc because one of my deps requires it, but another one hasn't released an upgrade yet") will come up. To me, the question is just the cost-benefit to the community of the different approaches.

In my view, an approach that allows you to temporarily fix up some code without bumping the version deals less damage to the notion of versions that an approach that allows you to say, across your entire graph: "whenever you see libc 0.3, pretend you saw libc 1.0".

Mary:

I'm having a bug with rust-url, which I got from crates.io.

rust-url authors:

Hm that error looks really weird. We depend on libc 0.3 and char is unsigned. How can this be?

Mary, days later and after several back-and-forths:

Oh, I just noticed that we replaced libc 0.3 with libc 1.0 globally.

I think this story is worse than the equivalent story where the user explicitly replaced rust-url with a git version. While the same kind of confusion might occur (and probably will), the need to be explicit about the dependencies you're actually changing, as well as the additional work needed to confirm that the code actually compiles and its tests run, makes the situation strictly less bad, in my opinion.

Some lower-order concerns:

  1. The plan of record here is to replace nodes in the dependency graph that say libc 0.3 (registry) with libc 0.3 (github). Because of the fact crates on crates.io use names and version ranges to express dependencies, this replacement will have fewer unexpected effects on the process of dependency resolution. In contrast, replacing libc 0.3 (registry) with libc 1.0 (github) can (and will) have significant effects on the resolved dependencies. (Another way to say this is that the approach of record allows us to treat replaces as pure metadata, rather than have to update the resolution itself).
  2. While the version-changing solution might work in some cases, it will often fail for one of these reasons:
    1. One of the dependents of the replaced dependency depends on details of types that changed. This will result in a compile-time error. (this might be hard to track down when applied all at once globally, but at least it's a compile-time error).
    2. One or more of the dependents of the replaced dependency depends on behavioral details that changed that are not encoded in types. This would be hard to track down, since the first clue you have that something went wrong is a full-on integration test at the app level.
    3. One or more dependents of the replaced dependency depends on a subtle change in unsafe code (the size of unsafe types, or the meaning of them). These changes might not be caught until a user reports a crash.
    4. Generally, these problems could be mitigated by testing each dependent against the update, and making sure that the code compiles and the tests pass. It's still possible to do a blind pass, but the process of updating a dependent, sloppy or not, puts you in the right mindset about the impact of the change. In my experience, this process results in lower overall time spent, because the process of tracking down problems is incremental and clearly targeted ("I'm updating url, and it doesn't compile, so I need to fix this compile error in url"), which is less true about global changes.
  3. Changes at the app top-level that can be done without any changes to the code in dependencies seem more likely to overlook and accumulate over time, rather than folding them back into the upstream dependencies. They can also continue to apply to new versions of the underlying package with almost no work, which would further make them easy to persist. The github-based model has a natural endpoint when the patch is accepted upstream and published to crates.io, and a natural model for moving the process along (it's harder to let it fall through the cracks when there is a natural PR sitting around).
  4. In light of these concerns, I prefer to ship the current approach, which is a clear improvement over the status quo, and keep an eye on the pain-points that are not yet addressed. Once this feature ships, I believe we will be better able to focus on those pain-points, when they are brought into sharper relief.

I hope this helps clarify things more.

@johntyree
Copy link

I was writing something here, but you've hit all the points much more clearly than I was about to.

I'll just add that I think our disagreement comes from a difference in philosophy about what the tool should be responsible for doing. I think it should be for helping developers ship applications, rather than teaching them about community management or open source etiquette.

That said, you've clearly put a lot of thought into this and there's no reason it can't be revisited if what you're proposing turns out to be insufficient. I say go ahead and let's see how it does.

@codyps
Copy link
Contributor

codyps commented Apr 7, 2016

Trying to force people's work flows by adding an extra if not_something_i_personally_like() { forbid() } restriction isn't a good idea.

All the arguing about making it harder to break things ignores that we already have less controllable replacements via .cargo/config paths. replace (from my view point) simply allows me to have more control over how things are replaced (compared to paths). Ideally, I could use it to limit how much extra testing I have to do. But we're going to force more people to use paths instead? Isn't that more error prone?

Previous limitations that assumed semver were done on crates.io instead of in cargo directly. Why is it appropriate to put this restriction in cargo when it's arguments rely on semver?

@wycats
Copy link
Contributor

wycats commented Apr 7, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Apr 7, 2016

📌 Commit 54d738b has been approved by wycats

@bors
Copy link
Contributor

bors commented Apr 7, 2016

⌛ Testing commit 54d738b with merge 31214eb...

bors added a commit that referenced this pull request Apr 7, 2016
Implement top-level overrides

This commit is an implementation of top-level overrides to be encoded into the
manifest itself directly. This style of override is distinct from the existing
`paths` support in `.cargo/config` in two important ways:

* Top level overrides are intended to be checked in and shared amongst all
  developers of a project.
* Top level overrides are reflected in `Cargo.lock`.

The second point is crucially important here as it will ensure that an override
on one machine behaves the same as an override on another machine. This solves
many long-standing problems with `paths`-based overrides which suffer from some
level of nondeterminism as they're not encoded.

From a syntactical point of view, an override looks like:

```toml
[replace]
"libc:0.2.0" = { git = 'https://github.com/my-username/libc';, branch = '0.2-fork' }
```

This declaration indicates that whenever resolution would otherwise encounter
the `libc` package version 0.2.0 from crates.io, it should instead replace it
with the custom git dependency on a specific branch.

The key "libc:0.2.0" here is actually a package id specification which will
allow selecting various components of a graph. For example the same named
package coming from two distinct locations can be selected against, as well as
multiple versions of one crate in a dependency graph. The replacement dependency
has the same syntax as the `[dependencies]` section of Cargo.toml.

One of the major uses of this syntax will be, for example, using a temporary
fork of a crate while the changes are pushed upstream to the original repo. This
will avoid the need to change the intermediate projects immediately, and over
time once fixes have landed upstream the `[replace]` section in a `Cargo.toml`
can be removed.

There are also two crucial restrictions on overrides.

* A crate with the name `foo` can only get overridden with packages also of
  the name `foo`.
* A crate can only get overridden with a crate of the exact same version.

A consequence of these restrictions is that crates.io cannot be used to replace
anything from crates.io. There's only one version of something on crates.io, so
there's nothing else to replace it with (name/version are a unique key).

Closes #942
@bors
Copy link
Contributor

bors commented Apr 7, 2016

@bors bors merged commit 54d738b into rust-lang:master Apr 7, 2016
@alexcrichton alexcrichton deleted the top-level-overrides branch April 7, 2016 22:05
@alexcrichton
Copy link
Member Author

@jmesmon

Cargo is effectively a workflow tool, so we're not really forcing a workflow here but rather extending the already existing one?

You're also right that paths dependencies are relatively broken today, and we have plans to fix them but it will likely make them somewhat more restrictive as well (similar to what this PR is doing).

@codyps
Copy link
Contributor

codyps commented Apr 15, 2016

@alexcrichton

Cargo is effectively a workflow tool, so we're not really forcing a workflow here but rather extending the already existing one?

It isn't clear why you're defining cargo as a "workflow tool" here. I presume you're arguing that because it is a workflow tool, it's alright for it to restrict workflows for arbitrary reasons? I guess I don't agree with that.

Alternately, I guess you could be saying that because [replace] is new, you can put whatever restriction on it you want. I also disagree here. Saying "if you do what we want, you can use new feature X, if not you're stuck using less good old feature Y" isn't a very nice thing to do.

You're also right that paths dependencies are relatively broken today, and we have plans to fix them but it will likely make them somewhat more restrictive as well (similar to what this PR is doing).

Even if I keep entirely to myself (via local config files), cargo still believes it should be allowed to arbitrarily restrict my workflow as long as the stated goal is to improve collaboration? I guess I don't agree there either.

I suppose I'm operating under the impression that cargo should be a useful tool for everyone working with rust, not just those that fit with some folks idea of how things should work.
Restricting it to only some workflows to achieve social/community ends is a good way to frustrate people.

@alexcrichton
Copy link
Member Author

@jmesmon I was mostly just responding to your comment about "trying to force people's work flows" by pointing out that Cargo is itself a workflow tool.

In general, yes, you're indeed right! Cargo shouldn't be preventing use cases and should be a general tool for all. That doesn't mean, however, that literally every feature should be added to Cargo. For example to work around the restriction of the same version you just need to fork a repo and change the version backwards. The idea is that it's a step up from the normal workflow, not forbidden.

@SimonSapin
Copy link
Contributor

FWIW, I just pushed a branch called 1.0.0-pretending-to-be-0.5.9 to servo/rust-url doing exactly what it says, just to be able to use a [replace] override in servo/servo. This seems like unnecessary jumping-through-hoops.

@SimonSapin
Copy link
Contributor

And so that I’m not just complaining, I have to say that this feature allowed me to run Servo’s full test suite on CI servers for a large PR before all changes in dependencies were merged and published. This would have been a lot more painful to pull off before. Thanks a lot @alexcrichton !

@alexcrichton
Copy link
Member Author

This seems like unnecessary jumping-through-hoops.

It seems... wrong though to say that 1.0.0 can masquerade with 0.5.9? Every crate would essentially require source level changes to be compatible with 1.0.0 of rust-url, right? In the case that they're being changed already bumping the version dep seems reasonable?

This would have been a lot more painful to pull off before.

Awesome! Glad to hear it's working out :)

@lambda
Copy link

lambda commented Apr 22, 2016

It seems... wrong though to say that 1.0.0 can masquerade with 0.5.9? Every crate would essentially require source level changes to be compatible with 1.0.0 of rust-url, right? In the case that they're being changed already bumping the version dep seems reasonable?

I don't know about rust-url specifically, but for many packages bumping from a 0.x.y version to a 1.0.0 version doesn't necessarily mean a breaking change, but can simply mean that you have decided the existing API is expected to be stable for the forseeable future, and want to advertise that via the version number.

Also, even if breaking changes are introduced, it's possible for them not to break very much, and you to want to do this replacement in order to judge how much will be broken if they update to see how much work you're inflicting on your downstream, similar to a crater run. For instance, if you add one variant to an infrequently used enum that could be exhaustively matched, that's technically a breaking change, but if only one or two packages in the ecosystem ever actually match against that enum, you may have some application with a dependency graph in which you can substitute in the new version without breakage.

I think one of the reasons that this requirement for a strict version match seems wrong is that it makes it harder to identify the actual version of a package in question. As mentioned above, you could have multiple different packages called libfoo 0.2.3 floating around at any given time; while the unique hash can distinguish them if using git dependencies, that's pretty much an opaque blob that's hard for people to mentally compare, and can't be ordered without looking at the Git history. If I need to maintain a longer-term local fork for whatever reason, and so introduce multiple patches to libfoo 0.2.3, it would be hard to tell which of ssh://my-server/my-repo.git#abc123 and ssh://my-server/my-repo.git#456def is actually newer when looking at the Cargo.lock; and the Cargo.toml may only contain { git = "ssh://my-server/my-repo.git"; branch = "0.2.3-patches" }

I suppose that one workaround would be to establish a convention that you use the branch, or even better tag, in your [replace] specification to keep track of the local patch version, such as tag = "0.2.3-patch1 and so on. The question is, is this workaround preferable to just allowing you to replace libfoo 0.2.3 with a libfoo 0.2.3-patch1 version?

Hmm. After reading the semver spec more closely, it appears that semver doesn't support a 0.2.3-patch1 version properly, since that would be seen as being a prerelease of 0.2.3, not later than 0.2.3. It doesn't look like semver has any way of adding additional local patches that compare greater than the upstream version they patch. This is something that, coming from a Debian packaging background, I would expect; in Debian, 0.2.3p1 would compare greater than 0.2.3, but ~ can be used as a prerelease indicator that would compare lest, so 0.2.3~p1 would be considered a prerelease of 0.2.3. Semver only seems to support appending build metadata, which is ignored for comparison, and prerelease, which compares earlier, with no way to append something that compares greater without just incrementing the patch number (which could then conflict with a

So, maybe given that constraint that you can't really indicate properly in semver a version number for a local patch, that using a convention with git tags or branch names is the best you can do for keeping track of local patches to a particular upstream version.

@SimonSapin
Copy link
Contributor

SimonSapin commented Apr 22, 2016

It seems... wrong though to say that 1.0.0 can masquerade with 0.5.9?

It can’t really, but I didn’t manage to get things to build otherwise. Maybe related to #2603.

@codyps
Copy link
Contributor

codyps commented Apr 22, 2016

@alexcrichton

Every crate would essentially require source level changes to be compatible with 1.0.0 of rust-url

Even presuming semver for cases other than the '<1' to '>=1' bump, this isn't true. semver is a promise of compatibility based on version numbers, but is never a promise of total incompatibility.

Which is another reason, even if we presume everyone is following semver, that trying to restrict package combinations in something that is already an override of normal resolution is not a good idea. Essentially: cargo can't know better than it's users.

@alexcrichton
Copy link
Member Author

@lambda, @jmesmon

As @wycast has commented above (and expanded on) the use case of upgrades is not the sole purpose of this PR. This is also intended for things like hotfixes and bug fixes.

Also, as I explained earlier it definitely is indeed the case that major version upgrades do not cause breakage for all packages, we are assuming that this is not the majority of use cases.

@traoremp
Copy link

@alexcrichton Does this solve the case in which you'd like to override crate-type .. i.e. you'd like to build a shared library or dylib from the dependency as mentionned in: #1826 (comment)

@SimonSapin
Copy link
Contributor

@traoremp For that use case, would it work to have a two-line wrapper crate like this?

extern crate foo;
pub use foo::*;

Then you could pick whatever crate-type in its Cargo.toml.

@traoremp
Copy link

traoremp commented Nov 23, 2017

@SimonSapin Oh I just understood what you meant. Ok but why not just give the ability to do that as a dependency field that would specify how to beuild the dependency ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow top-level overrides to be stored in the manifest