This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5589

watchman: add the possibility to set the exact watchman binary location
AbandonedPublic

Authored by lothiraldan on Jan 15 2019, 11:57 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

This is necessary to make rolling releases of new watchman versions across
users.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lothiraldan created this revision.Jan 15 2019, 11:57 AM
indygreg added a subscriber: indygreg.Jan 16 2019, 4:32 PM

Overall I like the feature. But the class initialization logic is wonky. That's not your fault: you happen to be wading into a mess. Since it looks like there will be a v2 on this series, I'd encourage you to add some inline comments about the __init__ mess at the least, or ideally refactor it so the code is cleaner.

hgext/fsmonitor/pywatchman/__init__.py
567–571

Why the nested __init__?

Perhaps this initialization code could be cleaned up so we are assigning an instance instead of a class to self.transport?

lothiraldan added inline comments.Jan 17 2019, 11:46 AM
hgext/fsmonitor/pywatchman/__init__.py
567–571

I see several ways to refactor the code around:

  • Pass the watchman_exe parameter to all Transport class even if they don't need it. The CLIProcessTransport is already ignoring the timeout attribute.
  • Pass a functools.partial of CLIProcessTransport.__init__.
  • Pass the client instance to the transport, which will likely results in a memory leak due to the circular reference.
  • Store an instance as client.transport but that would requires to move passing the sockpath and update the transport to manage a ready state, which would be false until they get all the needed parameters.

I have a small preference for solution 1). Solution 4) seems the most heavyweight

lothiraldan abandoned this revision.Feb 13 2019, 11:23 AM

Resent as D5954

Revision Contents
Changeset List

Diff 13225

hgext/fsmonitor/__init__.py

default=False, default=False,
) )
configitem('fsmonitor', 'timeout', configitem('fsmonitor', 'timeout',
default='2', default='2',
) )
configitem('fsmonitor', 'blacklistusers', configitem('fsmonitor', 'blacklistusers',
default=list, default=list,
) )
configitem('fsmonitor', 'watchman_exe',
default='watchman',
)
configitem('hgwatchman', 'verbose', configitem('hgwatchman', 'verbose',
default=False, default=False,
) )
configitem('experimental', 'fsmonitor.transaction_notify', configitem('experimental', 'fsmonitor.transaction_notify',
default=False, default=False,
) )
# This extension is incompatible with the following blacklisted extensions # This extension is incompatible with the following blacklisted extensions

hgext/fsmonitor/pywatchman/__init__.py

limits this implementation to JSON input and output only at this time. limits this implementation to JSON input and output only at this time.
It is the responsibility of the caller to set the send and It is the responsibility of the caller to set the send and
receive codecs appropriately. receive codecs appropriately.
""" """
proc = None proc = None
closed = True closed = True
def __init__(self, sockpath, timeout): def __init__(self, watchman_exe):
def __init__(sockpath, timeout):
self.sockpath = sockpath self.sockpath = sockpath
self.timeout = timeout self.timeout = timeout
self.watchman_exe = watchman_exe
return __init__
indygregUnsubmitted
Not Done

Why the nested __init__?

Perhaps this initialization code could be cleaned up so we are assigning an instance instead of a class to self.transport?

indygreg: Why the nested `__init__`? Perhaps this initialization code could be cleaned up so we are…
lothiraldanAuthorUnsubmitted
Not Done

I see several ways to refactor the code around:

  • Pass the watchman_exe parameter to all Transport class even if they don't need it. The CLIProcessTransport is already ignoring the timeout attribute.
  • Pass a functools.partial of CLIProcessTransport.__init__.
  • Pass the client instance to the transport, which will likely results in a memory leak due to the circular reference.
  • Store an instance as client.transport but that would requires to move passing the sockpath and update the transport to manage a ready state, which would be false until they get all the needed parameters.

I have a small preference for solution 1). Solution 4) seems the most heavyweight

