Org 9.6-pre and Bash sessions

Org 9.6-pre and Bash sessions

[Rudolf Adamkovič]

Hello there!

I noticed today that Bash source blocks with ':session" does not seem to
work with Org 9.6-pre (3e8648775).  Does anyone else have the problem?

CONFIGURATION FILE:
------------------

> (setq-default org-use-extra-keys t)
> (with-eval-after-load 'org
>   (add-to-list 'org-babel-load-languages '(shell . t))
>   ;; The weirdest API ever.
>   (org-babel-do-load-languages
>    'org-babel-load-languages
>    org-babel-load-languages))

TEST FILE:
---------

> * Something
> 
> 1. Something
> 
>    #+begin_src bash :session test :results output
>    if true
>    then
>      echo "hello"
>    fi
>    #+end_src

REPRODUCTION STEPS:
------------------

Execute the source block with C-c C-c.

Expected:

  Block executes with the result "hello".

Actual:

  Emacs says "Executing Bash unknown at position 22..." and freezes.
  
  (C-g unfreezes Emacs.)

NOTES:
-----

When I remove the leading whitespace ...

> * Something
> 
> 1. Something
> 
> #+begin_src bash :session test :results output
> if true
> then
>   echo "hello"
> fi
> #+end_src

... then Emacs still freezes, but at least the message makes sense,
saying "Bash code block" instead of "Bash unknown".

Rudy
-- 
"Simplicity is complexity resolved."
-- Constantin Brâncuși, 1876-1957

Rudolf Adamkovič <salutis@me.com> [he/him]
Studenohorská 25
84103 Bratislava
Slovakia

Re: Org 9.6-pre and Bash sessions

[Michael Welle]

Hello,

Rudolf Adamkovič <salutis@me.com> writes:

> Hello there!
>
> I noticed today that Bash source blocks with ':session" does not seem to
> work with Org 9.6-pre (3e8648775).  Does anyone else have the problem?
funnily I observed something similar while generating semester projects
for my students.

Example:

#+name: seprj_conf_table
| xxxzwei | 234 2bar        |
| xxxeins | 2342bar         |
| xxxdrei | aaaa${eins}bbbb |

#+begin_src sh :session foo :var conf=seprj_conf_table :separator =
  IFS=$'^\n\t'
  for val in "$conf" ; do
    eval "export '$val'"
  done
  env |grep xxx
#+end_src

With main this hangs until C-g. I have an other machine with a little bit
older Org:

commit 9b62bcc9c60302467d1d45b6c240057e80fce656 (HEAD)
Author: Lars Ingebrigtsen <larsi@gnus.org>
Date:   Sat Sep 24 15:17:23 2022 +0200

    Backport commit 8dacd8cd9 from Emacs

That works as expected. So I guess the regression was introduced within
the last week or two.

Regards
hmw

Re: Org 9.6-pre and Bash sessions

[Ihor Radchenko]

Rudolf Adamkovič <salutis@me.com> writes:

> I noticed today that Bash source blocks with ':session" does not seem to
> work with Org 9.6-pre (3e8648775).  Does anyone else have the problem?

Confirmed.
The cause is a35d16368
Author:     Ihor Radchenko <yantar92@gmail.com>
ob-shell: Fix output containing strings matching `comint-prompt-regexp'

* lisp/ob-comint.el (org-babel-comint-wait-for-output): Do not rely on
`face-at-point' returning non-nil.
* lisp/ob-shell.el (org-babel-shell-set-prompt-commands): New constant
holding shell-specific commands to change prompt.
(org-babel-prompt-command): New variable holding command to be user to
set distinguishable prompt.
(org-babel-shell-initialize): Set `org-babel-prompt-command' according
to shell name.
(org-babel-sh-prompt): New variable holding default shell prompt.
(org-babel-sh-initiate-session): Change the default prompt to
`org-babel-sh-prompt' and alter `comint-prompt-regexp' to match it
tightly.

Fixes https://list.orgmode.org/CKK9TULBP2BG.2UITT31YJV03J@laptop/T/#mc8e3ca2f5f1b9a94040a68b4c6201234b209041c

> CONFIGURATION FILE:
> ------------------
>
>> (setq-default org-use-extra-keys t)
>> (with-eval-after-load 'org
>>   (add-to-list 'org-babel-load-languages '(shell . t))
>>   ;; The weirdest API ever.
>>   (org-babel-do-load-languages
>>    'org-babel-load-languages
>>    org-babel-load-languages))
>
> TEST FILE:
> ---------
>
>> * Something
>> 
>> 1. Something
>> 
>>    #+begin_src bash :session test :results output
>>    if true
>>    then
>>      echo "hello"
>>    fi
>>    #+end_src
>
> REPRODUCTION STEPS:
> ------------------
>
> Execute the source block with C-c C-c.
>
> Expected:
>
>   Block executes with the result "hello".

Is it what you actually saw in the past? I just tried with older Org
versions on my system and the output is

   : [00m > > > hello

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org 9.6-pre and Bash sessions

[Rudolf Adamkovič]

Ihor Radchenko <yantar92@gmail.com> writes:

> Is it what you actually saw in the past? I just tried with older Org
> versions on my system and the output is
>
>    : [00m > > > hello

No.  By "expected", I mean what I expect as a user (and what I would
assert on in a regression test as a developer).

For the record, with Org in Emacs 29, I get:

:
: > > > hello

on the first run and then

:
: > > hello

on the subsequent runs.

Better than the new version but still wrong. :)

Rudy
-- 
"Genius is 1% inspiration and 99% perspiration."
-- Thomas Alva Edison, 1932

Rudolf Adamkovič <salutis@me.com> [he/him]
Studenohorská 25
84103 Bratislava
Slovakia

Re: Org 9.6-pre and Bash sessions

[Ihor Radchenko]

Rudolf Adamkovič <salutis@me.com> writes:

> :
> : > > hello
>
> on the subsequent runs.
>
> Better than the new version but still wrong. :)

And this is what drove me crazy during debugging. I do not understand
the logic of all that ob-comint code.

I have identified that the hang happens because Org does not change PS2
prompt. Just PS1. But fixing this would yield to

: org_babel_sh_prompt> org_babel_sh_prompt> hello

Then, I tried to see how the original code works. And it does not
:facepalm:

I asked emacs-devel
https://yhetil.org/emacs-devel/87y1tgqhmc.fsf@localhost/T/#u

No clue how to read the script output programatically in comint buffer. 

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org 9.6-pre and Bash sessions

[Ihor Radchenko]

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

Ihor Radchenko <yantar92@posteo.net> writes:

> Rudolf Adamkovič <salutis@me.com> writes:
>
>> :
>> : > > hello
>>
>> on the subsequent runs.
>>
>> Better than the new version but still wrong. :)
>
> And this is what drove me crazy during debugging. I do not understand
> the logic of all that ob-comint code.
>
> I have identified that the hang happens because Org does not change PS2
> prompt. Just PS1. But fixing this would yield to
>
> : org_babel_sh_prompt> org_babel_sh_prompt> hello
>
> Then, I tried to see how the original code works. And it does not
> :facepalm:
>
> I asked emacs-devel
> https://yhetil.org/emacs-devel/87y1tgqhmc.fsf@localhost/T/#u

See the attached tentative patch.
I'd appreciate some testing. Hopefully, I did not break anything.
Comint is tricky.


