-
Notifications
You must be signed in to change notification settings - Fork 84
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
Logging: Swap println/eprintln for a logging library #924
Comments
This is possible, but as it's a process-global thing it may be a bit tough to pull off quickly.
I'm not concerned about the unsafe block.
I agree it should be possible to consume from bash but I also think bash is also a pretty sad "lowest common denominator". I'll play a bit with what it looks like to handle output from fd3 from bash as a reference. |
Agreed, I can play with the fd implementation for now. I will make sure dropping
It is also harder to consume from python. Reading from stdout and writing to stdin is built-in in both POpen and run functions. https://docs.python.org/3/library/subprocess.html#popen-constructor For progress, POpen would be used with
|
Currently, bootc uses the println and eprinln macros to log command progress. This makes it so that it is difficult to output structured output for programs to consume, as both 0 and 1 fds are used interchangeably for output, and to control the output logging level.
This is especially true for #921 , as in its current stage it requires the
--json-fd=<fd>
param, which introduces an unsafe block and it will be especially difficult to consume from bash (requires opening a new descriptor and makes difficult to pipe).Therefore, bootc should switch to a standard logging framework that pipes to stderr exclusively, and better yet through indicatif, so that both progress bars and structured output over stdout work correctly and universally.
If such change happens before #921 is included in a release,
--json-fd=<fd>
can be dropped in favor of--json
without introducing incompatibilities.The text was updated successfully, but these errors were encountered: