Closed
Bug 1444097
Opened 7 years ago
Closed 7 years ago
Clean up env_logger dependencies
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
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.
Comment 1•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Assignee | ||
Comment 5•7 years ago
|
||
Submitted https://github.com/servo/servo/pull/20245 for a related change on the Servo side.
Comment 6•7 years ago
|
||
This is awesome, thanks Matt!
Comment 7•7 years ago
|
||
mozreview-review |
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
Comment 9•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee | ||
Comment 10•7 years ago
|
||
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...
Comment 11•7 years ago
|
||
(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.
Comment 12•7 years ago
|
||
(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.
Description
•