GNU bug report logs - #62556
29.0.60; [PATCH] Fix regression when calling 'eshell-command' with a pipeline in the background

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 62556 in the body.
You can then email your comments to 62556 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.60; [PATCH] Fix regression when calling 'eshell-command' with a
 pipeline in the background
Date: Thu, 30 Mar 2023 21:18:10 -0700

[Message part 1 (text/plain, inline)]

Starting from "emacs -Q":

  M-x eshell-command RET *echo hi | *cat & RET

You'll see the following error in the *Messages* buffer instead of the 
result:

  eshell-eval-command: Unmatched delimiter: (#<process echo> . 
#<process cat>)

This is a regression from my fix for bug#53715, which changed how
Eshell pipelines return the processes in the pipeline. Attached are some 
patches to fix it. One for the emacs-29 branch, and one for master.

Eli, is it ok to merge the patch for emacs-29? I tried to keep the 
change as minimal as possible for that branch.

The patch for master is a bit more extensive, and also fixes another 
issue where this would fail due to incorrect syntax in the Eshell 
command form:

  (eshell-command "*echo hi &" t)

Previously, it turned the command into "*echo hi & >>> 
#<current-buffer>", but that's not right; the "&" needs to go last.

For master, I also thought it would be nice to clean up 'eshell-command' 
slightly (see patch 0002); this just changes it to handle its arguments 
in the interactive spec.

[emacs-29--0001-Fix-using-background-commands-in-eshell-command.patch (text/plain, attachment)]

[emacs-30--0001-Fix-using-background-commands-in-eshell-command.patch (text/plain, attachment)]

[emacs-30--0002-Use-the-interactive-spec-to-set-arguments-for-eshell.patch (text/plain, attachment)]

Message #8 received at 62556 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 62556 <at> debbugs.gnu.org
Subject: Re: bug#62556: 29.0.60;
 [PATCH] Fix regression when calling 'eshell-command' with a pipeline
 in the background
Date: Fri, 31 Mar 2023 09:29:03 +0300

> Date: Thu, 30 Mar 2023 21:18:10 -0700
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> This is a regression from my fix for bug#53715, which changed how
> Eshell pipelines return the processes in the pipeline. Attached are some 
> patches to fix it. One for the emacs-29 branch, and one for master.

Looks like there are _two_ patches for master, not one?

> Eli, is it ok to merge the patch for emacs-29?

Yes, thanks.

Message #11 received at 62556 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62556 <at> debbugs.gnu.org
Subject: Re: bug#62556: 29.0.60; [PATCH] Fix regression when calling
 'eshell-command' with a pipeline in the background
Date: Fri, 31 Mar 2023 13:09:24 -0700

On 3/30/2023 11:29 PM, Eli Zaretskii wrote:
>> Date: Thu, 30 Mar 2023 21:18:10 -0700
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>> This is a regression from my fix for bug#53715, which changed how
>> Eshell pipelines return the processes in the pipeline. Attached are some
>> patches to fix it. One for the emacs-29 branch, and one for master.
> 
> Looks like there are _two_ patches for master, not one?

Yeah, sorry. That was clumsy wording on my part.

>> Eli, is it ok to merge the patch for emacs-29?
> 
> Yes, thanks.

Thanks. Merged to emacs-29 as 6419d78fa6f. I'll wait a couple days 
before merging the patches for master in case anyone else has comments, 
since they're a bit more substantial.

Message #14 received at 62556 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 62556 <at> debbugs.gnu.org
Subject: Re: bug#62556: 29.0.60; [PATCH] Fix regression when calling
 'eshell-command' with a pipeline in the background
Date: Sat, 01 Apr 2023 09:35:16 +0300

> Date: Fri, 31 Mar 2023 13:09:24 -0700
> Cc: 62556 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> Thanks. Merged to emacs-29 as 6419d78fa6f.

Some of the new tests (and one old as well) fail on MS-Windows:

  3 unexpected results:
     FAILED  eshell-test/eshell-command/background
     FAILED  eshell-test/eshell-command/background-pipeline
     FAILED  eshell-test/subcommand-reset-in-pipeline

I fixed the first 2, but I don't understand what goes wrong in the
last one and why:

  Test eshell-test/subcommand-reset-in-pipeline backtrace:
    signal(ert-test-failed (((should (eshell-command-result--equal comma
    ert-fail(((should (eshell-command-result--equal command (eshell-test
    (if (unwind-protect (setq value-7 (apply fn-5 args-6)) (setq form-de
    (let (form-description-9) (if (unwind-protect (setq value-7 (apply f
    (let ((value-7 'ert-form-evaluation-aborted-8)) (let (form-descripti
    (let* ((fn-5 #'eshell-command-result--equal) (args-6 (condition-case
    eshell-command-result-equal("*cat $<echo $eshell-in-pipeline-p | ech
    (let ((template (car tail))) (eshell-command-result-equal (format te
    (while tail (let ((template (car tail))) (eshell-command-result-equa
    (let ((tail '("echo {%s} | *cat" "echo ${%s} | *cat" "*cat $<%s> | *
    (closure (t) nil (let* ((fn-31 #'executable-find) (args-32 (conditio
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name eshell-test/subcommand-reset-in-pipel
    ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
    ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
    ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
    ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
    eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
    command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/eshell/eshell-tests.
    command-line()
    normal-top-level()
  Test eshell-test/subcommand-reset-in-pipeline condition:
      (ert-test-failed
       ((should
	 (eshell-command-result--equal command
				       (eshell-test-command-result command)
				       result))
	:form
	(eshell-command-result--equal "*cat $<echo $eshell-in-pipeline-p | echo> | *cat" nil "first")
	:value nil :explanation
	(nonequal-result
	 (command "*cat $<echo $eshell-in-pipeline-p | echo> | *cat")
	 (result nil)
	 (expected "first"))))
     FAILED  16/16  eshell-test/subcommand-reset-in-pipeline (1.796875 sec) at lisp/eshell/eshell-tests.el:80

The test that fails is this one:

    (eshell-command-result-equal
     (format template "echo $eshell-in-pipeline-p | echo")
     "first")

when it is run for the last template, "*cat $<%s> | *cat".

As another annoyance, I get this prompt, and must type "yes RET":

     passed   1/16  eshell-test/command-running-p (0.203125 sec)
     passed   2/16  eshell-test/eshell-command/background (0.015625 sec)
  Buffer "*Eshell Async Command Output*" has a running process; kill it? (yes or no) yes
     passed   3/16  eshell-test/eshell-command/background-pipeline (2.453125 sec)

Any suggestions?

Message #17 received at 62556 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62556 <at> debbugs.gnu.org
Subject: Re: bug#62556: 29.0.60; [PATCH] Fix regression when calling
 'eshell-command' with a pipeline in the background
Date: Sat, 1 Apr 2023 00:36:33 -0700

On 3/31/2023 11:35 PM, Eli Zaretskii wrote:
> I fixed the first 2, but I don't understand what goes wrong in the
> last one and why:
[snip]
>    Test eshell-test/subcommand-reset-in-pipeline condition:
>        (ert-test-failed
>         ((should
> 	 (eshell-command-result--equal command
> 				       (eshell-test-command-result command)
> 				       result))
> 	:form
> 	(eshell-command-result--equal "*cat $<echo $eshell-in-pipeline-p | echo> | *cat" nil "first")
> 	:value nil :explanation
> 	(nonequal-result
> 	 (command "*cat $<echo $eshell-in-pipeline-p | echo> | *cat")
> 	 (result nil)
> 	 (expected "first"))))
>       FAILED  16/16  eshell-test/subcommand-reset-in-pipeline (1.796875 sec) at lisp/eshell/eshell-tests.el:80

Lars reported a similar issue a while back too: 
<https://lists.gnu.org/archive/html/emacs-devel/2022-09/msg00314.html>. 
I *think* it's due to a race condition somewhere, but I've had quite a 
bit of difficulty fixing it. I have a couple ideas though, but they're 
probably too invasive for the release branch.

> As another annoyance, I get this prompt, and must type "yes RET":
> 
>       passed   1/16  eshell-test/command-running-p (0.203125 sec)
>       passed   2/16  eshell-test/eshell-command/background (0.015625 sec)
>    Buffer "*Eshell Async Command Output*" has a running process; kill it? (yes or no) yes
>       passed   3/16  eshell-test/eshell-command/background-pipeline (2.453125 sec)
> 
> Any suggestions?

Does this patch fix it?

----------------------------------------

diff --git a/test/lisp/eshell/eshell-tests.el 
b/test/lisp/eshell/eshell-tests.el
index bf7ec0389f0..fbff51a8873 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -136,6 +136,8 @@ eshell-test/eshell-command/background
       ;; buffer.
       (eshell-command "*echo hi &")
       (with-current-buffer "*Eshell Async Command Output*"
+        (while (get-buffer-process (current-buffer))
+          (accept-process-output))
         (goto-char (point-min))
         (should (looking-at "\\[echo\\(\\.exe\\)?\\(<[0-9]+>\\)?\\]"))))))

@@ -149,6 +151,8 @@ eshell-test/eshell-command/background-pipeline
       ;; XXX: As above, we can't write to the current buffer here.
       (eshell-command "*echo hi | *cat &")
       (with-current-buffer "*Eshell Async Command Output*"
+        (while (get-buffer-process (current-buffer))
+          (accept-process-output))
         (goto-char (point-min))
         (should (looking-at "\\[cat\\(\\.exe\\)?\\(<[0-9]+>\\)?\\]"))))))

Message #20 received at 62556 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 62556 <at> debbugs.gnu.org
Subject: Re: bug#62556: 29.0.60; [PATCH] Fix regression when calling
 'eshell-command' with a pipeline in the background
Date: Sat, 01 Apr 2023 11:05:02 +0300

> Date: Sat, 1 Apr 2023 00:36:33 -0700
> Cc: 62556 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> > As another annoyance, I get this prompt, and must type "yes RET":
> > 
> >       passed   1/16  eshell-test/command-running-p (0.203125 sec)
> >       passed   2/16  eshell-test/eshell-command/background (0.015625 sec)
> >    Buffer "*Eshell Async Command Output*" has a running process; kill it? (yes or no) yes
> >       passed   3/16  eshell-test/eshell-command/background-pipeline (2.453125 sec)
> > 
> > Any suggestions?
> 
> Does this patch fix it?

Yes, thanks.

Message #23 received at 62556 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62556 <at> debbugs.gnu.org
Subject: Re: bug#62556: 29.0.60; [PATCH] Fix regression when calling
 'eshell-command' with a pipeline in the background
Date: Sat, 1 Apr 2023 10:31:44 -0700

On 4/1/2023 1:05 AM, Eli Zaretskii wrote:
>> Date: Sat, 1 Apr 2023 00:36:33 -0700
>> Cc: 62556 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>> Does this patch fix it?
> 
> Yes, thanks.

Thanks. Pushed as 89e337c3fc9.

I'll make sure these issues are addressed in the patches for master and 
then push them in a day or so, too.

Message #28 received at 62556-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62556-done <at> debbugs.gnu.org
Subject: Re: bug#62556: 29.0.60; [PATCH] Fix regression when calling
 'eshell-command' with a pipeline in the background
Date: Sun, 2 Apr 2023 15:09:01 -0700

On 4/1/2023 10:31 AM, Jim Porter wrote:
> I'll make sure these issues are addressed in the patches for master and 
> then push them in a day or so, too.

Pushed to master as 093a360251a. Closing this now.

Hopefully this works on MS-Windows; I think I wrote it in a way where it 
should behave the same on all platforms. If not, just let me know, and 
I'll try to fix it.

Message #31 received at 62556 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 62556 <at> debbugs.gnu.org
Subject: Re: bug#62556: 29.0.60; [PATCH] Fix regression when calling
 'eshell-command' with a pipeline in the background
Date: Thu, 06 Apr 2023 13:32:22 +0300

> Date: Sun, 2 Apr 2023 15:09:01 -0700
> From: Jim Porter <jporterbugs <at> gmail.com>
> Cc: 62556-done <at> debbugs.gnu.org
> 
> On 4/1/2023 10:31 AM, Jim Porter wrote:
> > I'll make sure these issues are addressed in the patches for master and 
> > then push them in a day or so, too.
> 
> Pushed to master as 093a360251a. Closing this now.
> 
> Hopefully this works on MS-Windows; I think I wrote it in a way where it 
> should behave the same on all platforms. If not, just let me know, and 
> I'll try to fix it.

Which tests are those?  The commit you name has no changes for the
test suite, so I'm unsure which tests to run.

Message #34 received at 62556 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62556 <at> debbugs.gnu.org
Subject: Re: bug#62556: 29.0.60; [PATCH] Fix regression when calling
 'eshell-command' with a pipeline in the background
Date: Thu, 6 Apr 2023 11:08:05 -0700

On 4/6/2023 3:32 AM, Eli Zaretskii wrote:
> Which tests are those?  The commit you name has no changes for the
> test suite, so I'm unsure which tests to run.

The commit is 267fca267fe, and the test file I changed is 
test/lisp/eshell/eshell-tests.el. (That was the commit immediately prior 
to 093a360251a in my patch series.)

Message #37 received at 62556 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 62556 <at> debbugs.gnu.org
Subject: Re: bug#62556: 29.0.60; [PATCH] Fix regression when calling
 'eshell-command' with a pipeline in the background
Date: Thu, 06 Apr 2023 21:28:00 +0300

> Date: Thu, 6 Apr 2023 11:08:05 -0700
> Cc: 62556 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> On 4/6/2023 3:32 AM, Eli Zaretskii wrote:
> > Which tests are those?  The commit you name has no changes for the
> > test suite, so I'm unsure which tests to run.
> 
> The commit is 267fca267fe, and the test file I changed is 
> test/lisp/eshell/eshell-tests.el. (That was the commit immediately prior 
> to 093a360251a in my patch series.)

Thanks, those tests work on MS-Windows.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.