gtk-rs / glib

DEPRECATED, use https://github.com/gtk-rs/gtk-rs-core repository instead!
http://gtk-rs.org/
MIT License
93 stars 62 forks source link

Proposal: support for logging through the log crate #694

Closed xanathar closed 4 years ago

xanathar commented 4 years ago

This PR is (as now) more meant as a base for discussion rather than something meant to be merged (though, in case, it's tested and somewhat ready).

This is a follow-up to this issue: https://github.com/gtk-rs/glib/issues/658

The changes contain:

The two parts above are "activated" by two different features in Cargo.toml: bridged_logging and bridged_logging_domain_macros.

I understand these are probably a bit "opinionated" in their implementation, so feedback is welcome; probably the most controversial part could be the overridden macros: if those are an obstacle they can be removed and only the adapter kept.

I think a sizable part of people interfacing glib code with Rust code using gtk-rs will want to have the two logging systems cooperating; currently this is done by custom code or external third party crates, so this might be helpful.

Example code using this, would be:

static glib_logger: glib::log_bridge::GlibLogger = glib::log_bridge::GlibLogger {
    format: glib::log_bridge::GlibLoggerFormat::Structured,
    domain: glib::log_bridge::GlibLoggerDomain::CrateTarget,
};

fn main() {
    log::set_logger(&glib_logger);
    log::set_max_level(log::LevelFilter::Debug);

    info!("This line will get logged by glib");

    error!("This is a Rust error");

    info!("This is from Rust %s and doesn't crash");

    g_info!("domain", "This is using glib logging");
}
sdroege commented 4 years ago
* a `GlibLogger` struct implementing the `log::Log` trait, so that code using the [log crate](https://crates.io/crates/log) will log to glib automatically (with customizable behavior)

I like this in principle, will review tomorrow :) I wanted to have this in glib since a while.

* a series of optional macros which override the macros defined in the [log crate](https://crates.io/crates/log), which allow to specify the logging domain in a `G_LOG_DOMAIN` static/const

This is confusing me a bit. How does that differ from g_info! etc?

xanathar commented 4 years ago

This is confusing me a bit. How does that differ from g_info! etc?

Two ways mainly:

As I said, those are the most opinionated part.

sdroege commented 4 years ago

Looks good to me generally

xanathar commented 4 years ago

Sorry I mistakenly closed the PR. Undid the action, but sorry for the noise.

xanathar commented 4 years ago

CI failures seem to be related to formatting. I will fix them by today, along with a couple of docs comments which are not accurate after the latest changes, and I will undraft this.

Open questions:

sdroege commented 4 years ago

What do we want to do with GlibLoggerDomain::Custom(&'static str)

I don't see its point personally, but maybe someone else does? :)

What to do with the override macros (is a separate commit enough)

I like them and just want a second opinion.

GuillaumeGomez commented 4 years ago

I like the macros too. :)

sdroege commented 4 years ago

It looks like the macros disappeared from the changes somehow?

sdroege commented 4 years ago

Looks good to me otherwise

xanathar commented 4 years ago

It looks like the macros disappeared from the changes somehow?

You told me to do a second separate commit... so I removed them and adding them later in a new commit. Have I misunderstood what you wanted ?

sdroege commented 4 years ago

You told me to do a second separate commit... so I removed them and adding them later in a new commit. Have I misunderstood what you wanted ?

A separate commit in this PR. It's a separate functional change, so should be a separate commit but it can be part of the same PR as it's related.

Personally I would also clean up the commit history of this PR so it doesn't contain all the back and forth but just a clean history but I guess we can forget that for the gtk repository anyway :)

xanathar commented 4 years ago

Personally I would also clean up the commit history of this PR so it doesn't contain all the back and forth but just a clean history but I guess we can forget that for the gtk repository anyway :)

My github-fu is a bit lacking, but I might have done it without accidentally closing the PR like I did last time 😅

sdroege commented 4 years ago

My github-fu is a bit lacking, but I might have done it without accidentally closing the PR like I did last time sweat_smile

Looks good to me, thanks a lot :) git is definitely not easy

xanathar commented 4 years ago

Thanks to you :)

xanathar commented 4 years ago

Changed the code with all the suggestions collected so far.

xanathar commented 4 years ago

I've clarified the usage in documentation, fixed the code examples and added the new features to Travis' CI (hoping it gets green :) ). This should address all feedback received so far.

sdroege commented 4 years ago

CI seems happy :) Thanks!