[-- Attachment #2: 0001-ob-shell-Fix-multi-line-scripts-in-sessions.patch --]
[-- Type: text/x-patch, Size: 4766 bytes --]

From 4c6eead351cbdce1b9210a738c65b3a139d1cc0c Mon Sep 17 00:00:00 2001
Message-Id: <4c6eead351cbdce1b9210a738c65b3a139d1cc0c.1666330028.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Fri, 21 Oct 2022 13:21:57 +0800
Subject: [PATCH] ob-shell: Fix multi-line scripts in sessions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/ob-comint.el (org-babel-comint-with-output): Cleanup empty
output.  Such output is spitted unnecessarily for multi-line scripts.
* lisp/ob-shell.el (org-babel-shell-set-prompt-commands): Disable
PS2 and equivalent prompts.  Make sure that PROMPT_COMMAND does not
interfere with PS1 setting in POSIX shells.
(org-babel-sh-evaluate): Do not send input line-by-line.  Instead, let
`org-babel-coming-with-output' handle waiting for the output as well
as recording it.  Update to the new `org-babel-coming-with-output'
behavior of cleaning empty outputs.
* testing/lisp/test-ob-shell.el (test-ob-shell/session): Add test.

Reported-by: Rudolf Adamkovič <salutis@me.com>
Link: https://list.orgmode.org/orgmode/m2r0zboix1.fsf@me.com/
---
 lisp/ob-comint.el             |  2 +-
 lisp/ob-shell.el              | 29 ++++++++++++++---------------
 testing/lisp/test-ob-shell.el |  6 +++++-
 3 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 36a55d36c..ec7d3f6c9 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -109,7 +109,7 @@ (defmacro org-babel-comint-with-output (meta &rest body)
 		      "\n" "[\r\n]+" (regexp-quote (or ,full-body "")))
 		     string-buffer))
 	   (setq string-buffer (substring string-buffer (match-end 0))))
