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

Convert to using launchctl bootstrap instead of deprecated launchctl load -w #112

Merged
merged 10 commits into from Apr 6, 2017

Conversation

Projects
None yet
3 participants
@johnhawkinson
Copy link
Contributor

johnhawkinson commented Apr 6, 2017

As discussed in #111, convert to using launchctl bootstrap (preceded by launchctl enable) instead of the deprecated launchctl load -w form.

This gives us error reporting, which fixes #111. It also means we're not using a deprecated API, which is good. And it also paves the way for better separation of root vs. user (e.g. so the same command need not do different things in those cases), and perhaps also to fix the problems of running launchctl under tmux/screen (maybe).

While we're here, document the -v flag, which was useful here. (Why doesn't the usage message standout in bold like in the builtin commands?)

Example of error detection we didn't have before:

bash-3.2# brew services -v run ntp
Unsetting PERL5LIB to protect you from yourself
/bin/launchctl enable system/homebrew.mxcl.ntp
/bin/launchctl bootstrap system /usr/local/opt/ntp/homebrew.mxcl.ntp.plist
/usr/local/Cellar/ntp/4.2.8p10/homebrew.mxcl.ntp.plist: Path had bad ownership/permissions
Error: Failure while executing: /bin/launchctl bootstrap system /usr/local/opt/ntp/homebrew.mxcl.ntp.plist

versus

bash-3.2# brew services -v run ntp
Unsetting PERL5LIB to protect you from yourself
/bin/launchctl load -w /usr/local/opt/ntp/homebrew.mxcl.ntp.plist
/usr/local/Cellar/ntp/4.2.8p10/homebrew.mxcl.ntp.plist: Path had bad ownership/permissions
==> Successfully started `ntp` (label: homebrew.mxcl.ntp)

Also, with a deliberately malformed plist, we used to fail undetectably:

bash-3.2# brew services start ntp
Unsetting PERL5LIB to protect you from yourself
/Library/LaunchDaemons/homebrew.mxcl.ntp.plist: Invalid property list
==> Successfully started `ntp` (label: homebrew.mxcl.ntp)
bash-3.2# brew services stop ntp
Unsetting PERL5LIB to protect you from yourself
Error: Service `ntp` is not started.
bash-3.2# 

and now we do it right:

bash-3.2# brew -v services start ntp
Unsetting PERL5LIB to protect you from yourself
==> Generated plist for ntp:
   <?xml version="1.0" encoding="UTF-8"?>
   <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
   <plist version="1.0">
   <dict>
     <key>KeepAlive</key>
     <true/>
     <key>Label</key>
     <string>homebrew.mxcl.ntp</string>
     <key>ProgramArguments</key>
     <array>
       <string>/usr/local/opt/ntp/sbin/ntpd</string>
       <string>-c</string>
       <string>/private/etc/ntp-restrict.conf</string>
       <string>-n</string>
       <string>-g</string>
       <string>-p</string>
       <string>/var/run/ntpd.pid</string>
       <string>-f</string>
       <string>/var/db/ntp.drift</string>
       <string>-l</string>
       <string>/var/log/ntp.log</stringg>
     </array>
     <key>RunAtLoad</key>
     <true/>
   </dict>
   </plist>
   

/bin/launchctl enable system/homebrew.mxcl.ntp
/bin/launchctl bootstrap system /Library/LaunchDaemons/homebrew.mxcl.ntp.plist
/Library/LaunchDaemons/homebrew.mxcl.ntp.plist: Invalid property list
Error: Failure while executing: /bin/launchctl bootstrap system /Library/LaunchDaemons/homebrew.mxcl.ntp.plist
bash-3.2# 

Note, this was also discussed, somewhat datedly, at Homebrew/legacy-homebrew#33259.
There was a lot of confusion in that thread, and in particular, there seemed to be the assumption that launchctl enable should come after launchctl load; that's not the case, and so doesn't work, which lead to proposals of using launchctl kickstart. That's unnecessary, and also sounds bad and inelegant :)

johnhawkinson added some commits Apr 6, 2017

services: Use launcchtl enable+bootstrap, not deprecated load -w
"launchctl load" has been deprecated since 10.10/Yosemite, if not
before.  There are at least two good reasons not to use it beyond
that:
  1. It has bad error reporting
  2. It behaves differently as root vs. user

Instead, we move to "launchctl enable" and "launchctl bootstrap", both
of which are non-deprecated. We mimic the "load -w" behavior of
determining the domain target based on whether we are root.  Perhaps
in a future restructuring that should change.

NB: Under 10.10, the mirror image of "launchctl boostrap" is
unimplemented, namely "launchctl unbootstrap" (or, as it's called in
10.11, "launchctl bootout"). So we continue to use "launchctl unload
-w" to remove services. This is fine and does not cause any problems.
@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 6, 2017

Note that this will break backwards compatibility on earlier versions of macOS.

@johnhawkinson

