watchman: 2.5.1 - file watching service. #20349
Conversation
Created a new formula for Facebook's file watching service.
Updated formula based on reviewer feedback: * Added runtime dependency pcre * Opened a support ticket with watchman for the weird compilation issue that requires two `make` commands to be issued. * Moved post build details to a caveat section.
Added patch provided by watchman maintainers to fix broken make command. See URL provided in the change set.
|
I've added a patch to the formula provided by the maintainers for the broken make command. |
| end | ||
|
|
||
| test do | ||
| # TODO testing requires 'arc': https://github.com/facebook/watchman#tools |
There was a problem hiding this comment.
There are scripts under travis/ that fetch and run the tests for the travis environment; I'm happy to take a patches that tweak them to run for homebrew (probably just to skip the top half of travis/deps.sh)
|
I tagged and pushed v2.6 with the build fix so that you can de-cruft your build formula; thanks! |
|
Thanks wez! I'll test/try the 2.6 tag and checkout the travis scripts. |
Changes: * Updated to v2.6. * Removed patch for makefile (no longer needed). * Changed pcre dependency to recommended, and modified configuration step to account for its presence.
|
I've updated to v2.6 (removed patch to makefile). At this point I haven't added the test phase - it requires a couple more packages (arcanist and libphutil) that aren't in brew. To test correctly with these libraries I think I'd need to make formulas for those dependencies as well? |
|
FWIW, from my perspective, you shouldn't need to run the tests if you are installing a tagged release (we shouldn't release it if it is known broken!), so I consider the test portion of this something that could be implemented later, if at all. |
| # display OSX specific notes about file handle size limits: | ||
| <<-EOS.undent | ||
|
|
||
| The default per-process descriptor limit on current versions of OS X is |
There was a problem hiding this comment.
Does this documentation exist on the web somewhere we can provide a link to?
There was a problem hiding this comment.
Yes, and a link is provided right before the end of the section.
There was a problem hiding this comment.
sid0 is right - I provided a link to the documentation in the caveats section - should it go somewhere else?
There was a problem hiding this comment.
If this block of text is available at the URL provide, let's not duplicate the block of text in the formula.
There was a problem hiding this comment.
One sentence and a link should suffice.
|
Is there anything else I need to do for this before it is accepted? |
|
Remove the failing test and squash to a single commit for review |
|
Note that we're going to tag 2.8 in the next day or two; you should probably verify it against master |
|
2.8 turned out to be a little broken, so we've tagged 2.8.1. Please update your patch to use that. |
| def install | ||
| system "./autogen.sh" | ||
| if build.with? 'pcre' | ||
| system "./configure", "--disable-debug", "--disable-dependency-tracking", |
There was a problem hiding this comment.
Build a list of arguments in an args array, and conditionally add --with-pcre if needed.
|
wez and sid0 - I've tried 2.8 and 2.8.1, neither of them compile for me. I was however able to compile 2.6 and 2.7 - I've opened a bug over on watchman's site: facebook/watchman#9 |
|
Squashed and issued new pull request: #21611 |
Created a new formula for Facebook's file watching service.