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

services/yabai: cleanup and add errorLogFile/outLogFile #1231

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

khaneliman
Copy link
Contributor

Similar to #840 for allowing one to monitor the application's output in their logs.

Copy link
Contributor

@Samasaur1 Samasaur1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed some inconsistencies about the log files. Intuitively (and based on #1226 which includes the upstream plist) I'd say we probably want separate standard output and standard error logs by default

modules/services/yabai/default.nix Show resolved Hide resolved
modules/services/yabai/default.nix Outdated Show resolved Hide resolved
@Samasaur1
Copy link
Contributor

Otherwise this PR looks great to me

@khaneliman khaneliman changed the title services/yabai: cleanup and add logFile services/yabai: cleanup and add errorLogFile/outLogFile Dec 22, 2024
@khaneliman khaneliman requested a review from Samasaur1 December 22, 2024 22:15
Comment on lines +76 to +88
errorLogFile = mkOption {
type = types.path;
default = "/var/tmp/yabai.error.log";
example = "/Users/khaneliman/Library/Logs/yabai.log";
description = "Path to the yabai error log file";
};

outLogFile = mkOption {
type = types.path;
default = "/var/tmp/yabai.out.log";
example = "/Users/khaneliman/Library/Logs/yabai.log";
description = "Path to the yabai stdout log file";
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you picked /var/tmp over /tmp or /var/log? Upstream just puts them in /tmp, but /var/log (like you did for the scripting additions) also makes sense to me

Copy link
Contributor Author

@khaneliman khaneliman Dec 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User agents dont have access to /var/log but the global daemon does. It would probably be nice to use ~/Library/Logs/ but I believe I couldn't use ~ when testing before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. What about /tmp instead of /var/tmp? I don't really care either way, I've just never seen /var/tmp used before and didn't actually know that existed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/tmp could be purged on reboots, /var/tmp wouldn't be. So depends on what you think the default behavior should be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack sorry something is up with my notifications so I missed this. My personal preference is /tmp because it's what upstream appears to use, I think it's more visible, and I think it's fine if these logs don't persist across a reboot.

That said, I don't have permissions to merge this, so we could leave it up to @Enzime

@Samasaur1
Copy link
Contributor

Not gonna comment on the other two PRs but I have the same question about log location

@0rphee
Copy link

0rphee commented Dec 23, 2024

Just chiming in, the default yabai plist (as I'm writing this) can be found here: https://github.com/koekeishiya/yabai/blob/f4ada8d557aeb9c8ba0d031ee6f645acffff50fd/src/misc/service.h#L8-L42

As a user, I think its better to keep most of it as-is, to avoid differences in behavior from a 'normal' yabai installation. However, it would be nice to allow the option to override the launch agent configuration.

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.

3 participants