-	 (split-string string-buffer comint-prompt-regexp)))))
+         (delete "" (split-string string-buffer comint-prompt-regexp))))))
 
 (defun org-babel-comint-input-command (buffer cmd)
   "Pass CMD to BUFFER.
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 4d579ae87..d38d2d335 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -47,10 +47,15 @@ (defvar org-babel-default-header-args:shell '())
 (defvar org-babel-shell-names)
 
 (defconst org-babel-shell-set-prompt-commands
-  '(("fish" . "function fish_prompt\n\techo \"%s\"\nend")
-    ("csh" . "set prompt=\"%s\"")
+  '(;; Fish has no PS2 equivalent.
+    ("fish" . "function fish_prompt\n\techo \"%s\"\nend")
+    ;; prompt2 is like PS2 in POSIX shells.
+    ("csh" . "set prompt=\"%s\"\nset prompt2=\"\"")
+    ;; PowerShell, similar to fish, does not have PS2 equivalent.
     ("posh" . "function prompt { \"%s\" }")
-    (t . "PS1=\"%s\""))
+    ;; PROMPT_COMMAND can override PS1 settings.  Disable it.
+    ;; Disable PS2 to avoid garbage in multi-line inputs.
+    (t . "PROMPT_COMMAND=;PS1=\"%s\";PS2="))
   "Alist assigning shells with their prompt setting command.
 
 Each element of the alist associates a shell type from
@@ -299,20 +304,14 @@ (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
 	     #'org-babel-sh-strip-weird-long-prompt
 	     (mapcar
 	      #'org-trim
-	      (butlast
+	      (butlast ; Remove eoe indicator
 	       (org-babel-comint-with-output
 		   (session org-babel-sh-eoe-output t body)
-		 (dolist (line (append (split-string (org-trim body) "\n")
-				       (list org-babel-sh-eoe-indicator)))
-		   (insert line)
-		   (comint-send-input nil t)
-		   (while (save-excursion
-			    (goto-char comint-last-input-end)
-			    (not (re-search-forward
-				  comint-prompt-regexp nil t)))
-		     (accept-process-output
-		      (get-buffer-process (current-buffer))))))
-	       2))
+                 (insert (org-trim body) "\n"
+                         org-babel-sh-eoe-indicator)
+		 (comint-send-input nil t))
+               ;; Remove `org-babel-sh-eoe-indicator' output line.
+	       1))
 	     "\n"))
 	   ;; External shell script, with or without a predefined
 	   ;; shebang.
diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index a0d5a8d22..05c369174 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -46,7 +46,11 @@ (ert-deftest test-ob-shell/session ()
 ob-comint.el, which was not previously tested."
   (let ((res (org-babel-execute:sh "echo 1; echo 2" '((:session . "yes")))))
     (should res)
-    (should (listp res))))
+    (should (listp res)))
+  ;; Test multi-line input.
+  (let ((res (org-babel-execute:sh "if true; then\necho \"yes\"\nfi" '((:session . "yes")))))
+    (should res)
+    (should (string= "yes" res))))
 
 ; A list of tests using the samples in ob-shell-test.org
 (ert-deftest ob-shell/generic-uses-no-arrays ()
-- 
2.35.1


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



-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org 9.6-pre and Bash sessions

[Rudolf Adamkovič]

Ihor Radchenko <yantar92@posteo.net> writes:

> See the attached tentative patch.
> I'd appreciate some testing. Hopefully, I did not break anything.

I did some testing and found no issues.

Great work!

Below, I include some suggestions regarding the patch.

> * lisp/ob-comint.el (org-babel-comint-with-output): Cleanup empty
> output.  Such output is spitted unnecessarily for multi-line scripts.

- 'cleanup' (noun) -> 'clean up' (verb)
- 'spitted' -> ??? (did you mean 'split', perhaps?)

> * testing/lisp/test-ob-shell.el (test-ob-shell/session): Add test.

- 'test' -> 'a test'

> +  (let ((res (org-babel-execute:sh "if true; then\necho \"yes\"\nfi" '((:session . "yes")))))

This line has 93 columns, which makes it needlessly hard to read.

Consider,

(let ((result (org-babel-execute:sh
                 "if true \n then \n echo yes \n fi"
                 '((:session . "yes")))))
    (should result)
    (should (string= "yes" result)))

I replaced the remaining semicolons with newlines, given the test
focuses on "multi-line scripts".  Further, I removed the needless
(escaped) quotes around "yes".  Lastly, I renamed 'res' to 'result',
improving readability and consistency, given the file already uses
'result' in another 'let'.

Rudy
-- 
"Programming reliably -- must be an activity of an undeniably
mathematical nature […] You see, mathematics is about thinking, and
doing mathematics is always trying to think as well as possible."
-- Edsger W. Dijkstra, 1981

Rudolf Adamkovič <salutis@me.com> [he/him]
Studenohorská 25
84103 Bratislava
Slovakia

Re: Org 9.6-pre and Bash sessions

[Ihor Radchenko]

Rudolf Adamkovič <salutis@me.com> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> See the attached tentative patch.
>> I'd appreciate some testing. Hopefully, I did not break anything.
>
> I did some testing and found no issues.
>
> Great work!
>
> Below, I include some suggestions regarding the patch.

Thanks!
Applied onto main incorporating your suggestions.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=801c93638ab4b6616567035c6438ad229bc14c1d

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org 9.6-pre and Bash sessions

[Rudolf Adamkovič]

Ihor Radchenko <yantar92@posteo.net> writes:

>> I did some testing and found no issues.

Update: Today I needed to run some Bash, and it seems broken.

PROBLEM 1: WITHOUT A SESSION
============================

CONFIGURATION:

  Emacs 29, Org from Git (2f5e7103e59f06631e985d3dd39af21b5b7464ea)

REPRODUCTION STEPS:

  Run the following code block:
  
  #+begin_src bash :results output
  echo one > one.txt
  echo two > two.txt
  diff one.txt two.txt
  #+end_src
  
ACTUAL RESULTS:
  
  #+RESULTS:
  
EXPECTED RESULTS:
  
  #+RESULTS:
  : 1c1
  : < one
  : ---
  : > two

PROBLEM 2: WITH A SESSION
=========================

CONFIGURATION:

  Emacs 29, Org from Git (2f5e7103e59f06631e985d3dd39af21b5b7464ea)

REPRODUCTION STEPS:

  #+begin_src bash :session bash :results output
  echo one > one.txt
  echo two > two.txt
  diff one.txt two.txt
  #+end_src

ACTUAL:
  
  #+RESULTS:
  : org_babel_sh_prompt> 1c1
  : < one
  : ---
  : > two
  
EXPECTED:
  
  #+RESULTS:
  : 1c1
  : < one
  : ---
  : > two

ADDENDUM
========

A friend of mine, who uses Org that comes with Emacs 28.1 on Linux,
tried the PROBLEM 1 source block shown above, the one *without* a
session, and he got no results either.

(We might need better tests.)

Rudy
-- 
"One can begin to reason only when a clear picture has been formed in
the imagination."
-- Walter Warwick Sawyer, Mathematician's Delight, 1943

Rudolf Adamkovič <salutis@me.com> [he/him]
Studenohorská 25
84103 Bratislava
Slovakia

Re: Org 9.6-pre and Bash sessions

[Ihor Radchenko]

Rudolf Adamkovič <salutis@me.com> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>>> I did some testing and found no issues.
>
> Update: Today I needed to run some Bash, and it seems broken.
>
> PROBLEM 1: WITHOUT A SESSION
> ============================
>
> CONFIGURATION:
>
>   Emacs 29, Org from Git (2f5e7103e59f06631e985d3dd39af21b5b7464ea)
>
> REPRODUCTION STEPS:
>
>   Run the following code block:
>   
>   #+begin_src bash :results output
>   echo one > one.txt
>   echo two > two.txt
>   diff one.txt two.txt
>   #+end_src
>   
> ACTUAL RESULTS:
>   
>   #+RESULTS:
>   
> EXPECTED RESULTS:
>   
>   #+RESULTS:
>   : 1c1
>   : < one
>   : ---
>   : > two

This is actually expected. Without session, ob-shell discards results of
src blocks that fail and display *Error* window (empty in this case).

If you try 

  #+begin_src bash :results output
  echo one > one.txt
  echo two > two.txt
  diff one.txt two.txt || true
  #+end_src

things will work as expected.

I guess we can display a bit more info in `org-babel-eval'.

> PROBLEM 2: WITH A SESSION
> =========================
>
> CONFIGURATION:
>
>   Emacs 29, Org from Git (2f5e7103e59f06631e985d3dd39af21b5b7464ea)
>
> REPRODUCTION STEPS:
>
>   #+begin_src bash :session bash :results output
>   echo one > one.txt
>   echo two > two.txt
>   diff one.txt two.txt
>   #+end_src
>
> ACTUAL:
>   
>   #+RESULTS:
>   : org_babel_sh_prompt> 1c1
>   : < one
>   : ---
>   : > two
>   
> EXPECTED:
>   
>   #+RESULTS:
>   : 1c1
>   : < one
>   : ---
>   : > two

Confirmed.
I know what is the problem. It is comint sometimes throwing output
without newlines, which leads to comint-prompt-regexp not matching all
the prompts after concatenating. Will take a look tomorrow.

> ADDENDUM
> ========
>
> A friend of mine, who uses Org that comes with Emacs 28.1 on Linux,
> tried the PROBLEM 1 source block shown above, the one *without* a
> session, and he got no results either.
>
> (We might need better tests.)

Better tests are always needed :)
https://orgmode.org/list/874k81bsvz.fsf@localhost

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org 9.6-pre and Bash sessions

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> This is actually expected. Without session, ob-shell discards results of
> src blocks that fail and display *Error* window (empty in this case).
>
> If you try 
>
>   #+begin_src bash :results output
>   echo one > one.txt
>   echo two > two.txt
>   diff one.txt two.txt || true
>   #+end_src
>
> things will work as expected.
>
> I guess we can display a bit more info in `org-babel-eval'.

I improved the error buffer a bit and also made babel return the
available output with non-0 exit code.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=ab7eff9d9c0968392bd9783d72c56a4023bd10be
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=f8a9cd23087799519bd709ea5c208ed29cb6587f

>>   #+begin_src bash :session bash :results output
>>   echo one > one.txt
>>   echo two > two.txt
>>   diff one.txt two.txt
>>   #+end_src
>>
>> ACTUAL:
>>   
>>   #+RESULTS:
>>   : org_babel_sh_prompt> 1c1
>>   : < one
>>   : ---
>>   : > two
>>   
>> EXPECTED:
>>   
>>   #+RESULTS:
>>   : 1c1
>>   : < one
>>   : ---
>>   : > two
>
> Confirmed.
> I know what is the problem. It is comint sometimes throwing output
> without newlines, which leads to comint-prompt-regexp not matching all
> the prompts after concatenating. Will take a look tomorrow.

Fixed.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=1ef420b19d2e310d9ef67e090ef84cdb6da4744d

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org 9.6-pre and Bash sessions

[Rudolf Adamkovič]

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

Ihor,

Thank you for investigating, explaining, and also fixing the problem!  I
pulled the latest 'main' and everything works a bit, it seems.

Then, to avoid walking in circles, I decided to write some tests, for I
think shell blocks should never, never, never break in such basic ways,
let alone in production Emacs.

Please, see the WIP patch attached to this message.

I noticed that the following tests do not pass:

1. ob-shell/error-output-after-success

   We seem to trash error output, such as warnings, on success.  I think
   we should not do this.  Now, on the execution of "echo X &>2", Org
   says "Code block produced no output."  But that does hold true.  The
   block did produce output, just on the other output stream, namely
   error output.

2. ob-shell/error-output-after-failure

   We seem not to show the exit code in this case.  Why?

3. ob-shell/exit-codes

   Should we add a newline after the exit code message?

Rudy


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-test-ob-shell-Add-tests-for-some-recent-changes.patch --]
[-- Type: text/x-patch, Size: 3021 bytes --]

From 90432170552a6e922d48d51138b2062aa11e5575 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rudolf=20Adamkovi=C4=8D?= <salutis@me.com>
Date: Wed, 26 Oct 2022 13:35:26 +0200
Subject: [PATCH] WIP test-ob-shell: Add tests for some recent changes

---
 testing/lisp/test-ob-shell.el | 60 +++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index 4c00faa49..8cd967343 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -170,6 +170,66 @@ ob-comint.el, which was not previously tested."
 	      "#+BEGIN_SRC sh :results table\necho 'I \"want\" it all'\n#+END_SRC"
 	    (org-babel-execute-src-block)))))
 
+;;; Standard output
+
+(ert-deftest ob-shell/standard-output-after-failure ()
+  "Test standard output after exiting with a non-zero code."
+  (should (= 1
+             (org-babel-execute:sh
+              "echo 1; exit 2" nil))))
+
+;;; Error output
+
+(ert-deftest ob-shell/error-output-after-success ()
+  "Test that error output shows in the error buffer after exiting
+with a zero code."
+  (should
+   (string= "1
+[ Babel evaluation exited with code 2 ]"
+            (progn (org-babel-eval-wipe-error-buffer)
+                   (org-babel-execute:sh
+                    "echo 1 >&2" nil)
+                   (with-current-buffer org-babel-error-buffer-name
+                     (buffer-string))))))
+
+(ert-deftest ob-shell/error-output-after-failure ()
+  "Test that error output shows in the error buffer, alongside the
+exit code, after exiting with a non-zero code."
+  (should
+   (string= "1
+[ Babel evaluation exited with code 2 ]"
+            (progn (org-babel-eval-wipe-error-buffer)
+                   (org-babel-execute:sh
+                    "echo 1 >&2; exit 2" nil)
+                   (with-current-buffer org-babel-error-buffer-name
+                     (buffer-string))))))
+
+;;; Exit codes
+
+(ert-deftest ob-shell/exit-code ()
+  "Test that the exit code shows in the error buffer after exiting
+with a non-zero return code."
+  (should
+   (string= "[ Babel evaluation exited with code 1 ]"
+            (progn (org-babel-eval-wipe-error-buffer)
+                   (org-babel-execute:sh
+                    "exit 1" nil)
+                   (with-current-buffer org-babel-error-buffer-name
+                     (buffer-string))))))
+
+(ert-deftest ob-shell/exit-codes ()
+  "Test that multiple exit codes show in the error buffer after
+exiting with a non-zero return code more than once."
+  (should
+   (string= "[ Babel evaluation exited with code 1 ]
+[ Babel evaluation exited with code 2 ]"
+            (progn (org-babel-eval-wipe-error-buffer)
+                   (org-babel-execute:sh
+                    "exit 1" nil)
+                   (org-babel-execute:sh
+                    "exit 2" nil)
+                   (with-current-buffer org-babel-error-buffer-name
+                     (buffer-string))))))
 
 (provide 'test-ob-shell)
 
-- 
2.38.1


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

-- 
"One can begin to reason only when a clear picture has been formed in
the imagination."
-- Walter Warwick Sawyer, Mathematician's Delight, 1943

Rudolf Adamkovič <salutis@me.com> [he/him]
Studenohorská 25
84103 Bratislava
Slovakia

Re: Org 9.6-pre and Bash sessions

[Rudolf Adamkovič]

Rudolf Adamkovič <salutis@me.com> writes:

> I pulled the latest 'main' and everything works a bit, it seems.

I meant to say "a bit better" not "a bit". :)

Rudy
-- 
"It is no paradox to say that in our most theoretical moods we may be
nearest to our most practical applications."
-- Alfred North Whitehead, 1861-1947

Rudolf Adamkovič <salutis@me.com> [he/him]
Studenohorská 25
84103 Bratislava
Slovakia

Re: Org 9.6-pre and Bash sessions

[Ihor Radchenko]

Rudolf Adamkovič <salutis@me.com> writes:

> Thank you for investigating, explaining, and also fixing the problem!  I
> pulled the latest 'main' and everything works a bit, it seems.
>
> Then, to avoid walking in circles, I decided to write some tests, for I
> think shell blocks should never, never, never break in such basic ways,
> let alone in production Emacs.

Thanks!
I will not look much into details of the patch for now. Let's discuss
the general expectations from shell blocks first. You seem to be
missing several things described in the documentation.

> I noticed that the following tests do not pass:
>
> 1. ob-shell/error-output-after-success
>
>    We seem to trash error output, such as warnings, on success.  I think
>    we should not do this.  Now, on the execution of "echo X &>2", Org
>    says "Code block produced no output."  But that does hold true.  The
>    block did produce output, just on the other output stream, namely
>    error output.

Trashing error output is expected by default.
I suggest you to read through
https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html

The default :results argument is output, which implies:

    output returns stdout, default

As expected, stdout is empty in your example. ob-shell follows the spec
here.

Another question is if users may want to obtain strerr in some cases. A
new feature like :results stderr might be implemented. Though users can
already do the usual 2>&1 to merge stderr into stdout.

> 2. ob-shell/error-output-after-failure
>
>    We seem not to show the exit code in this case.  Why?

Exit code was not shown in the past until my commit
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=ab7eff9d9c0968392bd9783d72c56a4023bd10be

I guess that showing the error code unconditionally (when exit code is
non-0) could be an option.

Now done in
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=d87a496b54b502d98e49f8bd6596e0562be9d105

> 3. ob-shell/exit-codes
>
>    Should we add a newline after the exit code message?

Done.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org 9.6-pre and Bash sessions

[Rudolf Adamkovič]

Ihor Radchenko <yantar92@posteo.net> writes:

>> 1. ob-shell/error-output-after-success
>>
>>    We seem to trash error output, such as warnings, on success.  I
>>    think we should not do this.  Now, on the execution of "echo X
>>    &>2", Org says "Code block produced no output."  But that does
>>    hold true.  The block did produce output, just on the other output
>>    stream, namely error output.

> Trashing error output is expected by default. [...] I suggest you to
> read through [...] ob-shell follows the spec here.

I get that ':results output' makes Org capture standard output, but not
error output, for its #+RESULTS.  However, if you look at the test in
question, you will see that it expects error output in the *Org-Babel
Error Output* buffer, not in the #+RESULTS.  We should not trash error
output; it belongs in the *Org-Babel Error Output* buffer.

>> 2. ob-shell/error-output-after-failure

> Now done in [...]

Thanks!  This works *correctly* now.

>> 3. ob-shell/exit-codes
>>
>>    Should we add a newline after the exit code message?

> Done.

I updated the tests and now every string needs a final newline, which
does not seem correct.  If we let the tests guide our design, all
becomes clear.  Buffers that use the 'compilation-mode' never append
data on re-execution.  Instead, they clear the buffer.

Thus, we should

  1. undo this change and instead
  2. call 'org-babel-eval-wipe-error-buffer' before execution.

Rudy
-- 
"The whole science is nothing more than a refinement of everyday
thinking."
-- Albert Einstein, 1879-1955

Rudolf Adamkovič <salutis@me.com> [he/him]
Studenohorská 25
84103 Bratislava
Slovakia

Re: Org 9.6-pre and Bash sessions

[Ihor Radchenko]

Rudolf Adamkovič <salutis@me.com> writes:

>>> 1. ob-shell/error-output-after-success
>>>
>>>    We seem to trash error output, such as warnings, on success.  I
>>>    think we should not do this.  Now, on the execution of "echo X
>>>    &>2", Org says "Code block produced no output."  But that does
>>>    hold true.  The block did produce output, just on the other output
>>>    stream, namely error output.
>
>> Trashing error output is expected by default. [...] I suggest you to
>> read through [...] ob-shell follows the spec here.
>
> I get that ':results output' makes Org capture standard output, but not
> error output, for its #+RESULTS.  However, if you look at the test in
> question, you will see that it expects error output in the *Org-Babel
> Error Output* buffer, not in the #+RESULTS.  We should not trash error
> output; it belongs in the *Org-Babel Error Output* buffer.

I do not think that it make sense to display that buffer when the code
finishes successfully. I can see this kind of behaviour breaking/spamming
automated scripts or export---code working in the past may throw error
output into unsuspecting users.

>>> 3. ob-shell/exit-codes
>>>
>>>    Should we add a newline after the exit code message?
>
>> Done.
>
> I updated the tests and now every string needs a final newline, which
> does not seem correct.  If we let the tests guide our design, all
> becomes clear.  Buffers that use the 'compilation-mode' never append
> data on re-execution.  Instead, they clear the buffer.
>
> Thus, we should
>
>   1. undo this change and instead
>   2. call 'org-babel-eval-wipe-error-buffer' before execution.

I do not think that it is a good idea.
Code block execution may involve a whole chain of blocks when expanding
references. If we wipe the error buffer and multiple blocks are failing,
some errors may go unnoticed by the user.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org 9.6-pre and Bash sessions

[Rudolf Adamkovič]

Ihor Radchenko <yantar92@posteo.net> writes:

> I do not think that it make sense to display that buffer when the code
> finishes successfully. I can see this kind of behaviour
> breaking/spamming automated scripts or export---code working in the
> past may throw error output into unsuspecting users.

But the exit code has nothing to do with the standard error.

Unix programs can, and often do, halt with non-zero exit codes while
producing error output containing important information, such as
deprecation warnings.  Further, many programs use error output as the
alternative "anything but the result" stream.

Preserving user data, instead of trashing it, data does not count as
"spamming ... unsuspected users".  On the contrary!

For example, I use a program for work that uploads data to a certain
3rd-party server.  It exits with a zero code but also shows extremely
important notices on error output.  As an "unsuspecting user", if I used
Babel to run the program, I would end up in a trouble.

So, we should never implicitly trash user-generated data, let alone
based on a "completely made up" belief that a non-zero exit code somehow
implies "no important error output".  It does not.

(I speak only about Unix-like systems here.  Perhaps on other operating
systems, things work differently.)

> I do not think that it is a good idea.  Code block execution may
> involve a whole chain of blocks when expanding references. If we wipe
> the error buffer and multiple blocks are failing, some errors may go
> unnoticed by the user.

In that case, can we prepend a newline instead of appending it?  That
way, the tests would look "more normal" and the buffer content would
have the content one would expect, with no "hanging whitespace".

Rudy
-- 
"Genius is 1% inspiration and 99% perspiration."
p-- Thomas Alva Edison, 1932

Rudolf Adamkovič <salutis@me.com> [he/him]
Studenohorská 25
84103 Bratislava
Slovakia

[FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[Ihor Radchenko]

Rudolf Adamkovič <salutis@me.com> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> I do not think that it make sense to display that buffer when the code
>> finishes successfully. I can see this kind of behaviour
>> breaking/spamming automated scripts or export---code working in the
>> past may throw error output into unsuspecting users.
>
> But the exit code has nothing to do with the standard error.
>
> Unix programs can, and often do, halt with non-zero exit codes while
> producing error output containing important information, such as
> deprecation warnings.  Further, many programs use error output as the
> alternative "anything but the result" stream.
>
> Preserving user data, instead of trashing it, data does not count as
> "spamming ... unsuspected users".  On the contrary!
>
> For example, I use a program for work that uploads data to a certain
> 3rd-party server.  It exits with a zero code but also shows extremely
> important notices on error output.  As an "unsuspecting user", if I used
> Babel to run the program, I would end up in a trouble.
>
> So, we should never implicitly trash user-generated data, let alone
> based on a "completely made up" belief that a non-zero exit code somehow
> implies "no important error output".  It does not.
>
> (I speak only about Unix-like systems here.  Perhaps on other operating
> systems, things work differently.)

Dear All,

As explained in the above quote, it may be reasonable to display stderr
in the shell (and possibly other) src blocks upon execution.

+ Stderr may contain important information even if the code block
  succeeds
- Displaying stderr will raise *Error* buffer, which may or may not be
  expected or desired.

What do you think?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org 9.6-pre and Bash sessions

[Ihor Radchenko]

Rudolf Adamkovič <salutis@me.com> writes:

>> I do not think that it is a good idea.  Code block execution may
>> involve a whole chain of blocks when expanding references. If we wipe
>> the error buffer and multiple blocks are failing, some errors may go
>> unnoticed by the user.
>
> In that case, can we prepend a newline instead of appending it?  That
> way, the tests would look "more normal" and the buffer content would
> have the content one would expect, with no "hanging whitespace".

Feel free to change it in the patch together with tests.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[tomas]

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

On Sat, Oct 29, 2022 at 04:05:19AM +0000, Ihor Radchenko wrote:
> Rudolf Adamkovič <salutis@me.com> writes:
> 
> > Ihor Radchenko <yantar92@posteo.net> writes:
> >
> >> I do not think that it make sense to display that buffer when the code
> >> finishes successfully. I can see this kind of behaviour
> >> breaking/spamming automated scripts or export---code working in the
> >> past may throw error output into unsuspecting users.
> >
> > But the exit code has nothing to do with the standard error.

[...]

> > For example, I use a program for work that uploads data to a certain
> > 3rd-party server.  It exits with a zero code but also shows extremely
> > important notices on error output.  As an "unsuspecting user", if I used
> > Babel to run the program, I would end up in a trouble.

[...]

> Dear All,
> 
> As explained in the above quote, it may be reasonable to display stderr
> in the shell (and possibly other) src blocks upon execution.
> 
> + Stderr may contain important information even if the code block
>   succeeds
> - Displaying stderr will raise *Error* buffer, which may or may not be
>   expected or desired.
> 
> What do you think?

My take as an Org user is that this makes a lot of sense. I don't know
whether there is an elegant way to accomodate all the use cases in
an elegant way, but to provide a concrete example where I'd have found
it handy...

While preparing a handout for an introduction to shell programming
(at a very basic level), I wanted to embed little examples with
their results. Org rocks at this kind of task.

But in this case it's important to show everything the students are
going to see. One could argue that the error part is even the most
important.

So what I needed was not only the stderr (optionally somehow separated
from stdout -- optionally as someone would see it in some terminal
session), but also all the above even when the exit code was nonzero.

Ideally, a display of that exit code, too.

I ended up massaging prologue and epilogue, which worked nicely,
but sadly is language dependent for a set of concepts which are,
one could argue, independent of the language [1].

So yes, I would be thrilled by such a possibility.

Cheers

[1] These are OS conventions. So also a language of sorts, but at
another level.

-- 
tomás

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[Samuel Wales]

long ago i made the contents of my shell blocks look like this:

{
  code
} 2>&1
:


On 10/28/22, tomas@tuxteam.de <tomas@tuxteam.de> wrote:
> On Sat, Oct 29, 2022 at 04:05:19AM +0000, Ihor Radchenko wrote:
>> Rudolf Adamkovič <salutis@me.com> writes:
>>
>> > Ihor Radchenko <yantar92@posteo.net> writes:
>> >
>> >> I do not think that it make sense to display that buffer when the code
>> >> finishes successfully. I can see this kind of behaviour
>> >> breaking/spamming automated scripts or export---code working in the
>> >> past may throw error output into unsuspecting users.
>> >
>> > But the exit code has nothing to do with the standard error.
>
> [...]
>
>> > For example, I use a program for work that uploads data to a certain
>> > 3rd-party server.  It exits with a zero code but also shows extremely
>> > important notices on error output.  As an "unsuspecting user", if I
>> > used
>> > Babel to run the program, I would end up in a trouble.
>
> [...]
>
>> Dear All,
>>
>> As explained in the above quote, it may be reasonable to display stderr
>> in the shell (and possibly other) src blocks upon execution.
>>
>> + Stderr may contain important information even if the code block
>>   succeeds
>> - Displaying stderr will raise *Error* buffer, which may or may not be
>>   expected or desired.
>>
>> What do you think?
>
> My take as an Org user is that this makes a lot of sense. I don't know
> whether there is an elegant way to accomodate all the use cases in
> an elegant way, but to provide a concrete example where I'd have found
> it handy...
>
> While preparing a handout for an introduction to shell programming
> (at a very basic level), I wanted to embed little examples with
> their results. Org rocks at this kind of task.
>
> But in this case it's important to show everything the students are
> going to see. One could argue that the error part is even the most
> important.
>
> So what I needed was not only the stderr (optionally somehow separated
> from stdout -- optionally as someone would see it in some terminal
> session), but also all the above even when the exit code was nonzero.
>
> Ideally, a display of that exit code, too.
>
> I ended up massaging prologue and epilogue, which worked nicely,
> but sadly is language dependent for a set of concepts which are,
> one could argue, independent of the language [1].
>
> So yes, I would be thrilled by such a possibility.
>
> Cheers
>
> [1] These are OS conventions. So also a language of sorts, but at
> another level.
>
> --
> tomás
>


-- 
The Kafka Pandemic

A blog about science, health, human rights, and misopathy:
https://thekafkapandemic.blogspot.com

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[tomas]

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

On Fri, Oct 28, 2022 at 11:43:39PM -0700, Samuel Wales wrote:
> long ago i made the contents of my shell blocks look like this:
> 
> {
>   code
> } 2>&1
> :

As I hinted at, I also wanted to have the exit code documented. So
my setup was a bit more involved than this. Plus, I didn't want all
that code scaffolding to show up in the typeset results (my audience
had enough to digest as it was), so I hid all that in the prologue/
epilogue.

Cheers
-- 
t

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[Ihor Radchenko]

tomas@tuxteam.de writes:

> As I hinted at, I also wanted to have the exit code documented. So
> my setup was a bit more involved than this. Plus, I didn't want all
> that code scaffolding to show up in the typeset results (my audience
> had enough to digest as it was), so I hid all that in the prologue/
> epilogue.

Note that what we are discussing here is different from your
description. A popup window accumulating stderr is displayed upon code
evaluation. stderr does not go into results.

What you are describing would better fit into a new :results option.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[tomas]

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

On Sat, Oct 29, 2022 at 09:09:04AM +0000, Ihor Radchenko wrote:
> tomas@tuxteam.de writes:
> 
> > As I hinted at, I also wanted to have the exit code documented [...]

> Note that what we are discussing here is different from your
> description. A popup window accumulating stderr is displayed upon code
> evaluation. stderr does not go into results.
> 
> What you are describing would better fit into a new :results option.

Oh, I missed the popup part. Sounds useful for yet another profile,
yes.

Thanks and sorry for the noise.

And thanks for your incredible work on Org. Do you ever sleep?

In awe
-- 
tomás

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[Max Nikulin]

On 29/10/2022 11:05, Ihor Radchenko wrote:
> 
> As explained in the above quote, it may be reasonable to display stderr
> in the shell (and possibly other) src blocks upon execution.
> 
> + Stderr may contain important information even if the code block
>    succeeds
> - Displaying stderr will raise *Error* buffer, which may or may not be
>    expected or desired.
> 
> What do you think?

I agree that stderr may be important. I would not mind to have a buffer 
that combines both stderr and stdout (maybe fontified). Despite order of 
messages in separate streams is not strictly defined, it still may shed 
some light on the reason what went wrong.

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[Ihor Radchenko]

tomas@tuxteam.de writes:

>> What you are describing would better fit into a new :results option.
>
> Oh, I missed the popup part. Sounds useful for yet another profile,
> yes.

If you need it, feel free to open a separate feature request for extra
:results options in ob-shell. Possibly providing more details how you
envision this header argument to work.

> And thanks for your incredible work on Org. Do you ever sleep?

Remember that we may have radically different time zones. What appears
to be nightly emails may not necessarily be sent at night. 

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> I agree that stderr may be important. I would not mind to have a buffer 
> that combines both stderr and stdout (maybe fontified). Despite order of 
> messages in separate streams is not strictly defined, it still may shed 
> some light on the reason what went wrong.

I am not sure about the right approach of mixing streams.
What we currently do is directing stderr to a separate file. See
`org-babel--shell-command-on-region' and `org-babel-eval'.

Also, note that our error display code is not only used by shell
scripts. Also, by other interpreted languages like python. And we use
compilation-mode, which provides reasonable fontification in many cases.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[tomas]

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

On Sun, Oct 30, 2022 at 03:31:32AM +0000, Ihor Radchenko wrote:
> tomas@tuxteam.de writes:
> 
> >> What you are describing would better fit into a new :results option.
> >
> > Oh, I missed the popup part. Sounds useful for yet another profile,
> > yes.
> 
> If you need it, feel free to open a separate feature request for extra
> :results options in ob-shell. Possibly providing more details how you
> envision this header argument to work.

I'll mull over it. Perhaps I come up with some (patch) proposal.

> > And thanks for your incredible work on Org. Do you ever sleep?
> 
> Remember that we may have radically different time zones. What appears
> to be nightly emails may not necessarily be sent at night. 

I'm aware of that. My awe is more directed at your bandwidth than at
your timings.

Cheers
-- 
t

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[Ihor Radchenko]

tomas@tuxteam.de writes:

>> If you need it, feel free to open a separate feature request for extra
>> :results options in ob-shell. Possibly providing more details how you
>> envision this header argument to work.
>
> I'll mull over it. Perhaps I come up with some (patch) proposal.

Thinking more about it in the context of the current feature request,
displaying the stderr buffer unconditionally may be undesired when the
script already does 2>&1 or, potentially, uses imaginary :results stderr
header argument.

>> > And thanks for your incredible work on Org. Do you ever sleep?
>> 
>> Remember that we may have radically different time zones. What appears
>> to be nightly emails may not necessarily be sent at night. 
>
> I'm aware of that. My awe is more directed at your bandwidth than at
> your timings.

I use Org mode and Org agenda to manage the maintenance 😀

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[tomas]

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

On Sun, Oct 30, 2022 at 07:09:30AM +0000, Ihor Radchenko wrote:
> tomas@tuxteam.de writes:
> 
> >> If you need it, feel free to open a separate feature request for extra
> >> :results options in ob-shell. Possibly providing more details how you
> >> envision this header argument to work.
> >
> > I'll mull over it. Perhaps I come up with some (patch) proposal.
> 
> Thinking more about it in the context of the current feature request,
> displaying the stderr buffer unconditionally may be undesired when the
> script already does 2>&1 or, potentially, uses imaginary :results stderr
> header argument.

Definitely. It's not for everyone.

> >> > And thanks for your incredible work on Org. Do you ever sleep?

[...]

> I use Org mode and Org agenda to manage the maintenance 😀

Still genuinely impressed.

Cheers
-- 
t

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions)

