Add launchd daemon support for macos#16
Conversation
https://github.com/brasic/launchd Also add support for starting, stopping and uninstalling the service.
This pulls in a breaking change from launchd: brasic/launchd@f5c0c73 Now the signature of ForRunningProgram has no error so we don't need to handle it.
BlakeWilliams
left a comment
There was a problem hiding this comment.
left a few minor comments/questions, but LGTM
internal/cmd/service.go
Outdated
| return | ||
| } | ||
|
|
||
| fmt.Printf("Configured to start at boot. Uninstall using:\n\t%s service uninstall\n", currentExecutableName()) |
There was a problem hiding this comment.
Should these Printlns use the logger? The logger should only be file backed for the server command.
There was a problem hiding this comment.
I don't quite grok this part of your question:
The logger should only be file backed for the server command.
That said, the service commands are always intended to be invoked by humans and these strings are for interactive presentation at the time they invoke the command. Can you help me understand why a log line would be better here?
There was a problem hiding this comment.
That comment had some built-in assumed context, sorry about that.
I've used the logger to output to STDOUT in most places iirc, except for server so there's a consistent interface/place to log. That way it's easy to move how/where things are logged to without making a ton of sweeping changes. e.g. if we wanted a silent version of this command.
If there's a meaningful difference between logging via fmt.Printf and the logger, feel free to ignore me. 👍
There was a problem hiding this comment.
The meaningful difference is this one:
./remote-development-manager service stop
2022/11/12 10:23:31 Service is not running, nothing to do.
vs
./remote-development-manager service stop
Service is not running, nothing to do.
From an aesthetic POV if I run a command and see a message meant for me to read immediately I would think something was broken or wrong if it looked like a log entry.
This is definitely nitpicking and bikeshedding but there's a difference between "event messages" from a running daemon in a log that could happen at any time and record when a thing happened and "response messages" from a command which only happen as a direct response to user input to let them know the result of their action.
WDYT about one of the following two options?
- passing a separate "interactive" logger configured without timestamps to cobra command objects when invoked from the CLI? Then we could use the logger interface here without the ugly timestamps, and obtain the ability to use these things in a headless context in the future by passing them a different logger.
- Swapping most of these messages that are really just "hey, you asked me to start this service and I did so I'm going tell you to be nice" to use stderr instead.
There was a problem hiding this comment.
This is definitely nitpicking and bikeshedding but there's a difference between "event messages" from a running daemon in a log that could happen at any time and record when a thing happened and "response messages" from a command which only happen as a direct response to user input to let them know the result of their action.
Good point, I agree with that.
- passing a separate "interactive" logger configured without timestamps to cobra command objects when invoked from the CLI? Then we could use the logger interface here without the ugly timestamps, and obtain the ability to use these things in a headless context in the future by passing them a different logger.
No strong feelings on which approach we take (feel free to use your best judgement), but the advantage of being able to log the information no matter the context (headless or interactive, etc.) makes me lean slightly towards this one.
There was a problem hiding this comment.
I feel pretty good about the approach I took in 5c6ea00. In the process I found that there were a number of places where the default stderr logger was being used instead of the server logger field.
edit: and of course tests are failing 😅 Will take a look later.
Co-authored-by: Blake Williams <blakewilliams@github.com>
Some logging changes exposed the fact that we were not actually shutting down the server on SIGINT/TERM. Change that, and add some entries to the server log indicating when a server came up and went down.
This consolidates everything under a single logger which is unprefixed and delivered to stderr when being used for human-readable messages (ie CLI context) and timestamp-prefixed and multiplexed to both the log file and stdout when used in server context. We manage the transition from CLI to long-lived process by reconfiguring the existing logger built in main.go. One advantage of this is that things that need to log inside main.go but the server log should know about (like signal receipt) get sent to the right place instead of a now-likely-disconnected stderr.
These tests are order dependendent and not parallel safe, apparently because some of them launch goroutines that outlive the tests themselves yet continue to use package global resources like httpClient and the implicitly global unix socket at path. Give each test its own copy of these resources to fix flaky tests.
|
Picking this up after a few months 😅 Turns out the test failures were pre-existing flakes, I've fixed them in the most recent commit. |
This adds a new
servicesubcommand that sets uprdmto run as a launchd service on MacOS.Here's a quick demo:
I originally wrote all the launchd management plumbing in this PR but after a while I realized it was kind of unrelated to this project so I spun it into a new library at https://github.com/brasic/launchd.