eshell-batch-file

eshell-batch-file

[Eli Zaretskii]

Jim,

Is eshell-batch-file supposed to work on MS-Windows?  Its
implementation seems to be full of Unix-isms, so maybe it isn't
supposed to work?  If it is supposed to work, can you describe its
internal workings, so I could take a look?  The relevant test in the
test suite currently fails on MS-Windows, but I'm uncertain whether
it's because it expects a file with hash-bang to be executable by the
shell or for some more fundamental reasons.

Thanks.

Re: eshell-batch-file

[Jim Porter]

On 6/8/2024 11:11 PM, Eli Zaretskii wrote:
> Is eshell-batch-file supposed to work on MS-Windows?  Its
> implementation seems to be full of Unix-isms, so maybe it isn't
> supposed to work?  If it is supposed to work, can you describe its
> internal workings, so I could take a look?

I think it should work on MS-Windows, although MS-Windows users would 
need to configure things differently. (Users should be able to set up a 
file association handler for ".esh" files that calls Emacs with with the 
right arguments.)

If you do the following on the command line, do things work?

     echo "echo hello" > script.esh
     emacs --batch -f eshell-batch-file script.esh

(It should just print "hello" and nothing else.)

> The relevant test in the
> test suite currently fails on MS-Windows, but I'm uncertain whether
> it's because it expects a file with hash-bang to be executable by the
> shell or for some more fundamental reasons.

Sorry about that. That test should probably be disabled on MS-Windows as 
you've done. I've added a new test though that should hopefully pass on 
all systems, including MS-Windows.

Re: eshell-batch-file

[Eli Zaretskii]

> Date: Sun, 9 Jun 2024 11:55:22 -0700
> Cc: emacs-devel@gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 6/8/2024 11:11 PM, Eli Zaretskii wrote:
> > Is eshell-batch-file supposed to work on MS-Windows?  Its
> > implementation seems to be full of Unix-isms, so maybe it isn't
> > supposed to work?  If it is supposed to work, can you describe its
> > internal workings, so I could take a look?
> 
> I think it should work on MS-Windows, although MS-Windows users would 
> need to configure things differently. (Users should be able to set up a 
> file association handler for ".esh" files that calls Emacs with with the 
> right arguments.)
> 
> If you do the following on the command line, do things work?
> 
>      echo "echo hello" > script.esh
>      emacs --batch -f eshell-batch-file script.esh
> 
> (It should just print "hello" and nothing else.)

It works.

> > The relevant test in the
> > test suite currently fails on MS-Windows, but I'm uncertain whether
> > it's because it expects a file with hash-bang to be executable by the
> > shell or for some more fundamental reasons.
> 
> Sorry about that. That test should probably be disabled on MS-Windows as 
> you've done. I've added a new test though that should hopefully pass on 
> all systems, including MS-Windows.

Thanks, it works, and I've now removed skip-unless from that test.

Shell quoting in Eshell (was: eshell-batch-file)

[Eli Zaretskii]

Jim,

As long as I have your attention: there's a subtle point with using
shell-quote-argument with Eshell.  By default, that function adapts
itself to the system's default shell.  In particular, on Windows the
quoting is as expected by cmd.exe and the startup code of Windows
programs.  This is TRT when invoking the external shell, but it is
wrong when invoking programs via Eshell, because Eshell implements
quoting rules of a Posix shell.  So in many cases one needs to call
shell-quote-argument with its 2nd argument non-nil.  The problem, in
particular with the Eshell test suite, is that that the same utility
functions are used with many different commands and many different
situations, and so in general it is almost impossible to fix this
inside the utility function which actually quotes the arguments,
because the function doesn't know how will the command be called and
even what are the arguments it is quoting.

Curiously, there is already eshell-quote-argument, but it doesn't
produce the same effect as shell-quote-argument with 2nd arg non-nil,
which is why in esh-proc-tests.el I couldn't use
eshell-quote-argument.  Any idea why eshell-quote-argument is
different?

In any case, I suggest at some point to discuss how to handle quoting
in a more systematic way, because it was a cause of constant problems
when I ran the Eshell tests today and fixed those which failed.

Thanks.

Re: Shell quoting in Eshell (was: eshell-batch-file)

[Jim Porter]

