* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
@ 2022-10-29 21:33 Jim Porter
2022-10-30 6:14 ` Eli Zaretskii
0 siblings, 1 reply; 19+ messages in thread
From: Jim Porter @ 2022-10-29 21:33 UTC (permalink / raw)
To: 58877
[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]
(Note: I originally mentioned this in bug#51993, but it's only
somewhat-related to that bug. To make things easier to follow, I pulled
this part out into a separate bug.)
To see this in action:
$ emacs -Q -f server-start
$ emacsclient -c foo.txt
;; From the first (non-client) frame:
C-x 5 0 ;; delete-frame
;; From the second (client) frame:
M-x kill-emacs
;; Emacs prompts:
Error (error Attempt to delete the sole visible or iconified frame);
continue? (y or n)
Pressing "y" will properly kill Emacs, but there's no real use for the
warning in this case. It happens because 'server-force-stop'
(indirectly) tries to delete the last (client) frame before killing
Emacs, meaning that Emacs would have zero frames for a bit.
Since 'server-force-stop's docstring says, "This function is meant to be
called from `kill-emacs-hook'," it should be safe to have
'server-force-stop' avoid deleting any frames: they'll just get deleted
when Emacs is actually killed.
Attached is a patch to do this. Note that I named the new argument
"noframe" because that matches the existing code in server.el (see
'server-delete-client'). It's a bit of a misnomer though, and maybe
"keep-frames" would be better...
[-- Attachment #2: 0001-Don-t-explicitly-delete-client-frames-when-killing-E.patch --]
[-- Type: text/plain, Size: 2175 bytes --]
From 522574ce69e3c198a40b57e2e211e14562334f33 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 19 Nov 2021 20:14:33 -0800
Subject: [PATCH] Don't explicitly delete client frames when killing Emacs
anyway
This eliminates a useless error prompt when killing Emacs from a
client frame when there are no other frames.
* lisp/server.el (server-start): Add NOFRAME argument to avoid
deleting frames.
(server-force-stop): Pass 'noframe' to 'server-stop'.
---
lisp/server.el | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/lisp/server.el b/lisp/server.el
index 90d97c1538..2b7213b890 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -611,7 +611,7 @@ server-get-auth-key
(server-generate-key)))
;;;###autoload
-(defun server-start (&optional leave-dead inhibit-prompt)
+(defun server-start (&optional leave-dead inhibit-prompt noframe)
"Allow this Emacs process to be a server for client processes.
This starts a server communications subprocess through which client
\"editors\" can send your editing commands to this Emacs job.
@@ -625,6 +625,11 @@ server-start
running, ask the user for confirmation first, unless optional
argument INHIBIT-PROMPT is non-nil.
+If NOFRAME is non-nil, don't delete any existing frames
+associated with a client process. This is useful, for example,
+when killing Emacs, in which case the frames will get deleted
+anyway.
+
To force-start a server, do \\[server-force-delete] and then
\\[server-start].
@@ -683,7 +688,7 @@ server-start
(setq leave-dead t)))
;; If this Emacs already had a server, clear out associated status.
(while server-clients
- (server-delete-client (car server-clients)))
+ (server-delete-client (car server-clients) noframe))
;; Now any previous server is properly stopped.
(if leave-dead
(progn
@@ -742,7 +747,7 @@ server-start
(defun server-force-stop ()
"Kill all connections to the current server.
This function is meant to be called from `kill-emacs-hook'."
- (server-start t t))
+ (server-start t t 'noframe))
;;;###autoload
(defun server-force-delete (&optional name)
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-10-29 21:33 bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt Jim Porter
@ 2022-10-30 6:14 ` Eli Zaretskii
2022-10-30 21:14 ` Jim Porter
0 siblings, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2022-10-30 6:14 UTC (permalink / raw)
To: Jim Porter; +Cc: 58877
> Date: Sat, 29 Oct 2022 14:33:42 -0700
> From: Jim Porter <jporterbugs@gmail.com>
>
> Attached is a patch to do this. Note that I named the new argument
> "noframe" because that matches the existing code in server.el (see
> 'server-delete-client'). It's a bit of a misnomer though, and maybe
> "keep-frames" would be better...
Hmm... it doesn't look very elegant to add to server-start an extra
argument whose purpose is to affect server-delete-client. Can we do
this in some more elegant way? For example, how about making most of
the code in server-start an internal function with an additional
argument, and then have server-start and server-force-stop call that
internal function? At the very least the new argument to server-start
shouldn't be advertised, I think, since it's entirely for internal use
by us.
Thanks.
^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-10-30 6:14 ` Eli Zaretskii
@ 2022-10-30 21:14 ` Jim Porter
2022-11-22 5:06 ` Jim Porter
0 siblings, 1 reply; 19+ messages in thread
From: Jim Porter @ 2022-10-30 21:14 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 58877
[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]
On 10/29/2022 11:14 PM, Eli Zaretskii wrote:
>> Date: Sat, 29 Oct 2022 14:33:42 -0700
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> Attached is a patch to do this. Note that I named the new argument
>> "noframe" because that matches the existing code in server.el (see
>> 'server-delete-client'). It's a bit of a misnomer though, and maybe
>> "keep-frames" would be better...
>
> Hmm... it doesn't look very elegant to add to server-start an extra
> argument whose purpose is to affect server-delete-client. Can we do
> this in some more elegant way? For example, how about making most of
> the code in server-start an internal function with an additional
> argument, and then have server-start and server-force-stop call that
> internal function? At the very least the new argument to server-start
> shouldn't be advertised, I think, since it's entirely for internal use
> by us.
Thanks for taking a look. I had hesitated to make any big changes to
this code since it doesn't have regression tests, but I think the
attached patch should be more elegant while retaining the flow of the
code as much as possible. Do you know of a good way to write regression
tests for this code? I couldn't find any existing tests that start/stop
Emacs processes in a way that I could use for testing this. It seems
like we'd need to be able to start a new Emacs process and then be able
to send arbitrary key combinations to it to drive it...
Back to the patch itself though: one small functional change I made was
that I slightly changed how 'server-ensure-safe-dir' is called in
'server-start'. (I only mention this because this code looks to be
security-related, so I think it should have extra attention.)
It used to check the 'server-dir', defined as:
(if server-use-tcp server-auth-dir server-socket-dir)
Now, it checks the directory of the server *file*. The file is defined as:
(expand-file-name server-name server-dir)
This could be different if 'server-name' (a defcustom) contains a slash.
I think the new code is actually safer, since it checks the proper
directory even if a user customized 'server-name' to have a slash or
'..' or whatever. Still, I thought this change deserved a mention so
that I don't inadvertently open a security hole.
[-- Attachment #2: 0001-Don-t-explicitly-delete-client-frames-when-killing-E.patch --]
[-- Type: text/plain, Size: 8648 bytes --]
From 54ee256adaad53a38712ca194bd9e1ba8b00a397 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 19 Nov 2021 20:14:33 -0800
Subject: [PATCH] Don't explicitly delete client frames when killing Emacs
anyway
This eliminates a useless error prompt when killing Emacs from a
client frame when there are no other frames (bug#58877).
* lisp/server.el (server-running-external): New error.
(server--file-name): New function...
(server-eval-at): ... use it.
(server-start): Factor out server stopping code into...
(server-stop): ... here.
(server-force-stop): Use 'server-stop', and tell it not to delete
frames.
---
lisp/server.el | 132 +++++++++++++++++++++++++++++--------------------
1 file changed, 78 insertions(+), 54 deletions(-)
diff --git a/lisp/server.el b/lisp/server.el
index 90d97c1538..095de06cf9 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -287,6 +287,8 @@ server-socket-dir
"The directory in which to place the server socket.
If local sockets are not supported, this is nil.")
+(define-error 'server-running-external "External server running")
+
(defun server-clients-with (property value)
"Return a list of clients with PROPERTY set to VALUE."
(let (result)
@@ -610,6 +612,59 @@ server-get-auth-key
(error "The key `%s' is invalid" server-auth-key))
(server-generate-key)))
+(defsubst server--file-name ()
+ "Return the file name to use for the server socket."
+ (let ((server-dir (if server-use-tcp server-auth-dir server-socket-dir)))
+ (expand-file-name server-name server-dir)))
+
+(defun server-stop (&optional noframe)
+ "If this Emacs process has a server communication subprocess, stop it.
+If the server is running in some other Emac process (see
+`server-running-p'), signal a `server-running-external' error.
+
+If NOFRAME is non-nil, don't delete any existing frames
+associated with a client process. This is useful, for example,
+when killing Emacs, in which case the frames will get deleted
+anyway."
+ (let ((server-file (server--file-name)))
+ (when server-process
+ ;; Kill it dead!
+ (ignore-errors (delete-process server-process))
+ (unless noframe
+ (server-log (message "Server stopped")))
+ (setq server-process nil))
+ (unwind-protect
+ ;; Check to see if an uninitialized external socket has been
+ ;; passed in. If that is the case, skip checking
+ ;; `server-running-p' as this will return the wrong result.
+ (if (and internal--daemon-sockname
+ (not server--external-socket-initialized))
+ (setq server--external-socket-initialized t)
+ ;; Delete the socket files made by previous server
+ ;; invocations.
+ (if (not (eq t (server-running-p server-name)))
+ ;; Remove any leftover socket or authentication file.
+ (ignore-errors
+ (let (delete-by-moving-to-trash)
+ (delete-file server-file)
+ ;; Also delete the directory that the server file was
+ ;; created in -- but only in /tmp (see bug#44644).
+ ;; There may be other servers running, too, so this may
+ ;; fail.
+ (when (equal (file-name-directory
+ (directory-file-name
+ (file-name-directory server-file)))
+ "/tmp/")
+ (ignore-errors
+ (delete-directory (file-name-directory server-file))))))
+ (setq server-mode nil) ;; already set by the minor mode code
+ (signal 'server-running-external
+ (list (format "There is an existing Emacs server, named %S."
+ server-name)))))
+ ;; If this Emacs already had a server, clear out associated status.
+ (while server-clients
+ (server-delete-client (car server-clients) noframe)))))
+
;;;###autoload
(defun server-start (&optional leave-dead inhibit-prompt)
"Allow this Emacs process to be a server for client processes.
@@ -643,55 +698,25 @@ server-start
(inhibit-prompt t)
(t (yes-or-no-p
"The current server still has clients; delete them? "))))
- (let* ((server-dir (if server-use-tcp server-auth-dir server-socket-dir))
- (server-file (expand-file-name server-name server-dir)))
- (when server-process
- ;; kill it dead!
- (ignore-errors (delete-process server-process)))
- ;; Check to see if an uninitialized external socket has been
- ;; passed in, if that is the case, skip checking
- ;; `server-running-p' as this will return the wrong result.
- (if (and internal--daemon-sockname
- (not server--external-socket-initialized))
- (setq server--external-socket-initialized t)
- ;; Delete the socket files made by previous server invocations.
- (if (not (eq t (server-running-p server-name)))
- ;; Remove any leftover socket or authentication file.
- (ignore-errors
- (let (delete-by-moving-to-trash)
- (delete-file server-file)
- ;; Also delete the directory that the server file was
- ;; created in -- but only in /tmp (see bug#44644).
- ;; There may be other servers running, too, so this may
- ;; fail.
- (when (equal (file-name-directory
- (directory-file-name
- (file-name-directory server-file)))
- "/tmp/")
- (ignore-errors
- (delete-directory (file-name-directory server-file))))))
- (setq server-mode nil) ;; already set by the minor mode code
- (display-warning
- 'server
- (concat "Unable to start the Emacs server.\n"
- (format "There is an existing Emacs server, named %S.\n"
- server-name)
- (substitute-command-keys
- "To start the server in this Emacs process, stop the existing
+ ;; If a server is already running, try to stop it.
+ (condition-case err
+ (server-stop)
+ (server-running-external
+ (display-warning
+ 'server
+ (concat "Unable to start the Emacs server.\n"
+ (cadr err)
+ (substitute-command-keys
+ "\nTo start the server in this Emacs process, stop the existing
server or call `\\[server-force-delete]' to forcibly disconnect it."))
- :warning)
- (setq leave-dead t)))
- ;; If this Emacs already had a server, clear out associated status.
- (while server-clients
- (server-delete-client (car server-clients)))
- ;; Now any previous server is properly stopped.
- (if leave-dead
- (progn
- (unless (eq t leave-dead) (server-log (message "Server stopped")))
- (setq server-process nil))
- ;; Make sure there is a safe directory in which to place the socket.
- (server-ensure-safe-dir server-dir)
- (when server-process
+ :warning)
+ (setq leave-dead t)))
+ ;; Now any previous server is properly stopped.
+ (unless leave-dead
+ (let ((server-file (server--file-name)))
+ ;; Make sure there is a safe directory in which to place the socket.
+ (server-ensure-safe-dir (file-name-directory server-file))
+ (when server-process
(server-log (message "Restarting server")))
(with-file-modes ?\700
(add-hook 'suspend-tty-functions #'server-handle-suspend-tty)
@@ -742,7 +767,7 @@ server-start
(defun server-force-stop ()
"Kill all connections to the current server.
This function is meant to be called from `kill-emacs-hook'."
- (server-start t t))
+ (ignore-errors (server-stop 'noframe)))
;;;###autoload
(defun server-force-delete (&optional name)
@@ -1858,11 +1883,10 @@ server-eval-at
cannot contact the specified server. For example:
(server-eval-at \"server\" \\='(emacs-pid))
returns the process ID of the Emacs instance running \"server\"."
- (let* ((server-dir (if server-use-tcp server-auth-dir server-socket-dir))
- (server-file (expand-file-name server server-dir))
- (coding-system-for-read 'binary)
- (coding-system-for-write 'binary)
- address port secret process)
+ (let ((server-file (server--file-name))
+ (coding-system-for-read 'binary)
+ (coding-system-for-write 'binary)
+ address port secret process)
(unless (file-exists-p server-file)
(error "No such server: %s" server))
(with-temp-buffer
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-10-30 21:14 ` Jim Porter
@ 2022-11-22 5:06 ` Jim Porter
2022-11-24 11:51 ` Eli Zaretskii
0 siblings, 1 reply; 19+ messages in thread
From: Jim Porter @ 2022-11-22 5:06 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 58877
[-- Attachment #1: Type: text/plain, Size: 579 bytes --]
On 10/30/2022 2:14 PM, Jim Porter wrote:
> Thanks for taking a look. I had hesitated to make any big changes to
> this code since it doesn't have regression tests...
Since server.el could probably use more tests anyway, I added a few ERT
tests covering the most common use cases (see the first patch), and then
added another test for the second patch. The test in the second patch is
pretty indirect, but that's because it's testing something that normally
happens when killing Emacs; it'd be hard to kill the current Emacs
instance and still be able to check test results!
[-- Attachment #2: 0001-Add-more-tests-for-the-Emacs-server.patch --]
[-- Type: text/plain, Size: 6581 bytes --]
From 339661e4f2876adef8f21d89c4e3c09e5e8df053 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 19 Nov 2022 22:26:45 -0800
Subject: [PATCH 1/2] ; Add more tests for the Emacs server
* test/lisp/server-tests.el (server-tests/emacs-client)
(server-tests/max-wait-time): New constants.
(server-tests/start-emacsclient): New function.
(server-tests/with-server, server-tests/wait-until): New macros.
(server-tests/variable): New variable.
(server-test/server-start-sets-minor-mode): Rename to...
(server-tests/server-start/sets-minor-mode): ... this.
(server-tests/server-start/stop-prompt-with-client)
(server-tests/server-start/no-stop-prompt-without-client)
(server-tests/emacsclient/server-edit)
(server-tests/emacsclient/create-frame, server-test/emacsclient/eval):
New tests.
* test/lib-src/emacsclient-tests.el: Mention the above file.
---
test/lib-src/emacsclient-tests.el | 4 +-
test/lisp/server-tests.el | 111 +++++++++++++++++++++++++++---
2 files changed, 105 insertions(+), 10 deletions(-)
diff --git a/test/lib-src/emacsclient-tests.el b/test/lib-src/emacsclient-tests.el
index 1302fbe30c..0fa3c6facf 100644
--- a/test/lib-src/emacsclient-tests.el
+++ b/test/lib-src/emacsclient-tests.el
@@ -19,7 +19,9 @@
;;; Commentary:
-;;
+;; Tests for the emacsclient executable. For tests involving the
+;; interaction between emacsclient and an Emacs server, see
+;; test/lisp/server-tests.el.
;;; Code:
diff --git a/test/lisp/server-tests.el b/test/lisp/server-tests.el
index 351b8ef8d1..48ef110943 100644
--- a/test/lisp/server-tests.el
+++ b/test/lisp/server-tests.el
@@ -22,20 +22,113 @@
(require 'ert)
(require 'server)
+(defconst server-tests/emacsclient
+ (if installation-directory
+ (expand-file-name "lib-src/emacsclient" installation-directory)
+ "emacsclient")
+ "The emacsclient binary to test.")
+
+(defun server-tests/start-emacsclient (&rest args)
+ "Run emacsclient, passing ARGS as arguments to it."
+ (let ((socket-name (process-get server-process :server-file)))
+ (make-process
+ :name server-tests/emacsclient
+ :command (append (list server-tests/emacsclient
+ "--socket-name" socket-name)
+ args))))
+
+(defmacro server-tests/with-server (&rest body)
+ "Start the Emacs server, evaluate BODY, and then stop the server."
+ (declare (indent 0))
+ `(progn
+ (server-start)
+ (unwind-protect
+ (progn (should (processp server-process))
+ ,@body)
+ (let ((inhibit-message t))
+ (server-start t t))
+ (should (null server-process))
+ (should (null server-clients)))))
+
+(defconst server-tests/max-wait-time 5
+ "The maximum time to wait in `server-tests/wait-until', in seconds.")
+
+(defmacro server-tests/wait-until (form)
+ "Wait until FORM is non-nil, timing out and failing if it takes too long."
+ `(let ((start (current-time)))
+ (while (not ,form)
+ (when (> (float-time (time-since start))
+ server-tests/max-wait-time)
+ (ert-fail (format "timed out waiting for %S to be non-nil" ',form)))
+ (sit-for 0.1))))
+
+(defvar server-tests/variable nil)
+
;;; Tests:
-(ert-deftest server-test/server-start-sets-minor-mode ()
+(ert-deftest server-tests/server-start/sets-minor-mode ()
"Ensure that calling `server-start' also sets `server-mode' properly."
- (server-start)
- (unwind-protect
- (progn
- ;; Make sure starting the server activates the minor mode.
- (should (eq server-mode t))
- (should (memq 'server-mode global-minor-modes)))
- ;; Always stop the server, even if the above checks fail.
- (server-start t))
+ (server-tests/with-server
+ ;; Make sure starting the server activates the minor mode.
+ (should (eq server-mode t))
+ (should (memq 'server-mode global-minor-modes)))
;; Make sure stopping the server deactivates the minor mode.
(should (eq server-mode nil))
(should-not (memq 'server-mode global-minor-modes)))
+(ert-deftest server-tests/server-start/stop-prompt-with-client ()
+ "Ensure that stopping the server prompts when there are clients."
+ (server-tests/with-server
+ (let ((yes-or-no-p-called nil)
+ (emacsclient (server-tests/start-emacsclient "-c")))
+ (server-tests/wait-until (length= (frame-list) 2))
+ (cl-letf (((symbol-function 'yes-or-no-p)
+ (lambda (_prompt)
+ (setq yes-or-no-p-called t))))
+ (server-start t)
+ (should yes-or-no-p-called))
+ (server-tests/wait-until (eq (process-status emacsclient) 'exit)))))
+
+(ert-deftest server-tests/server-start/no-stop-prompt-without-client ()
+ "Ensure that stopping the server doesn't prompt when there are no clients."
+ (server-tests/with-server
+ (let ((yes-or-no-p-called nil))
+ (cl-letf (((symbol-function 'yes-or-no-p)
+ (lambda (_prompt)
+ (setq yes-or-no-p-called t))))
+ (let ((inhibit-message t))
+ (server-start t))
+ (should-not yes-or-no-p-called)))))
+
+(ert-deftest server-tests/emacsclient/server-edit ()
+ "Test that calling `server-edit' from a client buffer exits the client."
+ (server-tests/with-server
+ (let ((emacsclient (server-tests/start-emacsclient "file.txt")))
+ (server-tests/wait-until (get-buffer "file.txt"))
+ (should (eq (process-status emacsclient) 'run))
+ (should (length= server-clients 1))
+ (with-current-buffer "file.txt"
+ (server-edit))
+ (server-tests/wait-until (eq (process-status emacsclient) 'exit)))))
+
+(ert-deftest server-tests/emacsclient/create-frame ()
+ "Test that \"emacsclient -c\" creates a frame."
+ (server-tests/with-server
+ (let ((emacsclient (server-tests/start-emacsclient "-c")))
+ (server-tests/wait-until (length= (frame-list) 2))
+ (should (eq (process-status emacsclient) 'run))
+ (should (length= server-clients 1))
+ (should (eq (frame-parameter (car (frame-list)) 'client)
+ (car server-clients)))))
+ ;; The client frame should go away after the server stops.
+ (should (length= (frame-list) 1)))
+
+(ert-deftest server-tests/emacsclient/eval ()
+ "Test that \"emacsclient --eval\" works correctly."
+ (server-tests/with-server
+ (let ((value (random)))
+ (server-tests/start-emacsclient
+ "--eval" (format "(setq server-tests/variable %d)" value))
+ (server-tests/wait-until (eq server-tests/variable value)))))
+
;;; server-tests.el ends here
--
2.25.1
[-- Attachment #3: 0002-Don-t-explicitly-delete-client-frames-when-killing-E.patch --]
[-- Type: text/plain, Size: 10904 bytes --]
From ec8baf1595a27ef0aa9a90dea230af44419e987e Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 21 Nov 2022 11:47:08 -0800
Subject: [PATCH 2/2] Don't explicitly delete client frames when killing Emacs
anyway
This eliminates a useless error prompt when killing Emacs from a
client frame when there are no other frames (bug#58877).
* lisp/server.el (server-running-external): New error.
(server--file-name): New function...
(server-eval-at): ... use it.
(server-start): Factor out server stopping code into...
(server-stop): ... here.
(server-force-stop): Use 'server-stop', and tell it not to delete
frames.
* test/lisp/server-tests.el
(server-tests/server-force-stop/keeps-frames): New test.
---
lisp/server.el | 130 ++++++++++++++++++++++----------------
test/lisp/server-tests.el | 35 ++++++++++
2 files changed, 112 insertions(+), 53 deletions(-)
diff --git a/lisp/server.el b/lisp/server.el
index 553890ce29..6db33fadb1 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -287,6 +287,8 @@ server-socket-dir
"The directory in which to place the server socket.
If local sockets are not supported, this is nil.")
+(define-error 'server-running-external "External server running")
+
(defun server-clients-with (property value)
"Return a list of clients with PROPERTY set to VALUE."
(let (result)
@@ -610,6 +612,54 @@ server-get-auth-key
(error "The key `%s' is invalid" server-auth-key))
(server-generate-key)))
+(defsubst server--file-name ()
+ "Return the file name to use for the server socket."
+ (let ((server-dir (if server-use-tcp server-auth-dir server-socket-dir)))
+ (expand-file-name server-name server-dir)))
+
+(defun server-stop (&optional noframe)
+ "If this Emacs process has a server communication subprocess, stop it.
+If the server is running in some other Emac process (see
+`server-running-p'), signal a `server-running-external' error.
+
+If NOFRAME is non-nil, don't delete any existing frames
+associated with a client process. This is useful, for example,
+when killing Emacs, in which case the frames will get deleted
+anyway."
+ (let ((server-file (server--file-name)))
+ (when server-process
+ ;; Kill it dead!
+ (ignore-errors (delete-process server-process))
+ (unless noframe
+ (server-log (message "Server stopped")))
+ (setq server-process nil
+ server-mode nil
+ global-minor-modes (delq 'server-mode global-minor-modes)))
+ (unwind-protect
+ ;; Delete the socket files made by previous server
+ ;; invocations.
+ (if (not (eq t (server-running-p server-name)))
+ ;; Remove any leftover socket or authentication file.
+ (ignore-errors
+ (let (delete-by-moving-to-trash)
+ (delete-file server-file)
+ ;; Also delete the directory that the server file was
+ ;; created in -- but only in /tmp (see bug#44644).
+ ;; There may be other servers running, too, so this may
+ ;; fail.
+ (when (equal (file-name-directory
+ (directory-file-name
+ (file-name-directory server-file)))
+ "/tmp/")
+ (ignore-errors
+ (delete-directory (file-name-directory server-file))))))
+ (signal 'server-running-external
+ (list (format "There is an existing Emacs server, named %S"
+ server-name))))
+ ;; If this Emacs already had a server, clear out associated status.
+ (while server-clients
+ (server-delete-client (car server-clients) noframe)))))
+
;;;###autoload
(defun server-start (&optional leave-dead inhibit-prompt)
"Allow this Emacs process to be a server for client processes.
@@ -643,55 +693,30 @@ server-start
(inhibit-prompt t)
(t (yes-or-no-p
"The current server still has clients; delete them? "))))
- (let* ((server-dir (if server-use-tcp server-auth-dir server-socket-dir))
- (server-file (expand-file-name server-name server-dir)))
- (when server-process
- ;; kill it dead!
- (ignore-errors (delete-process server-process)))
- ;; Check to see if an uninitialized external socket has been
- ;; passed in, if that is the case, skip checking
- ;; `server-running-p' as this will return the wrong result.
- (if (and internal--daemon-sockname
- (not server--external-socket-initialized))
- (setq server--external-socket-initialized t)
- ;; Delete the socket files made by previous server invocations.
- (if (not (eq t (server-running-p server-name)))
- ;; Remove any leftover socket or authentication file.
- (ignore-errors
- (let (delete-by-moving-to-trash)
- (delete-file server-file)
- ;; Also delete the directory that the server file was
- ;; created in -- but only in /tmp (see bug#44644).
- ;; There may be other servers running, too, so this may
- ;; fail.
- (when (equal (file-name-directory
- (directory-file-name
- (file-name-directory server-file)))
- "/tmp/")
- (ignore-errors
- (delete-directory (file-name-directory server-file))))))
- (display-warning
- 'server
- (concat "Unable to start the Emacs server.\n"
- (format "There is an existing Emacs server, named %S.\n"
- server-name)
- (substitute-command-keys
- "To start the server in this Emacs process, stop the existing
-server or call `\\[server-force-delete]' to forcibly disconnect it."))
- :warning)
- (setq leave-dead t)))
- ;; If this Emacs already had a server, clear out associated status.
- (while server-clients
- (server-delete-client (car server-clients)))
+ ;; If a server is already running, try to stop it.
+ (condition-case err
+ ;; Check to see if an uninitialized external socket has been
+ ;; passed in. If that is the case, don't try to stop the
+ ;; server. (`server-stop' checks `server-running-p', which
+ ;; would return the wrong result).
+ (if (and internal--daemon-sockname
+ (not server--external-socket-initialized))
+ (setq server--external-socket-initialized t)
+ (server-stop))
+ (server-running-external
+ (display-warning
+ 'server
+ (concat "Unable to start the Emacs server.\n"
+ (cadr err)
+ (substitute-command-keys
+ "\nTo start the server in this Emacs process, stop the existingserver or call `\\[server-force-delete]' to forcibly disconnect it."))
+ :warning)
+ (setq leave-dead t)))
;; Now any previous server is properly stopped.
- (if leave-dead
- (progn
- (unless (eq t leave-dead) (server-log (message "Server stopped")))
- (setq server-mode nil
- global-minor-modes (delq 'server-mode global-minor-modes)
- server-process nil))
+ (unless leave-dead
+ (let ((server-file (server--file-name)))
;; Make sure there is a safe directory in which to place the socket.
- (server-ensure-safe-dir server-dir)
+ (server-ensure-safe-dir (file-name-directory server-file))
(when server-process
(server-log (message "Restarting server")))
(with-file-modes ?\700
@@ -745,7 +770,7 @@ server-start
(defun server-force-stop ()
"Kill all connections to the current server.
This function is meant to be called from `kill-emacs-hook'."
- (server-start t t))
+ (ignore-errors (server-stop 'noframe)))
;;;###autoload
(defun server-force-delete (&optional name)
@@ -1866,11 +1891,10 @@ server-eval-at
cannot contact the specified server. For example:
(server-eval-at \"server\" \\='(emacs-pid))
returns the process ID of the Emacs instance running \"server\"."
- (let* ((server-dir (if server-use-tcp server-auth-dir server-socket-dir))
- (server-file (expand-file-name server server-dir))
- (coding-system-for-read 'binary)
- (coding-system-for-write 'binary)
- address port secret process)
+ (let ((server-file (server--file-name))
+ (coding-system-for-read 'binary)
+ (coding-system-for-write 'binary)
+ address port secret process)
(unless (file-exists-p server-file)
(error "No such server: %s" server))
(with-temp-buffer
diff --git a/test/lisp/server-tests.el b/test/lisp/server-tests.el
index 48ef110943..370cf86148 100644
--- a/test/lisp/server-tests.el
+++ b/test/lisp/server-tests.el
@@ -131,4 +131,39 @@ server-tests/emacsclient/eval
"--eval" (format "(setq server-tests/variable %d)" value))
(server-tests/wait-until (eq server-tests/variable value)))))
+(ert-deftest server-tests/server-force-stop/keeps-frames ()
+ "Ensure that `server-force-stop' doesn't delete frames. See bug#58877.
+Note: since that bug is about a behavior when killing Emacs, this
+test is somewhat indirect. (Killing the current Emacs instance
+would make it hard to check test results!) Instead, it only
+tests that `server-force-stop' doesn't delete frames (and even
+then, requires a few tricks to run as a regression test). So
+long as this works, the problem in bug#58877 shouldn't occur."
+ (let (terminal)
+ (unwind-protect
+ (server-tests/with-server
+ (let ((emacsclient (server-tests/start-emacsclient "-c")))
+ (server-tests/wait-until (length= (frame-list) 2))
+ (should (eq (process-status emacsclient) 'run))
+
+ ;; Don't delete the terminal for the client; that would
+ ;; kill its frame immediately too. (This is only an issue
+ ;; when running these tests via the command line;
+ ;; normally, in an interactive session, we don't need to
+ ;; worry about this. But since we want to check that
+ ;; `server-force-stop' doesn't delete frames under normal
+ ;; circumstances, we need to bypass terminal deletion
+ ;; here.)
+ (setq terminal (process-get (car server-clients) 'terminal))
+ (process-put (car server-clients) 'no-delete-terminal t)
+
+ (server-force-stop))
+ ;; Ensure we didn't delete the frame.
+ (should (length= (frame-list) 2)))
+ ;; Clean up after ourselves and delete the terminal.
+ (when (and terminal
+ (eq (terminal-live-p terminal) t)
+ (not (eq system-type 'windows-nt)))
+ (delete-terminal terminal)))))
+
;;; server-tests.el ends here
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-22 5:06 ` Jim Porter
@ 2022-11-24 11:51 ` Eli Zaretskii
2022-11-25 1:36 ` Jim Porter
0 siblings, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2022-11-24 11:51 UTC (permalink / raw)
To: Jim Porter; +Cc: 58877
> Date: Mon, 21 Nov 2022 21:06:31 -0800
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: 58877@debbugs.gnu.org
>
> On 10/30/2022 2:14 PM, Jim Porter wrote:
> > Thanks for taking a look. I had hesitated to make any big changes to
> > this code since it doesn't have regression tests...
> Since server.el could probably use more tests anyway, I added a few ERT
> tests covering the most common use cases (see the first patch), and then
> added another test for the second patch. The test in the second patch is
> pretty indirect, but that's because it's testing something that normally
> happens when killing Emacs; it'd be hard to kill the current Emacs
> instance and still be able to check test results!
This is fine with me, but please fix the typo before installing:
> +(defun server-stop (&optional noframe)
> + "If this Emacs process has a server communication subprocess, stop it.
> +If the server is running in some other Emac process (see
^^^^
^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-24 11:51 ` Eli Zaretskii
@ 2022-11-25 1:36 ` Jim Porter
2022-11-25 13:25 ` Eli Zaretskii
0 siblings, 1 reply; 19+ messages in thread
From: Jim Porter @ 2022-11-25 1:36 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 58877-done
On 11/24/2022 3:51 AM, Eli Zaretskii wrote:
>> Date: Mon, 21 Nov 2022 21:06:31 -0800
>> From: Jim Porter <jporterbugs@gmail.com>
>> Cc: 58877@debbugs.gnu.org
>>
>> Since server.el could probably use more tests anyway, I added a few ERT
>> tests covering the most common use cases (see the first patch), and then
>> added another test for the second patch. The test in the second patch is
>> pretty indirect, but that's because it's testing something that normally
>> happens when killing Emacs; it'd be hard to kill the current Emacs
>> instance and still be able to check test results!
>
> This is fine with me, but please fix the typo before installing:
Whoops! Good catch, thanks.
Merged as 28c444f72a9843ce335032db1fa0f484dfeb4833 with the typo fixed.
Closing this now.
^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-25 1:36 ` Jim Porter
@ 2022-11-25 13:25 ` Eli Zaretskii
2022-11-25 19:31 ` Jim Porter
0 siblings, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2022-11-25 13:25 UTC (permalink / raw)
To: Jim Porter; +Cc: 58877
> Date: Thu, 24 Nov 2022 17:36:11 -0800
> Cc: 58877-done@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
>
> Merged as 28c444f72a9843ce335032db1fa0f484dfeb4833 with the typo fixed.
Thanks.
Are the tests supposed to be portable? Here on MS-Windows 5 tests fail:
5 unexpected results:
FAILED server-tests/emacsclient/create-frame
FAILED server-tests/emacsclient/eval
FAILED server-tests/emacsclient/server-edit
FAILED server-tests/server-force-stop/keeps-frames
FAILED server-tests/server-start/stop-prompt-with-client
All of them fail due to some timeout:
Test server-tests/emacsclient/create-frame backtrace:
signal(ert-test-failed ("timed out waiting for (length= (frame-list)
ert-fail("timed out waiting for (length= (frame-list) 2) to ...")
so this sounds like some fundamental issue common to all those tests.
On GNU/Linux all the tests pass.
Let me know if you want me to investigate something.
^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-25 13:25 ` Eli Zaretskii
@ 2022-11-25 19:31 ` Jim Porter
2022-11-25 20:18 ` Eli Zaretskii
0 siblings, 1 reply; 19+ messages in thread
From: Jim Porter @ 2022-11-25 19:31 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 58877
[-- Attachment #1: Type: text/plain, Size: 671 bytes --]
On 11/25/2022 5:25 AM, Eli Zaretskii wrote:
> Are the tests supposed to be portable?
In theory, they should be. Apparently they aren't in practice.
> All of them fail due to some timeout:
>
> Test server-tests/emacsclient/create-frame backtrace:
> signal(ert-test-failed ("timed out waiting for (length= (frame-list)
> ert-fail("timed out waiting for (length= (frame-list) 2) to ...")
>
> so this sounds like some fundamental issue common to all those tests.
Hm, it looks like the emacsclient may not be starting up correctly.
Could you try the attached patch? I doubt this will fix the tests, but
hopefully you'll get some more-useful error messages.
[-- Attachment #2: 0001-Improve-robustness-of-server.el-tests.patch --]
[-- Type: text/plain, Size: 9769 bytes --]
From 0cd506665942455e595c7f60ad643b0c0c9864cd Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 25 Nov 2022 11:13:06 -0800
Subject: [PATCH] Improve robustness of server.el tests
* test/lisp/server-tests.el (server-tests/start-emacsclient): Rename
to...
(server-tests/start-client): ... this, and set the process's buffer.
(server-tests/with-server): Override 'server-name' so we don't
conflict with "real" Emacs servers.
(server-tests/with-client): New macro...
(server-tests/server-start/stop-prompt-with-client)
(server-tests/emacsclient/server-edit)
(server-tests/emacsclient/create-frame)
(server-tests/emacsclient/create-frame): ... use it.
(server-tests/server-start/stop-prompt-with-client): Simplify.
---
test/lisp/server-tests.el | 116 +++++++++++++++++++++++---------------
1 file changed, 71 insertions(+), 45 deletions(-)
diff --git a/test/lisp/server-tests.el b/test/lisp/server-tests.el
index 370cf86148..52c5ad3db1 100644
--- a/test/lisp/server-tests.el
+++ b/test/lisp/server-tests.el
@@ -22,17 +22,31 @@
(require 'ert)
(require 'server)
+(defconst server-tests/max-wait-time 5
+ "The maximum time to wait in `server-tests/wait-until', in seconds.")
+
(defconst server-tests/emacsclient
(if installation-directory
(expand-file-name "lib-src/emacsclient" installation-directory)
"emacsclient")
"The emacsclient binary to test.")
-(defun server-tests/start-emacsclient (&rest args)
+(defmacro server-tests/wait-until (form)
+ "Wait until FORM is non-nil, timing out and failing if it takes too long."
+ `(let ((start (current-time)))
+ (while (not ,form)
+ (when (> (float-time (time-since start))
+ server-tests/max-wait-time)
+ (ert-fail (format "timed out waiting for %S to be non-nil" ',form)))
+ (sit-for 0.1))))
+
+(defun server-tests/start-client (args)
"Run emacsclient, passing ARGS as arguments to it."
- (let ((socket-name (process-get server-process :server-file)))
+ (let ((socket-name (process-get server-process :server-file))
+ (buffer (generate-new-buffer "emacsclient")))
(make-process
:name server-tests/emacsclient
+ :buffer buffer
:command (append (list server-tests/emacsclient
"--socket-name" socket-name)
args))))
@@ -40,7 +54,9 @@ server-tests/start-emacsclient
(defmacro server-tests/with-server (&rest body)
"Start the Emacs server, evaluate BODY, and then stop the server."
(declare (indent 0))
- `(progn
+ ;; Override the `server-name' so that these tests don't interfere
+ ;; with any existing Emacs servers on the system.
+ `(let ((server-name "server-tests--server"))
(server-start)
(unwind-protect
(progn (should (processp server-process))
@@ -50,17 +66,28 @@ server-tests/with-server
(should (null server-process))
(should (null server-clients)))))
-(defconst server-tests/max-wait-time 5
- "The maximum time to wait in `server-tests/wait-until', in seconds.")
-
-(defmacro server-tests/wait-until (form)
- "Wait until FORM is non-nil, timing out and failing if it takes too long."
- `(let ((start (current-time)))
- (while (not ,form)
- (when (> (float-time (time-since start))
- server-tests/max-wait-time)
- (ert-fail (format "timed out waiting for %S to be non-nil" ',form)))
- (sit-for 0.1))))
+(defmacro server-tests/with-client (client-symbol args exit-status &rest body)
+ "Start an Emacs client with ARGS and evaluate BODY.
+This binds the client process to CLIENT-SYMBOL. If EXIT-STATUS is
+non-nil, then after BODY is evaluated, make sure the client
+process's status matches it."
+ (declare (indent 3))
+ (let ((exit-status-symbol (make-symbol "exit-status"))
+ (starting-client-count-symbol (make-symbol "starting-client-count")))
+ `(let ((,starting-client-count-symbol (length server-clients))
+ (,exit-status-symbol ,exit-status)
+ (,client-symbol (server-tests/start-client ,args)))
+ (ert-info ((with-current-buffer (process-buffer ,client-symbol)
+ (buffer-string))
+ :prefix "Output: ")
+ (server-tests/wait-until
+ (or (= (length server-clients)
+ (1+ ,starting-client-count-symbol))
+ (eq (process-status ,client-symbol) ,exit-status-symbol)))
+ ,@body
+ (when ,exit-status-symbol
+ (server-tests/wait-until (eq (process-status ,client-symbol)
+ ,exit-status-symbol)))))))
(defvar server-tests/variable nil)
@@ -79,57 +106,55 @@ server-tests/server-start/sets-minor-mode
(ert-deftest server-tests/server-start/stop-prompt-with-client ()
"Ensure that stopping the server prompts when there are clients."
(server-tests/with-server
- (let ((yes-or-no-p-called nil)
- (emacsclient (server-tests/start-emacsclient "-c")))
- (server-tests/wait-until (length= (frame-list) 2))
- (cl-letf (((symbol-function 'yes-or-no-p)
- (lambda (_prompt)
- (setq yes-or-no-p-called t))))
+ (server-tests/with-client emacsclient '("-c") 'exit
+ (should (length= (frame-list) 2))
+ (cl-letf* ((yes-or-no-p-called nil)
+ ((symbol-function 'yes-or-no-p)
+ (lambda (_prompt)
+ (setq yes-or-no-p-called t))))
(server-start t)
- (should yes-or-no-p-called))
- (server-tests/wait-until (eq (process-status emacsclient) 'exit)))))
+ (should yes-or-no-p-called)))))
(ert-deftest server-tests/server-start/no-stop-prompt-without-client ()
"Ensure that stopping the server doesn't prompt when there are no clients."
(server-tests/with-server
- (let ((yes-or-no-p-called nil))
- (cl-letf (((symbol-function 'yes-or-no-p)
- (lambda (_prompt)
- (setq yes-or-no-p-called t))))
- (let ((inhibit-message t))
- (server-start t))
- (should-not yes-or-no-p-called)))))
+ (cl-letf* ((inhibit-message t)
+ (yes-or-no-p-called nil)
+ ((symbol-function 'yes-or-no-p)
+ (lambda (_prompt)
+ (setq yes-or-no-p-called t))))
+ (server-start t)
+ (should-not yes-or-no-p-called))))
(ert-deftest server-tests/emacsclient/server-edit ()
"Test that calling `server-edit' from a client buffer exits the client."
(server-tests/with-server
- (let ((emacsclient (server-tests/start-emacsclient "file.txt")))
+ (server-tests/with-client emacsclient '("file.txt") 'exit
(server-tests/wait-until (get-buffer "file.txt"))
(should (eq (process-status emacsclient) 'run))
- (should (length= server-clients 1))
(with-current-buffer "file.txt"
- (server-edit))
- (server-tests/wait-until (eq (process-status emacsclient) 'exit)))))
+ (server-edit)))))
(ert-deftest server-tests/emacsclient/create-frame ()
"Test that \"emacsclient -c\" creates a frame."
- (server-tests/with-server
- (let ((emacsclient (server-tests/start-emacsclient "-c")))
- (server-tests/wait-until (length= (frame-list) 2))
+ (let ((starting-frame-count (length (frame-list))))
+ (server-tests/with-server
+ (server-tests/with-client emacsclient '("-c") nil
+ (should (length= (frame-list) (1+ starting-frame-count)))
(should (eq (process-status emacsclient) 'run))
- (should (length= server-clients 1))
(should (eq (frame-parameter (car (frame-list)) 'client)
(car server-clients)))))
;; The client frame should go away after the server stops.
- (should (length= (frame-list) 1)))
+ (should (length= (frame-list) starting-frame-count))))
(ert-deftest server-tests/emacsclient/eval ()
"Test that \"emacsclient --eval\" works correctly."
(server-tests/with-server
(let ((value (random)))
- (server-tests/start-emacsclient
- "--eval" (format "(setq server-tests/variable %d)" value))
- (server-tests/wait-until (eq server-tests/variable value)))))
+ (server-tests/with-client emacsclient
+ (list "--eval" (format "(setq server-tests/variable %d)" value))
+ 'exit
+ (should (= server-tests/variable value))))))
(ert-deftest server-tests/server-force-stop/keeps-frames ()
"Ensure that `server-force-stop' doesn't delete frames. See bug#58877.
@@ -139,12 +164,13 @@ server-tests/server-force-stop/keeps-frames
tests that `server-force-stop' doesn't delete frames (and even
then, requires a few tricks to run as a regression test). So
long as this works, the problem in bug#58877 shouldn't occur."
- (let (terminal)
+ (let ((starting-frame-count (length (frame-list)))
+ terminal)
(unwind-protect
(server-tests/with-server
- (let ((emacsclient (server-tests/start-emacsclient "-c")))
- (server-tests/wait-until (length= (frame-list) 2))
+ (server-tests/with-client emacsclient '("-c") 'exit
(should (eq (process-status emacsclient) 'run))
+ (should (length= (frame-list) (1+ starting-frame-count)))
;; Don't delete the terminal for the client; that would
;; kill its frame immediately too. (This is only an issue
@@ -159,7 +185,7 @@ server-tests/server-force-stop/keeps-frames
(server-force-stop))
;; Ensure we didn't delete the frame.
- (should (length= (frame-list) 2)))
+ (should (length= (frame-list) (1+ starting-frame-count))))
;; Clean up after ourselves and delete the terminal.
(when (and terminal
(eq (terminal-live-p terminal) t)
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-25 19:31 ` Jim Porter
@ 2022-11-25 20:18 ` Eli Zaretskii
2022-11-25 20:57 ` Jim Porter
0 siblings, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2022-11-25 20:18 UTC (permalink / raw)
To: Jim Porter; +Cc: 58877
> Date: Fri, 25 Nov 2022 11:31:07 -0800
> Cc: 58877@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
>
> Hm, it looks like the emacsclient may not be starting up correctly.
> Could you try the attached patch? I doubt this will fix the tests, but
> hopefully you'll get some more-useful error messages.
Here are the failure info from each failed test after this patch:
Test server-tests/emacsclient/create-frame condition:
Output:
(ert-test-failed "timed out waiting for (or (= (length server-clients) (1+ starting-client-count)) (eq (process-status emacsclient) exit-status)) to be non-nil")
FAILED 1/7 server-tests/emacsclient/create-frame (5.062500 sec) at lisp/server-tests.el:138
Test server-tests/emacsclient/eval condition:
Output:
(wrong-type-argument number-or-marker-p nil)
FAILED 2/7 server-tests/emacsclient/eval (0.109375 sec) at lisp/server-tests.el:150
Test server-tests/emacsclient/server-edit condition:
Output:
(ert-test-failed "timed out waiting for (get-buffer \"file.txt\") to be non-nil")
FAILED 3/7 server-tests/emacsclient/server-edit (5.156250 sec) at lisp/server-tests.el:129
Test server-tests/server-force-stop/keeps-frames condition:
Output:
(ert-test-failed
((should
(eq
(process-status emacsclient)
'run))
:form
(eq exit run)
:value nil))
FAILED 4/7 server-tests/server-force-stop/keeps-frames (0.109375 sec) at lisp/server-tests.el:159
Test server-tests/server-start/stop-prompt-with-client condition:
Output:
(ert-test-failed
((should
(length=
(frame-list)
2))
:form
(length=
(#<frame F1 06d263c0>)
2)
:value nil))
FAILED 7/7 server-tests/server-start/stop-prompt-with-client (0.125000 sec) at lisp/server-tests.el:106
^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-25 20:18 ` Eli Zaretskii
@ 2022-11-25 20:57 ` Jim Porter
2022-11-26 14:43 ` Eli Zaretskii
0 siblings, 1 reply; 19+ messages in thread
From: Jim Porter @ 2022-11-25 20:57 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 58877
[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]
On 11/25/2022 12:18 PM, Eli Zaretskii wrote:
>> Date: Fri, 25 Nov 2022 11:31:07 -0800
>> Cc: 58877@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> Hm, it looks like the emacsclient may not be starting up correctly.
>> Could you try the attached patch? I doubt this will fix the tests, but
>> hopefully you'll get some more-useful error messages.
>
> Here are the failure info from each failed test after this patch:
>
> Test server-tests/emacsclient/create-frame condition:
> Output:
> (ert-test-failed "timed out waiting for (or (= (length server-clients) (1+ starting-client-count)) (eq (process-status emacsclient) exit-status)) to be non-nil")
> FAILED 1/7 server-tests/emacsclient/create-frame (5.062500 sec) at lisp/server-tests.el:138
Oops, sorry about that. I didn't realize until now that 'ert-info'
computes its message immediately, rather than at the time of printing
the info.[1] I've added a bit of code to ert.el to support this case,
which will hopefully produce better output.
I also added the server.el logs to the output of test failures. Note
that they'll print the environment variables of the client, so it's
probably worth skimming over them before posting just in case there are
any secrets in there.
[1] Maybe there's a better way to do this, but it should at least work
for the time being. I'm open to alternatives here so that these tests
are as informative as possible when they fail.
[-- Attachment #2: 0001-Improve-robustness-of-server.el-tests.patch --]
[-- Type: text/plain, Size: 12274 bytes --]
From 7a34b31c17b9563051c54d0fd7289b44f9e36f12 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 25 Nov 2022 11:13:06 -0800
Subject: [PATCH] Improve robustness of server.el tests
* lisp/emacs-lisp/ert.el (ert--insert-infos): Allow 'message' to be a
function that is called when inserting the info.
(ert-info): Update docstring to describe using a function for
MESSAGE-FORM.
* lisp/server.el (server-start): Log when the server is starting.
* test/lisp/server-tests.el (server-tests/start-emacsclient): Rename
to...
(server-tests/start-client): ... this, and set the process's buffer.
(server-tests/with-server): Override 'server-name' so we don't
conflict with "real" Emacs servers.
(server-tests/with-client): New macro...
(server-tests/server-start/stop-prompt-with-client)
(server-tests/emacsclient/server-edit)
(server-tests/emacsclient/create-frame)
(server-tests/emacsclient/create-frame): ... use it.
(server-tests/server-start/stop-prompt-with-client): Simplify.
---
lisp/emacs-lisp/ert.el | 9 ++-
lisp/server.el | 1 +
test/lisp/server-tests.el | 138 +++++++++++++++++++++++---------------
3 files changed, 93 insertions(+), 55 deletions(-)
diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index c25ade22d6..67cbe62538 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -673,8 +673,11 @@ ert-info
To be used within ERT tests. MESSAGE-FORM should evaluate to a
string that will be displayed together with the test result if
-the test fails. PREFIX-FORM should evaluate to a string as well
-and is displayed in front of the value of MESSAGE-FORM."
+the test fails. MESSAGE-FORM can also evaluate to a function; in
+this case, it will be called when displaying the info.
+
+PREFIX-FORM should evaluate to a string as well and is displayed
+in front of the value of MESSAGE-FORM."
(declare (debug ((form &rest [sexp form]) body))
(indent 1))
`(let ((ert--infos (cons (cons ,prefix-form ,message-form) ert--infos)))
@@ -1352,6 +1355,8 @@ ert--insert-infos
(end nil))
(unwind-protect
(progn
+ (when (functionp message)
+ (setq message (funcall message)))
(insert message "\n")
(setq end (point-marker))
(goto-char begin)
diff --git a/lisp/server.el b/lisp/server.el
index beb46853b7..2102f8569b 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -756,6 +756,7 @@ server-start
:service server-file
:plist '(:authenticated t)))))
(unless server-process (error "Could not start server process"))
+ (server-log "Starting server")
(process-put server-process :server-file server-file)
(setq server-mode t)
(push 'server-mode global-minor-modes)
diff --git a/test/lisp/server-tests.el b/test/lisp/server-tests.el
index 370cf86148..bb07f69c28 100644
--- a/test/lisp/server-tests.el
+++ b/test/lisp/server-tests.el
@@ -22,17 +22,31 @@
(require 'ert)
(require 'server)
+(defconst server-tests/max-wait-time 5
+ "The maximum time to wait in `server-tests/wait-until', in seconds.")
+
(defconst server-tests/emacsclient
(if installation-directory
(expand-file-name "lib-src/emacsclient" installation-directory)
"emacsclient")
"The emacsclient binary to test.")
-(defun server-tests/start-emacsclient (&rest args)
+(defmacro server-tests/wait-until (form)
+ "Wait until FORM is non-nil, timing out and failing if it takes too long."
+ `(let ((start (current-time)))
+ (while (not ,form)
+ (when (> (float-time (time-since start))
+ server-tests/max-wait-time)
+ (ert-fail (format "timed out waiting for %S to be non-nil" ',form)))
+ (sit-for 0.1))))
+
+(defun server-tests/start-client (args)
"Run emacsclient, passing ARGS as arguments to it."
- (let ((socket-name (process-get server-process :server-file)))
+ (let ((socket-name (process-get server-process :server-file))
+ (buffer (generate-new-buffer "emacsclient")))
(make-process
:name server-tests/emacsclient
+ :buffer buffer
:command (append (list server-tests/emacsclient
"--socket-name" socket-name)
args))))
@@ -40,27 +54,46 @@ server-tests/start-emacsclient
(defmacro server-tests/with-server (&rest body)
"Start the Emacs server, evaluate BODY, and then stop the server."
(declare (indent 0))
- `(progn
+ ;; Override the `server-name' so that these tests don't interfere
+ ;; with any existing Emacs servers on the system.
+ `(let ((server-name "server-tests--server")
+ (server-log t))
(server-start)
- (unwind-protect
- (progn (should (processp server-process))
- ,@body)
- (let ((inhibit-message t))
- (server-start t t))
- (should (null server-process))
- (should (null server-clients)))))
-
-(defconst server-tests/max-wait-time 5
- "The maximum time to wait in `server-tests/wait-until', in seconds.")
-
-(defmacro server-tests/wait-until (form)
- "Wait until FORM is non-nil, timing out and failing if it takes too long."
- `(let ((start (current-time)))
- (while (not ,form)
- (when (> (float-time (time-since start))
- server-tests/max-wait-time)
- (ert-fail (format "timed out waiting for %S to be non-nil" ',form)))
- (sit-for 0.1))))
+ (ert-info ((lambda ()
+ (with-current-buffer (get-buffer-create server-buffer)
+ (buffer-string)))
+ :prefix "Server logs: ")
+ (unwind-protect
+ (progn (should (processp server-process))
+ ,@body)
+ (let ((inhibit-message t))
+ (server-start t t))
+ (should (null server-process))
+ (should (null server-clients))))))
+
+(defmacro server-tests/with-client (client-symbol args exit-status &rest body)
+ "Start an Emacs client with ARGS and evaluate BODY.
+This binds the client process to CLIENT-SYMBOL. If EXIT-STATUS is
+non-nil, then after BODY is evaluated, make sure the client
+process's status matches it."
+ (declare (indent 3))
+ (let ((exit-status-symbol (make-symbol "exit-status"))
+ (starting-client-count-symbol (make-symbol "starting-client-count")))
+ `(let ((,starting-client-count-symbol (length server-clients))
+ (,exit-status-symbol ,exit-status)
+ (,client-symbol (server-tests/start-client ,args)))
+ (ert-info ((lambda ()
+ (with-current-buffer (process-buffer ,client-symbol)
+ (buffer-string)))
+ :prefix "Client output: ")
+ (server-tests/wait-until
+ (or (= (length server-clients)
+ (1+ ,starting-client-count-symbol))
+ (eq (process-status ,client-symbol) ,exit-status-symbol)))
+ ,@body
+ (when ,exit-status-symbol
+ (server-tests/wait-until (eq (process-status ,client-symbol)
+ ,exit-status-symbol)))))))
(defvar server-tests/variable nil)
@@ -79,57 +112,55 @@ server-tests/server-start/sets-minor-mode
(ert-deftest server-tests/server-start/stop-prompt-with-client ()
"Ensure that stopping the server prompts when there are clients."
(server-tests/with-server
- (let ((yes-or-no-p-called nil)
- (emacsclient (server-tests/start-emacsclient "-c")))
- (server-tests/wait-until (length= (frame-list) 2))
- (cl-letf (((symbol-function 'yes-or-no-p)
- (lambda (_prompt)
- (setq yes-or-no-p-called t))))
+ (server-tests/with-client emacsclient '("-c") 'exit
+ (should (length= (frame-list) 2))
+ (cl-letf* ((yes-or-no-p-called nil)
+ ((symbol-function 'yes-or-no-p)
+ (lambda (_prompt)
+ (setq yes-or-no-p-called t))))
(server-start t)
- (should yes-or-no-p-called))
- (server-tests/wait-until (eq (process-status emacsclient) 'exit)))))
+ (should yes-or-no-p-called)))))
(ert-deftest server-tests/server-start/no-stop-prompt-without-client ()
"Ensure that stopping the server doesn't prompt when there are no clients."
(server-tests/with-server
- (let ((yes-or-no-p-called nil))
- (cl-letf (((symbol-function 'yes-or-no-p)
- (lambda (_prompt)
- (setq yes-or-no-p-called t))))
- (let ((inhibit-message t))
- (server-start t))
- (should-not yes-or-no-p-called)))))
+ (cl-letf* ((inhibit-message t)
+ (yes-or-no-p-called nil)
+ ((symbol-function 'yes-or-no-p)
+ (lambda (_prompt)
+ (setq yes-or-no-p-called t))))
+ (server-start t)
+ (should-not yes-or-no-p-called))))
(ert-deftest server-tests/emacsclient/server-edit ()
"Test that calling `server-edit' from a client buffer exits the client."
(server-tests/with-server
- (let ((emacsclient (server-tests/start-emacsclient "file.txt")))
+ (server-tests/with-client emacsclient '("file.txt") 'exit
(server-tests/wait-until (get-buffer "file.txt"))
(should (eq (process-status emacsclient) 'run))
- (should (length= server-clients 1))
(with-current-buffer "file.txt"
- (server-edit))
- (server-tests/wait-until (eq (process-status emacsclient) 'exit)))))
+ (server-edit)))))
(ert-deftest server-tests/emacsclient/create-frame ()
"Test that \"emacsclient -c\" creates a frame."
- (server-tests/with-server
- (let ((emacsclient (server-tests/start-emacsclient "-c")))
- (server-tests/wait-until (length= (frame-list) 2))
+ (let ((starting-frame-count (length (frame-list))))
+ (server-tests/with-server
+ (server-tests/with-client emacsclient '("-c") nil
+ (should (length= (frame-list) (1+ starting-frame-count)))
(should (eq (process-status emacsclient) 'run))
- (should (length= server-clients 1))
(should (eq (frame-parameter (car (frame-list)) 'client)
(car server-clients)))))
;; The client frame should go away after the server stops.
- (should (length= (frame-list) 1)))
+ (should (length= (frame-list) starting-frame-count))))
(ert-deftest server-tests/emacsclient/eval ()
"Test that \"emacsclient --eval\" works correctly."
(server-tests/with-server
(let ((value (random)))
- (server-tests/start-emacsclient
- "--eval" (format "(setq server-tests/variable %d)" value))
- (server-tests/wait-until (eq server-tests/variable value)))))
+ (server-tests/with-client emacsclient
+ (list "--eval" (format "(setq server-tests/variable %d)" value))
+ 'exit
+ (should (= server-tests/variable value))))))
(ert-deftest server-tests/server-force-stop/keeps-frames ()
"Ensure that `server-force-stop' doesn't delete frames. See bug#58877.
@@ -139,12 +170,13 @@ server-tests/server-force-stop/keeps-frames
tests that `server-force-stop' doesn't delete frames (and even
then, requires a few tricks to run as a regression test). So
long as this works, the problem in bug#58877 shouldn't occur."
- (let (terminal)
+ (let ((starting-frame-count (length (frame-list)))
+ terminal)
(unwind-protect
(server-tests/with-server
- (let ((emacsclient (server-tests/start-emacsclient "-c")))
- (server-tests/wait-until (length= (frame-list) 2))
+ (server-tests/with-client emacsclient '("-c") 'exit
(should (eq (process-status emacsclient) 'run))
+ (should (length= (frame-list) (1+ starting-frame-count)))
;; Don't delete the terminal for the client; that would
;; kill its frame immediately too. (This is only an issue
@@ -159,7 +191,7 @@ server-tests/server-force-stop/keeps-frames
(server-force-stop))
;; Ensure we didn't delete the frame.
- (should (length= (frame-list) 2)))
+ (should (length= (frame-list) (1+ starting-frame-count))))
;; Clean up after ourselves and delete the terminal.
(when (and terminal
(eq (terminal-live-p terminal) t)
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-25 20:57 ` Jim Porter
@ 2022-11-26 14:43 ` Eli Zaretskii
2022-11-26 19:04 ` Jim Porter
0 siblings, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2022-11-26 14:43 UTC (permalink / raw)
To: Jim Porter; +Cc: 58877
> Date: Fri, 25 Nov 2022 12:57:37 -0800
> Cc: 58877@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
>
> >> Hm, it looks like the emacsclient may not be starting up correctly.
> >> Could you try the attached patch? I doubt this will fix the tests, but
> >> hopefully you'll get some more-useful error messages.
> >
> > Here are the failure info from each failed test after this patch:
> >
> > Test server-tests/emacsclient/create-frame condition:
> > Output:
> > (ert-test-failed "timed out waiting for (or (= (length server-clients) (1+ starting-client-count)) (eq (process-status emacsclient) exit-status)) to be non-nil")
> > FAILED 1/7 server-tests/emacsclient/create-frame (5.062500 sec) at lisp/server-tests.el:138
>
> Oops, sorry about that. I didn't realize until now that 'ert-info'
> computes its message immediately, rather than at the time of printing
> the info.[1] I've added a bit of code to ert.el to support this case,
> which will hopefully produce better output.
Never mind, I think I know what's the cause of the problem: I have my
production session of Emacs running on the system ,and it already has the
server started. So a plain call to server-start fails.
I think you need to modify the tests to ensure the server file is created in
a temporary directory. And keep in mind that the variable which affects
that is different depending on whether server-use-tcp is or isn't non-nil.
^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-26 14:43 ` Eli Zaretskii
@ 2022-11-26 19:04 ` Jim Porter
2022-11-26 19:45 ` Eli Zaretskii
0 siblings, 1 reply; 19+ messages in thread
From: Jim Porter @ 2022-11-26 19:04 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 58877
[-- Attachment #1: Type: text/plain, Size: 986 bytes --]
On 11/26/2022 6:43 AM, Eli Zaretskii wrote:
> Never mind, I think I know what's the cause of the problem: I have my
> production session of Emacs running on the system ,and it already has the
> server started. So a plain call to server-start fails.
>
> I think you need to modify the tests to ensure the server file is created in
> a temporary directory. And keep in mind that the variable which affects
> that is different depending on whether server-use-tcp is or isn't non-nil.
Hm, I'm surprised the tests failed again for you then. Both of the
patches I posted to fix the tests set the 'server-name' to a (hopefully)
unique value.
Still, I think it's a good idea to use a temporary directory to hold the
server file, so here's a patch that does that. I've tried this patch
with a production Emacs server running at the same time in several
different configurations (with 'server-use-tcp' on and off, and with
--daemon), and the tests all pass locally for me on GNU/Linux.
[-- Attachment #2: 0001-Improve-robustness-of-server.el-tests.patch --]
[-- Type: text/plain, Size: 12547 bytes --]
From 71c699fab412da4c46574da5d63dc8aeadca8712 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 25 Nov 2022 11:13:06 -0800
Subject: [PATCH] Improve robustness of server.el tests
* lisp/emacs-lisp/ert.el (ert--insert-infos): Allow 'message' to be a
function that is called when inserting the info.
(ert-info): Update docstring to describe using a function for
MESSAGE-FORM.
* lisp/server.el (server-start): Log when the server is starting.
* test/lisp/server-tests.el (server-tests/start-emacsclient): Rename
to...
(server-tests/start-client): ... this, and set the process's buffer.
(server-tests/with-server): Put the server file in a temporary
directory so we don't conflict with real Emacs servers.
(server-tests/with-client): New macro...
(server-tests/server-start/stop-prompt-with-client)
(server-tests/emacsclient/server-edit)
(server-tests/emacsclient/create-frame)
(server-tests/emacsclient/create-frame): ... use it.
(server-tests/server-start/stop-prompt-with-client): Simplify.
---
lisp/emacs-lisp/ert.el | 9 ++-
lisp/server.el | 1 +
test/lisp/server-tests.el | 142 ++++++++++++++++++++++++--------------
3 files changed, 97 insertions(+), 55 deletions(-)
diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el
index c25ade22d6..67cbe62538 100644
--- a/lisp/emacs-lisp/ert.el
+++ b/lisp/emacs-lisp/ert.el
@@ -673,8 +673,11 @@ ert-info
To be used within ERT tests. MESSAGE-FORM should evaluate to a
string that will be displayed together with the test result if
-the test fails. PREFIX-FORM should evaluate to a string as well
-and is displayed in front of the value of MESSAGE-FORM."
+the test fails. MESSAGE-FORM can also evaluate to a function; in
+this case, it will be called when displaying the info.
+
+PREFIX-FORM should evaluate to a string as well and is displayed
+in front of the value of MESSAGE-FORM."
(declare (debug ((form &rest [sexp form]) body))
(indent 1))
`(let ((ert--infos (cons (cons ,prefix-form ,message-form) ert--infos)))
@@ -1352,6 +1355,8 @@ ert--insert-infos
(end nil))
(unwind-protect
(progn
+ (when (functionp message)
+ (setq message (funcall message)))
(insert message "\n")
(setq end (point-marker))
(goto-char begin)
diff --git a/lisp/server.el b/lisp/server.el
index beb46853b7..2102f8569b 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -756,6 +756,7 @@ server-start
:service server-file
:plist '(:authenticated t)))))
(unless server-process (error "Could not start server process"))
+ (server-log "Starting server")
(process-put server-process :server-file server-file)
(setq server-mode t)
(push 'server-mode global-minor-modes)
diff --git a/test/lisp/server-tests.el b/test/lisp/server-tests.el
index 370cf86148..4d74b77fdf 100644
--- a/test/lisp/server-tests.el
+++ b/test/lisp/server-tests.el
@@ -22,17 +22,31 @@
(require 'ert)
(require 'server)
+(defconst server-tests/max-wait-time 5
+ "The maximum time to wait in `server-tests/wait-until', in seconds.")
+
(defconst server-tests/emacsclient
(if installation-directory
(expand-file-name "lib-src/emacsclient" installation-directory)
"emacsclient")
"The emacsclient binary to test.")
-(defun server-tests/start-emacsclient (&rest args)
+(defmacro server-tests/wait-until (form)
+ "Wait until FORM is non-nil, timing out and failing if it takes too long."
+ `(let ((start (current-time)))
+ (while (not ,form)
+ (when (> (float-time (time-since start))
+ server-tests/max-wait-time)
+ (ert-fail (format "timed out waiting for %S to be non-nil" ',form)))
+ (sit-for 0.1))))
+
+(defun server-tests/start-client (args)
"Run emacsclient, passing ARGS as arguments to it."
- (let ((socket-name (process-get server-process :server-file)))
+ (let ((socket-name (process-get server-process :server-file))
+ (buffer (generate-new-buffer "emacsclient")))
(make-process
:name server-tests/emacsclient
+ :buffer buffer
:command (append (list server-tests/emacsclient
"--socket-name" socket-name)
args))))
@@ -40,27 +54,50 @@ server-tests/start-emacsclient
(defmacro server-tests/with-server (&rest body)
"Start the Emacs server, evaluate BODY, and then stop the server."
(declare (indent 0))
- `(progn
+ ;; Override the `server-name' so that these tests don't interfere
+ ;; with any existing Emacs servers on the system.
+ `(let* ((temporary-file-directory (file-name-as-directory
+ (make-temp-file "server-tests" t)))
+ (server-name (expand-file-name
+ "test-server" temporary-file-directory))
+ (server-log t))
(server-start)
- (unwind-protect
- (progn (should (processp server-process))
- ,@body)
- (let ((inhibit-message t))
- (server-start t t))
- (should (null server-process))
- (should (null server-clients)))))
-
-(defconst server-tests/max-wait-time 5
- "The maximum time to wait in `server-tests/wait-until', in seconds.")
-
-(defmacro server-tests/wait-until (form)
- "Wait until FORM is non-nil, timing out and failing if it takes too long."
- `(let ((start (current-time)))
- (while (not ,form)
- (when (> (float-time (time-since start))
- server-tests/max-wait-time)
- (ert-fail (format "timed out waiting for %S to be non-nil" ',form)))
- (sit-for 0.1))))
+ (ert-info ((lambda ()
+ (with-current-buffer (get-buffer-create server-buffer)
+ (buffer-string)))
+ :prefix "Server logs: ")
+ (unwind-protect
+ (progn (should (processp server-process))
+ ,@body)
+ (let ((inhibit-message t))
+ (server-start t t))
+ (delete-directory temporary-file-directory t)
+ (should (null server-process))
+ (should (null server-clients))))))
+
+(defmacro server-tests/with-client (client-symbol args exit-status &rest body)
+ "Start an Emacs client with ARGS and evaluate BODY.
+This binds the client process to CLIENT-SYMBOL. If EXIT-STATUS is
+non-nil, then after BODY is evaluated, make sure the client
+process's status matches it."
+ (declare (indent 3))
+ (let ((exit-status-symbol (make-symbol "exit-status"))
+ (starting-client-count-symbol (make-symbol "starting-client-count")))
+ `(let ((,starting-client-count-symbol (length server-clients))
+ (,exit-status-symbol ,exit-status)
+ (,client-symbol (server-tests/start-client ,args)))
+ (ert-info ((lambda ()
+ (with-current-buffer (process-buffer ,client-symbol)
+ (buffer-string)))
+ :prefix "Client output: ")
+ (server-tests/wait-until
+ (or (= (length server-clients)
+ (1+ ,starting-client-count-symbol))
+ (eq (process-status ,client-symbol) ,exit-status-symbol)))
+ ,@body
+ (when ,exit-status-symbol
+ (server-tests/wait-until (eq (process-status ,client-symbol)
+ ,exit-status-symbol)))))))
(defvar server-tests/variable nil)
@@ -79,57 +116,55 @@ server-tests/server-start/sets-minor-mode
(ert-deftest server-tests/server-start/stop-prompt-with-client ()
"Ensure that stopping the server prompts when there are clients."
(server-tests/with-server
- (let ((yes-or-no-p-called nil)
- (emacsclient (server-tests/start-emacsclient "-c")))
- (server-tests/wait-until (length= (frame-list) 2))
- (cl-letf (((symbol-function 'yes-or-no-p)
- (lambda (_prompt)
- (setq yes-or-no-p-called t))))
+ (server-tests/with-client emacsclient '("-c") 'exit
+ (should (length= (frame-list) 2))
+ (cl-letf* ((yes-or-no-p-called nil)
+ ((symbol-function 'yes-or-no-p)
+ (lambda (_prompt)
+ (setq yes-or-no-p-called t))))
(server-start t)
- (should yes-or-no-p-called))
- (server-tests/wait-until (eq (process-status emacsclient) 'exit)))))
+ (should yes-or-no-p-called)))))
(ert-deftest server-tests/server-start/no-stop-prompt-without-client ()
"Ensure that stopping the server doesn't prompt when there are no clients."
(server-tests/with-server
- (let ((yes-or-no-p-called nil))
- (cl-letf (((symbol-function 'yes-or-no-p)
- (lambda (_prompt)
- (setq yes-or-no-p-called t))))
- (let ((inhibit-message t))
- (server-start t))
- (should-not yes-or-no-p-called)))))
+ (cl-letf* ((inhibit-message t)
+ (yes-or-no-p-called nil)
+ ((symbol-function 'yes-or-no-p)
+ (lambda (_prompt)
+ (setq yes-or-no-p-called t))))
+ (server-start t)
+ (should-not yes-or-no-p-called))))
(ert-deftest server-tests/emacsclient/server-edit ()
"Test that calling `server-edit' from a client buffer exits the client."
(server-tests/with-server
- (let ((emacsclient (server-tests/start-emacsclient "file.txt")))
+ (server-tests/with-client emacsclient '("file.txt") 'exit
(server-tests/wait-until (get-buffer "file.txt"))
(should (eq (process-status emacsclient) 'run))
- (should (length= server-clients 1))
(with-current-buffer "file.txt"
- (server-edit))
- (server-tests/wait-until (eq (process-status emacsclient) 'exit)))))
+ (server-edit)))))
(ert-deftest server-tests/emacsclient/create-frame ()
"Test that \"emacsclient -c\" creates a frame."
- (server-tests/with-server
- (let ((emacsclient (server-tests/start-emacsclient "-c")))
- (server-tests/wait-until (length= (frame-list) 2))
+ (let ((starting-frame-count (length (frame-list))))
+ (server-tests/with-server
+ (server-tests/with-client emacsclient '("-c") nil
+ (should (length= (frame-list) (1+ starting-frame-count)))
(should (eq (process-status emacsclient) 'run))
- (should (length= server-clients 1))
(should (eq (frame-parameter (car (frame-list)) 'client)
(car server-clients)))))
;; The client frame should go away after the server stops.
- (should (length= (frame-list) 1)))
+ (should (length= (frame-list) starting-frame-count))))
(ert-deftest server-tests/emacsclient/eval ()
"Test that \"emacsclient --eval\" works correctly."
(server-tests/with-server
(let ((value (random)))
- (server-tests/start-emacsclient
- "--eval" (format "(setq server-tests/variable %d)" value))
- (server-tests/wait-until (eq server-tests/variable value)))))
+ (server-tests/with-client emacsclient
+ (list "--eval" (format "(setq server-tests/variable %d)" value))
+ 'exit
+ (should (= server-tests/variable value))))))
(ert-deftest server-tests/server-force-stop/keeps-frames ()
"Ensure that `server-force-stop' doesn't delete frames. See bug#58877.
@@ -139,12 +174,13 @@ server-tests/server-force-stop/keeps-frames
tests that `server-force-stop' doesn't delete frames (and even
then, requires a few tricks to run as a regression test). So
long as this works, the problem in bug#58877 shouldn't occur."
- (let (terminal)
+ (let ((starting-frame-count (length (frame-list)))
+ terminal)
(unwind-protect
(server-tests/with-server
- (let ((emacsclient (server-tests/start-emacsclient "-c")))
- (server-tests/wait-until (length= (frame-list) 2))
+ (server-tests/with-client emacsclient '("-c") 'exit
(should (eq (process-status emacsclient) 'run))
+ (should (length= (frame-list) (1+ starting-frame-count)))
;; Don't delete the terminal for the client; that would
;; kill its frame immediately too. (This is only an issue
@@ -159,7 +195,7 @@ server-tests/server-force-stop/keeps-frames
(server-force-stop))
;; Ensure we didn't delete the frame.
- (should (length= (frame-list) 2)))
+ (should (length= (frame-list) (1+ starting-frame-count))))
;; Clean up after ourselves and delete the terminal.
(when (and terminal
(eq (terminal-live-p terminal) t)
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-26 19:04 ` Jim Porter
@ 2022-11-26 19:45 ` Eli Zaretskii
2022-11-26 20:17 ` Jim Porter
0 siblings, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2022-11-26 19:45 UTC (permalink / raw)
To: Jim Porter; +Cc: 58877
> Date: Sat, 26 Nov 2022 11:04:48 -0800
> Cc: 58877@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
>
> > I think you need to modify the tests to ensure the server file is created in
> > a temporary directory. And keep in mind that the variable which affects
> > that is different depending on whether server-use-tcp is or isn't non-nil.
>
> Hm, I'm surprised the tests failed again for you then. Both of the
> patches I posted to fix the tests set the 'server-name' to a (hopefully)
> unique value.
>
> Still, I think it's a good idea to use a temporary directory to hold the
> server file, so here's a patch that does that. I've tried this patch
> with a production Emacs server running at the same time in several
> different configurations (with 'server-use-tcp' on and off, and with
> --daemon), and the tests all pass locally for me on GNU/Linux.
Still fails, and here's why:
Client output: d:\gnu\git\emacs\trunk\lib-src\emacsclient.exe: unrecognized option '--socket-name'
Try 'd:\gnu\git\emacs\trunk\lib-src\emacsclient.exe --help' for more information
Process d:/gnu/git/emacs/trunk/lib-src/emacsclient exited abnormally with code 1
There's no --socket-name switch on MS-Windows; you need to use --server-file
instead.
If I change server-tests/start-client to use --server-file, the next problem
rears its ugly head:
Client output: *ERROR*: Not using an ASCII terminal now; cannot make a new ASCII frame
Process d:/gnu/git/emacs/trunk/lib-src/emacsclient exited abnormally with code 1
This is because emacsclient on Windows cannot create new terminal frames,
only new GUI frames, unless Emacs was started as "emacs -nw".
Why do these tests need TTY frames?
Btw, with the above changes, only 3 tests fail:
3 unexpected results:
FAILED server-tests/emacsclient/create-frame
FAILED server-tests/server-force-stop/keeps-frames
FAILED server-tests/server-start/stop-prompt-with-client
So this is progress, thanks.
^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-26 19:45 ` Eli Zaretskii
@ 2022-11-26 20:17 ` Jim Porter
2022-11-26 20:35 ` Eli Zaretskii
0 siblings, 1 reply; 19+ messages in thread
From: Jim Porter @ 2022-11-26 20:17 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 58877
On 11/26/2022 11:45 AM, Eli Zaretskii wrote:
>> Date: Sat, 26 Nov 2022 11:04:48 -0800
>> Cc: 58877@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>>> I think you need to modify the tests to ensure the server file is created in
>>> a temporary directory. And keep in mind that the variable which affects
>>> that is different depending on whether server-use-tcp is or isn't non-nil.
>>
>> Hm, I'm surprised the tests failed again for you then. Both of the
>> patches I posted to fix the tests set the 'server-name' to a (hopefully)
>> unique value.
>>
>> Still, I think it's a good idea to use a temporary directory to hold the
>> server file, so here's a patch that does that. I've tried this patch
>> with a production Emacs server running at the same time in several
>> different configurations (with 'server-use-tcp' on and off, and with
>> --daemon), and the tests all pass locally for me on GNU/Linux.
>
> Still fails, and here's why:
>
> Client output: d:\gnu\git\emacs\trunk\lib-src\emacsclient.exe: unrecognized option '--socket-name'
> Try 'd:\gnu\git\emacs\trunk\lib-src\emacsclient.exe --help' for more information
>
> Process d:/gnu/git/emacs/trunk/lib-src/emacsclient exited abnormally with code 1
>
> There's no --socket-name switch on MS-Windows; you need to use --server-file
> instead.
Ah ha, that explains it. I didn't realize that distinction.
> If I change server-tests/start-client to use --server-file, the next problem
> rears its ugly head:
>
> Client output: *ERROR*: Not using an ASCII terminal now; cannot make a new ASCII frame
>
> Process d:/gnu/git/emacs/trunk/lib-src/emacsclient exited abnormally with code 1
>
> This is because emacsclient on Windows cannot create new terminal frames,
> only new GUI frames, unless Emacs was started as "emacs -nw".
>
> Why do these tests need TTY frames?
The test code just calls "emacsclient -c", and they should be happy with
any kind of frame, TTY or GUI. I guess that by running in batch mode,
Emacs will try to create TTY frames, but that fails on MS Windows?
> Btw, with the above changes, only 3 tests fail:
>
> 3 unexpected results:
> FAILED server-tests/emacsclient/create-frame
> FAILED server-tests/server-force-stop/keeps-frames
> FAILED server-tests/server-start/stop-prompt-with-client
>
> So this is progress, thanks.
All these tests involve creating client frames, so that makes sense. I'm
not sure how to coax Emacs into creating GUI frames on MS Windows for
these tests though. Still, I don't think these failures are a sign of
anything broken exactly; it's just the combination of MS Windows'
limitations with trying to create frames in an Emacs batch session.
Unless you have an idea for how to fix that (I've got no clue,
unfortunately), how about just skipping these tests on MS Windows, with
a comment explaining what the limitation is?
^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-26 20:17 ` Jim Porter
@ 2022-11-26 20:35 ` Eli Zaretskii
2022-11-26 21:44 ` Jim Porter
0 siblings, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2022-11-26 20:35 UTC (permalink / raw)
To: Jim Porter; +Cc: 58877
> Date: Sat, 26 Nov 2022 12:17:59 -0800
> Cc: 58877@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
>
> > Client output: *ERROR*: Not using an ASCII terminal now; cannot make a new ASCII frame
> >
> > Process d:/gnu/git/emacs/trunk/lib-src/emacsclient exited abnormally with code 1
> >
> > This is because emacsclient on Windows cannot create new terminal frames,
> > only new GUI frames, unless Emacs was started as "emacs -nw".
> >
> > Why do these tests need TTY frames?
>
> The test code just calls "emacsclient -c", and they should be happy with
> any kind of frame, TTY or GUI. I guess that by running in batch mode,
> Emacs will try to create TTY frames, but that fails on MS Windows?
I guess. And I'm not prepared to spend time trying to make emacsclient work
with a batch-mode server on MS-Windows.
> > Btw, with the above changes, only 3 tests fail:
> >
> > 3 unexpected results:
> > FAILED server-tests/emacsclient/create-frame
> > FAILED server-tests/server-force-stop/keeps-frames
> > FAILED server-tests/server-start/stop-prompt-with-client
> >
> > So this is progress, thanks.
>
> All these tests involve creating client frames, so that makes sense. I'm
> not sure how to coax Emacs into creating GUI frames on MS Windows for
> these tests though. Still, I don't think these failures are a sign of
> anything broken exactly; it's just the combination of MS Windows'
> limitations with trying to create frames in an Emacs batch session.
>
> Unless you have an idea for how to fix that (I've got no clue,
> unfortunately), how about just skipping these tests on MS Windows, with
> a comment explaining what the limitation is?
Fine with me, thanks.
^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-26 20:35 ` Eli Zaretskii
@ 2022-11-26 21:44 ` Jim Porter
2022-11-28 1:28 ` Jim Porter
0 siblings, 1 reply; 19+ messages in thread
From: Jim Porter @ 2022-11-26 21:44 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 58877
On 11/26/2022 12:35 PM, Eli Zaretskii wrote:
>> Date: Sat, 26 Nov 2022 12:17:59 -0800
>> Cc: 58877@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> Unless you have an idea for how to fix that (I've got no clue,
>> unfortunately), how about just skipping these tests on MS Windows, with
>> a comment explaining what the limitation is?
>
> Fine with me, thanks.
Thanks. I've merged the fixes for the tests as
14d54212ea46dbd8c950c9852318597e0e47908d. If you see any other issues,
just let me know.
^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-26 21:44 ` Jim Porter
@ 2022-11-28 1:28 ` Jim Porter
2022-11-28 3:31 ` Eli Zaretskii
0 siblings, 1 reply; 19+ messages in thread
From: Jim Porter @ 2022-11-28 1:28 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 58877
[-- Attachment #1: Type: text/plain, Size: 802 bytes --]
On 11/26/2022 1:44 PM, Jim Porter wrote:
> On 11/26/2022 12:35 PM, Eli Zaretskii wrote:
>>> Date: Sat, 26 Nov 2022 12:17:59 -0800
>>> Cc: 58877@debbugs.gnu.org
>>> From: Jim Porter <jporterbugs@gmail.com>
>>>
>>> Unless you have an idea for how to fix that (I've got no clue,
>>> unfortunately), how about just skipping these tests on MS Windows, with
>>> a comment explaining what the limitation is?
>>
>> Fine with me, thanks.
>
> Thanks. I've merged the fixes for the tests as
> 14d54212ea46dbd8c950c9852318597e0e47908d. If you see any other issues,
> just let me know.
Here's one tiny followup fix. Currently on master, if you restart the
Emacs server, it emits a "Server stopped" message. That's not right, and
it should do what it used to: emit a "Restarting server" message. Here's
a fix.
[-- Attachment #2: 0001-Don-t-emit-a-Server-stopped-message-when-restarting-.patch --]
[-- Type: text/plain, Size: 3494 bytes --]
From 1724f23f3d8b3bef18bcd97e2f892e943f03c0da Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 27 Nov 2022 17:22:49 -0800
Subject: [PATCH] ; Don't emit a "Server stopped" message when restarting the
Emacs server
* lisp/server.el (server-stop): Return non-nil when we actually stop
the server. Don't message about stopping the server here (but do log
it).
(server-start): Emit the appropriate message about stopping or
restarting the server.
---
lisp/server.el | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/lisp/server.el b/lisp/server.el
index 2102f8569b..1b027f88ce 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -619,20 +619,22 @@ server--file-name
(defun server-stop (&optional noframe)
"If this Emacs process has a server communication subprocess, stop it.
-If the server is running in some other Emacs process (see
+If this actually stopped the server, return non-nil. If the
+server is running in some other Emacs process (see
`server-running-p'), signal a `server-running-external' error.
If NOFRAME is non-nil, don't delete any existing frames
associated with a client process. This is useful, for example,
when killing Emacs, in which case the frames will get deleted
anyway."
- (let ((server-file (server--file-name)))
+ (let ((server-file (server--file-name))
+ stopped-p)
(when server-process
;; Kill it dead!
(ignore-errors (delete-process server-process))
- (unless noframe
- (server-log (message "Server stopped")))
- (setq server-process nil
+ (server-log "Stopped server")
+ (setq stopped-p t
+ server-process nil
server-mode nil
global-minor-modes (delq 'server-mode global-minor-modes)))
(unwind-protect
@@ -658,7 +660,8 @@ server-stop
server-name))))
;; If this Emacs already had a server, clear out associated status.
(while server-clients
- (server-delete-client (car server-clients) noframe)))))
+ (server-delete-client (car server-clients) noframe)))
+ stopped-p))
;;;###autoload
(defun server-start (&optional leave-dead inhibit-prompt)
@@ -702,7 +705,8 @@ server-start
(if (and internal--daemon-sockname
(not server--external-socket-initialized))
(setq server--external-socket-initialized t)
- (server-stop))
+ (when (server-stop)
+ (message (if leave-dead "Stopped server" "Restarting server"))))
(server-running-external
(display-warning
'server
@@ -717,8 +721,6 @@ server-start
(let ((server-file (server--file-name)))
;; Make sure there is a safe directory in which to place the socket.
(server-ensure-safe-dir (file-name-directory server-file))
- (when server-process
- (server-log (message "Restarting server")))
(with-file-modes ?\700
(add-hook 'suspend-tty-functions #'server-handle-suspend-tty)
(add-hook 'delete-frame-functions #'server-handle-delete-frame)
@@ -756,7 +758,7 @@ server-start
:service server-file
:plist '(:authenticated t)))))
(unless server-process (error "Could not start server process"))
- (server-log "Starting server")
+ (server-log "Started server")
(process-put server-process :server-file server-file)
(setq server-mode t)
(push 'server-mode global-minor-modes)
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-28 1:28 ` Jim Porter
@ 2022-11-28 3:31 ` Eli Zaretskii
2022-11-28 6:27 ` Jim Porter
0 siblings, 1 reply; 19+ messages in thread
From: Eli Zaretskii @ 2022-11-28 3:31 UTC (permalink / raw)
To: Jim Porter; +Cc: 58877
> Date: Sun, 27 Nov 2022 17:28:41 -0800
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: 58877@debbugs.gnu.org
>
> > Thanks. I've merged the fixes for the tests as
> > 14d54212ea46dbd8c950c9852318597e0e47908d. If you see any other issues,
> > just let me know.
>
> Here's one tiny followup fix. Currently on master, if you restart the
> Emacs server, it emits a "Server stopped" message. That's not right, and
> it should do what it used to: emit a "Restarting server" message. Here's
> a fix.
Thanks, please install.
^ permalink raw reply [flat|nested] 19+ messages in thread
* bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt
2022-11-28 3:31 ` Eli Zaretskii
@ 2022-11-28 6:27 ` Jim Porter
0 siblings, 0 replies; 19+ messages in thread
From: Jim Porter @ 2022-11-28 6:27 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 58877
On 11/27/2022 7:31 PM, Eli Zaretskii wrote:
>> Date: Sun, 27 Nov 2022 17:28:41 -0800
>> From: Jim Porter <jporterbugs@gmail.com>
>> Cc: 58877@debbugs.gnu.org
>>
>> Here's one tiny followup fix. Currently on master, if you restart the
>> Emacs server, it emits a "Server stopped" message. That's not right, and
>> it should do what it used to: emit a "Restarting server" message. Here's
>> a fix.
>
> Thanks, please install.
Thanks for taking a look. Merged as
a85ff22300736212e38f43cc7d56e8e3d4ae1203.
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-11-28 6:27 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-29 21:33 bug#58877: 29.0.50; [PATCH] When killing Emacs from a client frame with no other frames, Emacs shows a useless error prompt Jim Porter
2022-10-30 6:14 ` Eli Zaretskii
2022-10-30 21:14 ` Jim Porter
2022-11-22 5:06 ` Jim Porter
2022-11-24 11:51 ` Eli Zaretskii
2022-11-25 1:36 ` Jim Porter
2022-11-25 13:25 ` Eli Zaretskii
2022-11-25 19:31 ` Jim Porter
2022-11-25 20:18 ` Eli Zaretskii
2022-11-25 20:57 ` Jim Porter
2022-11-26 14:43 ` Eli Zaretskii
2022-11-26 19:04 ` Jim Porter
2022-11-26 19:45 ` Eli Zaretskii
2022-11-26 20:17 ` Jim Porter
2022-11-26 20:35 ` Eli Zaretskii
2022-11-26 21:44 ` Jim Porter
2022-11-28 1:28 ` Jim Porter
2022-11-28 3:31 ` Eli Zaretskii
2022-11-28 6:27 ` Jim Porter
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.