Clean up env_logger dependencies

RESOLVED FIXED in Firefox 60

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

The u2fhid crate has an unnecessary env_logger dependency, and the js and bindgen crates enable env_logger's "regex" feature which we want to disable to reduce code size.
(In reply to Matt Brubeck (:mbrubeck) from comment #0)
> The u2fhid crate has an unnecessary env_logger dependency, and the js and
> bindgen crates enable env_logger's "regex" feature which we want to disable
> to reduce code size.

Does the bindgen dependency actually end up in binaries? That'd be weird.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)

> Does the bindgen dependency actually end up in binaries? That'd be weird.

Bindgen doesn't, but it causes the binary to contain "env_logger with the "regex" feature.

This is because of https://github.com/rust-lang/cargo/issues/4866.  Cargo unifies features from build- and non-build-dependencies, compiles each crate once with the union of features, and links the result into both build scripts and final output.  So if we have both build-dependencies and runtime dependencies on "env_logger", we can end shipping extra features enabled only by the build-dependencies.
Submitted https://github.com/servo/servo/pull/20245 for a related change on the Servo side.
This is awesome, thanks Matt!
Comment on attachment 8957175 [details]
Bug 1444097 - Clean up env_logger dependencies.

https://reviewboard.mozilla.org/r/226120/#review232062

That's annoying. I understand why cargo does this, but it's annoying. I guess the alternative would be building multiple copies of a crate, but that'd only work if they're split between build/dev-dependencies and target dependencies.
Attachment #8957175 - Flags: review?(ted) → review+
Pushed by mbrubeck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d62a6ff5d9a
Clean up env_logger dependencies. r=ted
https://hg.mozilla.org/mozilla-central/rev/8d62a6ff5d9a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
These patches plus bug 1443257 reduced the installer size by about 100 KB:

https://treeherder.mozilla.org/perf.html#/graphs?series=autoland,1545789,1,2&zoom=1520502729402.5356,1520607958000,34071840.83853195,34274825.91315881&selected=autoland,1545789,314335,426699242,2

(Android shown as an example, but the size reduction was about the same on all platforms.)

This fixes a regression from the regex crate sneaking back into our binaries. We need better tooling or processes for noticing and preventing these regressions...
(In reply to Matt Brubeck (:mbrubeck) from comment #10)
> These patches plus bug 1443257 reduced the installer size by about 100 KB:

That's amazing - thanks Matt!

> 
> https://treeherder.mozilla.org/perf.html#/graphs?series=autoland,1545789,1,
> 2&zoom=1520502729402.5356,1520607958000,34071840.83853195,34274825.
> 91315881&selected=autoland,1545789,314335,426699242,2
> 
> (Android shown as an example, but the size reduction was about the same on
> all platforms.)
> 
> This fixes a regression from the regex crate sneaking back into our
> binaries. We need better tooling or processes for noticing and preventing
> these regressions...

We've recently added perfherder alerts on installer size at the 100k threshold, so hopefully we'd catch it if it happened again.
(In reply to Matt Brubeck (:mbrubeck) from comment #10)
> This fixes a regression from the regex crate sneaking back into our
> binaries. We need better tooling or processes for noticing and preventing
> these regressions...

It would be nice if we had a straightforward way to ask cargo "what's the full set of crates that will be linked into this top-level crate?" Then we could track that list, or even have lints to prohibit things like regex.

Alternately, we could add a metric that tracks the size of the gkrust static library, although we'd need to do some fiddling to ignore the size of debug info to get sensible numbers.
You need to log in before you can comment on or make changes to this bug.