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

watchman: launchctl unload post-install #9456

Merged
merged 1 commit into from
Feb 10, 2017
Merged

Conversation

wez
Copy link
Contributor

@wez wez commented Jan 30, 2017

This is taking a run at: facebook/watchman#358

Watchman installs and updates its own launchd.plist file on behalf of the user; it is not integrated with brew services.
In the linked issue, watchman would stop working for some homebrew users. The problem was that the launchd.plist used socket activation to set up the service, and because launchd is maintaining the socket, and the socket connection attempt just hangs, watchman never had a chance to fix up the plist and things stayed broken.

To help solve this, we're forcing an unload in the post-install portion of the recipe.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

This is taking a run at: facebook/watchman#358

Watchman installs and updates its own launchd.plist file on behalf of the user; it is not integrated with `brew services`.
In the linked issue, watchman would stop working for some homebrew users.  The problem was that the launchd.plist used socket activation to set up the service, and because launchd is maintaining the socket, and the socket connection attempt just hangs, watchman never had a chance to fix up the plist and things stayed broken.

To help solve this, we're forcing an unload in the post-install portion of the recipe.
@MikeMcQuaid
Copy link
Member

So, to confirm I've understood, watchman did it's own launchctl setup and this was broken so you're now removing it on post-install? How will this be fixed for existing installs inside or outside of Homebrew? Is it worth perhaps integrating watchman with Homebrew Services at the same time?

@wez
Copy link
Contributor Author

wez commented Feb 1, 2017

Watchman does its own launctl setup, but because of a bug in launchd with socket activation (if you remove the binary referenced by the plist, launch will just leave the socket unconnected with no error response), combined with a bug in launchtl with not returning an error code on failure (the man page hilariously describes this as a backwards compatibility concern), combined with the strongly versioned paths in homebrew, homebrew users are sometimes left with a hung watchman instance after upgrading from an older watchman version.

Watchman 4.8 (which we're going to tag next week) removes the socket activation component for new installs, but will also have to deal with this upgrade concern before that takes effect. In the meantime (until all homebrew users are on 4.8+) we need homebrew to give us a signal when the installed version changes; the easiest way to do this is a post install script to unregister the plist deployed by watchman.

It is technically possible to configure watchman to not do its own launchd stuff, but it means deploying a global configuration file and a helper script that will cause the service to start at the appropriate time. That's something that I'd like to see happen, but not something I have bandwidth to tackle at the moment, and we'd probably still want to do something like the post install step to swing users over to the brew services approach.

Does that make sense?

@wez
Copy link
Contributor Author

wez commented Feb 7, 2017

Friendly ping on this. I've got the 4.8 version update queued up behind this one :)

@MikeMcQuaid
Copy link
Member

@wez Pulling this temporarily but just FYI that it feels a little weird to fix non-Homebrew-specific issues like this. Thanks anyway.

@MikeMcQuaid MikeMcQuaid merged commit b546ce7 into Homebrew:master Feb 10, 2017
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants