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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

pulkit (Pulkit Goyal)
lothiraldan created this revision.
Herald added subscribers: mercurial-devel, mjpieters.
Herald added a reviewer: hg-reviewers.

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5589

AFFECTED FILES
  hgext/fsmonitor/__init__.py
  hgext/fsmonitor/pywatchman/__init__.py
  hgext/fsmonitor/watchmanclient.py

CHANGE DETAILS

diff --git a/hgext/fsmonitor/watchmanclient.py b/hgext/fsmonitor/watchmanclient.py
--- a/hgext/fsmonitor/watchmanclient.py
+++ b/hgext/fsmonitor/watchmanclient.py
@@ -82,9 +82,11 @@
         try:
             if self._watchmanclient is None:
                 self._firsttime = False
+                watchman_exe = self._ui.configpath('fsmonitor', 'watchman_exe')
                 self._watchmanclient = pywatchman.client(
                     timeout=self._timeout,
-                    useImmutableBser=True)
+                    useImmutableBser=True,
+                    watchman_exe=watchman_exe)
             return self._watchmanclient.query(*watchmanargs)
         except pywatchman.CommandError as ex:
             if 'unable to resolve root' in ex.msg:
diff --git a/hgext/fsmonitor/pywatchman/__init__.py b/hgext/fsmonitor/pywatchman/__init__.py
--- a/hgext/fsmonitor/pywatchman/__init__.py
+++ b/hgext/fsmonitor/pywatchman/__init__.py
@@ -563,9 +563,12 @@
     proc = None
     closed = True
 
-    def __init__(self, sockpath, timeout):
-        self.sockpath = sockpath
-        self.timeout = timeout
+    def __init__(self, watchman_exe):
+        def __init__(sockpath, timeout):
+            self.sockpath = sockpath
+            self.timeout = timeout
+            self.watchman_exe = watchman_exe
+        return __init__
 
     def close(self):
         if self.proc:
@@ -579,7 +582,7 @@
         if self.proc:
             return self.proc
         args = [
-            'watchman',
+            self.watchman_exe,
             '--sockname={0}'.format(self.sockpath),
             '--logfile=/BOGUS',
             '--statefile=/BOGUS',
@@ -756,17 +759,20 @@
     unilateral = ['log', 'subscription']
     tport = None
     useImmutableBser = None
+    watchman_exe = None
 
     def __init__(self,
                  sockpath=None,
                  timeout=1.0,
                  transport=None,
                  sendEncoding=None,
                  recvEncoding=None,
-                 useImmutableBser=False):
+                 useImmutableBser=False,
+                 watchman_exe=None):
         self.sockpath = sockpath
         self.timeout = timeout
         self.useImmutableBser = useImmutableBser
+        self.watchman_exe = watchman_exe
 
         if inspect.isclass(transport) and issubclass(transport, Transport):
             self.transport = transport
@@ -777,7 +783,7 @@
             elif transport == 'local':
                 self.transport = UnixSocketTransport
             elif transport == 'cli':
-                self.transport = CLIProcessTransport
+                self.transport = CLIProcessTransport(self.watchman_exe)
                 if sendEncoding is None:
                     sendEncoding = 'json'
                 if recvEncoding is None:
@@ -817,7 +823,7 @@
         if path:
             return path
 
-        cmd = ['watchman', '--output-encoding=bser', 'get-sockname']
+        cmd = [self.watchman_exe, '--output-encoding=bser', 'get-sockname']
         try:
             args = dict(stdout=subprocess.PIPE,
                         stderr=subprocess.PIPE,
diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
--- a/hgext/fsmonitor/__init__.py
+++ b/hgext/fsmonitor/__init__.py
@@ -161,6 +161,9 @@
 configitem('fsmonitor', 'blacklistusers',
     default=list,
 )
+configitem('fsmonitor', 'watchman_exe',
+    default='watchman',
+)
 configitem('hgwatchman', 'verbose',
     default=False,
 )



To: lothiraldan, #hg-reviewers
Cc: mjpieters, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

pulkit (Pulkit Goyal)
indygreg added a comment.


  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.

INLINE COMMENTS

> __init__.py:567-571
> +        def __init__(sockpath, timeout):
> +            self.sockpath = sockpath
> +            self.timeout = timeout
> +            self.watchman_exe = watchman_exe
> +        return __init__

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`?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5589

To: lothiraldan, #hg-reviewers
Cc: indygreg, mjpieters, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

pulkit (Pulkit Goyal)
In reply to this post by pulkit (Pulkit Goyal)
lothiraldan added inline comments.

INLINE COMMENTS

> indygreg wrote in __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`?

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5589

To: lothiraldan, #hg-reviewers
Cc: indygreg, mjpieters, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

pulkit (Pulkit Goyal)
In reply to this post by pulkit (Pulkit Goyal)
lothiraldan updated this revision to Diff 13414.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D5589?vs=13225&id=13414

REVISION DETAIL
  https://phab.mercurial-scm.org/D5589

AFFECTED FILES
  hgext/fsmonitor/__init__.py
  hgext/fsmonitor/pywatchman/__init__.py
  hgext/fsmonitor/watchmanclient.py

CHANGE DETAILS

diff --git a/hgext/fsmonitor/watchmanclient.py b/hgext/fsmonitor/watchmanclient.py
--- a/hgext/fsmonitor/watchmanclient.py
+++ b/hgext/fsmonitor/watchmanclient.py
@@ -82,9 +82,11 @@
         try:
             if self._watchmanclient is None:
                 self._firsttime = False
+                watchman_exe = self._ui.configpath('fsmonitor', 'watchman_exe')
                 self._watchmanclient = pywatchman.client(
                     timeout=self._timeout,
-                    useImmutableBser=True)
+                    useImmutableBser=True,
+                    watchman_exe=watchman_exe)
             return self._watchmanclient.query(*watchmanargs)
         except pywatchman.CommandError as ex:
             if 'unable to resolve root' in ex.msg:
diff --git a/hgext/fsmonitor/pywatchman/__init__.py b/hgext/fsmonitor/pywatchman/__init__.py
--- a/hgext/fsmonitor/pywatchman/__init__.py
+++ b/hgext/fsmonitor/pywatchman/__init__.py
@@ -317,7 +317,7 @@
     """ local unix domain socket transport """
     sock = None
 
-    def __init__(self, sockpath, timeout):
+    def __init__(self, sockpath, timeout, watchman_exe):
         self.sockpath = sockpath
         self.timeout = timeout
 
@@ -397,7 +397,7 @@
 class WindowsNamedPipeTransport(Transport):
     """ connect to a named pipe """
 
-    def __init__(self, sockpath, timeout):
+    def __init__(self, sockpath, timeout, watchman_exe):
         self.sockpath = sockpath
         self.timeout = int(math.ceil(timeout * 1000))
         self._iobuf = None
@@ -563,9 +563,10 @@
     proc = None
     closed = True
 
-    def __init__(self, sockpath, timeout):
+    def __init__(self, sockpath, timeout, watchman_exe):
         self.sockpath = sockpath
         self.timeout = timeout
+        self.watchman_exe = watchman_exe
 
     def close(self):
         if self.proc:
@@ -579,7 +580,7 @@
         if self.proc:
             return self.proc
         args = [
-            'watchman',
+            self.watchman_exe,
             '--sockname={0}'.format(self.sockpath),
             '--logfile=/BOGUS',
             '--statefile=/BOGUS',
@@ -756,17 +757,20 @@
     unilateral = ['log', 'subscription']
     tport = None
     useImmutableBser = None
+    watchman_exe = None
 
     def __init__(self,
                  sockpath=None,
                  timeout=1.0,
                  transport=None,
                  sendEncoding=None,
                  recvEncoding=None,
-                 useImmutableBser=False):
+                 useImmutableBser=False,
+                 watchman_exe=None):
         self.sockpath = sockpath
         self.timeout = timeout
         self.useImmutableBser = useImmutableBser
+        self.watchman_exe = watchman_exe
 
         if inspect.isclass(transport) and issubclass(transport, Transport):
             self.transport = transport
@@ -817,7 +821,7 @@
         if path:
             return path
 
-        cmd = ['watchman', '--output-encoding=bser', 'get-sockname']
+        cmd = [self.watchman_exe, '--output-encoding=bser', 'get-sockname']
         try:
             args = dict(stdout=subprocess.PIPE,
                         stderr=subprocess.PIPE,
@@ -858,7 +862,7 @@
         if self.sockpath is None:
             self.sockpath = self._resolvesockname()
 
-        self.tport = self.transport(self.sockpath, self.timeout)
+        self.tport = self.transport(self.sockpath, self.timeout, self.watchman_exe)
         self.sendConn = self.sendCodec(self.tport)
         self.recvConn = self.recvCodec(self.tport)
 
diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
--- a/hgext/fsmonitor/__init__.py
+++ b/hgext/fsmonitor/__init__.py
@@ -164,6 +164,9 @@
 configitem('fsmonitor', 'enable_on_non_interactive',
     default=True,
 )
+configitem('fsmonitor', 'watchman_exe',
+    default='watchman',
+)
 configitem('hgwatchman', 'verbose',
     default=True,
 )



To: lothiraldan, #hg-reviewers
Cc: indygreg, mjpieters, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

pulkit (Pulkit Goyal)
In reply to this post by pulkit (Pulkit Goyal)
lothiraldan updated this revision to Diff 14070.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D5589?vs=13414&id=14070

REVISION DETAIL
  https://phab.mercurial-scm.org/D5589

AFFECTED FILES
  hgext/fsmonitor/__init__.py
  hgext/fsmonitor/pywatchman/__init__.py
  hgext/fsmonitor/watchmanclient.py

CHANGE DETAILS

diff --git a/hgext/fsmonitor/watchmanclient.py b/hgext/fsmonitor/watchmanclient.py
--- a/hgext/fsmonitor/watchmanclient.py
+++ b/hgext/fsmonitor/watchmanclient.py
@@ -82,9 +82,11 @@
         try:
             if self._watchmanclient is None:
                 self._firsttime = False
+                watchman_exe = self._ui.configpath('fsmonitor', 'watchman_exe')
                 self._watchmanclient = pywatchman.client(
                     timeout=self._timeout,
-                    useImmutableBser=True)
+                    useImmutableBser=True,
+                    watchman_exe=watchman_exe)
             return self._watchmanclient.query(*watchmanargs)
         except pywatchman.CommandError as ex:
             if 'unable to resolve root' in ex.msg:
diff --git a/hgext/fsmonitor/pywatchman/__init__.py b/hgext/fsmonitor/pywatchman/__init__.py
--- a/hgext/fsmonitor/pywatchman/__init__.py
+++ b/hgext/fsmonitor/pywatchman/__init__.py
@@ -317,7 +317,7 @@
     """ local unix domain socket transport """
     sock = None
 
-    def __init__(self, sockpath, timeout):
+    def __init__(self, sockpath, timeout, watchman_exe):
         self.sockpath = sockpath
         self.timeout = timeout
 
@@ -397,7 +397,7 @@
 class WindowsNamedPipeTransport(Transport):
     """ connect to a named pipe """
 
-    def __init__(self, sockpath, timeout):
+    def __init__(self, sockpath, timeout, watchman_exe):
         self.sockpath = sockpath
         self.timeout = int(math.ceil(timeout * 1000))
         self._iobuf = None
@@ -563,9 +563,10 @@
     proc = None
     closed = True
 
-    def __init__(self, sockpath, timeout):
+    def __init__(self, sockpath, timeout, watchman_exe):
         self.sockpath = sockpath
         self.timeout = timeout
+        self.watchman_exe = watchman_exe
 
     def close(self):
         if self.proc:
@@ -579,7 +580,7 @@
         if self.proc:
             return self.proc
         args = [
-            'watchman',
+            self.watchman_exe,
             '--sockname={0}'.format(self.sockpath),
             '--logfile=/BOGUS',
             '--statefile=/BOGUS',
@@ -756,17 +757,20 @@
     unilateral = ['log', 'subscription']
     tport = None
     useImmutableBser = None
+    watchman_exe = None
 
     def __init__(self,
                  sockpath=None,
                  timeout=1.0,
                  transport=None,
                  sendEncoding=None,
                  recvEncoding=None,
-                 useImmutableBser=False):
+                 useImmutableBser=False,
+                 watchman_exe=None):
         self.sockpath = sockpath
         self.timeout = timeout
         self.useImmutableBser = useImmutableBser
+        self.watchman_exe = watchman_exe
 
         if inspect.isclass(transport) and issubclass(transport, Transport):
             self.transport = transport
@@ -817,7 +821,7 @@
         if path:
             return path
 
-        cmd = ['watchman', '--output-encoding=bser', 'get-sockname']
+        cmd = [self.watchman_exe, '--output-encoding=bser', 'get-sockname']
         try:
             args = dict(stdout=subprocess.PIPE,
                         stderr=subprocess.PIPE,
@@ -858,7 +862,7 @@
         if self.sockpath is None:
             self.sockpath = self._resolvesockname()
 
-        self.tport = self.transport(self.sockpath, self.timeout)
+        self.tport = self.transport(self.sockpath, self.timeout, self.watchman_exe)
         self.sendConn = self.sendCodec(self.tport)
         self.recvConn = self.recvCodec(self.tport)
 
diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
--- a/hgext/fsmonitor/__init__.py
+++ b/hgext/fsmonitor/__init__.py
@@ -164,6 +164,9 @@
 configitem('fsmonitor', 'enable_on_non_interactive',
     default=True,
 )
+configitem('fsmonitor', 'watchman_exe',
+    default='watchman',
+)
 configitem('fsmonitor', 'verbose',
     default=True,
 )



To: lothiraldan, #hg-reviewers
Cc: indygreg, mjpieters, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

pulkit (Pulkit Goyal)
In reply to this post by pulkit (Pulkit Goyal)
lothiraldan updated this revision to Diff 14073.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D5589?vs=14070&id=14073

REVISION DETAIL
  https://phab.mercurial-scm.org/D5589

AFFECTED FILES
  hgext/fsmonitor/__init__.py
  hgext/fsmonitor/pywatchman/__init__.py
  hgext/fsmonitor/watchmanclient.py

CHANGE DETAILS

diff --git a/hgext/fsmonitor/watchmanclient.py b/hgext/fsmonitor/watchmanclient.py
--- a/hgext/fsmonitor/watchmanclient.py
+++ b/hgext/fsmonitor/watchmanclient.py
@@ -82,9 +82,11 @@
         try:
             if self._watchmanclient is None:
                 self._firsttime = False
+                watchman_exe = self._ui.configpath('fsmonitor', 'watchman_exe')
                 self._watchmanclient = pywatchman.client(
                     timeout=self._timeout,
-                    useImmutableBser=True)
+                    useImmutableBser=True,
+                    watchman_exe=watchman_exe)
             return self._watchmanclient.query(*watchmanargs)
         except pywatchman.CommandError as ex:
             if 'unable to resolve root' in ex.msg:
