Skip to content
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

Change default log level to warning. #5885

Closed
wants to merge 1 commit into from

Conversation

gtjoseph
Copy link
Contributor

@gtjoseph gtjoseph commented Feb 19, 2025

Ghostty's default log level is now "warning" unless one of the following
conditions is met...

  • A "+action" is being run in which case it's "error".
  • There's no app_runtime (lib mode) in which case it's "error".
  • The GHOSTTY_LOG environment variable isn't empty in which case it's "info".
  • Ghostty was started from a desktop/launcher icon in which case it's "info".
  • A debug build is being run in which case it's "debug".

The conditons are evaluated from top to bottom and the last one met wins.

@00-kat
Copy link
Contributor

00-kat commented Feb 19, 2025

Cross-referencing the original discussion: #5723.

@pluiedev
Copy link
Member

Also I disagree that the default should be error. We should preserve current behavior and allow users who are bothered by it to adjust Ghostty in accordance to their wishes.

@gtjoseph
Copy link
Contributor Author

Also I disagree that the default should be error. We should preserve current behavior and allow users who are bothered by it to adjust Ghostty in accordance to their wishes.

I suppose I could leave the default level at info and just keep --quiet which would suppress everything except errors. I just don't get it though. Why would your average non-ghostty-developer want those messages? What info do they convey? Shouldn't the program be as friendly as possible to the larger audience especially when it's fairly trivial to do so and it doesn't compromise any other features?

@00-kat
Copy link
Contributor

00-kat commented Feb 20, 2025

The main issue with having opt-in as opposed to opt-out logging is that people will paste the logs without warnings and infos into any GitHub discussions they create. Though there are many who think that the default is too noisy, though, especially when compared to every other terminal emulator out there.

@gtjoseph gtjoseph force-pushed the main-logging-options branch from b8bf40c to 13330f2 Compare February 21, 2025 00:48
@gtjoseph gtjoseph changed the title Add -q/--quiet and -v/--verbose logging options. Change default log level to err and add -v/--verbose logging option. Feb 21, 2025
@gtjoseph gtjoseph force-pushed the main-logging-options branch from 13330f2 to d93dce8 Compare February 21, 2025 00:49
@gtjoseph
Copy link
Contributor Author

Also I disagree that the default should be error. We should preserve current behavior and allow users who are bothered by it to adjust Ghostty in accordance to their wishes.

I'm going to leave it at error. The point of the PR was to make the default logging behavior like that of most other terminal emulators. I.E. quiet. If the direction is to leave the default at info then the PR can just be closed.

@gtjoseph gtjoseph requested a review from pluiedev February 21, 2025 01:00
@mitchellh
Copy link
Contributor

I'm not fully sure about this change yet (PR without an issue so its at your own risk anyways).

But, I will say the default should be "warn." The "error" log level in Zig is meant to indicate a fatal, non-recoverable error where the program either exits immediately or is likely in a severely broken state. A warning is meant to indicate an unexpected, possibly problematic (but maybe not) issue.

If we have spurious "warn" logs we should quiet those rather than raise the default to error.

@mitchellh
Copy link
Contributor

Another thought that I want to make sure I get down, I've thought it a few times but keep forgetting to put it down so here seems to be best: I think its generally helpful to have verbose logs when starting from a desktop location, because if something goes wrong, its easier to have ready access to some log course (i.e. Console.app on macOS, syslog-style locations on Linux).

So we may want to consider only defaulting to quiet if we're not launched from the desktop (we have an API for this already internal_os.launchedFromDesktop()). And we may want to just keep the existing logic GHOSTTY_LOG env var otherwise. That would simplify this PR and minimize any changes.

@gtjoseph
Copy link
Contributor Author

I'm not fully sure about this change yet (PR without an issue so its at your own risk anyways).

Heh, that's on @pluiedev. She recommended I open the PR. :)

But, I will say the default should be "warn."

Makes sense.

If we have spurious "warn" logs we should quiet those rather than raise the default to error.

Agreed.

I think its generally helpful to have verbose logs when starting from a desktop location, because if something goes wrong, its easier to have ready access to some log course (i.e. Console.app on macOS, syslog-style locations on Linux).

AFAIK stdout and stderr go nowhere in Linux when started from a .desktop file. Well, unless it's a Gnome thing. Was that the reason for the macos-specific logging in logFn()?