lothiraldan: I see several ways to refactor the code around: - Pass the watchman_exe parameter to all…
def close(self): def close(self):
if self.proc: if self.proc:
if self.proc.pid is not None: if self.proc.pid is not None:
self.proc.kill() self.proc.kill()
self.proc.stdin.close() self.proc.stdin.close()
self.proc.stdout.close() self.proc.stdout.close()
self.proc = None self.proc = None
def _connect(self): def _connect(self):
if self.proc: if self.proc:
return self.proc return self.proc
args = [ args = [
'watchman', self.watchman_exe,
'--sockname={0}'.format(self.sockpath), '--sockname={0}'.format(self.sockpath),
'--logfile=/BOGUS', '--logfile=/BOGUS',
'--statefile=/BOGUS', '--statefile=/BOGUS',
'--no-spawn', '--no-spawn',
'--no-local', '--no-local',
'--no-pretty', '--no-pretty',
'-j', '-j',
] ]
sendConn = None sendConn = None
recvConn = None recvConn = None
subs = {} # Keyed by subscription name subs = {} # Keyed by subscription name
sub_by_root = {} # Keyed by root, then by subscription name sub_by_root = {} # Keyed by root, then by subscription name
logs = [] # When log level is raised logs = [] # When log level is raised
unilateral = ['log', 'subscription'] unilateral = ['log', 'subscription']
tport = None tport = None
useImmutableBser = None useImmutableBser = None
watchman_exe = None
def __init__(self, def __init__(self,
sockpath=None, sockpath=None,
timeout=1.0, timeout=1.0,
transport=None, transport=None,
sendEncoding=None, sendEncoding=None,
recvEncoding=None, recvEncoding=None,
useImmutableBser=False): useImmutableBser=False,
watchman_exe=None):
self.sockpath = sockpath self.sockpath = sockpath
self.timeout = timeout self.timeout = timeout
self.useImmutableBser = useImmutableBser self.useImmutableBser = useImmutableBser
self.watchman_exe = watchman_exe
if inspect.isclass(transport) and issubclass(transport, Transport): if inspect.isclass(transport) and issubclass(transport, Transport):
self.transport = transport self.transport = transport
else: else:
transport = transport or os.getenv('WATCHMAN_TRANSPORT') or 'local' transport = transport or os.getenv('WATCHMAN_TRANSPORT') or 'local'
if transport == 'local' and os.name == 'nt': if transport == 'local' and os.name == 'nt':
self.transport = WindowsNamedPipeTransport self.transport = WindowsNamedPipeTransport
elif transport == 'local': elif transport == 'local':
self.transport = UnixSocketTransport self.transport = UnixSocketTransport
elif transport == 'cli': elif transport == 'cli':
self.transport = CLIProcessTransport self.transport = CLIProcessTransport(self.watchman_exe)
if sendEncoding is None: if sendEncoding is None:
sendEncoding = 'json' sendEncoding = 'json'
if recvEncoding is None: if recvEncoding is None:
recvEncoding = sendEncoding recvEncoding = sendEncoding
else: else:
raise WatchmanError('invalid transport %s' % transport) raise WatchmanError('invalid transport %s' % transport)
sendEncoding = str(sendEncoding or os.getenv('WATCHMAN_ENCODING') or sendEncoding = str(sendEncoding or os.getenv('WATCHMAN_ENCODING') or
def _resolvesockname(self): def _resolvesockname(self):
# if invoked via a trigger, watchman will set this env var; we # if invoked via a trigger, watchman will set this env var; we
# should use it unless explicitly set otherwise # should use it unless explicitly set otherwise
path = os.getenv('WATCHMAN_SOCK') path = os.getenv('WATCHMAN_SOCK')
if path: if path:
return path return path
cmd = ['watchman', '--output-encoding=bser', 'get-sockname'] cmd = [self.watchman_exe, '--output-encoding=bser', 'get-sockname']
try: try:
args = dict(stdout=subprocess.PIPE, args = dict(stdout=subprocess.PIPE,
stderr=subprocess.PIPE, stderr=subprocess.PIPE,
close_fds=os.name != 'nt') close_fds=os.name != 'nt')
if os.name == 'nt': if os.name == 'nt':
# if invoked via an application with graphical user interface, # if invoked via an application with graphical user interface,
# this call will cause a brief command window pop-up. # this call will cause a brief command window pop-up.

hgext/fsmonitor/watchmanclient.py

# couldn't figure out our user # couldn't figure out our user
return None return None
def _command(self, *args): def _command(self, *args):
watchmanargs = (args[0], self._root) + args[1:] watchmanargs = (args[0], self._root) + args[1:]
try: try:
if self._watchmanclient is None: if self._watchmanclient is None:
self._firsttime = False self._firsttime = False
watchman_exe = self._ui.configpath('fsmonitor', 'watchman_exe')
self._watchmanclient = pywatchman.client( self._watchmanclient = pywatchman.client(
timeout=self._timeout, timeout=self._timeout,
useImmutableBser=True) useImmutableBser=True,
watchman_exe=watchman_exe)
return self._watchmanclient.query(*watchmanargs) return self._watchmanclient.query(*watchmanargs)
except pywatchman.CommandError as ex: except pywatchman.CommandError as ex:
if 'unable to resolve root' in ex.msg: if 'unable to resolve root' in ex.msg:
raise WatchmanNoRoot(self._root, ex.msg) raise WatchmanNoRoot(self._root, ex.msg)
raise Unavailable(ex.msg) raise Unavailable(ex.msg)
except pywatchman.WatchmanError as ex: except pywatchman.WatchmanError as ex:
raise Unavailable(str(ex)) raise Unavailable(str(ex))