Closed
Bug 1436830
Opened 6 years ago
Closed 6 years ago
Remove slog dependency
Categories
(Testing :: geckodriver, enhancement, P1)
Tracking
(firefox60 fixed)
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Rust beta is about to become stable. The work to upgrade Rust crates discovered a problem with slog tracked in [1]. slog is an unnecessarily complicated dependency to geckodriver which we only use because we need the ability to change log level at runtime. Instead of upgrading testing/geckodriver/src/logging.rs to the new slog syntax, we can roll our own implementation of log::Log.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Severity: normal → blocker
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•6 years ago
|
||
Comment on attachment 8949541 [details] Bug 1436830 - Roll own log::Log implementation in geckodriver. Here’s a naïve log::Log implementation using for the log crate [1] that we previously used slog_stdlog [2] for. It uses a std::sync::Arc to keep a thread-safe pointer to the heap for storing logging::LogLevel. Because log::set_boxed_logger() only takes a Box, I have had to share the Arc<logging::LogLevel> between a public-facing logging::Logger that is returned from logging::init() and an internal Box<InnerLogger>. This is the first time I use synchronisation primitives in Rust, so I’m sure there is a better way. The implementation seems to work and removes over 12.5k lines of code from third_party. Better still, it eliminates four slog dependencies from geckodriver making the build vastly quicker. In terms of verbosity of implementation in geckodriver, it is about the same number of lines, but I would humbly say it is a lot easier to understand. What do you think? [1] https://crates.io/crates/log [2] https://crates.io/crates/slog-stdlog
Attachment #8949541 -
Flags: feedback?(james)
Attachment #8949541 -
Flags: feedback?(giles)
Assignee | ||
Comment 6•6 years ago
|
||
Comment on attachment 8949541 [details] Bug 1436830 - Roll own log::Log implementation in geckodriver. Speaking to jgraham we seem to have found a way to make this work without std::sync::Arc, as the log crate these days exposes a log::set_max_level() function that appears to work. I’m reworking the patch.
Attachment #8949541 -
Flags: feedback?(james)
Attachment #8949541 -
Flags: feedback?(giles)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8949541 [details] Bug 1436830 - Roll own log::Log implementation in geckodriver. https://reviewboard.mozilla.org/r/218890/#review225696 It's too bad you need all this boilerplate just to add two new logging levels, but I don't see an obvious way to make it simpler without reducing scope. Thanks for writing tests! Fine to land. ::: testing/geckodriver/src/logging.rs:1 (Diff revision 3) > +//! Gecko-esque logger frontend for the [`log`] crate. s/frontend/implementation/ to match the terminology in the `log` crate. It's not clear what's a frontend and what's a backend here. :)
Attachment #8949541 -
Flags: review?(giles) → review+
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8949542 [details] Bug 1436830 - Drop slog dependency. https://reviewboard.mozilla.org/r/218892/#review225746 I wish updating dependencies was easier. It would be good to file issues (or patches, it should straightforward) to update other `log` crate users to 0.4 so we can drop the 0.3.8 variant.
Attachment #8949542 -
Flags: review?(giles) → review+
Assignee | ||
Comment 11•6 years ago
|
||
(In reply to Ralph Giles (:rillian) | needinfo me from comment #10) > Comment on attachment 8949542 [details] > Bug 1436830 - Drop slog dependency. > > https://reviewboard.mozilla.org/r/218892/#review225746 > > I wish updating dependencies was easier. It would be good to file issues (or > patches, it should straightforward) to update other `log` crate users to 0.4 > so we can drop the 0.3.8 variant. I’ve already taken care of that in https://bugzilla.mozilla.org/show_bug.cgi?id=1437570. (-:
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8949541 [details] Bug 1436830 - Roll own log::Log implementation in geckodriver. https://reviewboard.mozilla.org/r/218890/#review225974 This seems fine. Added a few nits that should be addressed. I guess the only advantage of the previous design would be if we wanted to support multiple firefoxes hanging off a single geckodriver, but that doesn't seem like a priority (and would require many other changes). ::: testing/geckodriver/src/logging.rs:10 (Diff revision 3) > +//! to provide a log implementation that shares many aesthetical traits with > +//! [Log.jsm] from Gecko. > +//! > +//! Using the [`error!`], [`warn!`], [`info!`], [`debug!`], and > +//! [`trace!`] macros from `log` will output a timestamp field, followed by the > +//! log level, and then the message. The fields are separated by a tabulator I think literally no one else ever uses the term "tabulator". The technical name is "CHARACTER TABULATION; horizonatal tabulattion (HT), tab", so let's go with "tab" in the comment. ::: testing/geckodriver/src/logging.rs:17 (Diff revision 3) > +//! `awk(1)`. > +//! > +//! This module shares the same API as `log`, except it provides additional > +//! entry functions [`init`] and [`init_with_level`] and additional log levels > +//! `Level::Fatal` and `Level::Config`. Converting these into the > +//! [`log::Level`] is reductionistic (lossy) so that `Level::Fatal` becomes s/reductionistic (lossy)/lossy/ ::: testing/geckodriver/src/logging.rs:173 (Diff revision 3) > - Ok(()) > + Ok(()) > - } > +} > + > +/// Returns the current maximum log level. > +pub fn max_level() -> Level { > + Level::from(MAX_LOG_LEVEL.load(Ordering::Relaxed)) I think this can just be written ``` MAX_LOG_LEVEL.load(Ordering::Relaxed).into() ```
Attachment #8949541 -
Flags: review?(james) → review+
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8949542 [details] Bug 1436830 - Drop slog dependency. https://reviewboard.mozilla.org/r/218892/#review225986 \o/
Attachment #8949542 -
Flags: review?(james) → review+
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8949541 [details] Bug 1436830 - Roll own log::Log implementation in geckodriver. https://reviewboard.mozilla.org/r/218890/#review225974 Good point. I think the short-term benefit is the heavier argument here. We would need to do substantive work to make slog also work with multiple Firefoxen.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•6 years ago
|
||
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c521b8c4a28 Roll own log::Log implementation in geckodriver. r=rillian,jgraham https://hg.mozilla.org/integration/mozilla-inbound/rev/532bca04e409 Drop slog dependency. r=rillian,jgraham
Comment 18•6 years ago
|
||
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cbd3741a4bb0 Roll own log::Log implementation in geckodriver. r=jgraham,rillian https://hg.mozilla.org/integration/autoland/rev/7270c9ee964b Drop slog dependency. r=jgraham,rillian
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cbd3741a4bb0 https://hg.mozilla.org/mozilla-central/rev/7270c9ee964b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c521b8c4a28 https://hg.mozilla.org/mozilla-central/rev/532bca04e409
You need to log in
before you can comment on or make changes to this bug.
Description
•