[Tim Cross]

Ihor Radchenko <yantar92@posteo.net> writes:

> Rudolf Adamkovič <salutis@me.com> writes:
>
>> Ihor Radchenko <yantar92@posteo.net> writes:
>>
>>> I do not think that it make sense to display that buffer when the code
>>> finishes successfully. I can see this kind of behaviour
>>> breaking/spamming automated scripts or export---code working in the
>>> past may throw error output into unsuspecting users.
>>
>> But the exit code has nothing to do with the standard error.
>>
>> Unix programs can, and often do, halt with non-zero exit codes while
>> producing error output containing important information, such as
>> deprecation warnings.  Further, many programs use error output as the
>> alternative "anything but the result" stream.
>>
>> Preserving user data, instead of trashing it, data does not count as
>> "spamming ... unsuspected users".  On the contrary!
>>
>> For example, I use a program for work that uploads data to a certain
>> 3rd-party server.  It exits with a zero code but also shows extremely
>> important notices on error output.  As an "unsuspecting user", if I used
>> Babel to run the program, I would end up in a trouble.
>>
>> So, we should never implicitly trash user-generated data, let alone
>> based on a "completely made up" belief that a non-zero exit code somehow
>> implies "no important error output".  It does not.
>>
>> (I speak only about Unix-like systems here.  Perhaps on other operating
>> systems, things work differently.)
>
> Dear All,
>
> As explained in the above quote, it may be reasonable to display stderr
> in the shell (and possibly other) src blocks upon execution.
>
> + Stderr may contain important information even if the code block
>   succeeds
> - Displaying stderr will raise *Error* buffer, which may or may not be
>   expected or desired.
>
> What do you think?

