Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify where ProgressBar and Logging are written #1130

Closed
vaind opened this issue Mar 3, 2022 · 3 comments · Fixed by #1174
Closed

Unify where ProgressBar and Logging are written #1130

vaind opened this issue Mar 3, 2022 · 3 comments · Fixed by #1174

Comments

@vaind
Copy link
Collaborator

vaind commented Mar 3, 2022

Environment

We're using sentry-cli to upload symbols as part of the build process in the sentry-unity SDK. I've noticed that when SENTRY_LOG_LEVEL env var is configured, some logs are written to stderr, even though they're just INFO level as an example. From a quick look, I assume this line is responsible:

if let Some(pb) = get_progress_bar() {
pb.println(msg);
} else {
writeln!(io::stderr(), "{}", msg).ok();
}

This is the place where we got it captured in stderr. Note: currently the log level environment var is not set there in the PR to avoid showing "WARNING: INFO" kind of logs to the user.

Steps to Reproduce

  1. set SENTRY_LOG_LEVEL=info,
  2. run the CLI, redirecting stdout and stderr to different streams.

Expected Result

I'd expect non-error logs to be printed to stdout

Actual Result

INFO logs end up in stderr


edit: added missing link to the code where this is hit

@kamilogorek
Copy link
Contributor

The problem now is two-fold. The default in the indicatif lib is stderr, but we basically override it -

pub fn make_progress_bar(len: u64) -> ProgressBar {
let pb = ProgressBar::new(len);
pb.set_draw_target(ProgressDrawTarget::to_term(Term::stdout(), None));
pb.set_style(ProgressStyle::default_bar().template(&format!(
"{} {{msg}}\n{{wide_bar}} {{pos}}/{{len}}",
style(">").cyan()
)));
pb
}
however, we still call ProgressBar::new_spinner and ProgressBar::new in some places without redirecting it to a single descriptor, making it not consistent. It's all over the place now.

I'll let myself change the title of this issue to be more generic. Thanks for reporting.

@kamilogorek kamilogorek changed the title Logs output to stderr Unify where ProgressBar and Logging are written Mar 3, 2022
@kamilogorek kamilogorek added this to the v2 milestone Mar 30, 2022
@kamilogorek
Copy link
Contributor

2.0.0-beta.0 has been released, give it a try and let me know if your issue has been resolved 🎉

(progressbar and all logs are now going to stderr, and stdout is used only for cli result output, which can be in some cases silenced with --quiet/--silent or piped).

@vaind
Copy link
Collaborator Author

vaind commented Apr 5, 2022

2.0.0-beta.0 has been released, give it a try and let me know if your issue has been resolved 🎉

Created an issue to do that: getsentry/sentry-unity#674

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

Successfully merging a pull request may close this issue.

2 participants