This comment has been minimized.

Copy link
Contributor

johnhawkinson commented Apr 6, 2017

Did I misunderstand @MikeMcQuaid:

That seems like a good idea. My concern is that it may limit the backwards compatibility of brew services but 10.10 and above seems fine at this point.

It's easy enough to conditionalize this on MacOS.version >= :yosemite and keep the old code, it's like 1 line plus the if/else (total 3). What's desired?

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 6, 2017

Of course you didn't misunderstand. To be explicit: i don't think we should break backward compatibility if it can be avoided.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 6, 2017

It's easy enough to conditionalize this on MacOS.version >= :yosemite and keep the old code, it's like 1 line plus the if/else (total 3). What's desired?

That would be good. I thought the changes would be more involved so it should be relatively straightforward to keep the old code around, thanks (and sorry to change my mind).

@johnhawkinson

This comment has been minimized.

Copy link
Contributor

johnhawkinson commented Apr 6, 2017

OK, slightly more as I added 2 comment lines.
I assume it's OK to break the safe_system / if $?.to_i_.nonzero? pair across the if block like that, but if not of course we can add more code duplication…

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 6, 2017

OK, slightly more as I added 2 comment lines.

We're sorry. You have exceeded your line quota. Please try again next time.

@@ -236,6 +237,33 @@ def restart(target)
end
end

# "load" a plist
def launchctl_load(plist, _function, service)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 6, 2017

Member

Skip _function if it's never used.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 6, 2017

Member

But it looks like it probably wants to be called function and used in ohai("Successfully started #{service.name} (label: #{service.label})")

This comment has been minimized.

@johnhawkinson

johnhawkinson Apr 6, 2017

Contributor

That was the intention, but I was concerned that changing the message from "started" run "ran" in the brew run case might break callers. So I wanted to make the API for the function call support that concept without commiting to it. Which way should I go? Is it OK to change the output for the brew run case?

(On the other hand, maybe brew run really does start things instead of running them. In which case this should all just go away?)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 6, 2017

Member

Is it OK to change the output for the brew run case?

Yes.

(On the other hand, maybe brew run really does start things instead of running them. In which case this should all just go away?)

That was to preserve backwards compatibility of commands.

@@ -236,6 +237,33 @@ def restart(target)
end
end

# "load" a plist

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 6, 2017

Member

Can skip this comment, pretty self-explanatory.

else
# New syntax has improved errr-handling.
safe_system launchctl, "enable", "#{domain_target}/#{service.label}"
if $?.to_i.nonzero?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 6, 2017

Member

safe_system will fail with an error so this failure check will never be hit (I realise the previous code also made this mistake).

safe_system launchctl, "bootstrap", domain_target, plist
end
if $?.to_i.nonzero?
odie("Failed to start `#{service.name}`")

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 6, 2017

Member

safe_system will fail with an error so this failure check will never be hit (I realise the previous code also made this mistake).

domain_target = "gui/#{Process.uid}"
end

if MacOS.version < :yosemite

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 6, 2017

Member

Pedantry but make this the else so the first block people see is the current version.

# This syntax was deprecated in Yosemite
safe_system launchctl, "load", "-w", plist
else
# New syntax has improved errr-handling.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 6, 2017

Member

Can remove this comment.

@johnhawkinson

This comment has been minimized.

Copy link
Contributor

johnhawkinson commented Apr 6, 2017

OK, I think that's all of them?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 6, 2017

@johnhawkinson Test seems to be failing; looks like mysql didn't come up.

@johnhawkinson

This comment has been minimized.

Copy link
Contributor

johnhawkinson commented Apr 6, 2017

@johnhawkinson Test seems to be failing; looks like mysql didn't come up.

Wha-huh? That seems so unlike---oh, how convenient. Final commit was cd4fc93 which changed only whitespace (removed a blank line). Prior commit also had a Travis run that succeeded. I think this means there's some nondeterminance in the mysql Travis test (consistent with the sleep 2 crap)?

I suppose the other possibility is there is nondeterminancy in the code in this PR, but that does not seem likely to me, since I didn't ever have it fail through repeated testing and whacking on my local system.

Thoughts? "It's never the test…except when it is."

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 6, 2017

Will rerun the test to see if it's random. If it is we might just bump the sleep a bit (proper engineering, that :trollface:)

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 6, 2017

Yeh, passed second time. Bumping sleep.

MikeMcQuaid added a commit that referenced this pull request Apr 6, 2017

travis.yml: wait longer for MySQL.
The previous sleep didn't seem to be enough see #112
@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 6, 2017

😴

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 6, 2017

Local tests look good to me. Nice work again here @johnhawkinson!

@MikeMcQuaid MikeMcQuaid merged commit e02fbad into Homebrew:master Apr 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 6, 2017

🎉

@lock lock bot added the outdated label Jan 2, 2019

@lock lock bot locked as resolved and limited conversation to collaborators Jan 2, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.