I think this is perhaps the tip of a more complex iceberg. Not sure if
we can address bits individually. Suspect we are better off
clarifying the basic babel model and then look at specific language
exceptions/limits and adjusting the model or documenting where back ends
need to diverge from the basic model.

Part of the challenge here is that the relevance/importance of stderr is
somewhat dependent on the language. Same holds true for handling of
error/return codes and to 'results' output.

For me, this is another symptom of our need to define a clearer model
for babel processes so that we can get some consistency across the
board. Such a model would likely also make it easier for people to
develop new babel back ends. We may even need 2 models, one for
interactive/repl based back ends and one for non-interactive/scripted
back ends. 

The 2 big questions I see are "What should be defaults be?" and "How do
we handle the options without adding lots of new switches or suffering
an option permutation blow out?".

I do agree with the other posts regarding the importance of stderr.

Org babel API (was: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions))

[Ihor Radchenko]

Tim Cross <theophilusx@gmail.com> writes:

> For me, this is another symptom of our need to define a clearer model
> for babel processes so that we can get some consistency across the
> board. Such a model would likely also make it easier for people to
> develop new babel back ends. We may even need 2 models, one for
> interactive/repl based back ends and one for non-interactive/scripted
> back ends. 

We do have two models already.
We have some small bits documented in
https://orgmode.org/worg/org-contrib/babel/languages/index.html