On 6/9/2024 12:15 PM, Eli Zaretskii wrote:
> ... The problem, in
> particular with the Eshell test suite, is that that the same utility
> functions are used with many different commands and many different
> situations, and so in general it is almost impossible to fix this
> inside the utility function which actually quotes the arguments,
> because the function doesn't know how will the command be called and
> even what are the arguments it is quoting.

I'll take a look at this in some more detail. I've tried to be cautious 
about this in the tests, but since I only have a GNU/Linux dev 
environment set up, it seems some mistakes have slipped through.

> Curiously, there is already eshell-quote-argument, but it doesn't
> produce the same effect as shell-quote-argument with 2nd arg non-nil,
> which is why in esh-proc-tests.el I couldn't use
> eshell-quote-argument.  Any idea why eshell-quote-argument is
> different?

Hmm, I'm not immediately sure, although I have a feeling that's a bug. 
In particular, 'eshell-quote-argument' doesn't quote parens, which seems 
like a problem since that's how you write Lisp forms in Eshell. Surely 
we'd want to quote those. I'll take a look though, and see if I can fix 
things so we can use 'eshell-quote-argument' in the tests at the right 
spots.

> In any case, I suggest at some point to discuss how to handle quoting
> in a more systematic way, because it was a cause of constant problems
> when I ran the Eshell tests today and fixed those which failed.

Thanks for all the fixes. I'll go through them and see if there are any 
subsequent improvements I should make. If you run into any similar 
problems in the future, feel free to ping me first and I can try to make 
the fixes on my end (so long as I'm available) so you don't have to.

Re: Shell quoting in Eshell (was: eshell-batch-file)

[Jim Porter]

On 6/9/2024 1:07 PM, Jim Porter wrote:
> On 6/9/2024 12:15 PM, Eli Zaretskii wrote:
>> Curiously, there is already eshell-quote-argument, but it doesn't
>> produce the same effect as shell-quote-argument with 2nd arg non-nil,
>> which is why in esh-proc-tests.el I couldn't use
>> eshell-quote-argument.  Any idea why eshell-quote-argument is
>> different?
> 
> Hmm, I'm not immediately sure, although I have a feeling that's a bug. 
> In particular, 'eshell-quote-argument' doesn't quote parens, which seems 
> like a problem since that's how you write Lisp forms in Eshell. Surely 
> we'd want to quote those. I'll take a look though, and see if I can fix 
> things so we can use 'eshell-quote-argument' in the tests at the right 
> spots.

Ah, I figured it out. 'eshell-quote-argument' only produces accurate 
results when called from inside an Eshell buffer. It also had a bug 
where quoting newlines didn't work properly. I've fixed the latter issue 
and documented/worked-around the former, and then updated the tests 
where appropriate.

There are just two changes you made that I'm not quite sure what to do 
about. When you get the chance, could you apply the following patch and 
report back the test failures you get?

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


diff --git a/test/lisp/eshell/esh-proc-tests.el 
b/test/lisp/eshell/esh-proc-tests.el
index 90bbb4fa14e..6dca1f7ec1c 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -289,7 +289,7 @@ esh-proc-test/kill-pipeline
       (eshell-wait-for-subprocess t)
       (should (string-match-p
                ;; "interrupt\n" is for MS-Windows.
-              (rx (or "interrupt\n" "killed\n" "killed: 9\n" ""))
+              (rx (or "interrupt\n" "killed\n" "killed: 9\n"))
                (buffer-substring-no-properties
                 output-start (eshell-end-of-output)))))))

