-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(log): add output control #16418
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
Conversation
So this preserve whether the config value is actually set.
This is for upcoming logging-to-shell feature
Tests are prepared to show the change of output destination control
| root_units.iter().map(|unit| unit_to_index[&unit]).collect(); | ||
|
|
||
| for (index, unit) in units.into_iter().enumerate() { | ||
| logger.log_batch(units.into_iter().enumerate().map(|(index, unit)| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not usually a fan of big map functions and tend to prefer explicit loops for them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shell holds a mutex lock and stdout lock, which are a bit expensive to work with. Other alternatives are having a buffer for shell print, or just eating the cost
| pub console: Option<bool>, | ||
| pub file: Option<bool>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR describes what this does but doesn't provide the motivation.
I find this
- confusing: it isn't very clear what these states mean and why you would use them
- odd to have build analysis and to be able to turn off
file - unclear on what purpose this is meant to serve, especially at this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is config doc update 3df1405.
Updated the PR description. Thanks for the nudge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User may want to pipe build log to stdout and process them in a stream manner in real time, rather than in a log file.
As I've said before, I want us to unify the build log and json message format, so how do decide what is a "build log" and a "regular message" in that future? I'd rather just wait, see how stabilization goes on this, and evaluate always putting these messages out.
Output to stdout so we can have a replacement for the unstable --timings=json option.
Do we need to have feature parity with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather just wait, see how stabilization goes on this, and evaluate always putting these messages out.
That is fair. We should wait and see.
Add output control for build analysis logs. Example config: ```toml [build.analysis] enabled = true console = true file = false ``` Console output is only enabled when `--message-format` has a JSON option. Note that logging is best-effort for now. If there is any broken pipe, no error or warning will show
|
Closed as we want to wait and see how we build logs and JSON message should work with each other in the future. Also, the original motivation of this was finding a good replacement for Thanks for the review! |
### What does this PR try to resolve? Arguments to remove it * The `--timings=json` is obsolete as `-Zbuild-analysis` logging is a more approachable option, which doesn't need passing `--timings=json` ahead of time. * There is no support infra built around `--timings=json` yet, while for `-Zbuild-analysis` we have `cargo report timings` already. * `--timings=json` is a UI feature inherently unstable, and has no tests. Counterargument: * `--timings=json` outputs to stdout, but there is no alternative yet also outputs to stdout. <#16418> was an attempt to add that back, but we then decide to punt until seeing requests or needs. ### How to test and review this PR? * `cargo help build` and check the manpage * `cargo build --help` and cehck the help text * `cargo build --timings` and it works
What does this PR try to resolve?
Add output control for build analysis logs.
The motivation of this are:
--timings=jsonoption.See doc update for more: 3df1405.
Example config:
How to test and review this PR?
Open questions:
Console output is only enabled when
--message-formathas a JSON option, so it won't interleave with status messages if people don't want JSON messages at all.Logging is best-effort for now. If there is any broken pipe, no error or warning will show
Alternative schemas
build.analysis.enabled. Each output option enablement control whether log is generatedbuild.analysis.file. console and file output are mutually exclusive