So we may want to consider only defaulting to quiet if we're not launched from the desktop (we have an API for this already internal_os.launchedFromDesktop()). And we may want to just keep the existing logic GHOSTTY_LOG env var otherwise. That would simplify this PR and minimize any changes.

Can do.

So to recap:

Default is warn unless started from the desktop in which case it's info.
If you start from a command line and want info, you can use -v/--verbose or GHOSTTY_LOG.
GHOSTTY_LOG would always apply now, instead of only when an action was being run or do you want to leave it for actions only?

@00-kat
Copy link
Contributor

00-kat commented Feb 21, 2025

AFAIK stdout and stderr go nowhere in Linux when started from a .desktop file. Well, unless it's a Gnome thing.

I've never checked my own system log (edit: just checked, the launcher I use doesn't redirect it) but some launchers seem to redirect it to the system log as this user found: #3614 (reply in thread).

@mitchellh
Copy link
Contributor

AFAIK stdout and stderr go nowhere in Linux when started from a .desktop file. Well, unless it's a Gnome thing. Was that the reason for the macos-specific logging in logFn()?

Yep! macOS has a builtin GUI app (Console.app) and CLI (log) to view macOS system logs and this way desktop app logs go to a place users can view, export, etc.

So to recap:

Default is warn unless started from the desktop in which case it's info.
If you start from a command line and want info, you can use -v/--verbose or GHOSTTY_LOG.
GHOSTTY_LOG would always apply now, instead of only when an action was being run or do you want to leave it for actions only?

Yeah I think this all makes sense, or at least I would take a look at that. I'm also suggesting we remove -v to avoid the arg iteration on startup. At least for now. The changes I suggested I'd merge now, I'm willing to discuss and consider the CLI flags later but I'm not sure.

@jcollie
Copy link
Member

jcollie commented Feb 22, 2025

AFAIK stdout and stderr go nowhere in Linux when started from a .desktop file.

Depending on the system, that all probably goes to the systemd user journal. Well, assuming you are using systemd. Use
journalctl --user -a -f to see if it shows up. The logs would be easier to find if we launched Ghostty from a systemd user service but that's a different discussion.

@gtjoseph
Copy link
Contributor Author

Yeah, I already checked journalctl --user. Nothing.
Fedora 41 XFCE.

Ghostty's default log level is now "warning" unless one of the following
conditions is met...
* A "+action" is being run in which case it's "error".
* There's no app_runtime (lib mode) in which case it's "error".
* The GHOSTTY_LOG environment variable isn't empty in which case it's "info".
* Ghostty was started from a desktop/launcher icon in which case it's "info".
* A debug build is being run in which case it's "debug".

The conditons are evaluated from top to bottom and the last one met wins.
@gtjoseph gtjoseph force-pushed the main-logging-options branch from d93dce8 to 5546fd7 Compare February 22, 2025 00:45
@gtjoseph gtjoseph changed the title Change default log level to err and add -v/--verbose logging option. Change default log level to warning. Feb 22, 2025
@gtjoseph
Copy link
Contributor Author

PR updated. See the PR description for new status.

@mitchellh Can I submit a new PR now to squash the warnings (or convert to info)? It doesn't depend on this PR.

@jcollie
Copy link
Member

jcollie commented Feb 22, 2025

Can I submit a new PR now to squash the warnings (or convert to info)? It doesn't depend on this PR.

What about instead of changing the logging level of some of these, we condition the log on the release level (i.e. they would be suppressed in anything but Debug level)?

@gtjoseph
Copy link
Contributor Author

What about instead of changing the logging level of some of these, we condition the log on the release level (i.e. they would be suppressed in anything but Debug level)?

I'm not following. Wouldn't that be the same as changing just their message levels to debug?

@jcollie
Copy link
Member

jcollie commented Feb 22, 2025

What about instead of changing the logging level of some of these, we condition the log on the release level (i.e. they would be suppressed in anything but Debug level)?

I'm not following. Wouldn't that be the same as changing just their message levels to debug?

The main difference is that warnings are emitted during a zig build test and debugs are not.

@gtjoseph
Copy link
Contributor Author

That didn't really clarify anything. :) It's late though and my brain is fried so let me read that again in the morning.

@gtjoseph
Copy link
Contributor Author

@jcollie Sleep didn't help. Can you give me a code snippet to demonstrate the result you're proposing?

@gtjoseph gtjoseph closed this Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants