Possible bug in tests with emacs 23.2.1 (debian stable)

Possible bug in tests with emacs 23.2.1 (debian stable)

[Mark Walters]

Hello

When I try and run the tests on my (fairly standard) debian stable
system they hang after "PASS   Search message: json, utf-8". This is
with latest git (07768fb1bb50e) and emacs 23.2.1.

As far as I can see emacs is not exiting when sent the "(kill-emacs)"
command from test-lib.sh (line 931 called from line 869). It seems
that this version of emacs prompts before exit asking "The current
server still has clients; delete them? (yes or no) ".

This seems to have been "fixed" (i.e. emacs does not ask for a prompt
in emacs revision 100150
http://bzr.savannah.gnu.org/lh/emacs/emacs-23/revision/100150).

If I change test-lib.sh line 869 to send "(setq kill-emacs-hook 'nil)
(kill-emacs)" instead of just (kill-emacs) then the tests work
correctly (*). But I definitely don't know enough emacs to know if
this is a sensible solution.

Note I get exactly the same behaviour when trying to build the debian
package from squeeze backports so it could be a bug in my setup in
which case my apologies for the noise!

Best wishes

Mark

(*) there are a small number (5) of unrelated test failures  that I
have not yet chased down

Re: Possible bug in tests with emacs 23.2.1 (debian stable)

[Tomi Ollila]

On Sun, 1 Jan 2012 10:05:59 +0000, Mark Walters <markwalters1009@gmail.com> wrote:
> Hello
> 
> When I try and run the tests on my (fairly standard) debian stable
> system they hang after "PASS   Search message: json, utf-8". This is
> with latest git (07768fb1bb50e) and emacs 23.2.1.
> 
> As far as I can see emacs is not exiting when sent the "(kill-emacs)"
> command from test-lib.sh (line 931 called from line 869). It seems
> that this version of emacs prompts before exit asking "The current
> server still has clients; delete them? (yes or no) ".
> 
> This seems to have been "fixed" (i.e. emacs does not ask for a prompt
> in emacs revision 100150
> http://bzr.savannah.gnu.org/lh/emacs/emacs-23/revision/100150).
> 
> If I change test-lib.sh line 869 to send "(setq kill-emacs-hook 'nil)
> (kill-emacs)" instead of just (kill-emacs) then the tests work
> correctly (*). But I definitely don't know enough emacs to know if
> this is a sensible solution.

Interesting case... what if we want to use this hook from notmuch in
the future -- and, naturally, provide a test for this functionality.

Perhaps... if we wanted to support workaround for this particular bug,
the (setq kill-emacs-hook nil) could be put after (server-start) in
test-lib.sh...

.. but, I checked code and tested that. In case that hook is disabled,
server shutdown cleanup is not done (at least dangling server socket
file is left behind)...

... so my "vote" is that we don't attempt to work around this bug. Do
we have KNOWN ISSUES file where this kind of things can be documented ?

> Note I get exactly the same behaviour when trying to build the debian
> package from squeeze backports so it could be a bug in my setup in
> which case my apologies for the noise!

It is not a bug in your setup; I tested it with emacs 23.2.1; running
emacs from command line, executing (server-start) there and then executed
emacsclient --eval '(kill-emacs)' in another terminal window.

> Best wishes
> 
> Mark

Tomi

Re: Possible bug in tests with emacs 23.2.1 (debian stable)

[Tomi Ollila]

On Mon, 02 Jan 2012 16:55:45 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Sun, 1 Jan 2012 10:05:59 +0000, Mark Walters <markwalters1009@gmail.com> wrote:

[ ... ]

> > As far as I can see emacs is not exiting when sent the "(kill-emacs)"
> > command from test-lib.sh (line 931 called from line 869). It seems
> > that this version of emacs prompts before exit asking "The current
> > server still has clients; delete them? (yes or no) ".
> > 
> > This seems to have been "fixed" (i.e. emacs does not ask for a prompt
> > in emacs revision 100150
> > http://bzr.savannah.gnu.org/lh/emacs/emacs-23/revision/100150).
> > 
> > If I change test-lib.sh line 869 to send "(setq kill-emacs-hook 'nil)
> > (kill-emacs)" instead of just (kill-emacs) then the tests work
> > correctly (*). But I definitely don't know enough emacs to know if
> > this is a sensible solution.

[ ... ]

> ... so my "vote" is that we don't attempt to work around this bug. Do
> we have KNOWN ISSUES file where this kind of things can be documented ?
> 
> > Note I get exactly the same behaviour when trying to build the debian
> > package from squeeze backports so it could be a bug in my setup in
> > which case my apologies for the noise!
> 
> It is not a bug in your setup; I tested it with emacs 23.2.1; running
> emacs from command line, executing (server-start) there and then executed
> emacsclient --eval '(kill-emacs)' in another terminal window.

I did some more testing; doing

emacsclient --eval '(defun yes-or-no-p (prompt) t)' --eval '(kill-emacs)'

Will make emacs 23.2.1 exit also, so IMO this "workaround" could be
used to "fix" the problem. 

> > Best wishes
> > 
> > Mark
> 
> Tomi

Tomi

Re: Possible bug in tests with emacs 23.2.1 (debian stable)

[Mark Walters]

On Tue, 03 Jan 2012 15:07:04 +0200, Tomi Ollila <tomi.ollila@nixu.com> wrote:

> I did some more testing; doing
> 
> emacsclient --eval '(defun yes-or-no-p (prompt) t)' --eval '(kill-emacs)'
> 
> Will make emacs 23.2.1 exit also, so IMO this "workaround" could be
> used to "fix" the problem. 

I can confirm that the patch below (using this suggestion) fixes the
tests for me on emacs 23.2.1 and, on a different machine with emacs
23.3.1 they work just as before.

Best wishes

Mark

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 82767c0..218ce91 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -866,7 +866,7 @@ test_done () {
 
 	echo
 
-	[ -n "$EMACS_SERVER" ] && test_emacs '(kill-emacs)'
+	[ -n "$EMACS_SERVER" ] && test_emacs '(defun yes-or-no-p (prompt) t) (kill-emacs)'
 
 	if [ "$test_failure" = "0" ]; then
 	    if [ "$test_broken" = "0" ]; then	    

[PATCH] make (kill-emacs) from emacsclient work with emacs 23.(1|2)

[Tomi Ollila]

emacsclient --eval '(kill-emacs)' doesn't work without interactive
user input. By removing the hook which asks user input makes things
work well enough in our test cases.
---
 test/test-lib.el |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/test/test-lib.el b/test/test-lib.el
index 3b817c3..c52854e 100644
--- a/test/test-lib.el
+++ b/test/test-lib.el
@@ -26,6 +26,19 @@
 ;; `read' call.
 (setq read-file-name-function (lambda (&rest _) (read)))
 
+;; Work around a bug in emacs 23.1 and emacs 23.2 which prevents
+;; noninteractive (kill-emacs) from emacsclient.
+(when (and (= emacs-major-version 23) (< emacs-minor-version 3))
+  (require 'server)
+  (fset 'server-start-real (symbol-function 'server-start))
+  (defun server-start (&optional leave-dead)
+    (interactive "P")
+    (let ((hc (length kill-emacs-hook)))
+      (unwind-protect
+	  (server-start-real leave-dead)
+	(if (> (length kill-emacs-hook) hc)
+	    (setq kill-emacs-hook (cdr kill-emacs-hook)))))))
+
 (defun notmuch-test-wait ()
   "Wait for process completion."
   (while (get-buffer-process (current-buffer))
-- 
1.7.7.3

Re: [PATCH] make (kill-emacs) from emacsclient work with emacs 23.(1|2)

[Austin Clements]

This is important to fix, but this solution seems needlessly
roundabout.  What about using an after-advice and simply delq'ing
whatever the offending hook is?  That wouldn't even need a version
check.

Quoth Tomi Ollila on Jan 11 at  4:49 pm:
> emacsclient --eval '(kill-emacs)' doesn't work without interactive
> user input. By removing the hook which asks user input makes things
> work well enough in our test cases.
> ---
>  test/test-lib.el |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/test/test-lib.el b/test/test-lib.el
> index 3b817c3..c52854e 100644
> --- a/test/test-lib.el
> +++ b/test/test-lib.el
> @@ -26,6 +26,19 @@
>  ;; `read' call.
>  (setq read-file-name-function (lambda (&rest _) (read)))
>  
> +;; Work around a bug in emacs 23.1 and emacs 23.2 which prevents
> +;; noninteractive (kill-emacs) from emacsclient.
> +(when (and (= emacs-major-version 23) (< emacs-minor-version 3))
> +  (require 'server)
> +  (fset 'server-start-real (symbol-function 'server-start))
> +  (defun server-start (&optional leave-dead)
> +    (interactive "P")
> +    (let ((hc (length kill-emacs-hook)))
> +      (unwind-protect
> +	  (server-start-real leave-dead)
> +	(if (> (length kill-emacs-hook) hc)
> +	    (setq kill-emacs-hook (cdr kill-emacs-hook)))))))
> +
>  (defun notmuch-test-wait ()
>    "Wait for process completion."
>    (while (get-buffer-process (current-buffer))

Re: [PATCH] make (kill-emacs) from emacsclient work with emacs 23.(1|2)

[Tomi Ollila]

On Wed, 11 Jan 2012 23:13:44 -0500, Austin Clements <amdragon@mit.edu> wrote:
> This is important to fix, but this solution seems needlessly
> roundabout.  What about using an after-advice and simply delq'ing
> whatever the offending hook is?  That wouldn't even need a version
> check.

delq could work -- thanks for the idea -- but removing the hook was
wrong shot from my part (the hook removes the cleanup I mentioned in
one of my previous mails).

To minimise behaviour changes (to zero in emacs 23.3+) to minimal
in 23.(1|2) my next suggestion goes along lines:

;; Work around a bug in emacs 23.1 and emacs 23.2 which prevents
;; noninteractive (kill-emacs) from emacsclient.
(when (and (= emacs-major-version 23) (< emacs-minor-version 3))
  (defadvice kill-emacs (before disable-yes-or-no-p)
    "Disable yes-or-no-p before executing kill-emacs"
    (defun yes-or-no-p (prompt) t))
  (ad-activate 'kill-emacs))

Now just (accidental) additions which use yes-or-no-p 
into kill-emacs-hook are not noticed in emacs 23.1 & 23.2.


Tomi

Re: [PATCH] make (kill-emacs) from emacsclient work with emacs 23.(1|2)

[Austin Clements]

Quoth Tomi Ollila on Jan 12 at 10:51 am:
> On Wed, 11 Jan 2012 23:13:44 -0500, Austin Clements <amdragon@mit.edu> wrote:
> > This is important to fix, but this solution seems needlessly
> > roundabout.  What about using an after-advice and simply delq'ing
> > whatever the offending hook is?  That wouldn't even need a version
> > check.
> 
> delq could work -- thanks for the idea -- but removing the hook was
> wrong shot from my part (the hook removes the cleanup I mentioned in
> one of my previous mails).

Ah, interesting.

> To minimise behaviour changes (to zero in emacs 23.3+) to minimal
> in 23.(1|2) my next suggestion goes along lines:
> 
> ;; Work around a bug in emacs 23.1 and emacs 23.2 which prevents
> ;; noninteractive (kill-emacs) from emacsclient.
> (when (and (= emacs-major-version 23) (< emacs-minor-version 3))
>   (defadvice kill-emacs (before disable-yes-or-no-p)
>     "Disable yes-or-no-p before executing kill-emacs"
>     (defun yes-or-no-p (prompt) t))
>   (ad-activate 'kill-emacs))

This seems reasonable.  You could shorten it a bit by changing
  (before disable-yes-or-no-p)
to
  (before disable-yes-or-no-p activate)
rather than calling ad-activate, but that's just a nit.

> Now just (accidental) additions which use yes-or-no-p 
> into kill-emacs-hook are not noticed in emacs 23.1 & 23.2.
> 
> 
> Tomi

[PATCH] test: make (kill-emacs) from emacsclient work with emacs 23.(1|2)

[Tomi Ollila]

emacsclient --eval '(kill-emacs)' makes emacs versions 23.1
and 23.2 ask user input from running emacs. Redefining
yes-or-no-p function when kill-emacs is executed for these
emacs versions in test-lib.el avoids this test problem.
---
Thanks Austin for your comments. Including 'activate' into defadvice
made it possible to use if (well, without progn...) instead of when
which makes this also marginally faster (as we're not byte-compiling
this).

 test/test-lib.el |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/test/test-lib.el b/test/test-lib.el
index 3b817c3..59c5868 100644
--- a/test/test-lib.el
+++ b/test/test-lib.el
@@ -26,6 +26,13 @@
 ;; `read' call.
 (setq read-file-name-function (lambda (&rest _) (read)))
 
+;; Work around a bug in emacs 23.1 and emacs 23.2 which prevents
+;; noninteractive (kill-emacs) from emacsclient.
+(if (and (= emacs-major-version 23) (< emacs-minor-version 3))
+  (defadvice kill-emacs (before disable-yes-or-no-p activate)
+    "Disable yes-or-no-p before executing kill-emacs"
+    (defun yes-or-no-p (prompt) t)))
+
 (defun notmuch-test-wait ()
   "Wait for process completion."
   (while (get-buffer-process (current-buffer))
-- 
1.7.7.3

Re: [PATCH] test: make (kill-emacs) from emacsclient work with emacs 23.(1|2)

[Austin Clements]

LGTM.

Quoth Tomi Ollila on Jan 13 at 10:17 am:
> emacsclient --eval '(kill-emacs)' makes emacs versions 23.1
> and 23.2 ask user input from running emacs. Redefining
> yes-or-no-p function when kill-emacs is executed for these
> emacs versions in test-lib.el avoids this test problem.
> ---
> Thanks Austin for your comments. Including 'activate' into defadvice
> made it possible to use if (well, without progn...) instead of when
> which makes this also marginally faster (as we're not byte-compiling
> this).
> 
>  test/test-lib.el |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/test/test-lib.el b/test/test-lib.el
> index 3b817c3..59c5868 100644
> --- a/test/test-lib.el
> +++ b/test/test-lib.el
> @@ -26,6 +26,13 @@
>  ;; `read' call.
>  (setq read-file-name-function (lambda (&rest _) (read)))
>  
> +;; Work around a bug in emacs 23.1 and emacs 23.2 which prevents
> +;; noninteractive (kill-emacs) from emacsclient.
> +(if (and (= emacs-major-version 23) (< emacs-minor-version 3))
> +  (defadvice kill-emacs (before disable-yes-or-no-p activate)
> +    "Disable yes-or-no-p before executing kill-emacs"
> +    (defun yes-or-no-p (prompt) t)))
> +
>  (defun notmuch-test-wait ()
>    "Wait for process completion."
>    (while (get-buffer-process (current-buffer))

Re: [PATCH] test: make (kill-emacs) from emacsclient work with emacs 23.(1|2)

[David Bremner]

On Fri, 13 Jan 2012 10:17:28 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> emacsclient --eval '(kill-emacs)' makes emacs versions 23.1
> and 23.2 ask user input from running emacs. Redefining
> yes-or-no-p function when kill-emacs is executed for these
> emacs versions in test-lib.el avoids this test problem.

pushed,

d