[pantsd] Add support for fetching watchman via BinaryUtil.

Review Request #3557 — Created March 10, 2016 and submitted

jsirois, nhoward_tw, peiyu, stuhood
  • Add BinaryUtil support for WatchmanLauncher.
  • Decouple WatchmanLauncher from FSEventService in favor of cleaner usage in PantsDaemonLauncher.
  • Implement testable_memoized_property in support of setting of memoized_property properties for testing.
  • Special case the removal of --watchman-path in favor of BinaryUtil-style options - don't think we need a deprecation cycle for this due to the experimental nature of current watchman support.
  • Test refactoring.


+ additional manual testing

    1. It looks like this needs a rebase to master.

  2. src/python/pants/pantsd/subsystem/watchman_launcher.py (Diff revision 1)

    So, I know the subsystem singletons are a big pain in the butt with regard to testing, but making properties mutable like this really rubs me the wrong way.

    If this is going to be for the purposes of testing, using the existing subsystem-reset code to handle this (and improving it if need be) would seem cleaner. Perhaps by adding support to the subsytem context managers in https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/subsystem/subsystem_util.py for using a mocked instance of a subsystem?

    1. this isn't about mocking subsystems - it's about mocking memoized properties, in this case to test internal methods of the WatchmanLauncher subsystem that rely on the object created by this property. @memoized_property currently disallows setting of its properties, which means that if you want to test something that gets otherwise neatly stood up in a property you have to actually mock.patch the higher level things called in the property (e.g. both select_binary and Watchman here) vs being able to simply create and set a wholesale mock object representing the computed value of the property in test.

      @mutable_memoized_property is just a means for allowing the use of ~@memoized_property over @property + if not self._thing: self._thing = Thing() boilerplate + thing._property = mock. I've had a number of reviews in the past where @memoized_property was recommended in these cases but it's never quite fit cleanly because of the inability to set properties for mocks - so thought I'd attempt to change that.

      with this said, I see two alternative options here - neither of which I'm a fan of:

      1) revert to @property + if not self._thing: self._thing = Thing() boilerplate + thing._property = mock
      2) mock.patch both select_binary and Watchman and use a @memoized_property here instead.

      or move forward with this. thoughts?

    2. As discussed, either making this annotation explicitly for testing, or adding support for dependency-injecting code in the subsystem context managers seems like the way to go.

    3. renamed this to testable_memoized_property for now.

  3. src/python/pants/pantsd/watchman.py (Diff revision 1)

    Should this take a pre-conversion log level (IE, a python warning level string) and then convert it?

    Alternatively, maybe the other constructor below should.

    1. what would be the benefit to this?

      my take is that Watchman is the abstraction of the watchman binary operation that is not coupled to pants internals in any way - so its inputs should be in terms of watchman parlance where log levels are represented as 0/1/2. and WatchmanLauncher handles options interfacing and creation of the Watchman instance - so is best suited for handling the conversion from the python warning level string (global_options.level) to the Watchman(log_level=...) input, as it does today.

    2. +1 to doing this in WatchmanLauncher then... it sounds like it is closer to being the interface between watchman and the rest of pants.

    3. modified this based on what I think you want here - let me know if this is incorrect.

  1. Thanks!

  2. Great, thanks!

    Is probably a @classmethod, but no big.

  1. s/mutable_memoized_property/testable_memoized_property/ in the description

  2. this seems to indicate log_level is one of the 0, 1, 2, but from the implementation it is the pants log level and converted into watchman levels, might clarify the comments

    1. moved this to the Watchman docstring.

  3. 0 to turn off logging is not provided, is it intentional? might need a comment

    1. good catch, added a mapping for 'warn' -> no logging for watchman.

  4. move 1 to a const for readability

  5. assertIsNone

Review request changed

Status: Closed (submitted)

Change Summary:

thanks gents! submitted @ 1299ddece5d6cb45adbe5ab797f1a151e8430196