Generally, the defaults are implemented in ob-core.el.
Session API is in ob-comint.el. Shell command API is in ob-eval.el.

> The 2 big questions I see are "What should be defaults be?" and "How do
> we handle the options without adding lots of new switches or suffering
> an option permutation blow out?".

I am sorry, but I do not fully understand what you are talking about.
Which defaults are you referring to?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org babel API (was: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions))

[Tim Cross]

Ihor Radchenko <yantar92@posteo.net> writes:

> Tim Cross <theophilusx@gmail.com> writes:
>
>> For me, this is another symptom of our need to define a clearer model
>> for babel processes so that we can get some consistency across the
>> board. Such a model would likely also make it easier for people to
>> develop new babel back ends. We may even need 2 models, one for
>> interactive/repl based back ends and one for non-interactive/scripted
>> back ends. 
>
> We do have two models already.
> We have some small bits documented in
> https://orgmode.org/worg/org-contrib/babel/languages/index.html
>
> Generally, the defaults are implemented in ob-core.el.
> Session API is in ob-comint.el. Shell command API is in ob-eval.el.
>
>> The 2 big questions I see are "What should be defaults be?" and "How do
>> we handle the options without adding lots of new switches or suffering
>> an option permutation blow out?".
>
> I am sorry, but I do not fully understand what you are talking about.
> Which defaults are you referring to?

