Skip to content

Conversation

@bergzand
Copy link
Contributor

@bergzand bergzand commented Oct 20, 2025

This refactors all (non-commented) println!s to use a logger via env_logger. This allows for setting the log level via environment variables and retains the filtering of the messages via the command line arguments (--verbose and --quiet).

A lot of the code is simplified because the verbose argument doesn't have to be passed around in most cases.

@bergzand
Copy link
Contributor Author

Needs #799 for the CI, and I have to take a look where the nightly test is failing exactly

@kaspar030
Copy link
Owner

Needs #799 for the CI, and I have to take a look where the nightly test is failing exactly

#799 is optional, though it would be nice to briefly check if there's any performance impact.

where
F: FnOnce() -> String,
{
if VERBOSE {
Copy link
Owner

Choose a reason for hiding this comment

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

This was actually using generics to create two versions, one with the printlns disabled. that did save a bunch of time when I tested it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It really wouldn't surprise me if I accidentally removed a bit too much here.

pub(crate) fn maybe_set_limit(limit: usize, verbose: u8) {
pub(crate) fn maybe_set_limit(limit: usize) {
JOBSERVER.get_or_init(|| {
if verbose > 1 {
Copy link
Owner

Choose a reason for hiding this comment

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

this was using >1, so maybe trace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 56bfb21

@bergzand
Copy link
Contributor Author

A number of println!s have a "laze:" at the start of the output. Do you want to retain that, and should that be in the main log write! template instead?

@kaspar030
Copy link
Owner

A number of println!s have a "laze:" at the start of the output. Do you want to retain that, and should that be in the main log write! template instead?

Well, maybe keep changing output method and changing output content separately.

I did make laze output laze: ... because it might intermingle with e.g., ninja's output. I don't think all of laze's output prefixes laze:, so not sure it can go into the main log template. Would be nicer. Is there opt-out?

@bergzand
Copy link
Contributor Author

I did make laze output laze: ... because it might intermingle with e.g., ninja's output.

That's what I assumed the reason was for the prefix. I do think it should be applied consistently, otherwise messages that do not have the prefix might confuse the reader of the origin.

Would be nicer. Is there opt-out?

An opt out of the prefix? Should be possible to add in a follow up.

Well, maybe keep changing output method and changing output content separately.

I already removed a 'warn' in the output content here and there, because that's duplicate now.

@bergzand bergzand marked this pull request as ready for review October 21, 2025 08:34
@bergzand
Copy link
Contributor Author

Output should now be identical to current main.

@github-actions
Copy link
Contributor

🐰 Bencher Report

Branchpr/logger
Testbedgithub-actions
Click to view all benchmark results
Benchmarkperf:task-clockBenchmark Result
msec x 1e3
(Result Δ%)
Upper Boundary
msec x 1e3
(Limit %)
laze -C RIOT build --global --generate-only📈 view plot
🚷 view threshold
9.34 x 1e3
(-1.07%)Baseline: 9.44 x 1e3
9.94 x 1e3
(94.01%)
🐰 View full continuous benchmarking report in Bencher

@coveralls
Copy link

Coverage Status

coverage: 80.796% (+0.6%) from 80.182%
when pulling bfd1ad4 on bergzand:pr/logger
into f1a88ed on kaspar030:main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants