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

Depending on unpublished versions of dependencies #2375

Closed
SimonSapin opened this issue Feb 9, 2016 · 11 comments
Closed

Depending on unpublished versions of dependencies #2375

SimonSapin opened this issue Feb 9, 2016 · 11 comments

Comments

@SimonSapin
Copy link
Contributor

I’m currently working on a branch of rust-url that will eventually become version 1.0.0. It includes pervasive (breaking) API changes. I want to port some projects like Cargo and Hyper that depend on rust-url to this new unpublished rust-url version to see how the API works out in practice. It is important to do this before the new version is published, so that I can make further breaking changes.

So I’ll go in my clone of Cargo, add paths = ["../rust-url"] in .cargo/config, and start fixing build errors.

When it’s time to make a git commit, I’d like to change the dependency requirement in Cargo.toml as well: url = "1.0". This causes an error, even though the path override does have that version:

% cargo build     
    Updating registry `https://github.com/rust-lang/crates.io-index`
no matching package named `url` found (required by `cargo`)
location searched: registry https://github.com/rust-lang/crates.io-index
version required: ^1.0
versions found: 0.5.4, 0.5.3, 0.5.2, ...

So I have to revert that change and don’t forget to add it back when url 1.0 is finally published.

It would be nice if Cargo could deal with unpublished crates/versions locally. (If course when publishing, the crates.io server should still check that dependencies are published as well.)

@SimonSapin
Copy link
Contributor Author

And now that I actually try it I see that some of Cargo’s own dependencies (curl, git2, git2-curl) need changes for rust-url as well, so the overall path override setup and pull request coordination becomes more complex.

@alexcrichton
Copy link
Member

This is unfortunately something that architecturally path overrides just can't do. Path overrides are only applied after the lock file is generate, so the version requirements in Cargo.toml must still be valid (as a lock file is generated).

I think what this wants is something like #942 where the override is stored directly in the manifest so it alters creation of the initial lock file. As a result I'm gonna close in favor of that, but thanks for the report!

@SimonSapin
Copy link
Contributor Author

Looking at #942 and #2385:

  • 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.

This does not work for the use case described here.

The scenario that happens all the time at least for me is that I want to make changes in e.g. Servo, and this requires changes in e.g. html5ever. So I add (uncomment) an override for html5ever and ideally I’d bump version numbers (both in the html5ever’s [package] and in the Servo’s [dependencies]) at that time, so that my local branches have all they need to land.

I don’t want a long-lived override shared with other developers, I want Cargo to transiently pretend that this yet-unpublished version is already published and pretend that it’s already getting it from crates.io. That’s the situation we’ll be in by the time my Servo PR needs to pass CI.

With #2385 as far as I understand, I’d need to go back and undo the override once the new html5ever version is published.

@SimonSapin
Copy link
Contributor Author

Path overrides are only applied after the lock file is generate, so the version requirements in Cargo.toml must still be valid (as a lock file is generated).

This "must" is what I’m asking to relax. "Pretend crates.io has this version" in addition to "override where to find sources".

@alexcrichton
Copy link
Member

This just isn't how path overrides work, and it unfortunately just can't ever be how they work. The only real extra step in using #2385 is the final "remove the override from the top level", and is that really that onerous?

@SimonSapin
Copy link
Contributor Author

I’m not particularly attached to path overrides in their current form. I’m only saying that the common case of my workflow is somewhat painful, and changing Cargo could improve that.

Considering that:

  • Each repository typically has a different set of reviewers
  • Each person (reviewers and contributor / PR author) may live in a different time zone
  • It’s not rare to work across multiple repos two or three recursive dependencies deep (e.g. servo / html5ever / tendril, or servo / hyper / cookies / url)
  • Landing any PR is blocked on landing corresponding PRs on dependencies and publishing on crates.io

Reducing the number of synchronization points between people on the critical path to land everything is significant.

The ideal case would be that I open a bunch of related PRs at the same time, and if no code change are needed reviewers can land them without coordinating with me again, while I’m sleeping or working on something else.

This is one of the reason many people prefer a single monolithic repository. I want to believe in the benefits of Servo sharing many crates with the rest of the Rust ecosystem, but I think it’s important to make it less painful when we can.

@alexcrichton
Copy link
Member

I think I understand what you're saying, but I also subsequently don't understand why something like top level overrides wouldn't work. This seems to exactly address the case you're talking about?

If you're modifying a dependency, then you can simultaneously send a PR to the dependency as well as the downstream user with a temporary override to your fork. When the upstream PR is merged and the version is bumped you can remove the temporary override and correct it to whatever was published. Am I missing something?

@SimonSapin
Copy link
Contributor Author

At the moment I’m working on a branch of rust-url 1.0 (not published yet). I have forks of cookies, hyper, websockets, and Servo, each in a new branch and with path overrides for each other and url. I can not update [dependencies] sections yet.

When everything compiles and all tests pass in each 5 repository, I’ll submit respective PRs and reviews can be done in parallel. In the best case, none of the PRs need code changes before landing. (This is unlikely, but let’s assume it for the example.) Then, these things need do happen in order:

  • A rust-url reviewer lands my PR and publishes a new version on crates.io
  • I go back to my cookies branch, update [dependencies], and push to my PR.
  • Only then can a cookies maintainer land my PR and publish
  • I update my hyper PR
  • It can be landed and published
  • I update my websocket PR
  • It can be landed and published
  • I update my Servo PR
  • It can landed

Each step can not happen before the previous one, and I need to be involved in every other step. If I could prepare my PRs ready to land as-is, much less synchronization would be needed.

If I’m trading path overrides for new-style overrides, there is no improvement if I need to update each PR to remove overrides when dependencies are published. And despite you calling them "top level", as far as I understand I need one per repository (except url) in order to be able to run tests.

@SimonSapin SimonSapin changed the title Depending on unpublished versions of dependencies through path overrides Depending on unpublished versions of dependencies Feb 18, 2016
@larsbergstrom
Copy link

@SimonSapin It sounds like what @alexcrichton is describing would definitely meet your scenario. Instead of putting a bunch of paths overrides in .cargo/config, like you do today, which has the dependency problem, ONLY in Cargo.toml you'd put in your overrides.

The major downside I see to Alex's approach is I don't know if #2385 will also support local filesystem paths. If it doesn't, then every time you make an edit, you have to commit, push to a branch, and have Cargo pull that down from GitHub, which seems less than fun.

Additionally, support for local filesystem paths would also provide a really nice solution for vendoring, as in #1926.

@alexcrichton
Copy link
Member

@SimonSapin I believe @larsbergstrom is right in that the support in #2385 by adding this to Cargo.toml means that all PRs can land instantly. With Cargo.toml, if it builds for you then it'll be guaranteed to build for others, so as long as the PR is in some buildable state then it won't depend on any other changes landing.

Removing the overrides, of course, would require sequential updates, but that seems to be inherent to the problem.

@larsbergstrom as mentioned in #2385 the [replace] section has the same syntax as the [dependencies] section, so you can override with local filesystem checkouts by just pointing path dependencies somewhere else.

@SimonSapin
Copy link
Contributor Author

I’ll have to try #2385 after it lands in Cargo Nightly so I can see how it turns out in practice.

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

No branches or pull requests

3 participants