Spurious rebuilds of paths-overrides

[larsbergstrom]

I think I've noticed something related to Servo's "spurious rebuilds" issue! They seem to be triggered by Cargo deciding that one of the paths-overrided dependencies needs to be rebuilt. So, we see it a LOT when working on a rustup (or things like my android changes touching a lot of stuff), but still see it a bit on Servo itself because we have a paths override even in our default build (https://github.com/servo/servo/blob/master/components/servo/.cargo/config#L1 ).

Basically, without touching my machine I can just re-reun a build and it will seemingly randomly occasionally have a "good rebuild" log, but much more often have a "bad rebuild" log. The bad ones always start with things that are being overridden - in this case, I'm overriding skia, angle, gleam, rust-offscreen-rendering-context, and glutin.

Fingerprint log of a good rebuild:
https://drive.google.com/file/d/0B9-EBhw4XiWEZlVRZUpGcjFqYVU/view?usp=sharing

Fingerprint log of a bad rebuild:
https://drive.google.com/file/d/0B9-EBhw4XiWEWU1TRGFaZVJfR2s/view?usp=sharing

Hope this is enough info to debug it!

[larsbergstrom]

With some output from the newer cargo fingerprinting stuff:
https://pastebin.mozilla.org/8848748

[larsbergstrom]

[16:24:44]  <acrichto>  larsberg: hm ok, so this is kinda coming back to me
[16:24:54]  <acrichto>  so we don't record a lockfile for path overrides
[16:25:09]  <acrichto>  larsberg: and as a result when you override, if there are two deps in the graph which satisfy there override's dep it may switch between those two
[16:25:22]  <acrichto>  larsberg: e.g. due to the nondeterminism of HashMap or something along the way
[16:25:37]  <acrichto>  larsberg: e.g. in that one gleam switched khronos_api deps
[16:25:51]  <acrichto>  not entirely sure what's up with the others though
[16:30:28]  <@larsberg> acrichto: Makes sense, and explains the non-determinism. So it would go away if we really hard-enforce that there are never two copies of a dependency?
[16:30:43]  <@larsberg> acrichto: We were planning to do that anyway for Servo; this would just move up the priority a lot :-)
[16:31:35]  <acrichto>  larsberg: for the gleam rebuild I think that'd do the trick, but this is definitely a bug in cargo b/c there should be no reason to not have two copies of a dep
[16:31:55]  <acrichto>  larsberg: for the others where the fingerprint file just couldn't be found, I'm not sure what's up there
[16:32:09]  <acrichto>  the only explanation I have is that the package id for things like the layout crate changed entirely
[16:32:13]  <acrichto>  between rebuilds
[16:32:16]  <acrichto>  (which makes no sense...)
[16:32:24]  <@larsberg> acrichto: I assume for the others it's just because we're hitting the cargo OS path crash right now :-)
[16:32:38]  <acrichto>  larsberg: OS path crash?
[16:32:55]  <@larsberg> acrichto: The one you said you just fixed today? (OS paths too long error)
[16:33:09]  <acrichto>  larsberg: oh did those crates just not build at all?
[16:33:23]  <acrichto>  if that's the case then that's totally fine
[16:33:24]  <acrichto>  lol
[16:33:33]  <@larsberg> acrichto: That's my suspicion :-) Thanks for the debugging help!
[16:33:55]  <acrichto>  larsberg: note that if you wanna fix this *now* you could change the dep of gleam -> khronos_api to be more exact
[16:34:01]  <acrichto>  e.g. make it a semver dep instead of a * dep
[16:34:18]  <acrichto>  yu're guaranteed there's only ever one dep in a semver range, so that'd make the problem moot temporarily as well

[alexcrichton]

To recap, my belief is that this is the following bug:

• There is a dependency A which shows up twice in the resolution graph (e.g. two semver incompatible versions).
• The dependency B which is being overridden depends on A and the version dependency covers both of the versions of A in the graph.
• Each time B is overridden with the custom path, it nondeterministically selects either version of A, causing rebuilds whenever it selects a new version.

There's a few possible solutions to this that I can think of:

• Emit a lockfile for the resolved graph after overrides (not a great solution, not even sure if this is practical)
• Try to "always choose the biggest" version instead of picking either version (I'm not sure if this is 100% reliable either).
• Be more picky about path overrides in terms of their upstream dependencies (e.g. must perhaps match exactly what's already in the graph structure itself).

The question of overriding a dependency with a whole new set of transitive dependencies has always been a little sketchy to me, so it's unclear how robust this can be made in the long-term.

@larsbergstrom for now I'd recommend not using path overrides for large scale refactors but instead using normal git and path dependencies which should always be reliable.

[larsbergstrom]

I'm having a lot of success right now just removing the * dependencies and removing any duplicatly included dependencies (it's an error for Servo, anyway), which is an easy workaround we should have been doing anyway :-)

I'm not really sure what you mean by normal git and path dependencies here instead of paths. Do you mean editing the Cargo.toml that has a Cargo reference to instead have a pointer to a git repo? If so, I suspect that will have really huge dependency chains - e.g., that I'd have to clone and edit a slew of intermediate repos to get, say, everybody who references gleam pointing at the same thing. Sorry if I'm misunderstanding!

[alexcrichton]

Ah yeah I meant editing Cargo.toml to point to the new dependency (in a first class, not overridden fashion). But if that's too painful then ah well, worth a shot!

[cyplo]

Hi !
Is this still an issue, @larsbergstrom ?

[larsbergstrom]

@cyplo Yes, but we can now use [replace] to avoid many of the scenarios, so long as the replaced dependency has not moved to a newer Cargo version number.

[alexcrichton]

Another case #3136 has exposed that wasn't handled correctly before:

If you've got a paths override pointing at a crate that itself has path dependencies, those dependencies have historically been followed. For the path dependencies, though, there's no lock file entries to indicate what they should be locked to. This would introduce spurious recompiles as they wouldn't always re-select the right versions.