In general, all the defaults for babel blocks, but morfe specifically
here, deffault handling for stderr.

Re: Org babel API (was: [FR] Display stderr contents after executing shell blocks (even when stdout :results output is requested) (was: Org 9.6-pre and Bash sessions))

[Ihor Radchenko]

Tim Cross <theophilusx@gmail.com> writes:

>>> The 2 big questions I see are "What should be defaults be?" and "How do
>>> we handle the options without adding lots of new switches or suffering
>>> an option permutation blow out?".
>>
>> I am sorry, but I do not fully understand what you are talking about.
>> Which defaults are you referring to?
>
> In general, all the defaults for babel blocks, but morfe specifically
> here, deffault handling for stderr.

stdout is the current default. And I'd say that it is expected default.
Sorry, I still do not understand the concrete problem you are trying to
highlight.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org 9.6-pre and Bash sessions

[Rudolf Adamkovič]

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

Ihor Radchenko <yantar92@posteo.net> writes:

> Feel free to change it in the patch together with tests.

Thank you for driving the discussion and for giving me the opportunity
to solve the problem (and not just temporarily, but with tests).  What
do you think about the attached patch?

Rudy


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ob-shell-Never-throw-away-standard-error.patch --]
[-- Type: text/x-patch, Size: 7362 bytes --]

From 7001ed80e5fd146f6acf586b3cf3ef20bbba04e7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rudolf=20Adamkovi=C4=8D?= <salutis@me.com>
Date: Wed, 26 Oct 2022 13:35:26 +0200
Subject: [PATCH] ob-shell: Never throw away standard error

* lisp/ob-eval.el (org-babel-eval-error-notify): Do not insert
superfluous whitespace.
* lisp/ob-eval.el (org-babel-eval): Show standard error even if the
command exits with a zero code.
* testing/lisp/test-ob-shell.el(
ob-shell/standard-output-after-success,
ob-shell/standard-output-after-failure,
ob-shell/error-output-after-success,
ob-shell/error-output-after-failure,
ob-shell/error-output-after-failure-multiple,
ob-shell/exit-code,
ob-shell/exit-code-multiple
): Add tests to avoid regressions.
---
 lisp/ob-eval.el               | 47 +++++++++++---------
 testing/lisp/test-ob-shell.el | 82 +++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+), 21 deletions(-)

