-
Notifications
You must be signed in to change notification settings - Fork 320
Log to stderr as well as the log file, on non-Darwin platforms #2155
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
base: main
Are you sure you want to change the base?
Conversation
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.
We do log to stderr
until we set up the global log file handlers using setUpGlobalLogFileHandler
. We should also log to stderr from there. With the current implementation, we would log every message twice if setting up the global log file handlers failed (once from the log handler and then once again from your fputs
call).
Gotcha, presumably something like this is better? |
@swift-ci please test |
@swift-ci please test |
I'm not sure this is actually possible with our current implementation. All the calls are direct to OSLog and That also makes me wonder if we want this in general, since it means we have very different output to LSP clients between the platforms. EDIT: Ah, it seems like various editors also treat stderr output as actual errors (eg. neovim logs stderr output with the format "[ERROR][timestamp] ..."). So it may indeed be best to avoid doing this generally. I've opened #2161 which seems like a better direction to go here. |
@swift-ci Please test macOS |
@swift-ci Please test Windows |
This is consistent with other LSP servers, and makes debugging sourcekit-lsp easier.
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.
Sorry for missing this. I just found the PR again, looks good to me, I just fixed a trailing whitespace issue on your branch.
@swift-ci Please test |
@swift-ci Please test Windows |
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.
Just to re-iterate the edit I made in the previous comment - I'm not sure this is a path we want to go down due to editors treating stderr output as errors. Neovim is one example here (it logs stderr output with the format "[ERROR][timestamp] ..."), but I imagine there's probably others.
I'd prefer something like #2161, but I'd also be open to adding this behind a flag to make things easier to debug in the meantime.
Based on @bnbarham’s comment, I agree that this isn’t the right direction to go.
@bnbarham I don't think that's correct. All the other LSP implementations I've looked at (e.g rust-analyzer, clangd) write logs to stderr without problems. For example, if I create a hello.cpp file and start clangd, I get the following output to stderr:
I mostly use Emacs and VS Code for my LSP needs, but I don't think that any editor treats stderr output as error. Note that LSP doesn't say anything about stderr. You don't even need to use stdin/stdout -- the spec supports stdio, pipes, sockets and node-ipc. If an LSP server wants to show an error to a user, there are You can see in the linked thread that a clangd maintainer is asking for more options for stderr logging, but gets a ton of value from having these logs! I certainly find stderr logs super useful when investigating LSP issues, and stderr seems like a very common convention. |
Yeah, it's not a pop-up, just:
Which seems like it could cause some confusion to someone looking at those logs. Given we already log to a file, my preference would be to |
This is consistent with other LSP servers, and makes debugging sourcekit-lsp easier.
This was initially discussed in #2116. Let me know if you think we should log to stderr on macOS too.