diff --git a/test/lisp/eshell/eshell-tests-helpers.el 
b/test/lisp/eshell/eshell-tests-helpers.el
index acbe57a7283..bfd829c95e9 100644
--- a/test/lisp/eshell/eshell-tests-helpers.el
+++ b/test/lisp/eshell/eshell-tests-helpers.el
@@ -179,12 +179,7 @@ eshell-test-command-result

  (defun eshell-command-result--equal (_command actual expected)
    "Compare the ACTUAL result of a COMMAND with its EXPECTED value."
-  (or (equal actual expected)
-      ;; Compare case-isensitively on case-insensitive filesystems.
-      (and (memq system-type '(windows-nt ms-dos))
-           (stringp actual)
-           (stringp expected)
-           (string-equal-ignore-case actual expected))))
+  (equal actual expected))

  (defun eshell-command-result--equal-explainer (command actual expected)
    "Explain the result of `eshell-command-result--equal'."

Re: Shell quoting in Eshell (was: eshell-batch-file)

[Eli Zaretskii]

> Date: Sun, 9 Jun 2024 15:37:05 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: emacs-devel@gnu.org
> 
> There are just two changes you made that I'm not quite sure what to do 
> about. When you get the chance, could you apply the following patch and 
> report back the test failures you get?

I get this failure:

  Test esh-proc-test/kill-pipeline backtrace:
    signal(ert-test-failed (((should (string-match-p (rx (or "interrupt\
    ert-fail(((should (string-match-p (rx (or "interrupt\n" "killed\n" "
    (if (unwind-protect (setq value-164 (apply fn-162 args-163)) (setq f
    (let (form-description-166) (if (unwind-protect (setq value-164 (app
    (let ((value-164 'ert-form-evaluation-aborted-165)) (let (form-descr
    (let* ((fn-162 #'string-match-p) (args-163 (condition-case err (list
    (let ((output-start (eshell-beginning-of-output))) (eshell-kill-proc
    (save-current-buffer (set-buffer eshell-buffer) (eshell-insert-comma
    (unwind-protect (save-current-buffer (set-buffer eshell-buffer) (esh
    (let ((eshell-buffer (eshell t))) (unwind-protect (save-current-buff
    (let ((process-environment (cons "HISTFILE" process-environment)) (e
    (progn (let ((process-environment (cons "HISTFILE" process-environme
    (unwind-protect (progn (let ((process-environment (cons "HISTFILE" p
    (let* ((coding-system-for-write nil) (temp-file (file-name-as-direct
    (save-current-buffer (let* ((coding-system-for-write nil) (temp-file
    #f(lambda () [t] (let ((value-155 (gensym "ert-form-evaluation-abort
    #f(compiled-function () #<bytecode -0x1b68e6722d1837b4>)()
    handler-bind-1(#f(compiled-function () #<bytecode -0x1b68e6722d1837b
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name esh-proc-test/kill-pipeline :document
    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" "--eval" "(setq treesit-extra-l
    command-line()
    normal-top-level()
  Test esh-proc-test/kill-pipeline condition:
      (ert-test-failed
       ((should
	 (string-match-p (rx ...)
			 (buffer-substring-no-properties output-start ...)))
	:form
	(string-match-p "\\(?:\\(?:interrupt\\|killed\\(?:: 9\\)?\\)\n\\)"
			"")
	:value nil))
     FAILED   4/23  esh-proc-test/kill-pipeline (0.131174 sec) at lisp/eshell/esh-proc-tests.el:283

Why do we expect to see "killed" or "interrupt" in this case?

Re: Shell quoting in Eshell (was: eshell-batch-file)

[Jim Porter]

On 6/15/2024 2:38 AM, Eli Zaretskii wrote:
> I get this failure:
[snip]
>    Test esh-proc-test/kill-pipeline condition:
>        (ert-test-failed
>         ((should
> 	 (string-match-p (rx ...)
> 			 (buffer-substring-no-properties output-start ...)))
> 	:form
> 	(string-match-p "\\(?:\\(?:interrupt\\|killed\\(?:: 9\\)?\\)\n\\)"
> 			"")
> 	:value nil))
>       FAILED   4/23  esh-proc-test/kill-pipeline (0.131174 sec) at lisp/eshell/esh-proc-tests.el:283
> 
> Why do we expect to see "killed" or "interrupt" in this case?

Eshell intends to print the exit message from the process sentinel of 
the tail process here (unless the message starts with "finished" or 
"exited"). After thinking this over, I realized that the test failure 
you encountered was actually a race condition: Eshell tries to kill all 
the processes in the pipeline, which -- if Eshell is fast enough -- 
sends a signal to the tail process, making us print the exit message. 
However, if Eshell is slow, the tail process might exit on its own 
before we can signal it, leading to no message.

I've now fixed this test so that we check that there are zero or one 
exit messages (the old code was too lenient in some regards).

I think the only remaining thing to do here is to figure out why you 
needed to change 'eshell-command-result--equal'. I imagine this is 
because I made a mistake somewhere with case normalization (my guess is 
in the globbing tests). Hopefully it's just a problem with the tests, 
but it could be that there's a bug hiding in Eshell proper.

Could you try the diff below and report back the failures when you get a 
chance?


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


diff --git a/test/lisp/eshell/eshell-tests-helpers.el 
b/test/lisp/eshell/eshell-tests-helpers.el
index acbe57a7283..bfd829c95e9 100644
--- a/test/lisp/eshell/eshell-tests-helpers.el
+++ b/test/lisp/eshell/eshell-tests-helpers.el
@@ -179,12 +179,7 @@ eshell-test-command-result

  (defun eshell-command-result--equal (_command actual expected)
    "Compare the ACTUAL result of a COMMAND with its EXPECTED value."
-  (or (equal actual expected)
-      ;; Compare case-isensitively on case-insensitive filesystems.
-      (and (memq system-type '(windows-nt ms-dos))
-           (stringp actual)
-           (stringp expected)
-           (string-equal-ignore-case actual expected))))
+  (equal actual expected))

  (defun eshell-command-result--equal-explainer (command actual expected)
    "Explain the result of `eshell-command-result--equal'."

Re: Shell quoting in Eshell (was: eshell-batch-file)

[Eli Zaretskii]

> Date: Sat, 15 Jun 2024 11:36:57 -0700
> Cc: emacs-devel@gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> > Why do we expect to see "killed" or "interrupt" in this case?
> 
> Eshell intends to print the exit message from the process sentinel of 
> the tail process here (unless the message starts with "finished" or 
> "exited"). After thinking this over, I realized that the test failure 
> you encountered was actually a race condition: Eshell tries to kill all 
> the processes in the pipeline, which -- if Eshell is fast enough -- 
> sends a signal to the tail process, making us print the exit message. 
> However, if Eshell is slow, the tail process might exit on its own 
> before we can signal it, leading to no message.
> 
> I've now fixed this test so that we check that there are zero or one 
> exit messages (the old code was too lenient in some regards).

The test succeeds, thanks.

> I think the only remaining thing to do here is to figure out why you 
> needed to change 'eshell-command-result--equal'. I imagine this is 
> because I made a mistake somewhere with case normalization (my guess is 
> in the globbing tests). Hopefully it's just a problem with the tests, 
> but it could be that there's a bug hiding in Eshell proper.

I don't see how you could fix this in the tests, since this is a basic
problem with comparing file names on MS-Windows.  See below.

> Could you try the diff below and report back the failures when you get a 
> chance?

Here:

  Test esh-cmd-test/which/plain/external-program 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
    (let ((eshell-module-loading-messages nil)) (let* ((fn-5 #'eshell-co
    (let ((ert--infos (cons (cons "Command logs: " #'eshell-get-debug-lo
    eshell-command-result-equal("which sh" "d:/usr/MSYS/bin/sh.exe\n")
    #f(lambda () [t] (let* ((fn-167 #'executable-find) (args-168 (condit
    #f(compiled-function () #<bytecode 0xc12a3827bbdeed7>)()
    handler-bind-1(#f(compiled-function () #<bytecode 0xc12a3827bbdeed7>
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name esh-cmd-test/which/plain/external-pro
    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" "--eval" "(setq treesit-extra-l
    command-line()
    normal-top-level()
  Test esh-cmd-test/which/plain/external-program condition:
      Command logs: command: "which sh"

      (ert-test-failed
       ((should
	 (eshell-command-result--equal command
				       (eshell-test-command-result command)
				       result))
	:form
	(eshell-command-result--equal "which sh"
				      #("D:/usr/MSYS/bin/sh.exe\n" 22 23
					(field command-output front-sticky
					       ... rear-nonsticky
					       ... insert-in-front-hooks ...))
				      "d:/usr/MSYS/bin/sh.exe\n")
	:value nil :explanation
	(nonequal-result (command "which sh")
			 (result #("D:/usr/MSYS/bin/sh.exe\n" 22 23 ...))
			 (expected "d:/usr/MSYS/bin/sh.exe\n"))))
     FAILED  59/66  esh-cmd-test/which/plain/external-program (0.003953 sec) at lisp/eshell/esh-cmd-tests.el:538

IOW: you run "which SOMETHING" and pretend to know how it will
capitalize the drive letter.

Re: Shell quoting in Eshell (was: eshell-batch-file)

[Jim Porter]

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

On 6/15/2024 12:10 PM, Eli Zaretskii wrote:
>    Test esh-cmd-test/which/plain/external-program condition:
[snip]
> 	:value nil :explanation
> 	(nonequal-result (command "which sh")
> 			 (result #("D:/usr/MSYS/bin/sh.exe\n" 22 23 ...))
> 			 (expected "d:/usr/MSYS/bin/sh.exe\n"))))
>       FAILED  59/66  esh-cmd-test/which/plain/external-program (0.003953 sec) at lisp/eshell/esh-cmd-tests.el:538
> 
> IOW: you run "which SOMETHING" and pretend to know how it will
> capitalize the drive letter.

Ah ha! If that's the only failure, then I think this would work? (If 
there are other failures, then I could add a PREDICATE argument to 
'eshell-command-result-equal' and use that in the necessary places.)

[-- Attachment #2: eshell-which.diff --]
[-- Type: text/plain, Size: 1843 bytes --]

diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
index 166a0ba1fff..9799afa5665 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -538,8 +538,12 @@ esh-cmd-test/which/plain/eshell-builtin
 (ert-deftest esh-cmd-test/which/plain/external-program ()
   "Check that `which' finds external programs."
   (skip-unless (executable-find "sh"))
-  (eshell-command-result-equal "which sh"
-                               (concat (executable-find "sh") "\n")))
+  (ert-info (#'eshell-get-debug-logs :prefix "Command logs: ")
+    (let ((actual (eshell-test-command-result "which sh"))
+          (expected (concat (executable-find "sh") "\n")))
+      (should (if (file-name-case-insensitive-p actual)
+                  (string-equal-ignore-case actual expected)
+                (string-equal actual expected))))))
 
 (ert-deftest esh-cmd-test/which/plain/not-found ()
   "Check that `which' reports an error for not-found commands."
diff --git a/test/lisp/eshell/eshell-tests-helpers.el b/test/lisp/eshell/eshell-tests-helpers.el
index acbe57a7283..bfd829c95e9 100644
--- a/test/lisp/eshell/eshell-tests-helpers.el
+++ b/test/lisp/eshell/eshell-tests-helpers.el
@@ -179,12 +179,7 @@ eshell-test-command-result
 
 (defun eshell-command-result--equal (_command actual expected)
   "Compare the ACTUAL result of a COMMAND with its EXPECTED value."
-  (or (equal actual expected)
-      ;; Compare case-isensitively on case-insensitive filesystems.
-      (and (memq system-type '(windows-nt ms-dos))
-           (stringp actual)
-           (stringp expected)
-           (string-equal-ignore-case actual expected))))
+  (equal actual expected))
 
 (defun eshell-command-result--equal-explainer (command actual expected)
   "Explain the result of `eshell-command-result--equal'."

Re: Shell quoting in Eshell (was: eshell-batch-file)

[Eli Zaretskii]

> Date: Sat, 15 Jun 2024 13:06:53 -0700
> Cc: emacs-devel@gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 6/15/2024 12:10 PM, Eli Zaretskii wrote:
> >    Test esh-cmd-test/which/plain/external-program condition:
> [snip]
> > 	:value nil :explanation
> > 	(nonequal-result (command "which sh")
> > 			 (result #("D:/usr/MSYS/bin/sh.exe\n" 22 23 ...))
> > 			 (expected "d:/usr/MSYS/bin/sh.exe\n"))))
> >       FAILED  59/66  esh-cmd-test/which/plain/external-program (0.003953 sec) at lisp/eshell/esh-cmd-tests.el:538
> > 
> > IOW: you run "which SOMETHING" and pretend to know how it will
> > capitalize the drive letter.
> 
> Ah ha! If that's the only failure, then I think this would work? (If 
> there are other failures, then I could add a PREDICATE argument to 
> 'eshell-command-result-equal' and use that in the necessary places.)

It works with that change, yes.  But please add a comment there
explaining why this is done.  Since the fix targets a single command,
it can be described in more detail.

Thanks.

Re: Shell quoting in Eshell (was: eshell-batch-file)

[Jim Porter]

On 6/15/2024 10:21 PM, Eli Zaretskii wrote:
>> Date: Sat, 15 Jun 2024 13:06:53 -0700
>> Cc: emacs-devel@gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> Ah ha! If that's the only failure, then I think this would work? (If
>> there are other failures, then I could add a PREDICATE argument to
>> 'eshell-command-result-equal' and use that in the necessary places.)
> 
> It works with that change, yes.  But please add a comment there
> explaining why this is done.  Since the fix targets a single command,
> it can be described in more detail.

Done in aefcccc1d41, with an explanatory comment. Thanks for trying out 
these various Eshell test patches.