Fixes for logging during bootstrap #25722
Conversation
This ensures bootstrap messages will be visible.
* Remove uses of print_with_color * Remove use of basename() - just print the whole path * Remove special case single line log formatting - always print metadata on a new line.
current_task() is used to discover the logger, so this must come after task.jl
A logging exception during bootstrap is probably an error - propagate this through instead of catching it.
test/logging.jl
Outdated
| """ | ||
|
|
||
| # Multiline message | ||
| @test genmsg(Warn, "line1\nline2", Main, "some/path.jl", 101) == | ||
| """ | ||
| ┌ Warning: line1 | ||
| ┌ Warn: line1 |
There was a problem hiding this comment.
It'd be better if this also used the full word, for consistency if nothing else.
There was a problem hiding this comment.
What I did here was remove the special case when formatting the log level as part of the effort to do the least amount of formatting in SimpleLogger while keeping it fairly readable.
I'm leaning toward just renaming CoreLogging.Warn to CoreLogging.Warning because it seems really kind of bad to have to special case the printing of LogLevel every time you want to print it! How about I add the special case back in here, and make a separate PR for the renaming.
There was a problem hiding this comment.
By the way, removing the special casing was more about simplicity and making custom log levels print correctly rather than changing Warn. The printing in ConsoleLogger is unaffected, which is what users will see by default.
|
Great, thanks! |
| # Logging | ||
| include("logging.jl") | ||
| using .CoreLogging | ||
| global_logger(SimpleLogger(Core.STDERR, CoreLogging.Info)) |
There was a problem hiding this comment.
I think the assignment to _global_logstate can just be done at the end of the logging.jl file, and then also drop the using statement
There was a problem hiding this comment.
The using (or an import) is required to bring the logging macro names into Base for later use in the sysimg, for example in loading.jl. So I'll leave this here unless there's a particular preference to do that with an import.
I set the logger here in sysimg because the value we're setting for the global logger is entirely related to bootstrap, not to the logging system per se. So I'd rather leave it in sysimg.jl because that's the place where the bootstrap ordering is defined.
|
Right, I've reverted the change to the |
base/logging.jl
Outdated
| @@ -490,7 +490,7 @@ function handle_message(logger::SimpleLogger, level, message, _module, group, id | |||
| end | |||
| buf = IOBuffer() | |||
| iob = IOContext(buf, logger.stream) | |||
| levelstr = string(level) | |||
| levelstr = level == Warn ? "Warning" : string(level) | |||
There was a problem hiding this comment.
My thought to make this less exceptional was that one might be able to specify a levelstr for each kind of message instead of just hacking this in there. Not important though since extension points can always be added in the future.
|
It looks like there were two more instances of this missed in the |
|
Oh, wait, I see you're trying to respect the |
Oh darn. So logging from the parser can be problematic if a generated function constructs a string and sends it through I'm sure there's a way out of this conundrum without going back to spamming STDERR (probably something task-related as you've previously suggested, or another related way of deferring messages). So far the response on #25413 didn't look promising but I've got some more ideas I can pursue. |
|
Or just if the generator returns an ast with deprecated syntax. Oh well, this is all going away in a few months after we cut the v0.7 release, so maybe not a big deal to spend too much time on. Better to get the logging infrastructure working well and ready for release. |
|
Oh right, hitting the problem in lowering generated ASTs is a lot more likely. |
|
By the way, which parts are going away? It would be nice to have some idea so I can avoid wasting time on those. |
|
Sorry, I meant deprecations are going away (so the flisp code in Julia v1.0 won't need to deal with them anyways & not much point in wasting your time on them now) |
|
Oh sure, no problem. |
Here's some fixes to make logging reliable while building the sysimg.
SimpleLoggerduring sysimg build instead ofNullLoggerso that build warnings will be reported - see fix "Deprecated syntax"begin" inside indexing expression" warning. #25699 (comment)logging.jllater in the sysimg, as it requirescurrent_task()to be defined. With extra effort this could be defined further up but this seems simplest for now.SimpleLoggerso it can be relied on earlier during bootstrap (don't rely on color printing or path manipulation). User friendly formatting is now provided byConsoleLoggerinstead.SimpleLogger; let them propagate out as obvious errors instead.