-
Notifications
You must be signed in to change notification settings - Fork 962
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
Don't require GUI to run as service #2465
Conversation
Use updated launchctl syntax Don't require running service to uninstall
@frederickjansen thanks, for the PR (I am obvioulsy not a code owner, but I am interested in a working version for OSX) and as I mentioned in #947 (comment) this is the proper solution. |
<string>Background</string> | ||
<string>Aqua</string> | ||
<string>LoginWindow</string> | ||
<string>StandardIO</string> |
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.
I would suggest to add System
to the supported session types as well, so the service can optionally also be run as a deamon.
credit to: DrJosh9000 over at buildkite for the suggestion
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.
Would that require running ./svc.sh install
as root, and then the script moving the .plist file to /Library/LaunchDaemons
? There's a specific check not to run anything as root in the code so I wonder if they'll want to support that.
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.
Yes, but I am not suggesting to move to deamons but instead the idea was not to break this hotfix #1056 (comment)
But you might even want to do that intentionally because as you mentionend :
There's a specific check not to run anything as root in the code so I wonder if they'll want to support that.
@tcptps I got the same error as before. "Load failed: 5: Input/output error" It then tells you to run it as root to get more detailed information. But since it's an agent specific to a user and not a daemon, running as root just throws a different error message. That's a problem you don't get with the updated syntax. |
Just tested the changes on a mac2.metal instance from AWS (M1 ARM) and works well! |
@TingluoHuang Can someone review this please? |
There are a couple of changes in this pull request:
Background
instead of the defaultAqua
. Since Mac machines are supported on AWS and it's cumbersome to require the use of a GUI, running this as a background service makes sense.launchctl
. Specifically I make use oflaunchctl bootstrap user/UID
andlaunchctl bootout user/UID
. Specifying theuser
context rather thangui
makes sure this service runs in the background as well.stop()
would trigger thefailed
state if the service was already stopped, halting execution of the rest of the cleanup code. This would mean that if you can't start the service and want to uninstall it, you can't. I think returningtrue
with the same error message makes more sense.I only later realized this pull request is similar to #2449, though it doesn't use the updated launchctl syntax. In my testing I wasn't able to get that pull request to work either. The updated syntax was required to run in the background for me. This fixes #947 and #1056.