Even if there's no modern logging system, dumping out those logs to stdout/stderr helps a lot on development.
Created attachment 439652 [details] PATCH
<rdar://problem/83740529>
Created attachment 439808 [details] PATCH
Comment on attachment 439808 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=439808&action=review > Source/WTF/wtf/Assertions.h:604 > +#define RELEASE_LOG_FP stdout It's not a good idea to output logs to stdout. It breaks command line application, DumpRenderTree and WebKitTestRunner. > Source/WTF/wtf/Assertions.h:606 > +#define RELEASE_LOG_FP stderr Why do you need LOG_STDERR? WebKit logs are output to stderr. You can enable all log channels by setting log env vars "all". > Source/WTF/wtf/Logger.h:354 > sd_journal_send_with_location(fileString.utf8().data(), lineString.utf8().data(), function, "WEBKIT_SUBSYSTEM=%s", channel.subsystem, "WEBKIT_CHANNEL=%s", channel.name, "MESSAGE=%s", logMessage.utf8().data(), nullptr); What about the idea implementing a compatible sd_journal_send_with_location for your platform.
> It's not a good idea to output logs to stdout. It breaks command line > application, DumpRenderTree and WebKitTestRunner. Well, I don't actually need stdout logging, but I thought it's nice to have option. About the DRT/WTR, stderr output also breaks the test result, doesn't it? We can run tests by building different binary for testing. > Why do you need LOG_STDERR? WebKit logs are output to stderr. You can enable > all log channels by setting log env vars "all". I don't understand that. I need release log and that is disable without enabling os_log or jounald. > What about the idea implementing a compatible sd_journal_send_with_location > for your platform. That was just too much for simple logging with stderr. Those extra info will be dump out when verbose logging.
(In reply to Basuke Suzuki from comment #5) > Well, I don't actually need stdout logging, but I thought it's nice to have > option. About the DRT/WTR, stderr output also breaks the test result, > doesn't it? We can run tests by building different binary for testing. The loggs in stderr don't make the test fail. But, if a test fail, run-webkit-tests show the stderr for the hint of debugging. For example, in the following test result, http/tests/xmlhttprequest/onload-progressevent-attributes.html failed and had a stderr message. https://build.webkit.org/results/WinCairo-64-bit-WKL-Release-Tests/r282824%20(2701)/results.html > > Why do you need LOG_STDERR? WebKit logs are output to stderr. You can enable > > all log channels by setting log env vars "all". > > I don't understand that. I need release log and that is disable without > enabling os_log or jounald. Oh, I misunderstood. > > What about the idea implementing a compatible sd_journal_send_with_location > > for your platform. > > That was just too much for simple logging with stderr. Those extra info will > be dump out when verbose logging. Got it.
Comment on attachment 439808 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=439808&action=review > ChangeLog:3 > + [PlayStation] Enable release logging to stdout or stderr This patch is almost platform independent change. Darin asked me not to use the platform prefix for such platform independent changes, for example [PlayStation] and [WinCairo].
How do you think an idea always enabling RELEASE_LOG, and if USE(OS_LOG) and USE(JOURNALD) are not true, use stderr.
(In reply to Fujii Hironori from comment #8) > How do you think an idea always enabling RELEASE_LOG, and if USE(OS_LOG) and > USE(JOURNALD) are not true, use stderr. That sounds good to me. Will result in simpler #if #elifs too. The logs are all disabled by default anyway: you need to opt-in using a confusing environment variable that I can never figure out how to use.
Comment on attachment 439808 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=439808&action=review > Source/WTF/wtf/Assertions.h:600 > +#error "Cannot unable both USE_LOG_STDOUT and USE_LOG_STDERR" "Cannot use both" >> Source/WTF/wtf/Logger.h:354 >> sd_journal_send_with_location(fileString.utf8().data(), lineString.utf8().data(), function, "WEBKIT_SUBSYSTEM=%s", channel.subsystem, "WEBKIT_CHANNEL=%s", channel.name, "MESSAGE=%s", logMessage.utf8().data(), nullptr); > > What about the idea implementing a compatible sd_journal_send_with_location for your platform. Seems like overkill?
(In reply to Fujii Hironori from comment #6) > The loggs in stderr don't make the test fail. But, if a test fail, > run-webkit-tests show the stderr for the hint of debugging. > For example, in the following test result, > http/tests/xmlhttprequest/onload-progressevent-attributes.html failed and > had a stderr message. > https://build.webkit.org/results/WinCairo-64-bit-WKL-Release-Tests/ > r282824%20(2701)/results.html Ah, make sense. That was one of my use case to use stdout so I agree to remove it for simplicity.
Comment on attachment 439808 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=439808&action=review >> ChangeLog:3 >> + [PlayStation] Enable release logging to stdout or stderr > > This patch is almost platform independent change. Darin asked me not to use the platform prefix for such platform independent changes, for example [PlayStation] and [WinCairo]. Right. Will remove the prefix. >> Source/WTF/wtf/Assertions.h:600 >> +#error "Cannot unable both USE_LOG_STDOUT and USE_LOG_STDERR" > > "Cannot use both" Thanks.
(In reply to Michael Catanzaro from comment #9) > (In reply to Fujii Hironori from comment #8) > > How do you think an idea always enabling RELEASE_LOG, and if USE(OS_LOG) and > > USE(JOURNALD) are not true, use stderr. > > That sounds good to me. Will result in simpler #if #elifs too. The logs are > all disabled by default anyway: you need to opt-in using a confusing > environment variable that I can never figure out how to use. That is kind of a big change for default configuration. There's tons of code omitted from runtime binary when not RELEASE_LOG_DISABLED. They are just there sitting silently without doing almost nothing. If we do this change, we should introduce ENABLE_RELEASE_LOG as other features do and make it completely opt-in feature. At that time, I agree with the default choice of logging. How about this?
I assumed it would not print anything unless special environment variables are enabled. I agree it should not print to stderr by default.
Created attachment 439895 [details] PATCH
Created attachment 439898 [details] PATCH
This is the shape including the points you guys reviewed. Removing USE_LOG_STDERR and added global flag ENABLE_RELEASE_LOG. Actual checking in the entire code base is untouched, still RELEASE_LOG_DISABLED is used. ENABLE_RELEASE_LOG is used to define that value at the beginning of Assertions.h.
Comment on attachment 439898 [details] PATCH A follow-up patch could replace RELEASE_LOG_DISABLED with ENABLE(RELEASE_LOG).
Comment on attachment 439898 [details] PATCH Thanks. I'll create a ticket for that.
Committed r283469 (242446@main): <https://commits.webkit.org/242446@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439898 [details].