1361005 - `mach vendor rust` failed (fails to update all dependencies)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P3)

Description

[:glob ✱]

`mach vendor rust` failed:

$ ./mach vendor rust
0:01.18 rm -rf /home/servo-sync/firefox-overlay/third_party/rust
 0:01.18 /home/servo-sync/.cargo/bin/cargo update --manifest-path /home/servo-sync/firefox-overlay/toolkit/library/rust/Cargo.toml -p gkrust
Error running mach:
.. 
The details of the failure are as follows:
Exception: Process executed with non-0 exit code 101: [u'/home/servo-sync/.cargo/bin/cargo', u'update', u'--manifest-path', u'/home/servo-sync/firefox-overlay/toolkit/library/rust/Cargo.toml', u'-p', u'gkrust']

$ cargo update --manifest-path /home/servo-sync/firefox-overlay/toolkit/library/rust/Cargo.toml -p gkrust
    Updating registry `https://github.com/rust-lang/crates.io-index`
error: failed to select a version for `smallvec` (required by `style`):
all possible versions conflict with previously selected versions of `smallvec`
  version 0.3.2 in use by smallvec v0.3.2
  possible versions to select: 0.3.3

emilio fixed this manually:

8:51 AM <emilio> heycam: if you're curious, had to pass -p smallvec before running the command with -p gkrust



i'm don't know went wrong here; if it's a mach, cargo, or some other issue, but this failure is unexpected.

Comment 1

[Cameron McCormack (:heycam)]

This is the commit that didn't get handled:

https://github.com/servo/servo/commit/0c5681140a5b44ae5481faf2d7353c4e7a272d50

Comment 2

[(not currently active) Ted Mielczarek]

It's certainly possible that this is a bug in the way we invoke cargo, but I don't know for sure.

Comment 3

[Alex Crichton [:acrichto]]

This looks like a legitimate error I think, but I'm curious how you also got into such a situation. Assuming that the Cargo.lock is always in sync with the Cargo.toml files in the repository, this error shouldn't come up. If, however, the Cargo.lock is out of sync (e.g. you just updated one of the Cargo.toml) files then it's possible for this error to arise.

Are you sure that Cargo.lock is in sync with all the Cargo.tomls when `./mach vendor rust` is executed? You can test that out by just running a normal build, and every time Cargo does a build it'll keep the Cargo.lock in sync (if possible)

Comment 4

[:glob ✱]

(In reply to Alex Crichton [:acrichto] from comment #3)
> Are you sure that Cargo.lock is in sync with all the Cargo.tomls when
> `./mach vendor rust` is executed? You can test that out by just running a
> normal build, and every time Cargo does a build it'll keep the Cargo.lock in
> sync (if possible)

this part of automation isn't capable of building; it just pulls in changes from github/servo and runs `mach vendor rust` after every change prior to pushing the changes to integration-autoland.

Comment 5

[Alex Crichton [:acrichto]]

Ah ok, then in that case it sounds like this is an expected error. The `cargo update` command above only allows changes to the `gkrust` package, nothing else. When pulling in this Servo update, however, it looked like it was required that the `smallvec` crate *also* change (needs to get upgraded from 0.3.2 to 0.3.3). In that case Cargo wasn't allowed to update smallvec when it needed to, so an error was generated.

Comment 6

[:glob ✱]

and again:

$ cargo update --manifest-path toolkit/library/rust/Cargo.toml -p gkrust
    Updating registry `https://github.com/rust-lang/crates.io-index`
error: failed to select a version for `cssparser` (required by `style`):
all possible versions conflict with previously selected versions of `cssparser`
  version 0.13.3 in use by cssparser v0.13.3
  possible versions to select: 0.13.5

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Alex Crichton [:acrichto] from bug 1369247 comment 4)
> This can typically happen when Cargo.lock/Cargo.toml are out of sync. In
> that situation a `cargo update -p gkrust` isn't guaranteed to succeed and
> the dependency graph may need a bit of massaging to get back into a
> resolvable state. This may also be a bug in `cargo update` depending on how
> you look at it, though.

So the question would be, in what condition would that happen?

I've seen both successful and failed revendoring in the past, and from that it is not clear to me why some work but others don't.

For example, some changes with successful revendoring (and you can see the revendoring commit from the "child" link):
* https://hg.mozilla.org/mozilla-central/rev/475503cada7c
* https://hg.mozilla.org/mozilla-central/rev/80aba64b0c6b
* https://hg.mozilla.org/mozilla-central/rev/f30e76c54839

and some changes failed to get revendoring automatically:
* https://hg.mozilla.org/mozilla-central/rev/a91c94c93cfc (manually revendored in https://hg.mozilla.org/mozilla-central/rev/51736db67723)
* https://hg.mozilla.org/mozilla-central/rev/2e058bcd4bce (manually revendored in https://hg.mozilla.org/mozilla-central/rev/e529bfc7e59c)

Comment 9

[Alex Crichton [:acrichto]]

Sure yeah, I'll try to help explain. Let's take a look at https://hg.mozilla.org/mozilla-central/rev/a91c94c93cfc more closely.

The Servo change brought an upgraded version on the dependency of `app_units`. This was previously locked to 0.4.0 but new crates were added which require '>= 0.4.1, < 0.5.0'. The Cargo.lock in Servo changed to refelect this, but the other Cargo.lock files in the repo were not.

So as part of the autovendoring I think the following command is running: (please correct me if I'm wrong though)

    cargo update --manifest-path toolkit/library/rust/Cargo.toml -p gkrust

The Cargo.lock for this project at toolkit/library/rust/Cargo.lock previously locked app_units to 0.4.0. This means that when you run `cargo update` you're only allowing Cargo to update the `gkrust` dependency (the `-p` flag passed). This, however, fails because Cargo thinks it's not allowed to update the `app_units` dependency. This then in turn creates an unresolvable resolution graph. Presumably gkrust transitively depends on some Servo stuff eventually, but this now requires 0.4.1 instead of 0.4.0. As 0.4.0 is locked down Cargo won't hand out 0.4.1, which causes this error.

Does that make sense for at least why it's happening? The three successful commits that auto-revendored look different in the sense that they either:

* Just added a new dependency. In this case Cargo can choose freely for the new dep, including an existing locked version, so there's no conflict regenerating a lock file on `cargo update -p gkrust`
* Changing major versions of an existing dep, similar to what happened when adding a new dep.

Basically the successful cases never raised the minimum version requirement *without* changing the major version. In other words, if a version requirement is bumped from 0.4.0 to 0.4.1, it can cause the breakage you're seeing. If the requirement is bumped from 0.4.0 to 0.5.0, then you won't see breakage.

So given all that I think Cargo can do better here. It should know what `cargo update -p gkrust` can still update `app_units`. It's still a conservative update and you shouldn't have to manually pass in `-p app_units` as well. I haven't though too much about this though, and may take some time to implement.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

So it sounds like a non-breaking version bump would break the command, while a breaking version bump would be fine. That, to be honest, sounds pretty weird.

If Cargo can do better here, that would be great. I'm not sure what can we do in the mean time... Maybe parsing the cargo output and figure out the package to update? That sounds rather fragile...

Comment 11

[Alex Crichton [:acrichto]]

I've filed an upstream issue for this: https://github.com/rust-lang/cargo/issues/4127

Comment 12

[Chris Peterson [:cpeterson]]

This Cargo vendoring bug does not need to block building Stylo support in Firefox.

Comment 13

[:glob ✱]

We no longer vendor servo, and the upstream cargo issue was resolved years ago.