Skip to content

fix 353 - fix this Error when using ssh : 'launchctl on macos fails with "Could not find domain for"'#1960

Merged
DrJosh9000 merged 1 commit intobuildkite:mainfrom
tcptps:fixes
Feb 21, 2023
Merged

fix 353 - fix this Error when using ssh : 'launchctl on macos fails with "Could not find domain for"'#1960
DrJosh9000 merged 1 commit intobuildkite:mainfrom
tcptps:fixes

Conversation

@tcptps
Copy link
Contributor

@tcptps tcptps commented Feb 19, 2023

fix 353 see buildkite/docs#353 (comment)
edit:
The Error occurs because by default (without LimitLoadToSessionType) launchd tries to load the agent into the Aqua context but this context doesnt exist when you are only logged in using ssh.
The -S Background option tells launchd to load the agent into the Background Context. See changes in this PR for matching changes to the docs: buildkite/docs#1875 (comment)

@tcptps tcptps mentioned this pull request Feb 19, 2023
Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, this looks pretty good!

One general comment: can the PR title and description be more descriptive? (353 is a bug in another repo, and it's helpful when browsing old PRs to get a sense of "what" and "why" without having to dig into the changes.)

<string>LoginWindow</string>
<string>Background</string>
<string>StandardIO</string>
</array>
Copy link
Contributor

Choose a reason for hiding this comment

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

I often see <string>System</string> as well (for instance Homebrew creates it). I'm not sure it's necessary though.

<key>BUILDKITE_AGENT_CONFIG</key>
<string>/Users/your-build-user/.buildkite-agent/buildkite-agent.cfg</string>
</dict>
<key>LimitLoadToSessionType</key>
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency can there be a blank line between the previous item and this new one?

@tcptps tcptps changed the title fix 353 fix 353 - fix this Error : 'launchctl on macos fails with "Could not find domain for"' Feb 21, 2023
@tcptps tcptps changed the title fix 353 - fix this Error : 'launchctl on macos fails with "Could not find domain for"' fix 353 - fix this Error when using ssh : 'launchctl on macos fails with "Could not find domain for"' Feb 21, 2023
@tcptps
Copy link
Contributor Author

tcptps commented Feb 21, 2023

Thanks for the swift reply and constructive criticism.
You may add System and you may add a Line.
System allows you to load it using-S Systemwhich basicly makes it a deamon.

@tcptps
Copy link
Contributor Author

tcptps commented Feb 21, 2023

I am Sorry I only found out about the draft PR now I should have used that.
I have to admit I dont really know about buildkite or the system behind it I just came across the Issue mentionend above and It helped me so I wanted to give back a little. But this means I cant really do any production testing, but If you want to automatically launch the agent on boot (without having to use the GUI and auto login) I see the following three solutions:

  1. Place the plist file in /Library/LaunchDeamons and use the UserName key to specify the user to run as ; but this might have unwanted sideeffects and you will need one plist per user
  2. . Place the plist file in /Library/LaunchAgents and add a deamon under /Library/LaunchDeamons that bootstraps all users launchctl bootstrap user/<uid> who are supposed to run the agent. ; downside: Agents can only be edited using root
  3. Place the plist file in ~/Library/LaunchAgents, add a deamon under /Library/LaunchDeamons that bootstraps all users who are supposed to run the agent.and copy the file from /System/Library/LaunchAgents/com.apple.xpc.otherbsd.plist to /Library/LaunchAgents/ but change the LimitLoadToSessionType key in the copied plist to Background and change the Label in the copied plist to something else like buildkite.otherbsd. The usr/libexec/otherbsd binary called by /System/Library/LaunchAgents/com.apple.xpc.otherbsd.plist bootstraps all of the agents in ~/Library/LaunchAgents, but the /System/Library/LaunchAgents/com.apple.xpc.otherbsd.plist has the LimitLoadToSessionType key set to Aqua therefore if you login via ssh or bootstrap the user using the Deamon, the agents in ~/Library/LaunchAgentsarent loaded.

Use Cases for these hacky workarounds (case 2+3) are in my opinion staging enviroments, where you want stage a user having installed a Background Agent in /Library/LaunchAgents (case number 2) or in ~/Library/LaunchAgents, (case 3), (or you need some other feature of Agents that cannot be provided using a deamon with the UserName key) . All other Use Cases are better covered using non hacky methods.

NOTE: you might have to add a ten second delay to the deamon before bootstraping the users

BTW just using -S Background might be sufficient without this code change, making this PR useless
It is not sufficient you need the key and this PR :)

@tcptps
Copy link
Contributor Author

tcptps commented Feb 21, 2023

Thanks for the PR, this looks pretty good!

One general comment: can the PR title and description be more descriptive? (353 is a bug in another repo, and it's helpful when browsing old PRs to get a sense of "what" and "why" without having to dig into the changes.)

I hope the edited title and first comment are suffient

Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR! I'm happy for this to merge as-is, and I'll follow up with any tweaks.

@DrJosh9000 DrJosh9000 merged commit ad143e4 into buildkite:main Feb 21, 2023
@tcptps
Copy link
Contributor Author

tcptps commented Feb 23, 2023

Thanks again for the PR! I'm happy for this to merge as-is, and I'll follow up with any tweaks.

I was unshure if you wanted me to make those changes and thought it would be better if you made them, because this way they would be exactly the way you like them. You should have had write access to the branch and therefore the PR but I guess I should have pointed this out - the more you learn :)

DrJosh9000 added a commit that referenced this pull request Feb 27, 2023
Add a blank line, add System as a session type, and copy the change into the other plist.
DrJosh9000 added a commit that referenced this pull request Feb 27, 2023
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.

2 participants