Skip to content
This repository was archived by the owner on Jun 8, 2021. It is now read-only.

Proposal: support for logging through the log crate #694

Merged
merged 2 commits into from
Sep 21, 2020
Merged

Proposal: support for logging through the log crate #694

merged 2 commits into from
Sep 21, 2020

Conversation

xanathar
Copy link
Contributor

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: #658

The changes contain:

  • a GlibLogger struct implementing the log::Log trait, so that code using the log crate will log to glib automatically (with customizable behavior)
  • a series of optional macros which override the macros defined in the log crate, which allow to specify the logging domain in a G_LOG_DOMAIN static/const

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
Copy link
Member

sdroege commented Sep 15, 2020

* 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
Copy link
Contributor Author

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

Two ways mainly:

  • they are named like the log crate macros (so they are quite a "plug&play" replacement, changing the macro_use in lib.rs/main.rs is usually enough)
  • they default to use a variable named G_LOG_DOMAIN (which must be in scope) if no context is provided, thus sort of emulating what G_LOG_DOMAIN does in C.

As I said, those are the most opinionated part.

/// Logs will use the specified domain for logging. Note that this option
/// due to the way the `log` crate is implemented, is global for all crates
/// in the current process.
Custom(&'static str),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence here.
On one side I assume there will be somebody who wants to add a Rust library to an existing glib based project and is happy to specify a single domain for all Rust logging; on the other they can get a better more future-proof result with a little more effort specifying a domain everywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, @GuillaumeGomez ? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good idea. However, a static str might be too restrictive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must be either that or a String

Copy link
Contributor Author

@xanathar xanathar Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear a String cannot be done (at the very least not without some dirty tricks), because the logger is a &'static and, as far as I can tell, there is no way to build a static String (all its constructors are not const fn).

Possible workarounds are with a lazy_static or with a RefCell inside the logger, but at that point I fear it becomes dirtier and harder to use correctly vs the potential gains.

(disclaimer: limited knowledge of Rust here, so I might be missing something)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use set_boxed_logger() but I agree that it's a bit ugly.

So what would be the use-case for this custom string, how would one use it and isn't it confusing to just have everything going into the log crate showing up the same way in the glib logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2 cents given the current state: let's remove this option, move forward with the rest and eventually add it in the future in another pr if and when any reasonable need+solution comes up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me as well.

@sdroege
Copy link
Member

sdroege commented Sep 16, 2020

Looks good to me generally

@xanathar xanathar closed this Sep 16, 2020
@xanathar
Copy link
Contributor Author

xanathar commented Sep 16, 2020

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

@xanathar xanathar reopened this Sep 16, 2020
@xanathar
Copy link
Contributor Author

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:

  • What do we want to do with GlibLoggerDomain::Custom(&'static str)
  • What to do with the override macros (is a separate commit enough)

@sdroege
Copy link
Member

sdroege commented Sep 17, 2020

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
Copy link
Member

I like the macros too. :)

@sdroege
Copy link
Member

sdroege commented Sep 18, 2020

It looks like the macros disappeared from the changes somehow?

@sdroege
Copy link
Member

sdroege commented Sep 18, 2020

Looks good to me otherwise

@xanathar
Copy link
Contributor Author

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
Copy link
Member

sdroege commented Sep 18, 2020

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 xanathar marked this pull request as ready for review September 18, 2020 14:15
@xanathar
Copy link
Contributor Author

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
Copy link
Member

sdroege commented Sep 18, 2020

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
Copy link
Contributor Author

Thanks to you :)

@xanathar
Copy link
Contributor Author

Changed the code with all the suggestions collected so far.

@xanathar
Copy link
Contributor Author

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
Copy link
Member

sdroege commented Sep 21, 2020

CI seems happy :) Thanks!

@sdroege sdroege merged commit 922be7c into gtk-rs:master Sep 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants