[PATCH] emacs: Use socket instead of port.

[PATCH] emacs: Use socket instead of port.

[Alex Kost]

[-- Attachment #1: Type: text/plain, Size: 800 bytes --]

Currently, if you try to run a repl command (for example, "M-x
guix-installed-packages") in a second Emacs instance, you'll get an
unfriendly error.  That's because `guix-default-port' is busy already
(by another Guix REPL), so you either have to change it manually or use
(setq guix-use-guile-server nil).  So with the attached patch, a socket
file with a generated name will be used instead of a port, which allows
you to run as many Emacs instances with Guix REPLs as you want.

Many thanks to Florian for the great idea!

There is one small thing though: Guile does not remove socket file after
exiting from "guile --listen=/tmp/foo" so these dead sockets will stay in
/tmp dir.  As there is no `comint-exit-hook' or alike, I don't see how a
socket file can be removed after the REPL is killed.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-emacs-Use-socket-instead-of-port.patch --]
[-- Type: text/x-patch, Size: 5506 bytes --]

From d12c0e0909491b5522cf08015c8dbd684d526d51 Mon Sep 17 00:00:00 2001
From: Alex Kost <alezost@gmail.com>
Date: Sat, 12 Dec 2015 11:23:03 +0300
Subject: [PATCH] emacs: Use socket instead of port.

Suggested by Florian Paul Schmidt.

* emacs/guix-backend.el (guix-default-port): Remove.
  (guix-repl-socket-file-name-function, guix-repl-current-socket): New
  variables.
  (guix-repl-socket-file-name): New procedure.
  (guix-get-guile-program): Take socket as an optional argument.
  (guix-start-repl-maybe): Adjust accordingly.
---
 emacs/guix-backend.el | 60 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/emacs/guix-backend.el b/emacs/guix-backend.el
index 82383e4..9612193 100644
--- a/emacs/guix-backend.el
+++ b/emacs/guix-backend.el
@@ -36,18 +36,13 @@
 ;; running code in the REPL (see
 ;; <https://github.com/jaor/geiser/issues/28>).
 ;;
-;; If you need to use "guix.el" in another Emacs (i.e. when there is
-;; a runnig "guile --listen..." REPL somewhere), you can either change
-;; `guix-default-port' in that Emacs instance or set
-;; `guix-use-guile-server' to t.
-;;
 ;; Guix REPLs (unlike the usual Geiser REPLs) are not added to
 ;; `geiser-repl--repls' variable, and thus cannot be used for evaluating
 ;; while editing scm-files.  The only purpose of Guix REPLs is to be an
 ;; intermediate between "Guix/Guile level" and "Emacs interface level".
 ;; That being said you can still want to use a Guix REPL while hacking
-;; auxiliary scheme-files for "guix.el".  You can just use "M-x
-;; connect-to-guile" (connect to "localhost" and `guix-default-port') to
+;; auxiliary scheme-files for "guix.el".  You can just use
+;; `geiser-connect-local' command with `guix-repl-current-socket' to
 ;; have a usual Geiser REPL with all stuff defined by "guix.el" package.
 
 ;;; Code:
@@ -98,11 +93,17 @@ REPL while some packages are being installed/removed in the main REPL."
   :type 'boolean
   :group 'guix-repl)
 
-(defcustom guix-default-port 37246
-  "Default port used if `guix-use-guile-server' is non-nil."
-  :type 'integer
+(defcustom guix-repl-socket-file-name-function
+  #'guix-repl-socket-file-name
+  "Function used to define a socket file name used by Guix REPL.
+The function is called without arguments."
+  :type '(choice (function-item guix-repl-socket-file-name)
+                 (function :tag "Other function"))
   :group 'guix-repl)
 
+(defvar guix-repl-current-socket nil
+  "Name of a socket file used by the current Guix REPL.")
+
 (defvar guix-repl-buffer nil
   "Main Geiser REPL buffer used for communicating with Guix.
 This REPL is used for processing package actions and for
@@ -139,17 +140,20 @@ See `guix-eval-in-repl' for details.")
   "Message telling about successful Guix operation."
   (message "Guix operation has been performed."))
 
-(defun guix-get-guile-program (&optional internal)
+(defun guix-get-guile-program (&optional socket)
   "Return a value suitable for `geiser-guile-binary'."
-  (if (or internal
-          (not guix-use-guile-server))
+  (if (null socket)
       guix-guile-program
     (append (if (listp guix-guile-program)
                 guix-guile-program
               (list guix-guile-program))
-            ;; Guile understands "--listen=..." but not "--listen ..."
-            (list (concat "--listen="
-                          (number-to-string guix-default-port))))))
+            (list (concat "--listen=" socket)))))
+
+(defun guix-repl-socket-file-name ()
+  "Return a name of a socket file used by Guix REPL."
+  (make-temp-name
+   (concat (file-name-as-directory temporary-file-directory)
+           "guix-repl-")))
 
 (defun guix-start-process-maybe (&optional start-msg end-msg)
   "Start Geiser REPL configured for Guix if needed.
@@ -176,19 +180,21 @@ display messages."
                  (get-buffer-process repl))
       (and start-msg (message start-msg))
       (setq guix-repl-operation-p nil)
-      (let ((geiser-guile-binary (guix-get-guile-program internal))
-            (geiser-guile-init-file (or internal guix-helper-file))
+      (unless internal
+        (if guix-repl-current-socket
+            ;; Guile leaves socket file after exit, so remove it if it
+            ;; exists (after the REPL restart).
+            (when (file-exists-p guix-repl-current-socket)
+              (delete-file guix-repl-current-socket))
+          (setq guix-repl-current-socket
+                (funcall guix-repl-socket-file-name-function))))
+      (let ((geiser-guile-binary (guix-get-guile-program
+                                  (unless internal
+                                    guix-repl-current-socket)))
+            (geiser-guile-init-file (unless internal guix-helper-file))
             (repl (get-buffer-create
                    (guix-get-repl-buffer-name internal))))
-        (condition-case err
-            (guix-start-repl repl
-                             (and internal
-                                  (geiser-repl--read-address
-                                   "localhost" guix-default-port)))
-          (text-read-only
-           (error (concat "Couldn't start Guix REPL.  Perhaps the port %s is busy.\n"
-                          "See buffer '%s' for details")
-                  guix-default-port (buffer-name repl))))
+        (guix-start-repl repl (and internal guix-repl-current-socket))
         (set repl-var repl)
         (and end-msg (message end-msg))
         (unless internal
-- 
2.6.3

Re: [PATCH] emacs: Use socket instead of port.

[Florian Paul Schmidt]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 12.12.2015 09:50, Alex Kost wrote:
> Currently, if you try to run a repl command (for example, "M-x 
> guix-installed-packages") in a second Emacs instance, you'll get
> an unfriendly error.  That's because `guix-default-port' is busy
> already (by another Guix REPL), so you either have to change it
> manually or use (setq guix-use-guile-server nil).  So with the
> attached patch, a socket file with a generated name will be used
> instead of a port, which allows you to run as many Emacs instances
> with Guix REPLs as you want.
> 
> Many thanks to Florian for the great idea!
> 
> There is one small thing though: Guile does not remove socket file
> after exiting from "guile --listen=/tmp/foo" so these dead sockets
> will stay in /tmp dir.  As there is no `comint-exit-hook' or alike,
> I don't see how a socket file can be removed after the REPL is
> killed.
> 

Hi,

maybe wrap it into an mktemp call? Sorry, I have to run for a meeting
now, but isn't there some semantics to mktemp, that the file
"disappears" directly, but the fd is kept open? Maybe a little bash
wrapper or maybe some elisp magic do the job?

Regards,
Flo

- -- 
https://fps.io
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJWa+IgAAoJEA5f4Coltk8ZWuIH/jU3DabsQHouzdODJwoeqfOi
SiASV3GjBUYo37LnmSe9CmwL6q+IfylEZFB79Jfy88IcjIW/LedPC92KNsJOGcKZ
t/i6A58SU604BhSaBChWU2Fg1G3SK/J5/FSA7U/netc60g1YKw7OOZx46iHp+Mt0
snoCCpN1v7mfD93HyOMK9VS60IXHEuSfoh6voksVgIeinX1Poql5X8eaTj1JRMT9
XlT1ddIvlWlC/RjRMNTGesd+KCHQTsW+xfnldnh6B2w+egjUpVfZc3jSIDrRZmeh
kQDrnr6ZSlK8AO7NF6ZIcxDgxAea1+ckdWwmHCsm2ElHm4CELfrq2VEUG+/iDEg=
=DhIx
-----END PGP SIGNATURE-----

Re: [PATCH] emacs: Use socket instead of port.

[Alex Kost]

Florian Paul Schmidt (2015-12-12 12:00 +0300) wrote:

> On 12.12.2015 09:50, Alex Kost wrote:
>> Currently, if you try to run a repl command (for example, "M-x
>> guix-installed-packages") in a second Emacs instance, you'll get
>> an unfriendly error.  That's because `guix-default-port' is busy
>> already (by another Guix REPL), so you either have to change it
>> manually or use (setq guix-use-guile-server nil).  So with the
>> attached patch, a socket file with a generated name will be used
>> instead of a port, which allows you to run as many Emacs instances
>> with Guix REPLs as you want.
>>
>> Many thanks to Florian for the great idea!
>>
>> There is one small thing though: Guile does not remove socket file
>> after exiting from "guile --listen=/tmp/foo" so these dead sockets
>> will stay in /tmp dir.  As there is no `comint-exit-hook' or alike,
>> I don't see how a socket file can be removed after the REPL is
>> killed.
>>
>
> Hi,
>
> maybe wrap it into an mktemp call? Sorry, I have to run for a meeting
> now, but isn't there some semantics to mktemp, that the file
> "disappears" directly, but the fd is kept open? Maybe a little bash
> wrapper or maybe some elisp magic do the job?

Thanks, I didn't know about "mktemp", but I don't see how it can help as
it just creates a temporary file (elisp procedure `make-temp-file' does
the same).  But the problem is not to create a file, but to delete it.

After all, I think the best (actually I don't see the other ways) would
be to remove a socket file during emacs exit.

--
Alex

Re: [PATCH] emacs: Use socket instead of port.

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> Currently, if you try to run a repl command (for example, "M-x
> guix-installed-packages") in a second Emacs instance, you'll get an
> unfriendly error.  That's because `guix-default-port' is busy already
> (by another Guix REPL), so you either have to change it manually or use
> (setq guix-use-guile-server nil).  So with the attached patch, a socket
> file with a generated name will be used instead of a port, which allows
> you to run as many Emacs instances with Guix REPLs as you want.
>
> Many thanks to Florian for the great idea!

That’s a good idea, indeed.

> There is one small thing though: Guile does not remove socket file after
> exiting from "guile --listen=/tmp/foo" so these dead sockets will stay in
> /tmp dir.  As there is no `comint-exit-hook' or alike, I don't see how a
> socket file can be removed after the REPL is killed.

You could maybe arrange to have Guile run something like:

  (dynamic-wind
    (const #t)
    (lambda ()
      (start-repl))  ;start a new REPL
    (lambda ()
      (false-if-exception (delete-file "/tmp/foo"))))

Not perfect, but I don’t have a better idea.

It may be good to fix it upfront though.  WDYT?

> From d12c0e0909491b5522cf08015c8dbd684d526d51 Mon Sep 17 00:00:00 2001
> From: Alex Kost <alezost@gmail.com>
> Date: Sat, 12 Dec 2015 11:23:03 +0300
> Subject: [PATCH] emacs: Use socket instead of port.
>
> Suggested by Florian Paul Schmidt.
>
> * emacs/guix-backend.el (guix-default-port): Remove.
>   (guix-repl-socket-file-name-function, guix-repl-current-socket): New
>   variables.
>   (guix-repl-socket-file-name): New procedure.
>   (guix-get-guile-program): Take socket as an optional argument.
>   (guix-start-repl-maybe): Adjust accordingly.

Otherwise LGTM.

Thanks,
Ludo’.

Re: [PATCH] emacs: Use socket instead of port.

[Alex Kost]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

Ludovic Courtès (2015-12-12 21:07 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
>> There is one small thing though: Guile does not remove socket file after
>> exiting from "guile --listen=/tmp/foo" so these dead sockets will stay in
>> /tmp dir.  As there is no `comint-exit-hook' or alike, I don't see how a
>> socket file can be removed after the REPL is killed.
>
> You could maybe arrange to have Guile run something like:
>
>   (dynamic-wind
>     (const #t)
>     (lambda ()
>       (start-repl))  ;start a new REPL
>     (lambda ()
>       (false-if-exception (delete-file "/tmp/foo"))))
>
> Not perfect, but I don’t have a better idea.

Hm, but it doesn't delete the socket after the quit, right?  At least,
it doesn't when I tried "guile -l /tmp/repl-server.scm" with the
following "/tmp/repl-server.scm":


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: repl-server.scm --]
[-- Type: text/x-scheme, Size: 299 bytes --]

(use-modules (system repl server))

(let ((socket "/tmp/s1"))
  (call-with-new-thread
   (lambda ()
     (dynamic-wind
       (const #t)
       (lambda ()
         (run-server (make-unix-domain-server-socket #:path socket)))
       (lambda ()
         (false-if-exception (delete-file socket)))))))

[-- Attachment #3: Type: text/plain, Size: 680 bytes --]


And after ",q" the file "/tmp/s1" stayed.

> It may be good to fix it upfront though.  WDYT?

Sorry, I don't understand.  Do you mean to fix it "upstream" — i.e., in
guile?  If so, I agree as it sounds natural to me if guile will delete a
socket file it created.

Anyway, as I mentioned in the reply to Florian, I think the best would
be to add a procedure for deleting socket file to `kill-emacs-hook' (as
in the attached updated patch).  This should prevent appearing dead
sockets.

P.S.  Maybe it's a wrong impression, but it looks (to me) that
connecting to a socket is significantly faster than using a port (so all
this Guix REPL stuff starts faster).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-emacs-Use-socket-instead-of-port.patch --]
[-- Type: text/x-patch, Size: 5811 bytes --]

From d6903c7c115809cf88f892e78785d920ff80184d Mon Sep 17 00:00:00 2001
From: Alex Kost <alezost@gmail.com>
Date: Sat, 12 Dec 2015 11:23:03 +0300
Subject: [PATCH] emacs: Use socket instead of port.

Suggested by Florian Paul Schmidt.

* emacs/guix-backend.el (guix-default-port): Remove.
  (guix-repl-socket-file-name-function, guix-repl-current-socket): New
  variables.
  (guix-repl-socket-file-name, guix-repl-delete-socket-maybe): New
  procedures.
  (guix-get-guile-program): Take socket as an optional argument.
  (guix-start-repl-maybe): Adjust accordingly.
---
 emacs/guix-backend.el | 68 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/emacs/guix-backend.el b/emacs/guix-backend.el
index 82383e4..0736f85 100644
--- a/emacs/guix-backend.el
+++ b/emacs/guix-backend.el
@@ -36,18 +36,13 @@
 ;; running code in the REPL (see
 ;; <https://github.com/jaor/geiser/issues/28>).
 ;;
-;; If you need to use "guix.el" in another Emacs (i.e. when there is
-;; a runnig "guile --listen..." REPL somewhere), you can either change
-;; `guix-default-port' in that Emacs instance or set
-;; `guix-use-guile-server' to t.
-;;
 ;; Guix REPLs (unlike the usual Geiser REPLs) are not added to
 ;; `geiser-repl--repls' variable, and thus cannot be used for evaluating
 ;; while editing scm-files.  The only purpose of Guix REPLs is to be an
 ;; intermediate between "Guix/Guile level" and "Emacs interface level".
 ;; That being said you can still want to use a Guix REPL while hacking
-;; auxiliary scheme-files for "guix.el".  You can just use "M-x
-;; connect-to-guile" (connect to "localhost" and `guix-default-port') to
+;; auxiliary scheme-files for "guix.el".  You can just use
+;; `geiser-connect-local' command with `guix-repl-current-socket' to
 ;; have a usual Geiser REPL with all stuff defined by "guix.el" package.
 
 ;;; Code:
@@ -98,11 +93,17 @@ REPL while some packages are being installed/removed in the main REPL."
   :type 'boolean
   :group 'guix-repl)
 
-(defcustom guix-default-port 37246
-  "Default port used if `guix-use-guile-server' is non-nil."
-  :type 'integer
+(defcustom guix-repl-socket-file-name-function
+  #'guix-repl-socket-file-name
+  "Function used to define a socket file name used by Guix REPL.
+The function is called without arguments."
+  :type '(choice (function-item guix-repl-socket-file-name)
+                 (function :tag "Other function"))
   :group 'guix-repl)
 
+(defvar guix-repl-current-socket nil
+  "Name of a socket file used by the current Guix REPL.")
+
 (defvar guix-repl-buffer nil
   "Main Geiser REPL buffer used for communicating with Guix.
 This REPL is used for processing package actions and for
@@ -139,17 +140,28 @@ See `guix-eval-in-repl' for details.")
   "Message telling about successful Guix operation."
   (message "Guix operation has been performed."))
 
-(defun guix-get-guile-program (&optional internal)
+(defun guix-get-guile-program (&optional socket)
   "Return a value suitable for `geiser-guile-binary'."
-  (if (or internal
-          (not guix-use-guile-server))
+  (if (null socket)
       guix-guile-program
     (append (if (listp guix-guile-program)
                 guix-guile-program
               (list guix-guile-program))
-            ;; Guile understands "--listen=..." but not "--listen ..."
-            (list (concat "--listen="
-                          (number-to-string guix-default-port))))))
+            (list (concat "--listen=" socket)))))
+
+(defun guix-repl-socket-file-name ()
+  "Return a name of a socket file used by Guix REPL."
+  (make-temp-name
+   (concat (file-name-as-directory temporary-file-directory)
+           "guix-repl-")))
+
+(defun guix-repl-delete-socket-maybe ()
+  "Delete `guix-repl-current-socket' file if it exists."
+  (and guix-repl-current-socket
+       (file-exists-p guix-repl-current-socket)
+       (delete-file guix-repl-current-socket)))
+
+(add-hook 'kill-emacs-hook 'guix-repl-delete-socket-maybe)
 
 (defun guix-start-process-maybe (&optional start-msg end-msg)
   "Start Geiser REPL configured for Guix if needed.
@@ -176,19 +188,21 @@ display messages."
                  (get-buffer-process repl))
       (and start-msg (message start-msg))
       (setq guix-repl-operation-p nil)
-      (let ((geiser-guile-binary (guix-get-guile-program internal))
-            (geiser-guile-init-file (or internal guix-helper-file))
+      (unless internal
+        ;; Guile leaves socket file after exit, so remove it if it
+        ;; exists (after the REPL restart).
+        (guix-repl-delete-socket-maybe)
+        (setq guix-repl-current-socket
+              (and guix-use-guile-server
+                   (or guix-repl-current-socket
+                       (funcall guix-repl-socket-file-name-function)))))
+      (let ((geiser-guile-binary (guix-get-guile-program
+                                  (unless internal
+                                    guix-repl-current-socket)))
+            (geiser-guile-init-file (unless internal guix-helper-file))
             (repl (get-buffer-create
                    (guix-get-repl-buffer-name internal))))
-        (condition-case err
-            (guix-start-repl repl
-                             (and internal
-                                  (geiser-repl--read-address
-                                   "localhost" guix-default-port)))
-          (text-read-only
-           (error (concat "Couldn't start Guix REPL.  Perhaps the port %s is busy.\n"
-                          "See buffer '%s' for details")
-                  guix-default-port (buffer-name repl))))
+        (guix-start-repl repl (and internal guix-repl-current-socket))
         (set repl-var repl)
         (and end-msg (message end-msg))
         (unless internal
-- 
2.6.3

Re: [PATCH] emacs: Use socket instead of port.

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> Hm, but it doesn't delete the socket after the quit, right?  At least,
> it doesn't when I tried "guile -l /tmp/repl-server.scm" with the
> following "/tmp/repl-server.scm":
>
>
> (use-modules (system repl server))
>
> (let ((socket "/tmp/s1"))
>   (call-with-new-thread
>    (lambda ()
>      (dynamic-wind
>        (const #t)
>        (lambda ()
>          (run-server (make-unix-domain-server-socket #:path socket)))
>        (lambda ()
>          (false-if-exception (delete-file socket)))))))
>
>
> And after ",q" the file "/tmp/s1" stayed.

Right, in that case, the thread is not properly terminated so it doesn’t
have a chance to run its dynwind handler.

Hmm, that sucks.

>> It may be good to fix it upfront though.  WDYT?
>
> Sorry, I don't understand.

I meant that we should probably fix this lingering socket problem before
committing the change.

> Anyway, as I mentioned in the reply to Florian, I think the best would
> be to add a procedure for deleting socket file to `kill-emacs-hook' (as
> in the attached updated patch).  This should prevent appearing dead
> sockets.

Indeed, sounds even better!

> P.S.  Maybe it's a wrong impression, but it looks (to me) that
> connecting to a socket is significantly faster than using a port (so all
> this Guix REPL stuff starts faster).

Hmm I don’t think it should be noticeably faster.  But it’s definitely
better because socket nodes are not a scarce/global resource (unlike TCP
ports), and they are integrated with the Unix file permission model.

> From d6903c7c115809cf88f892e78785d920ff80184d Mon Sep 17 00:00:00 2001
> From: Alex Kost <alezost@gmail.com>
> Date: Sat, 12 Dec 2015 11:23:03 +0300
> Subject: [PATCH] emacs: Use socket instead of port.
>
> Suggested by Florian Paul Schmidt.
>
> * emacs/guix-backend.el (guix-default-port): Remove.
>   (guix-repl-socket-file-name-function, guix-repl-current-socket): New
>   variables.
>   (guix-repl-socket-file-name, guix-repl-delete-socket-maybe): New
>   procedures.
>   (guix-get-guile-program): Take socket as an optional argument.
>   (guix-start-repl-maybe): Adjust accordingly.

LGTM, thanks!

Ludo’.

Re: [PATCH] emacs: Use socket instead of port.

[Alex Kost]

Ludovic Courtès (2015-12-15 14:13 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
[...]
>>> It may be good to fix it upfront though.  WDYT?
>>
>> Sorry, I don't understand.
>
> I meant that we should probably fix this lingering socket problem before
> committing the change.

Ah, OK, so I think the current solution is "good enough", so I'm
committing it (let's see in practice if there are any downsides).

>> Anyway, as I mentioned in the reply to Florian, I think the best would
>> be to add a procedure for deleting socket file to `kill-emacs-hook' (as
>> in the attached updated patch).  This should prevent appearing dead
>> sockets.
>
> Indeed, sounds even better!
>
>> P.S.  Maybe it's a wrong impression, but it looks (to me) that
>> connecting to a socket is significantly faster than using a port (so all
>> this Guix REPL stuff starts faster).
>
> Hmm I don’t think it should be noticeably faster.  But it’s definitely
> better because socket nodes are not a scarce/global resource (unlike TCP
> ports), and they are integrated with the Unix file permission model.

Thanks for explaining.

>> From d6903c7c115809cf88f892e78785d920ff80184d Mon Sep 17 00:00:00 2001
>> From: Alex Kost <alezost@gmail.com>
>> Date: Sat, 12 Dec 2015 11:23:03 +0300
>> Subject: [PATCH] emacs: Use socket instead of port.
>>
>> Suggested by Florian Paul Schmidt.
>>
>> * emacs/guix-backend.el (guix-default-port): Remove.
>>   (guix-repl-socket-file-name-function, guix-repl-current-socket): New
>>   variables.
>>   (guix-repl-socket-file-name, guix-repl-delete-socket-maybe): New
>>   procedures.
>>   (guix-get-guile-program): Take socket as an optional argument.
>>   (guix-start-repl-maybe): Adjust accordingly.
>
> LGTM, thanks!

Pushed, thanks!

-- 
Alex