1321035 - Single Cargo.lock file doesn't play well with features and vendoring

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

This bug is a split off from bug 1319968.

The stylo repository (https://hg.mozilla.org/incubator/stylo) currently has a patch to a Cargo.toml file:

diff --git a/toolkit/library/rust/shared/Cargo.toml b/toolkit/library/rust/shared/Cargo.toml
--- a/toolkit/library/rust/shared/Cargo.toml
+++ b/toolkit/library/rust/shared/Cargo.toml
@@ -6,6 +6,7 @@ license = "MPL-2.0"
 description = "Shared Rust code for libxul"

 [dependencies]
+geckoservo = { path = "../../../../servo/ports/geckolib" }
 mp4parse_capi = { path = "../../../../media/libstagefright/binding/mp4parse_capi" }
 nsstring = { path = "../../../../xpcom/rust/nsstring" }
 rust_url_capi = { path = "../../../../netwerk/base/rust-url-capi" }

The inclusion of geckoservo is unconditional. This means that Firefox build configurations not requiring Servo bits are pulling in geckoservo/geckolib, which is wrong. This means that Firefox wastes a lot of CPU time building Rust bits it doesn't need.

I tried to fix this with the following patch:

diff --git a/toolkit/library/rust/shared/Cargo.toml b/toolkit/library/rust/shared/Cargo.toml
--- a/toolkit/library/rust/shared/Cargo.toml
+++ b/toolkit/library/rust/shared/Cargo.toml
@@ -5,8 +5,11 @@ authors = ["[e-mail]"]
 license = "MPL-2.0"
 description = "Shared Rust code for libxul"

+[features]
+servo = ["geckoservo"]
+
 [dependencies]
-geckoservo = { path = "../../../../servo/ports/geckolib" }
+geckoservo = { path = "../../../../servo/ports/geckolib", optional = true }
 mp4parse_capi = { path = "../../../../media/libstagefright/binding/mp4parse_capi" }
 nsstring = { path = "../../../../xpcom/rust/nsstring" }
 rust_url_capi = { path = "../../../../netwerk/base/rust-url-capi" }

I also changed the Firefox build system to conditionally pass `--features servo` to cargo depending on whether we wanted to build servo.

This errored because Cargo wanted to update Cargo.lock, which it couldn't do because we pass --frozen into Cargo.

Furthermore, if you run `mach vendor rust` or `cargo update` manually, it appears to delete the optional direct and transitive dependencies pulled in from geckoservo/geckolib both from the filesystem and from Cargo.lock.

I'm not an expert on Cargo, but it appears Cargo is insisting there is 1 and only 1 "active configuration" and it will aggressively change the world to be in conformance with that. The reality of mozilla-central is that there are N configurations. Somehow we need to teach Cargo to be aware of these multiple combinations. How we do that, I have no clue.

Manish thinks this is a Cargo issue, so needinfo acrichto to triage.

This is likely a blocker to vendoring servo in mozilla-central, as we don't want servo-less builds building servo bits they won't use.

Comment 1

[Alex Crichton [:acrichto]]

Hm so this definitely sounds like a bug in Cargo as-is. No matter what you pass on the command line (--features and such) Cargo should always generate the same lock file to solve exactly this sort of situation.

Is there a source tree I could poke around and reproduce these sorts of issues? (sorry if it's obvious from the details here, I'm very much a newb in working with Gecko...)

Comment 2

[Gregory Szorc [:gps]]

  $ hg clone https://hg.mozilla.org/incubator/stylo

Or if you are in the office, the following will complete significantly faster:

  $ hg clone --uncompressed https://hg.mozilla.org/incubator/stylo

Comment 3

[Gregory Szorc [:gps]]

Manish says https://github.com/Manishearth/gecko-dev/commit/b49be20a4781b6d0cd4cffa9896db1620b7c3d6c may improve things.

Comment 4

[Alex Crichton [:acrichto]]

Aha Manish helped me out a bit offline.

So what's happening here is that the dependency (toolkit/library/rust/shared) is being modified to make a dependency optional but the main project (toolkit/library/rust) wasn't updated as well. The lock file represents the main project, which doesn't enable the optional dependency, so the lockfile wouldn't include all those deps.

But yeah what Manish's patch does is to ensure that the main project also has a feature which *could* enable the optional dependency, so everything is included in Cargo.lock, so that should work!

Comment 5

[Gregory Szorc [:gps]]

Manish's patch makes non-stylo builds work. `mach vendor cargo` also no longer exhibits its behavior of deleting tons of dependencies. My --enable-stylo build is in progress. But 2/3 is encouraging!

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I guess this is WFM?