diff --git a/hgext/fsmonitor/pywatchman/__init__.py b/hgext/fsmonitor/pywatchman/__init__.py
--- a/hgext/fsmonitor/pywatchman/__init__.py
+++ b/hgext/fsmonitor/pywatchman/__init__.py
@@ -317,7 +317,7 @@
     """ local unix domain socket transport """
     sock = None
 
-    def __init__(self, sockpath, timeout):
+    def __init__(self, sockpath, timeout, watchman_exe):
         self.sockpath = sockpath
         self.timeout = timeout
 
@@ -397,7 +397,7 @@
 class WindowsNamedPipeTransport(Transport):
     """ connect to a named pipe """
 
-    def __init__(self, sockpath, timeout):
+    def __init__(self, sockpath, timeout, watchman_exe):
         self.sockpath = sockpath
         self.timeout = int(math.ceil(timeout * 1000))
         self._iobuf = None
@@ -563,9 +563,10 @@
     proc = None
     closed = True
 
-    def __init__(self, sockpath, timeout):
+    def __init__(self, sockpath, timeout, watchman_exe):
         self.sockpath = sockpath
         self.timeout = timeout
+        self.watchman_exe = watchman_exe
 
     def close(self):
         if self.proc:
@@ -579,7 +580,7 @@
         if self.proc:
             return self.proc
         args = [
-            'watchman',
+            self.watchman_exe,
             '--sockname={0}'.format(self.sockpath),
             '--logfile=/BOGUS',
             '--statefile=/BOGUS',
@@ -756,17 +757,20 @@
     unilateral = ['log', 'subscription']
     tport = None
     useImmutableBser = None
+    watchman_exe = None
 
     def __init__(self,
                  sockpath=None,
                  timeout=1.0,
                  transport=None,
                  sendEncoding=None,
                  recvEncoding=None,
-                 useImmutableBser=False):
+                 useImmutableBser=False,
+                 watchman_exe=None):
         self.sockpath = sockpath
         self.timeout = timeout
         self.useImmutableBser = useImmutableBser
+        self.watchman_exe = watchman_exe
 
         if inspect.isclass(transport) and issubclass(transport, Transport):
             self.transport = transport
@@ -817,7 +821,7 @@
         if path:
             return path
 
-        cmd = ['watchman', '--output-encoding=bser', 'get-sockname']
+        cmd = [self.watchman_exe, '--output-encoding=bser', 'get-sockname']
         try:
             args = dict(stdout=subprocess.PIPE,
                         stderr=subprocess.PIPE,
@@ -858,7 +862,7 @@
         if self.sockpath is None:
             self.sockpath = self._resolvesockname()
 
-        self.tport = self.transport(self.sockpath, self.timeout)
+        self.tport = self.transport(self.sockpath, self.timeout, self.watchman_exe)
         self.sendConn = self.sendCodec(self.tport)
         self.recvConn = self.recvCodec(self.tport)
 
diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
--- a/hgext/fsmonitor/__init__.py
+++ b/hgext/fsmonitor/__init__.py
@@ -161,6 +161,9 @@
 configitem('fsmonitor', 'blacklistusers',
     default=list,
 )
+configitem('fsmonitor', 'watchman_exe',
+    default='watchman',
+)
 configitem('fsmonitor', 'verbose',
     default=True,
 )



To: lothiraldan, #hg-reviewers
Cc: indygreg, mjpieters, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

pulkit (Pulkit Goyal)
In reply to this post by pulkit (Pulkit Goyal)
lothiraldan abandoned this revision.
lothiraldan added a comment.


  Resent as https://phab.mercurial-scm.org/D5954

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5589

To: lothiraldan, #hg-reviewers
Cc: indygreg, mjpieters, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel