Closed Bug 1444097 Opened 3 years ago Closed 3 years ago

Clean up env_logger dependencies

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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: 3 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.
Depends on: 1445225
You need to log in before you can comment on or make changes to this bug.