Re: Introduce Tracing into Chalk

[nolanderc]

Replaces the old logging system that printed directly to stderr with macros from the tracing crate.

[nikomatsakis]

This all seems pretty straightforward, though I'm not sure what the ?foo notation is all about in the various macros. One thing that might be a nice addition is to extend the chalk book with a short description of how to use the debug logs -- it would also help in reviewing the PR.

I'd like to see something like:

• How to view the debug logs (document the environment variable options and so forth)
• How to write code that uses debug and friends, and the #[instrument] annotations

Finally, one concern I have with merging this is that it makes tracing a dependency of chalk which would in turn affect rustc -- do you think we can find some way to make it feature gated in a reasonable fashion? (i.e., behind a cargo feature?)

It'd be sort of nice if we could pick between the standard log crate that rustc uses and tracing via cargo feature, so that we can integrate more easily with rustc.

[nikomatsakis]

Woah! what is happening here. I guess this is a different for how tracing works?

[nolanderc]

Yup, in tracing the logging macros generate Events which may contain a number of fields. The ? sigil is a shorthand for creating a new field using the Debug implementation of answer.

[nolanderc]

?foo is a shorthand for creating a new field on the logging Event using the Debug implementation for foo. In contrast to log, tracing prefers to keep parameters separate from the message string. I'll see what I can do regarding the chalk book.

I'm a new contributor to the Rust project, so I'm not familiar with what the policy around adding new dependencies is. But for what it is worth, tracing has a feature flag that forces it to only emit log records: https://tracing.rs/tracing/#emitting-log-records. Would that be sufficient to resolve your concern?

[detrumi]

tracing has a feature flag that forces it to only emit log records: https://tracing.rs/tracing/#emitting-log-records. Would that be sufficient to resolve your concern?

I think the problem is that it adds a new dependency in Cargo.toml, which increases build times for rustc. A cargo feature allows you to opt-out of that dependency.

[nikomatsakis]

Well, there are two concerns:

• build times
• licensing and general "exposure"

We have a "accept-list" of dependencies it would have to be added to, IIRC, and we'd also want to check the license.

As it happens I'd like to experiment with porting rustc over to using tracing, but that's a job in and of itself.

[jackh726]

Ok, @nolanderc think you can rebase this? And maybe add docs in the chalk for how to view logs.

@nikomatsakis Now that chalk-ir doesn't depend on chalk-engine (the other way around), then tracing won't be a dependency for librustc_middle, which is what the whitelist is for. So good on that front.

Other than that, based on discussion on Zulip, I think this is fine :)

[nikomatsakis]

@jackh726

Now that chalk-ir doesn't depend on chalk-engine (the other way around), then tracing won't be a dependency for librustc_middle, which is what the whitelist is for. So good on that front.

I don't quite understand that. Can you spell it out a bit more?

[jackh726]

@nikomatsakis So, I realized that the debug macros are in chalk-ir now, so maybe that statement was confusing. But there are no calls that use them in chalk-ir. So, if we switch to using tracing, we can just make chalk-engine and chalk-solve have direct dependencies on it separately.

The whitelist in rustc is only for crates that librustc_middle depends on, directly or transitively. librustc_middle only depends on chalk-ir and chalk-derive, so this should be it won't depend on tracing.

[nikomatsakis]

The whitelist in rustc is only for crates that librustc_middle depends on, directly or transitively.

What? This sounds like a bug in rustc. The idea of the whitelist is to control what ships in rustc, I don't see why it matters where within rustc's tangled dep graph that dependency arises...

[jackh726]

See https://github.com/rust-lang/rust/blob/ad4bc3323b9299d867697e9653dcea1b5e1ad283/src/tools/tidy/src/deps.rs#L59

[jackh726]

Closing in favor of #525

Sorry we didn't get this pulled in @nolanderc