diff --git a/lisp/ob-eval.el b/lisp/ob-eval.el
index e1278bbad..af9283c0a 100644
--- a/lisp/ob-eval.el
+++ b/lisp/ob-eval.el
@@ -40,39 +40,44 @@
     (with-current-buffer buf
       (goto-char (point-max))
       (save-excursion
-        (insert stderr)
         (unless (bolp) (insert "\n"))
-        (insert (format "[ Babel evaluation exited with code %S ]\n" exit-code))))
+        (insert stderr)
+        (insert (format "[ Babel evaluation exited with code %S ]" exit-code))))
     (display-buffer buf))
   (message "Babel evaluation exited with code %S" exit-code))
 
 (defun org-babel-eval (command query)
   "Run COMMAND on QUERY.
+Return standard output produced by COMMAND.  If COMMAND exits
+with a non-zero code or produces error output, show it with
+`org-babel-eval-error-notify'.
+
 Writes QUERY into a temp-buffer that is processed with
-`org-babel--shell-command-on-region'.  If COMMAND succeeds then return
-its results, otherwise display STDERR with
-`org-babel-eval-error-notify'."
+`org-babel--shell-command-on-region'."
   (let ((error-buffer (get-buffer-create " *Org-Babel Error*")) exit-code)
     (with-current-buffer error-buffer (erase-buffer))
     (with-temp-buffer
       (insert query)
       (setq exit-code
-	    (org-babel--shell-command-on-region
-	     command error-buffer))
-      (if (or (not (numberp exit-code)) (> exit-code 0))
-	  (progn
-	    (with-current-buffer error-buffer
-	      (org-babel-eval-error-notify exit-code (buffer-string)))
-	    (save-excursion
-	      (when (get-buffer org-babel-error-buffer-name)
-		(with-current-buffer org-babel-error-buffer-name
-		  (unless (derived-mode-p 'compilation-mode)
-		    (compilation-mode))
-		  ;; Compilation-mode enforces read-only, but Babel expects the buffer modifiable.
-		  (setq buffer-read-only nil))))
-            ;; Return output, if any.
-	    (buffer-string))
-	(buffer-string)))))
+            (org-babel--shell-command-on-region
+             command error-buffer))
+      (let ((stderr (with-current-buffer error-buffer (buffer-string))))
+        (if (or (not (numberp exit-code))
+                (> exit-code 0)
+                (not (string-empty-p stderr)))
+            (progn
+              (org-babel-eval-error-notify exit-code stderr)
+              (save-excursion
+                (when (get-buffer org-babel-error-buffer-name)
+                  (with-current-buffer org-babel-error-buffer-name
+                    (unless (derived-mode-p 'compilation-mode)
+                      (compilation-mode))
+                    ;; Compilation-mode enforces read-only, but
+                    ;; Babel expects the buffer modifiable.
+                    (setq buffer-read-only nil))))
+              ;; Return output, if any.
+              (buffer-string))
+          (buffer-string))))))
 
 (defun org-babel-eval-read-file (file)
   "Return the contents of FILE as a string."
diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index 4c00faa49..7aa45cc8a 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -170,6 +170,88 @@ ob-comint.el, which was not previously tested."
 	      "#+BEGIN_SRC sh :results table\necho 'I \"want\" it all'\n#+END_SRC"
 	    (org-babel-execute-src-block)))))
 
+;;; Standard output
+
+(ert-deftest ob-shell/standard-output-after-success ()
+  "Test standard output after exiting with a zero code."
+  (should (= 1
+             (org-babel-execute:sh
+              "echo 1" nil))))
+
+(ert-deftest ob-shell/standard-output-after-failure ()
+  "Test standard output after exiting with a non-zero code."
+  (should (= 1
+             (org-babel-execute:sh
+              "echo 1; exit 2" nil))))
+
+;;; Standard error
+
+(ert-deftest ob-shell/error-output-after-success ()
+  "Test that standard error shows in the error buffer, alongside the
+exit code, after exiting with a zero code."
+  (should
+   (string= "1
+[ Babel evaluation exited with code 0 ]"
+            (progn (org-babel-eval-wipe-error-buffer)
+                   (org-babel-execute:sh
+                    "echo 1 >&2" nil)
+                   (with-current-buffer org-babel-error-buffer-name
+                     (buffer-string))))))
+
+(ert-deftest ob-shell/error-output-after-failure ()
+  "Test that standard error shows in the error buffer, alongside the
+exit code, after exiting with a non-zero code."
+  (should
+   (string= "1
+[ Babel evaluation exited with code 2 ]"
+            (progn (org-babel-eval-wipe-error-buffer)
+                   (org-babel-execute:sh
+                    "echo 1 >&2; exit 2" nil)
+                   (with-current-buffer org-babel-error-buffer-name
+                     (buffer-string))))))
+
+(ert-deftest ob-shell/error-output-after-failure-multiple ()
+  "Test that multiple standard error strings show in the error
+buffer, alongside multiple exit codes."
+  (should
+   (string= "1
+[ Babel evaluation exited with code 2 ]
+3
+[ Babel evaluation exited with code 4 ]"
+            (progn (org-babel-eval-wipe-error-buffer)
+                   (org-babel-execute:sh
+                    "echo 1 >&2; exit 2" nil)
+                   (org-babel-execute:sh
+                    "echo 3 >&2; exit 4" nil)
+                   (with-current-buffer org-babel-error-buffer-name
+                     (buffer-string))))))
+
+;;; Exit codes
+
+(ert-deftest ob-shell/exit-code ()
+  "Test that the exit code shows in the error buffer after exiting
+with a non-zero return code."
+  (should
+   (string= "[ Babel evaluation exited with code 1 ]"
+            (progn (org-babel-eval-wipe-error-buffer)
+                   (org-babel-execute:sh
+                    "exit 1" nil)
+                   (with-current-buffer org-babel-error-buffer-name
+                     (buffer-string))))))
+
+(ert-deftest ob-shell/exit-code-multiple ()
+  "Test that multiple exit codes show in the error buffer after
+exiting with a non-zero return code multiple times."
+  (should
+   (string= "[ Babel evaluation exited with code 1 ]
+[ Babel evaluation exited with code 2 ]"
+            (progn (org-babel-eval-wipe-error-buffer)
+                   (org-babel-execute:sh
+                    "exit 1" nil)
+                   (org-babel-execute:sh
+                    "exit 2" nil)
+                   (with-current-buffer org-babel-error-buffer-name
+                     (buffer-string))))))
 
 (provide 'test-ob-shell)
 
-- 
2.38.1


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

-- 
"One can begin to reason only when a clear picture has been formed in
the imagination."
-- Walter Warwick Sawyer, Mathematician's Delight, 1943

Rudolf Adamkovič <salutis@me.com> [he/him]
Studenohorská 25
84103 Bratislava
Slovakia

Re: Org 9.6-pre and Bash sessions

[Ihor Radchenko]

Rudolf Adamkovič <salutis@me.com> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> Feel free to change it in the patch together with tests.
>
> Thank you for driving the discussion and for giving me the opportunity
> to solve the problem (and not just temporarily, but with tests).  What
> do you think about the attached patch?

Applied onto main changing the commit summary to "org-babel: ...".
This change will affect all the babel backends that use
`org-babel-eval'.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=f7b16402e6a694d592210f766544f6114056b4b0

A small note: If stderr does not contain a trailing newline, it will be
displayed as

Stderr output with no trailing newline[ Babel evaluation exited with code 1 ]

One may argue that it is ugly.
On the other hand, adding an extra newline will make stderr with and
without the newline indistinguishable.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Org 9.6-pre and Bash sessions

[Rudolf Adamkovič]

Ihor Radchenko <yantar92@posteo.net> writes:

> Applied onto main changing the commit summary to "org-babel: ...".
> This change will affect all the babel backends that use
> `org-babel-eval'.

That makes sense.  Thank you for the correction!

> A small note: If stderr does not contain a trailing newline, it will
> be displayed as
>
> Stderr output with no trailing newline[ Babel evaluation exited with
> code 1 ]
>
> One may argue that it is ugly.  On the other hand, adding an extra
> newline will make stderr with and without the newline
> indistinguishable.

Ugly but correct.  Besides, virtually every shell does the same, so
every "civilized" program ends its output with a newline.  Also, it
makes me happy that we have this important behavior fixed in tests!

Thank you for all your help!

Rudy
-- 
"Genius is 1% inspiration and 99% perspiration."
-- Thomas Alva Edison, 1932

Rudolf Adamkovič <salutis@me.com> [he/him]
Studenohorská 25
84103 Bratislava
Slovakia