[PATCH] Async evaluation in ob-shell

[PATCH] Async evaluation in ob-shell

[Matt]

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

I'm excited to share that I've got async evaluation working (crudely) with ob-shell.  A rough implementation is attached.  

It has clear issues, such as the prompt being present in the output:

#+begin_src sh :session tester :async t
echo "By sending delimiters separately..."
sleep 3
slep 1
echo "typos don't cause problems--^"
#+end_src

#+RESULTS:
: org_babel_sh_prompt> By sending delimiters separately...
: org_babel_sh_prompt> org_babel_sh_prompt> sh: slep: command not found
: org_babel_sh_prompt> typos don't cause problems--^
: org_babel_sh_prompt>

It's not clear to me if that's something that a better regexp would handle or if it should be cleaned up in the callback function.  I'm still figuring out how it's done in ob-python and ob-R.

Any feedback or advice is welcome.



[-- Attachment #2: ob-shell-async-separate-calls.patch --]
[-- Type: application/octet-stream, Size: 1898 bytes --]

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 9e7b45a89..8adf7744b 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -269,12 +269,15 @@ var of the same value."
 	    (set-marker comint-last-output-start (point))
 	    (get-buffer (current-buffer)))))))
 
+(defconst ob-shell-async-indicator "echo 'ob_comint_async_shell_%s_%s'")
+
 (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
   "Pass BODY to the Shell process in BUFFER.
 If RESULT-TYPE equals `output' then return a list of the outputs
 of the statements in BODY, if RESULT-TYPE equals `value' then
 return the value of the last statement in BODY."
   (let* ((shebang (cdr (assq :shebang params)))
+         (async (org-babel-comint-use-async params))
 	 (results-params (cdr (assq :result-params params)))
 	 (value-is-exit-status
 	  (or (and
@@ -305,6 +308,23 @@ return the value of the last statement in BODY."
                           (list shell-command-switch
                                 (concat (file-local-name script-file)  " " cmdline)))))
 		(buffer-string))))
+           (async
+            (let ((uuid (org-id-uuid)))
+              (org-babel-comint-async-register
+               session
+               (current-buffer)
+               "ob_comint_async_shell_\\(.+?\\)_\\(.+\\)"
+               'org-babel-chomp
+               'org-babel-chomp)
+              (org-babel-comint-async-delete-dangling-and-eval
+                  session
+                (insert (format ob-shell-async-indicator "start" uuid))
+                (comint-send-input nil t)
+                (insert (org-trim body))
+                (comint-send-input nil t)
+                (insert (format ob-shell-async-indicator "end" uuid))
+                (comint-send-input nil t))
+              uuid))
 	   (session			; session evaluation
 	    (mapconcat
 	     #'org-babel-sh-strip-weird-long-prompt

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> I'm excited to share that I've got async evaluation working (crudely) with ob-shell.  A rough implementation is attached.  
>
> It has clear issues, such as the prompt being present in the output:

That's likely because of the same reasons why prompt did not get cleaned
in synchronous blocks earlier. See `org-babel-comint-with-output'.

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

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

I've attached two patches which replace the previous.

I found cleaning the output was dramatically helped by calling `buffer-substring-no-properties' instead of `buffer-substring' in `org-babel-comint-async-filter'.  I'm not sure why `buffer-substring' was originally used.  `make test' shows no failures, so I assume it doesn't make a difference...?

 ---- On Tue, 07 Feb 2023 06:40:51 -0500  Ihor Radchenko  wrote --- 
 > That's likely because of the same reasons why prompt did not get cleaned
 > in synchronous blocks earlier. See `org-babel-comint-with-output'.

That, my friend, was a wild ride.

I'm curious about people's feelings on `org-babel-comint-with-output'.

Personally, I get the heebie-jeebies.  I can't shake feeling that there's a better way, especially since `org-babel-comint-async-filter' achieves similar ends.  My hunch is that other Babel languages may want async and that now would be a good time to consolidate the common functionalities of `org-babel-comint-with-output' and  `org-babel-comint-async-filter' .   Maybe even unify the API.  So far, `org-babel-comint-with-output' touches 9 languages and `org-babel-comint-async-filter' appears to touch 2 (soon to be 3).   I suspect those numbers will only grow.

I also can't shake the feeling that I might become ob-comint maintainer at some point...(not yet!).  I'm curious what people's thoughts are.





[-- Attachment #2: 01-ob-shell-remove-properties-from-callback-string.patch --]
[-- Type: application/octet-stream, Size: 997 bytes --]

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 54bf5127e..679757735 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -233,7 +233,7 @@ STRING contains the output originally inserted into the comint buffer."
       (goto-char (point-min))
       (while (re-search-forward indicator nil t)
 	;; update dangling
-	(setq new-dangling (buffer-substring (point) (point-max)))
+	(setq new-dangling (buffer-substring-no-properties (point) (point-max)))
 	(cond ((equal (match-string 1) "end")
 	       ;; save UUID for insertion later
 	       (push (match-string 2) uuid-list))
@@ -271,7 +271,7 @@ STRING contains the output originally inserted into the comint buffer."
 	  (when (equal (match-string 1) "end")
 	    (let* ((uuid (match-string-no-properties 2))
 		   (res-str-raw
-		    (buffer-substring
+		    (buffer-substring-no-properties
 		     ;; move point to beginning of indicator
                      (- (match-beginning 0) 1)
 		     ;; find the matching start indicator

[-- Attachment #3: 02-ob-shell-async-non-file.patch --]
[-- Type: application/octet-stream, Size: 2022 bytes --]

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 9e7b45a89..23cb9683b 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -269,12 +269,18 @@ var of the same value."
 	    (set-marker comint-last-output-start (point))
 	    (get-buffer (current-buffer)))))))
 
+(defconst ob-shell-async-indicator "echo 'ob_comint_async_shell_%s_%s'")
+
+(defun ob-shell-async-chunk-callback (string)
+  (replace-regexp-in-string (concat org-babel-sh-prompt "*") "" string))
+
 (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
   "Pass BODY to the Shell process in BUFFER.
 If RESULT-TYPE equals `output' then return a list of the outputs
 of the statements in BODY, if RESULT-TYPE equals `value' then
 return the value of the last statement in BODY."
   (let* ((shebang (cdr (assq :shebang params)))
+         (async (org-babel-comint-use-async params))
 	 (results-params (cdr (assq :result-params params)))
 	 (value-is-exit-status
 	  (or (and
@@ -305,6 +311,23 @@ return the value of the last statement in BODY."
                           (list shell-command-switch
                                 (concat (file-local-name script-file)  " " cmdline)))))
 		(buffer-string))))
+           (async
+            (let ((uuid (org-id-uuid)))
+              (org-babel-comint-async-register
+               session
+               (current-buffer)
+               "ob_comint_async_shell_\\(.+?\\)_\\(.+\\)"
+               'ob-shell-async-chunk-callback
+               nil)
+              (org-babel-comint-async-delete-dangling-and-eval
+                  session
+                (insert (format ob-shell-async-indicator "start" uuid))
+                (comint-send-input nil t)
+                (insert (org-trim body))
+                (comint-send-input nil t)
+                (insert (format ob-shell-async-indicator "end" uuid))
+                (comint-send-input nil t))
+              uuid))
 	   (session			; session evaluation
 	    (mapconcat
 	     #'org-babel-sh-strip-weird-long-prompt

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> I found cleaning the output was dramatically helped by calling `buffer-substring-no-properties' instead of `buffer-substring' in `org-babel-comint-async-filter'.  I'm not sure why `buffer-substring' was originally used.  `make test' shows no failures, so I assume it doesn't make a difference...?

Could you please elaborate? What was the problem with
`buffer-substring'? I am asking because one of the previous versions of
`org-babel-comint-wait-for-output' relied upon 'face text property. See
a35d16368.

>  ---- On Tue, 07 Feb 2023 06:40:51 -0500  Ihor Radchenko  wrote --- 
>  > That's likely because of the same reasons why prompt did not get cleaned
>  > in synchronous blocks earlier. See `org-babel-comint-with-output'.
>
> That, my friend, was a wild ride.
>
> I'm curious about people's feelings on `org-babel-comint-with-output'.

My feelings are: WTF and how does it even work :)
More in https://yhetil.org/emacs-devel/87y1tgqhmc.fsf@localhost/
It is a mess, because comint.el is not designed for programmatic use.

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

 ---- On Thu, 09 Feb 2023 06:23:42 -0500  Ihor Radchenko  wrote --- 
 > Could you please elaborate? What was the problem with
 > `buffer-substring'? I am asking because one of the previous versions of
 > `org-babel-comint-wait-for-output' relied upon 'face text property. See
 > a35d16368.

The problem is I got mixed up about the printed string representation and thought having properties changed how functions operated on the string.  The 02-ob-shell-async-non-file.patch works fine with `buffer-substring'.  No need for the other patch.

It seems to me like 02-ob-shell-async-non-file.patch is all that's needed for basic async in ob-shell.  I'm able to run long processes like `guix pull' and `guix package -u' calls without issue and the results look like I expect.  Similarly for running a web server, such as `python3 -m http.server' and killing it.  

Unless there's something you or others think needs to be done, I can get it committed (and try to write a test or two for it).

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> It seems to me like 02-ob-shell-async-non-file.patch is all that's needed for basic async in ob-shell.  I'm able to run long processes like `guix pull' and `guix package -u' calls without issue and the results look like I expect.  Similarly for running a web server, such as `python3 -m http.server' and killing it.  
>
> Unless there's something you or others think needs to be done, I can get it committed (and try to write a test or two for it).

1. You should provide all the docstrings.
2. I generally feel that separate async and separate session code are
   either duplicating the same code or edge cases considered by session
   code may popup in async code unexpectedly.

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

 ---- On Sat, 11 Feb 2023 06:44:56 -0500  Ihor Radchenko  wrote --- 
 > 1. You should provide all the docstrings.
 > 2. I generally feel that separate async and separate session code are
 >    either duplicating the same code or edge cases considered by session
 >    code may popup in async code unexpectedly.

Excellent points.  Thank you.

As part of the commit, I want to include tests.

How to test an async block is non-obvious.  The initial evaluation returns a uuid which returns immediately and can be checked using a regex:

(defconst test-ob-shell/uuid-regex
  "[0-9a-fA-F]\\{8\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{12\\}")

Technically, this is a ob-comint aspect and may be more rightly included in (the currently non-existant) tests for that module.

Checking the final result from the callback is trickier.   The following works, but requires advice (which could potentially persist beyond the test) and a delay (which slows testing overall and whose duration likely depends on the hardware the test runs on).  Without the delay, I could not get the callback to execute within the test.  It would execute when running manually in an Org buffer, however.  I'm not sure why. 

(ert-deftest test-ob-shell/session-async-evaluation ()
  (let ((session-name "test-ob-shell/session-async-evaluation")
        (kill-buffer-query-functions nil)
        result)
    ;; perform check after the callback executes which looks for the
    ;; expected result
    (advice-add
     'ob-shell-async-chunk-callback
     :filter-return
     (lambda (&rest r)
       (let ((result (car r)))
         (if (not (string= result "1\n2\n"))
             (ert-fail (format "Expected 1\n2\n: %s" result)))
         result))
     `((name . ,session-name)))
    ;; always remove the advice, regardless of test outcome
    (unwind-protect
        (org-test-with-temp-text
            (concat "#+begin_src sh :session " session-name " :async t
echo 1
echo 2<point>
#+end_src")
          ;; execute the block; delay momentarily so that the callback
          ;; executes
          (setq result (org-trim (org-babel-execute-src-block)))
          (if (should
               (and
                ;; if the block runs...
                (string-match
                 test-ob-shell/uuid-regex
                 result)
                ;; ...and the callback executes without fail
                (not (sleep-for 0.1))))
              ;; clean up buffer on success
              (kill-buffer session-name)))
      (advice-remove 'ob-shell-async-chunk-callback session-name))))

This works for me using the last patch.

Thoughts?

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> Checking the final result from the callback is trickier.   The following works, but requires advice (which could potentially persist beyond the test) and a delay (which slows testing overall and whose duration likely depends on the hardware the test runs on).  Without the delay, I could not get the callback to execute within the test.  It would execute when running manually in an Org buffer, however.  I'm not sure why. 

Doesn't (while ... (sleep-for ...)) work?

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

 ---- On Wed, 15 Feb 2023 10:08:47 -0500  Ihor Radchenko  wrote --- 
 > Matt matt@excalamus.com> writes:
 > 
 > > Checking the final result from the callback is trickier.   The following works, but requires advice (which could potentially persist beyond the test) and a delay (which slows testing overall and whose duration likely depends on the hardware the test runs on).  Without the delay, I could not get the callback to execute within the test.  It would execute when running manually in an Org buffer, however.  I'm not sure why. 
 > 
 > Doesn't (while ... (sleep-for ...)) work?

I'm afraid I don't follow what you mean.

My biggest concern is the sleep-for itself.  Aside from the concerns above, the test doesn't work without it. 

For example, the check made by the advice looks for "1\n2\n".  I've changed the source block from "echo 1" to "echo 2"  to induce a failure (it will now receive "2\n2\n").  I re-evaluate the test and call M-: (ert "test-ob-shell/session-async-evaluation").  The test passes!  I can put a message or debug in the advice and see that it's never called.  However, if I uncomment the sleep-for, the advice runs, and the test fails as expected.

(ert-deftest test-ob-shell/session-async-evaluation ()
  (let ((session-name "test-ob-shell/session-async-evaluation")
        (kill-buffer-query-functions nil)
        result)
    ;; check callback return for correct results
    (advice-add
     'ob-shell-async-chunk-callback
     :filter-return
     (lambda (&rest r)
       (let ((result (car r)))
         (should (string= result "1\n2\n"))  ; this was previously an if statement
         result))
     `((name . ,session-name)))
    ;; always remove advice, regardless of test outcome
    (unwind-protect
        (org-test-with-temp-text
            (concat "#+begin_src sh :session " session-name " :async t
echo 2
echo 2<point>
#+end_src")
          ;; execute block; callback only executes when delay
          (setq result (org-trim (org-babel-execute-src-block)))
          (if (should
               (and 
                (string-match test-ob-shell/uuid-regex result)  ; block runs
                (not (sleep-for 0.1))                           ; callback doesn't fail
                ))                         
              (kill-buffer session-name)))
      (advice-remove 'ob-shell-async-chunk-callback session-name))))

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  > Doesn't (while ... (sleep-for ...)) work?
>
> I'm afraid I don't follow what you mean.

What I mean is:

1. Execute src block asynchronously
2. Run (while ... (sleep-for ...)) until the result appears (up to
   threshold seconds)
3. Check the result as usual or fail the test when we wait too long and
   no results is added.

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

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


 ---- On Fri, 17 Feb 2023 05:44:20 -0500  Ihor Radchenko  wrote --- 
 > What I mean is...

I follow you now.  Thank you.

I've attached a patch (with commit message) for adding async to ob-shell.  If it looks good, I can apply it to main.

[-- Attachment #2: 0001-ob-shell-Add-async-evaluation.patch --]
[-- Type: application/octet-stream, Size: 8065 bytes --]

From 17016044fd293f81a71c5d82d10d338aae07a6c5 Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Sun, 19 Feb 2023 18:11:51 -0500
Subject: [PATCH] ob-shell: Add async evaluation

* ob-shell.el (org-babel-sh-evaluate): Add branch for async within
session.  Allow :async header argument to be either t or blank.

* test-ob-shell.el: Add const regexp for uuids.
(test-ob-shell/session-async-valid-header-arg-values): Check that
:async header works for both t and blank values.
(test-ob-shell/session-async-inserts-uuid-before-results-are-returned):
(test-ob-shell/session-async-evaluation): Check that asynchronously
evaluated results are eventually placed in the buffer.

Link: https://list.orgmode.org/186283d230a.129f5feb61660123.3289004102603503414@excalamus.com/
---
 lisp/ob-shell.el              | 54 +++++++++++++++++++------
 testing/lisp/test-ob-shell.el | 76 +++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+), 13 deletions(-)

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 9e7b45a89..3a0283231 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -269,12 +269,22 @@ var of the same value."
 	    (set-marker comint-last-output-start (point))
 	    (get-buffer (current-buffer)))))))
 
+(defconst ob-shell-async-indicator "echo 'ob_comint_async_shell_%s_%s'"
+  "Session output delimiter template.
+See `org-babel-comint-async-indicator'.")
+
+(defun ob-shell-async-chunk-callback (string)
+  "Filter applied to results before insertion.
+See `org-babel-comint-async-chunk-callback'."
+  (replace-regexp-in-string (concat org-babel-sh-prompt "*") "" string))
+
 (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
   "Pass BODY to the Shell process in BUFFER.
 If RESULT-TYPE equals `output' then return a list of the outputs
 of the statements in BODY, if RESULT-TYPE equals `value' then
 return the value of the last statement in BODY."
   (let* ((shebang (cdr (assq :shebang params)))
+         (async (org-babel-comint-use-async params))
 	 (results-params (cdr (assq :result-params params)))
 	 (value-is-exit-status
 	  (or (and
@@ -306,19 +316,37 @@ return the value of the last statement in BODY."
                                 (concat (file-local-name script-file)  " " cmdline)))))
 		(buffer-string))))
 	   (session			; session evaluation
-	    (mapconcat
-	     #'org-babel-sh-strip-weird-long-prompt
-	     (mapcar
-	      #'org-trim
-	      (butlast ; Remove eoe indicator
-	       (org-babel-comint-with-output
-		   (session org-babel-sh-eoe-output t body)
-                 (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"))
+            (if async
+                (progn
+                  (let ((uuid (org-id-uuid)))
+                    (org-babel-comint-async-register
+                     session
+                     (current-buffer)
+                     "ob_comint_async_shell_\\(.+?\\)_\\(.+\\)"
+                     'ob-shell-async-chunk-callback
+                     nil)
+                    (org-babel-comint-async-delete-dangling-and-eval
+                        session
+                      (insert (format ob-shell-async-indicator "start" uuid))
+                      (comint-send-input nil t)
+                      (insert (org-trim body))
+                      (comint-send-input nil t)
+                      (insert (format ob-shell-async-indicator "end" uuid))
+                      (comint-send-input nil t))
+                    uuid))
+	      (mapconcat
+	       #'org-babel-sh-strip-weird-long-prompt
+	       (mapcar
+	        #'org-trim
+	        (butlast ; Remove eoe indicator
+	         (org-babel-comint-with-output
+		     (session org-babel-sh-eoe-output t body)
+                   (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.
 	   ((org-string-nw-p shebang)
diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index fe975771c..103d62048 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -33,6 +33,9 @@
 
 (org-test-for-executable "sh")
 
+(defconst test-ob-shell/uuid-regex
+  "[0-9a-fA-F]\\{8\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{12\\}")
+
 \f
 ;;; Code:
 (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies ()
@@ -75,6 +78,79 @@ the body of the tangled block does."
     (if (should (equal '((1) (2)) result))
         (kill-buffer session-name))))
 
+(ert-deftest test-ob-shell/session-async-valid-header-arg-values ()
+  "Test that session runs asynchronously for certain :async values."
+  (let ((session-name "test-ob-shell/session-async-valid-header-arg-values")
+        (kill-buffer-query-functions nil))
+    (cl-loop
+     for arg-val in '("t" "")  ; valid arg values
+     do
+     (org-test-with-temp-text
+         (concat "#+begin_src sh :session " session-name " :async " arg-val " 
+echo 1<point>
+#+end_src")
+       (if (should
+            (string-match
+             test-ob-shell/uuid-regex
+             (org-trim (org-babel-execute-src-block))))
+           (kill-buffer session-name))))))
+
+(ert-deftest test-ob-shell/session-async-inserts-uuid-before-results-are-returned ()
+  "Test that a uuid placeholder is inserted before results are inserted."
+  (let ((session-name "test-ob-shell/session-async-inserts-uuid-before-results-are-returned")
+        (kill-buffer-query-functions nil))
+    (org-test-with-temp-text
+        (concat "#+begin_src sh :session " session-name " :async t
+echo 1<point>
+#+end_src")
+      (if (should
+           (string-match
+            test-ob-shell/uuid-regex
+            (org-trim (org-babel-execute-src-block))))
+          (kill-buffer session-name)))))
+
+(ert-deftest test-ob-shell/session-async-evaluation ()
+  "Test the async evaluation process."
+  ;; A uuid placeholder is immediately returned by block execution.
+  ;; Advise callback to check the return value for the expected value
+  (let* ((session-name "test-ob-shell/session-async-evaluation")
+         (kill-buffer-query-functions nil)
+         (start-time (current-time))
+         (wait-time (time-add start-time 3))
+         result)
+    (advice-add
+     'ob-shell-async-chunk-callback
+     :filter-return
+     (lambda (&rest r)
+       (let ((result (car r)))
+         (should (string= result "1\n2\n"))  ; expect value
+         result))
+     `((name . ,session-name)))
+    ;; always remove advice, regardless of test outcome
+    (unwind-protect
+        (org-test-with-temp-text
+            (concat "#+begin_src sh :session " session-name " :async t
+echo 1
+echo 2<point>
+#+end_src")
+          (setq result (org-trim (org-babel-execute-src-block)))
+          (if (should
+               (and
+                ;; block should run, returning a uuid immediately
+                (string-match test-ob-shell/uuid-regex result)
+                ;; loop until callback replaces the uuid or wait-time
+                ;; is exceeded
+                (catch 'too-long
+                  (while (string-match test-ob-shell/uuid-regex (buffer-string))
+                    (progn
+                      (sleep-for 0.01)
+                      (when (time-less-p wait-time (current-time))
+                        (throw 'too-long (ert-fail "Took too long to get result from callback")))))
+                  t)))
+              ;; clean up session buffer on success
+              (kill-buffer session-name)))
+      (advice-remove 'ob-shell-async-chunk-callback session-name))))
+
 (ert-deftest test-ob-shell/generic-uses-no-arrays ()
   "Test generic serialization of array into a single string."
   (org-test-with-temp-text
-- 
2.39.1

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> +    (advice-add
> +     'ob-shell-async-chunk-callback
> +     :filter-return
> +     (lambda (&rest r)
> +       (let ((result (car r)))
> +         (should (string= result "1\n2\n"))  ; expect value
> +         result))
> +     `((name . ,session-name)))
> ...
> +                (catch 'too-long
> +                  (while (string-match test-ob-shell/uuid-regex (buffer-string))
> +                    (progn
> +                      (sleep-for 0.01)
> +                      (when (time-less-p wait-time (current-time))
> +                        (throw 'too-long (ert-fail "Took too long to get result from callback")))))
> +                  t)))

Why not simply doing the `should' test when the
`test-ob-shell/uuid-regex' match fails? Instead of returning `t'. Then,
we will not need to use advise.

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

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


 ---- On Mon, 20 Feb 2023 06:24:52 -0500  Ihor Radchenko  wrote --- 

 > Why not simply doing the `should' test when the
 > `test-ob-shell/uuid-regex' match fails? Instead of returning `t'. Then,
 > we will not need to use advise.

Great point. I had originally used advice to avoid a loop.  However, when it became apparent that the loop was necessary, I overlooked that the advice was no longer needed.

I've rewritten the test and updated the patch. 

[-- Attachment #2: 0001-ob-shell-Add-async-evaluation.patch --]
[-- Type: application/octet-stream, Size: 7281 bytes --]

From e8c63828550b995f071a33c700cffed606cb8c76 Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Mon, 20 Feb 2023 12:11:46 -0500
Subject: [PATCH] ob-shell: Add async evaluation

* ob-shell.el (org-babel-sh-evaluate): Add condition for async within
session.  Allow :async header argument to be either t or blank.

* test-ob-shell.el: Add const regexp for uuids.
(test-ob-shell/session-async-valid-header-arg-values): Check that
:async header works for both t and blank values.
(test-ob-shell/session-async-inserts-uuid-before-results-are-returned):
(test-ob-shell/session-async-evaluation): Check that asynchronously
evaluated results are eventually placed in the buffer.

Link: https://list.orgmode.org/186283d230a.129f5feb61660123.3289004102603503414@excalamus.com/
---
 lisp/ob-shell.el              | 54 ++++++++++++++++++++++++--------
 testing/lisp/test-ob-shell.el | 58 +++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+), 13 deletions(-)

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 9e7b45a89..3a0283231 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -269,12 +269,22 @@ var of the same value."
 	    (set-marker comint-last-output-start (point))
 	    (get-buffer (current-buffer)))))))
 
+(defconst ob-shell-async-indicator "echo 'ob_comint_async_shell_%s_%s'"
+  "Session output delimiter template.
+See `org-babel-comint-async-indicator'.")
+
+(defun ob-shell-async-chunk-callback (string)
+  "Filter applied to results before insertion.
+See `org-babel-comint-async-chunk-callback'."
+  (replace-regexp-in-string (concat org-babel-sh-prompt "*") "" string))
+
 (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
   "Pass BODY to the Shell process in BUFFER.
 If RESULT-TYPE equals `output' then return a list of the outputs
 of the statements in BODY, if RESULT-TYPE equals `value' then
 return the value of the last statement in BODY."
   (let* ((shebang (cdr (assq :shebang params)))
+         (async (org-babel-comint-use-async params))
 	 (results-params (cdr (assq :result-params params)))
 	 (value-is-exit-status
 	  (or (and
@@ -306,19 +316,37 @@ return the value of the last statement in BODY."
                                 (concat (file-local-name script-file)  " " cmdline)))))
 		(buffer-string))))
 	   (session			; session evaluation
-	    (mapconcat
-	     #'org-babel-sh-strip-weird-long-prompt
-	     (mapcar
-	      #'org-trim
-	      (butlast ; Remove eoe indicator
-	       (org-babel-comint-with-output
-		   (session org-babel-sh-eoe-output t body)
-                 (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"))
+            (if async
+                (progn
+                  (let ((uuid (org-id-uuid)))
+                    (org-babel-comint-async-register
+                     session
+                     (current-buffer)
+                     "ob_comint_async_shell_\\(.+?\\)_\\(.+\\)"
+                     'ob-shell-async-chunk-callback
+                     nil)
+                    (org-babel-comint-async-delete-dangling-and-eval
+                        session
+                      (insert (format ob-shell-async-indicator "start" uuid))
+                      (comint-send-input nil t)
+                      (insert (org-trim body))
+                      (comint-send-input nil t)
+                      (insert (format ob-shell-async-indicator "end" uuid))
+                      (comint-send-input nil t))
+                    uuid))
+	      (mapconcat
+	       #'org-babel-sh-strip-weird-long-prompt
+	       (mapcar
+	        #'org-trim
+	        (butlast ; Remove eoe indicator
+	         (org-babel-comint-with-output
+		     (session org-babel-sh-eoe-output t body)
+                   (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.
 	   ((org-string-nw-p shebang)
diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index 8366f9dbe..bcedea3b9 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -33,6 +33,9 @@
 
 (org-test-for-executable "sh")
 
+(defconst test-ob-shell/uuid-regex
+  "[0-9a-fA-F]\\{8\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{12\\}")
+
 \f
 ;;; Code:
 (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies ()
@@ -75,6 +78,61 @@ the body of the tangled block does."
     (if (should (equal '((1) (2)) result))
         (kill-buffer session-name))))
 
+(ert-deftest test-ob-shell/session-async-valid-header-arg-values ()
+  "Test that session runs asynchronously for certain :async values."
+  (let ((session-name "test-ob-shell/session-async-valid-header-arg-values")
+        (kill-buffer-query-functions nil))
+    (cl-loop
+     for arg-val in '("t" "")  ; valid arg values
+     do
+     (org-test-with-temp-text
+         (concat "#+begin_src sh :session " session-name " :async " arg-val " 
+echo 1<point>
+#+end_src")
+       (if (should
+            (string-match
+             test-ob-shell/uuid-regex
+             (org-trim (org-babel-execute-src-block))))
+           (kill-buffer session-name))))))
+
+(ert-deftest test-ob-shell/session-async-inserts-uuid-before-results-are-returned ()
+  "Test that a uuid placeholder is inserted before results are inserted."
+  (let ((session-name "test-ob-shell/session-async-inserts-uuid-before-results-are-returned")
+        (kill-buffer-query-functions nil))
+    (org-test-with-temp-text
+        (concat "#+begin_src sh :session " session-name " :async t
+echo 1<point>
+#+end_src")
+      (if (should
+           (string-match
+            test-ob-shell/uuid-regex
+            (org-trim (org-babel-execute-src-block))))
+          (kill-buffer session-name)))))
+
+(ert-deftest test-ob-shell/session-async-evaluation ()
+  "Test the async evaluation process."
+  (let* ((session-name "test-ob-shell/session-async-evaluation")
+         (kill-buffer-query-functions nil)
+         (start-time (current-time))
+         (wait-time (time-add start-time 3))
+         uuid-placeholder)
+    (org-test-with-temp-text
+        (concat "#+begin_src sh :session " session-name " :async t
+echo 1
+echo 2<point>
+#+end_src")
+      (setq uuid-placeholder (org-trim (org-babel-execute-src-block)))
+      (catch 'too-long
+        (while (string-match uuid-placeholder (buffer-string))
+          (progn
+            (sleep-for 0.01)
+            (when (time-less-p wait-time (current-time))
+              (throw 'too-long (ert-fail "Took too long to get result from callback"))))))
+    (search-forward "#+results")
+    (beginning-of-line 2)
+    (if (should (string= ": 1\n: 2\n" (buffer-substring-no-properties (point) (point-max))))
+          (kill-buffer session-name)))))
+
 (ert-deftest test-ob-shell/generic-uses-no-arrays ()
   "Test generic serialization of array into a single string."
   (org-test-with-temp-text
-- 
2.39.1

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> I've rewritten the test and updated the patch. 

Thanks!

> +(defun ob-shell-async-chunk-callback (string)
> +  "Filter applied to results before insertion.
> +See `org-babel-comint-async-chunk-callback'."
> +  (replace-regexp-in-string (concat org-babel-sh-prompt "*") "" string))

Why not using `comint-prompt-regexp'?

> +(ert-deftest test-ob-shell/session-async-valid-header-arg-values ()
> +  "Test that session runs asynchronously for certain :async values."
> +  (let ((session-name "test-ob-shell/session-async-valid-header-arg-values")
> +        (kill-buffer-query-functions nil))
> +    (cl-loop

A simple `dolist' would do here. There is no reason to use `cl-loop'.

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

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


 ---- On Wed, 22 Feb 2023 05:29:59 -0500  Ihor Radchenko  wrote --- 
 > > +(defun ob-shell-async-chunk-callback (string)
 > > +  "Filter applied to results before insertion.
 > > +See `org-babel-comint-async-chunk-callback'."
 > > +  (replace-regexp-in-string (concat org-babel-sh-prompt "*") "" string))
 > 
 > Why not using `comint-prompt-regexp'?
 > 
 > > +(ert-deftest test-ob-shell/session-async-valid-header-arg-values ()
 > > +  "Test that session runs asynchronously for certain :async values."
 > > +  (let ((session-name "test-ob-shell/session-async-valid-header-arg-values")
 > > +        (kill-buffer-query-functions nil))
 > > +    (cl-loop
 > 
 > A simple `dolist' would do here. There is no reason to use `cl-loop'.

Great points!  Changed.

[-- Attachment #2: 0002-ob-shell-Add-async-evaluation.patch --]
[-- Type: application/octet-stream, Size: 7227 bytes --]

From b66d776346c992ec085bd719ab73f3d1773f71cc Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Wed, 1 Mar 2023 20:31:46 -0500
Subject: [PATCH] ob-shell: Add async evaluation

* ob-shell.el (org-babel-sh-evaluate): Add condition for async within
session.  Allow :async header argument to be either t or blank.

* test-ob-shell.el: Add const regexp for uuids.
(test-ob-shell/session-async-valid-header-arg-values): Check that
:async header works for both t and blank values.
(test-ob-shell/session-async-inserts-uuid-before-results-are-returned):
(test-ob-shell/session-async-evaluation): Check that asynchronously
evaluated results are eventually placed in the buffer.

Link: https://list.orgmode.org/186283d230a.129f5feb61660123.3289004102603503414@excalamus.com/
---
 lisp/ob-shell.el              | 54 +++++++++++++++++++++++++--------
 testing/lisp/test-ob-shell.el | 56 +++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 13 deletions(-)

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 9e7b45a89..99ba2a7e9 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -269,12 +269,22 @@ var of the same value."
 	    (set-marker comint-last-output-start (point))
 	    (get-buffer (current-buffer)))))))
 
+(defconst ob-shell-async-indicator "echo 'ob_comint_async_shell_%s_%s'"
+  "Session output delimiter template.
+See `org-babel-comint-async-indicator'.")
+
+(defun ob-shell-async-chunk-callback (string)
+  "Filter applied to results before insertion.
+See `org-babel-comint-async-chunk-callback'."
+  (replace-regexp-in-string comint-prompt-regexp "" string))
+
 (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
   "Pass BODY to the Shell process in BUFFER.
 If RESULT-TYPE equals `output' then return a list of the outputs
 of the statements in BODY, if RESULT-TYPE equals `value' then
 return the value of the last statement in BODY."
   (let* ((shebang (cdr (assq :shebang params)))
+         (async (org-babel-comint-use-async params))
 	 (results-params (cdr (assq :result-params params)))
 	 (value-is-exit-status
 	  (or (and
@@ -306,19 +316,37 @@ return the value of the last statement in BODY."
                                 (concat (file-local-name script-file)  " " cmdline)))))
 		(buffer-string))))
 	   (session			; session evaluation
-	    (mapconcat
-	     #'org-babel-sh-strip-weird-long-prompt
-	     (mapcar
-	      #'org-trim
-	      (butlast ; Remove eoe indicator
-	       (org-babel-comint-with-output
-		   (session org-babel-sh-eoe-output t body)
-                 (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"))
+            (if async
+                (progn
+                  (let ((uuid (org-id-uuid)))
+                    (org-babel-comint-async-register
+                     session
+                     (current-buffer)
+                     "ob_comint_async_shell_\\(.+?\\)_\\(.+\\)"
+                     'ob-shell-async-chunk-callback
+                     nil)
+                    (org-babel-comint-async-delete-dangling-and-eval
+                        session
+                      (insert (format ob-shell-async-indicator "start" uuid))
+                      (comint-send-input nil t)
+                      (insert (org-trim body))
+                      (comint-send-input nil t)
+                      (insert (format ob-shell-async-indicator "end" uuid))
+                      (comint-send-input nil t))
+                    uuid))
+	      (mapconcat
+	       #'org-babel-sh-strip-weird-long-prompt
+	       (mapcar
+	        #'org-trim
+	        (butlast ; Remove eoe indicator
+	         (org-babel-comint-with-output
+		     (session org-babel-sh-eoe-output t body)
+                   (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.
 	   ((org-string-nw-p shebang)
diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index 8366f9dbe..c56a76acf 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -33,6 +33,9 @@
 
 (org-test-for-executable "sh")
 
+(defconst test-ob-shell/uuid-regex
+  "[0-9a-fA-F]\\{8\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{12\\}")
+
 \f
 ;;; Code:
 (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies ()
@@ -75,6 +78,59 @@ the body of the tangled block does."
     (if (should (equal '((1) (2)) result))
         (kill-buffer session-name))))
 
+(ert-deftest test-ob-shell/session-async-valid-header-arg-values ()
+  "Test that session runs asynchronously for certain :async values."
+  (let ((session-name "test-ob-shell/session-async-valid-header-arg-values")
+        (kill-buffer-query-functions nil))
+    (dolist (arg-val '("t" ""))
+     (org-test-with-temp-text
+         (concat "#+begin_src sh :session " session-name " :async " arg-val "
+echo 1<point>
+#+end_src")
+       (if (should
+            (string-match
+             test-ob-shell/uuid-regex
+             (org-trim (org-babel-execute-src-block))))
+           (kill-buffer session-name))))))
+
+(ert-deftest test-ob-shell/session-async-inserts-uuid-before-results-are-returned ()
+  "Test that a uuid placeholder is inserted before results are inserted."
+  (let ((session-name "test-ob-shell/session-async-inserts-uuid-before-results-are-returned")
+        (kill-buffer-query-functions nil))
+    (org-test-with-temp-text
+        (concat "#+begin_src sh :session " session-name " :async t
+echo 1<point>
+#+end_src")
+      (if (should
+           (string-match
+            test-ob-shell/uuid-regex
+            (org-trim (org-babel-execute-src-block))))
+          (kill-buffer session-name)))))
+
+(ert-deftest test-ob-shell/session-async-evaluation ()
+  "Test the async evaluation process."
+  (let* ((session-name "test-ob-shell/session-async-evaluation")
+         (kill-buffer-query-functions nil)
+         (start-time (current-time))
+         (wait-time (time-add start-time 3))
+         uuid-placeholder)
+    (org-test-with-temp-text
+        (concat "#+begin_src sh :session " session-name " :async t
+echo 1
+echo 2<point>
+#+end_src")
+      (setq uuid-placeholder (org-trim (org-babel-execute-src-block)))
+      (catch 'too-long
+        (while (string-match uuid-placeholder (buffer-string))
+          (progn
+            (sleep-for 0.01)
+            (when (time-less-p wait-time (current-time))
+              (throw 'too-long (ert-fail "Took too long to get result from callback"))))))
+    (search-forward "#+results")
+    (beginning-of-line 2)
+    (if (should (string= ": 1\n: 2\n" (buffer-substring-no-properties (point) (point-max))))
+          (kill-buffer session-name)))))
+
 (ert-deftest test-ob-shell/generic-uses-no-arrays ()
   "Test generic serialization of array into a single string."
   (org-test-with-temp-text
-- 
2.39.1

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> From b66d776346c992ec085bd719ab73f3d1773f71cc Mon Sep 17 00:00:00 2001
> From: Matthew Trzcinski <matt@excalamus.com>
> Date: Wed, 1 Mar 2023 20:31:46 -0500
> Subject: [PATCH] ob-shell: Add async evaluation

I tried the patch, and I am getting failed tests:

1 unexpected results:
   FAILED  test-ob-shell/session-async-evaluation  ((should (string= ": 1\n: 2\n" (buffer-substring-no-properties (point) (point-max)))) :form (string= ": 1\n: 2\n" ": 1\n: 2\n: org_babel_sh_prompt>\n") :value nil :explanation (arrays-of-different-length 8 31 ": 1\n: 2\n" ": 1\n: 2\n: org_babel_sh_prompt>\n" first-mismatch-at 8))

> +                  (let ((uuid (org-id-uuid)))

Do you need to use `org-id-uuid' here? ob-python directly uses `md5'.
If you still prefer org-id-uuid, we probably need to move it to
org-macs.el

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

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


 ---- On Fri, 03 Mar 2023 09:52:09 -0500  Ihor Radchenko  wrote --- 
 > I tried the patch, and I am getting failed tests:
 > 
 > 1 unexpected results:
 >    FAILED  test-ob-shell/session-async-evaluation  ((should (string= ": 1\n: 2\n" (buffer-substring-no-properties (point) (point-max)))) :form (string= ": 1\n: 2\n" ": 1\n: 2\n: org_babel_sh_prompt>\n") :value nil :explanation (arrays-of-different-length 8 31 ": 1\n: 2\n" ": 1\n: 2\n: org_babel_sh_prompt>\n" first-mismatch-at 8))

Sorry for missing that.  The issue is that when I replaced `org-babel-sh-prompt' with `comint-prompt-regexp', the regexp no longer matches the output and grabs the next prompt.  It looks like the reason is `comint-prompt-regexp' is set to "^org_babel_sh_prompt>  *" (with two spaces between the '>' and '*').  Attached is a revised patch which removes one of the spaces by changing how `org-babel-sh-initiate-session' sets the `comint-prompt-regexp'.  Another place this could be done is in the defvar for `org-babel-sh-prompt' itself (which ends with a space).  However, I think it's customary to leave a trailing space for prompts?

 > > +                  (let ((uuid (org-id-uuid)))
 > 
 > Do you need to use `org-id-uuid' here? ob-python directly uses `md5'.
 > If you still prefer org-id-uuid, we probably need to move it to
 > org-macs.el

I just need a random string.  `md5' would work for that.  However, might it be better to update ob-R and ob-python to use `org-id-uuid'?  Both of those manually declare the randomness.  It's conceivable that someone may delete or mistype the number (100000000), resulting in a lower entropy.  An md5 is also not a uuid, strictly speaking.  Of course, the chance of collision is still basically zero and the cost of collision about the same.  Using `org-id-uuid' would only provide a consistent way to do things.

[-- Attachment #2: 0003-ob-shell-Add-async-evaluation.patch --]
[-- Type: application/octet-stream, Size: 7720 bytes --]

From 941bbadb922051f404943c58c8f0c6482623f876 Mon Sep 17 00:00:00 2001
From: Matthew Trzcinski <matt@excalamus.com>
Date: Fri, 3 Mar 2023 12:40:15 -0500
Subject: [PATCH] ob-shell: Add async evaluation

* ob-shell.el (org-babel-sh-initiate-session): Remove extra space from
`comint-prompt-regexp'.
(org-babel-sh-evaluate): Add condition for async within
session.  Allow :async header argument to be either t or blank.

* test-ob-shell.el: Add const regexp for uuids.
(test-ob-shell/session-async-valid-header-arg-values): Check that
:async header works for both t and blank values.
(test-ob-shell/session-async-inserts-uuid-before-results-are-returned):
(test-ob-shell/session-async-evaluation): Check that asynchronously
evaluated results are eventually placed in the buffer.

Link: https://list.orgmode.org/186283d230a.129f5feb61660123.3289004102603503414@excalamus.com/
---
 lisp/ob-shell.el              | 56 ++++++++++++++++++++++++++---------
 testing/lisp/test-ob-shell.el | 56 +++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 14 deletions(-)

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 9e7b45a89..db5d2aabc 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -262,19 +262,29 @@ var of the same value."
              (format org-babel-prompt-command org-babel-sh-prompt))
             (setq-local comint-prompt-regexp
                         (concat "^" (regexp-quote org-babel-sh-prompt)
-                                " *"))
+                                "*"))
 	    ;; Needed for Emacs 23 since the marker is initially
 	    ;; undefined and the filter functions try to use it without
 	    ;; checking.
 	    (set-marker comint-last-output-start (point))
 	    (get-buffer (current-buffer)))))))
 
+(defconst ob-shell-async-indicator "echo 'ob_comint_async_shell_%s_%s'"
+  "Session output delimiter template.
+See `org-babel-comint-async-indicator'.")
+
+(defun ob-shell-async-chunk-callback (string)
+  "Filter applied to results before insertion.
+See `org-babel-comint-async-chunk-callback'."
+  (replace-regexp-in-string comint-prompt-regexp "" string))
+
 (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
   "Pass BODY to the Shell process in BUFFER.
 If RESULT-TYPE equals `output' then return a list of the outputs
 of the statements in BODY, if RESULT-TYPE equals `value' then
 return the value of the last statement in BODY."
   (let* ((shebang (cdr (assq :shebang params)))
+         (async (org-babel-comint-use-async params))
 	 (results-params (cdr (assq :result-params params)))
 	 (value-is-exit-status
 	  (or (and
@@ -306,19 +316,37 @@ return the value of the last statement in BODY."
                                 (concat (file-local-name script-file)  " " cmdline)))))
 		(buffer-string))))
 	   (session			; session evaluation
-	    (mapconcat
-	     #'org-babel-sh-strip-weird-long-prompt
-	     (mapcar
-	      #'org-trim
-	      (butlast ; Remove eoe indicator
-	       (org-babel-comint-with-output
-		   (session org-babel-sh-eoe-output t body)
-                 (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"))
+            (if async
+                (progn
+                  (let ((uuid (org-id-uuid)))
+                    (org-babel-comint-async-register
+                     session
+                     (current-buffer)
+                     "ob_comint_async_shell_\\(.+?\\)_\\(.+\\)"
+                     'ob-shell-async-chunk-callback
+                     nil)
+                    (org-babel-comint-async-delete-dangling-and-eval
+                        session
+                      (insert (format ob-shell-async-indicator "start" uuid))
+                      (comint-send-input nil t)
+                      (insert (org-trim body))
+                      (comint-send-input nil t)
+                      (insert (format ob-shell-async-indicator "end" uuid))
+                      (comint-send-input nil t))
+                    uuid))
+	      (mapconcat
+	       #'org-babel-sh-strip-weird-long-prompt
+	       (mapcar
+	        #'org-trim
+	        (butlast ; Remove eoe indicator
+	         (org-babel-comint-with-output
+		     (session org-babel-sh-eoe-output t body)
+                   (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.
 	   ((org-string-nw-p shebang)
diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index 8366f9dbe..c56a76acf 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -33,6 +33,9 @@
 
 (org-test-for-executable "sh")
 
+(defconst test-ob-shell/uuid-regex
+  "[0-9a-fA-F]\\{8\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{12\\}")
+
 \f
 ;;; Code:
 (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies ()
@@ -75,6 +78,59 @@ the body of the tangled block does."
     (if (should (equal '((1) (2)) result))
         (kill-buffer session-name))))
 
+(ert-deftest test-ob-shell/session-async-valid-header-arg-values ()
+  "Test that session runs asynchronously for certain :async values."
+  (let ((session-name "test-ob-shell/session-async-valid-header-arg-values")
+        (kill-buffer-query-functions nil))
+    (dolist (arg-val '("t" ""))
+     (org-test-with-temp-text
+         (concat "#+begin_src sh :session " session-name " :async " arg-val "
+echo 1<point>
+#+end_src")
+       (if (should
+            (string-match
+             test-ob-shell/uuid-regex
+             (org-trim (org-babel-execute-src-block))))
+           (kill-buffer session-name))))))
+
+(ert-deftest test-ob-shell/session-async-inserts-uuid-before-results-are-returned ()
+  "Test that a uuid placeholder is inserted before results are inserted."
+  (let ((session-name "test-ob-shell/session-async-inserts-uuid-before-results-are-returned")
+        (kill-buffer-query-functions nil))
+    (org-test-with-temp-text
+        (concat "#+begin_src sh :session " session-name " :async t
+echo 1<point>
+#+end_src")
+      (if (should
+           (string-match
+            test-ob-shell/uuid-regex
+            (org-trim (org-babel-execute-src-block))))
+          (kill-buffer session-name)))))
+
+(ert-deftest test-ob-shell/session-async-evaluation ()
+  "Test the async evaluation process."
+  (let* ((session-name "test-ob-shell/session-async-evaluation")
+         (kill-buffer-query-functions nil)
+         (start-time (current-time))
+         (wait-time (time-add start-time 3))
+         uuid-placeholder)
+    (org-test-with-temp-text
+        (concat "#+begin_src sh :session " session-name " :async t
+echo 1
+echo 2<point>
+#+end_src")
+      (setq uuid-placeholder (org-trim (org-babel-execute-src-block)))
+      (catch 'too-long
+        (while (string-match uuid-placeholder (buffer-string))
+          (progn
+            (sleep-for 0.01)
+            (when (time-less-p wait-time (current-time))
+              (throw 'too-long (ert-fail "Took too long to get result from callback"))))))
+    (search-forward "#+results")
+    (beginning-of-line 2)
+    (if (should (string= ": 1\n: 2\n" (buffer-substring-no-properties (point) (point-max))))
+          (kill-buffer session-name)))))
+
 (ert-deftest test-ob-shell/generic-uses-no-arrays ()
   "Test generic serialization of array into a single string."
   (org-test-with-temp-text
-- 
2.39.1

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  ---- On Fri, 03 Mar 2023 09:52:09 -0500  Ihor Radchenko  wrote --- 
>  > I tried the patch, and I am getting failed tests:
>  > 
>  > 1 unexpected results:
>
> Sorry for missing that.  The issue is that when I replaced
> `org-babel-sh-prompt' with `comint-prompt-regexp', the regexp no
> longer matches the output and grabs the next prompt.  It looks like
> the reason is `comint-prompt-regexp' is set to "^org_babel_sh_prompt>
> *" (with two spaces between the '>' and '*').  Attached is a revised
> patch which removes one of the spaces by changing how
> `org-babel-sh-initiate-session' sets the `comint-prompt-regexp'.
> Another place this could be done is in the defvar for
> `org-babel-sh-prompt' itself (which ends with a space).  However, I
> think it's customary to leave a trailing space for prompts?

The actual prompt is "org_babel_sh_prompt> ".
And we want to skip leading spaces in addition.

Adding " *" does not make the prompt match 2 spaces, but 1+.

Prompts with no single space are not prompts.

>
>  > > +                  (let ((uuid (org-id-uuid)))
>  > 
>  > Do you need to use `org-id-uuid' here? ob-python directly uses `md5'.
>  > If you still prefer org-id-uuid, we probably need to move it to
>  > org-macs.el
>
> I just need a random string.  `md5' would work for that.  However,
> might it be better to update ob-R and ob-python to use `org-id-uuid'?
> Both of those manually declare the randomness.  It's conceivable that
> someone may delete or mistype the number (100000000), resulting in a
> lower entropy.  An md5 is also not a uuid, strictly speaking.  Of
> course, the chance of collision is still basically zero and the cost
> of collision about the same.  Using `org-id-uuid' would only provide a
> consistent way to do things.

`md5' will be slightly faster compared to `org-id-uuid'. But it should
not matter.

If we want use `org-id-uuid', lets move it to org-macs.el. Requiring the
whole org-id.el must not be done. It has side effects of defining id:
links.

>                          (concat "^" (regexp-quote org-babel-sh-prompt)
> -                                " *"))
> +                                "*"))

This is wrong. It unconditionally makes the last char in
`org-babel-sh-prompt' 0+. (Imagine it is changed to non-space in
future).

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

 ---- On Sun, 05 Mar 2023 07:14:21 -0500  Ihor Radchenko  wrote --- 
 > > Matt matt@excalamus.com> writes:
 > >
 > > Sorry for missing that.  The issue is that when I replaced
 > > `org-babel-sh-prompt' with `comint-prompt-regexp', the regexp no
 > > longer matches the output and grabs the next prompt.  It looks like
 > > the reason is `comint-prompt-regexp' is set to "^org_babel_sh_prompt>
 > > *" (with two spaces between the '>' and '*').  Attached is a revised
 > > patch which removes one of the spaces by changing how
 > > `org-babel-sh-initiate-session' sets the `comint-prompt-regexp'.
 > > Another place this could be done is in the defvar for
 > > `org-babel-sh-prompt' itself (which ends with a space).  However, I
 > > think it's customary to leave a trailing space for prompts?
 > 
 > The actual prompt is "org_babel_sh_prompt> ".

Agreed.

 > And we want to skip leading spaces in addition.
 
What do you mean by this?

 > Adding " *" does not make the prompt match 2 spaces, but 1+.
 
Agreed.  

It's not clear to me what pattern you're looking to match.

 > >  > > +                  (let ((uuid (org-id-uuid)))
 > >  > 
 > >  > Do you need to use `org-id-uuid' here? ob-python directly uses `md5'.
 > >  > If you still prefer org-id-uuid, we probably need to move it to
 > >  > org-macs.el
 > >
 > > I just need a random string.  `md5' would work for that.  However,
 > > might it be better to update ob-R and ob-python to use `org-id-uuid'?
 > > Both of those manually declare the randomness.  It's conceivable that
 > > someone may delete or mistype the number (100000000), resulting in a
 > > lower entropy.  An md5 is also not a uuid, strictly speaking.  Of
 > > course, the chance of collision is still basically zero and the cost
 > > of collision about the same.  Using `org-id-uuid' would only provide a
 > > consistent way to do things.
 > 
 > `md5' will be slightly faster compared to `org-id-uuid'. But it should
 > not matter.
 > 
 > If we want use `org-id-uuid', lets move it to org-macs.el. Requiring the
 > whole org-id.el must not be done. It has side effects of defining id:
 > links.

In the next revision (once we figure out the regex), I can create a separate commit moving `org-id-uuid' to org-macs.el and updating ob-R and ob-python from `md5' to `org-id-uuid' (assuming that's not an issue for the maintainers of those).  If you think speed is a concern, however, I can switch ob-shell.el to use plain `md5'.

 > 
 > >                          (concat "^" (regexp-quote org-babel-sh-prompt)
 > > -                                " *"))
 > > +                                "*"))
 > 
 > This is wrong. It unconditionally makes the last char in
 > `org-babel-sh-prompt' 0+. (Imagine it is changed to non-space in
 > future).

When you say "imagine it is changed to non-space...", do you refer to `org-babel-sh-prompt'?

Honestly, it's not clear to me what pattern(s) we need to match.

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  > The actual prompt is "org_babel_sh_prompt> ".
>
> Agreed.
>
>  > And we want to skip leading spaces in addition.
>  
> What do you mean by this?

I was following the pattern described in the docstring of
`comint-prompt-regexp', where it is suggested that prompts should follow
the pattern "^<regexp> *".

In the case of ob-shell.el, `org-babel-sh-prompt' is a string to be used
as a prompt and the corresponding regexp patter will be
"^<prompt string> *". Hence

  (concat "^" (regexp-quote org-babel-sh-prompt) " *")

>  > Adding " *" does not make the prompt match 2 spaces, but 1+.
>  
> Agreed.  
>
> It's not clear to me what pattern you're looking to match.

I hope the above clarified things.

>  > `md5' will be slightly faster compared to `org-id-uuid'. But it should
>  > not matter.
>  > 
>  > If we want use `org-id-uuid', lets move it to org-macs.el. Requiring the
>  > whole org-id.el must not be done. It has side effects of defining id:
>  > links.
>
> In the next revision (once we figure out the regex), I can create a separate commit moving `org-id-uuid' to org-macs.el and updating ob-R and ob-python from `md5' to `org-id-uuid' (assuming that's not an issue for the maintainers of those).  If you think speed is a concern, however, I can switch ob-shell.el to use plain `md5'.

I am in favour of using `org-id-uuid'. It might also be a useful generic
function for other purposes.

A slight concern is that some third-party code might depend on the
current pattern used for UUID string in ob-python. But we made no
promises here.

To be a bit safer, we can also refactor `org-uuidgen-p' exposing the
regexp used to match UUID. Also, it will make sense to move
`org-uuidgen-p' to org-macs.el.

>  > 
>  > >                          (concat "^" (regexp-quote org-babel-sh-prompt)
>  > > -                                " *"))
>  > > +                                "*"))
>  > 
>  > This is wrong. It unconditionally makes the last char in
>  > `org-babel-sh-prompt' 0+. (Imagine it is changed to non-space in
>  > future).
>
> When you say "imagine it is changed to non-space...", do you refer to `org-babel-sh-prompt'?

Yes.

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

Hi Jack and Jeremie!  I'm curious your thoughts about what Ihor and I are discussing at the end of this message about `md5'.

 ---- On Tue, 07 Mar 2023 07:45:02 -0500  Ihor Radchenko  wrote --- 
 > Matt matt@excalamus.com> writes:
 > 
 > >  > The actual prompt is "org_babel_sh_prompt> ".
 > >
 > > Agreed.
 > >
 > >  > And we want to skip leading spaces in addition.
 > >  
 > > What do you mean by this?
 > 
 > I was following the pattern described in the docstring of
 > `comint-prompt-regexp', where it is suggested that prompts should follow
 > the pattern "^ *".
 > 
 > In the case of ob-shell.el, `org-babel-sh-prompt' is a string to be used
 > as a prompt and the corresponding regexp patter will be
 > "^ *". Hence
 > 
 >   (concat "^" (regexp-quote org-babel-sh-prompt) " *")
 > 
 > >  > Adding " *" does not make the prompt match 2 spaces, but 1+.
 > >  
 > > Agreed.  
 > >
 > > It's not clear to me what pattern you're looking to match.
 > 
 > I hope the above clarified things.

I'm confused because when I run the Org from HEAD, I get:

(concat "^" (regexp-quote org-babel-sh-prompt) " *") -> "^org_babel_sh_prompt>  *"

That's *two* spaces between '>' and '*', not one.  The second space comes from either 1) the definition of `org-babel-sh-prompt', which is "org_babel_sh_prompt> " (with a single space) or 2) the " *" in the  (concat "^" (regexp-quote org-babel-sh-prompt) " *") expression.   Currently, the two combine via the concat to give *two* spaces in the regexp.

If I understand you correctly, you're trying to match the pattern given in the `comint-prompt-regexp' which is *one* space.   That's what I'm trying to do, too.

Way back in https://list.orgmode.org/87sfeyc7qr.fsf@localhost/, we had this exchange:

 ---- On Mon, 20 Feb 2023 11:24:52 +0000  Ihor Radchenko  wrote --- 
> Matt <matt@excalamus.com> writes:
>
> > +(defun ob-shell-async-chunk-callback (string)
> > +  "Filter applied to results before insertion.
> > +See `org-babel-comint-async-chunk-callback'."
> > +  (replace-regexp-in-string (concat org-babel-sh-prompt "*") "" string))
>
 > Why not using `comint-prompt-regexp'?

I switched out `org-babel-sh-prompt'  with `comint-prompt-regexp' so that the expression looks like:

+(defun ob-shell-async-chunk-callback (string)
+  "Filter applied to results before insertion.
+See `org-babel-comint-async-chunk-callback'."
+  (replace-regexp-in-string comint-prompt-regexp "" string))

This causes the new test `test-ob-shell/session-async-evaluation' to fail, as you pointed out: https://list.orgmode.org/87bkl96g6e.fsf@localhost/

The test fails when we switch out the prompt in the callback because `comint-prompt-regexp' has two spaces in it.  The second space causes a prompt to not be filtered (by the callback).  The output becomes ": 1\n: 2\n: org_babel_sh_prompt>\n" instead of  ": 1\n: 2\n" .  This looks like a bug in the `comint-prompt-regexp''.

It could be that `test-ob-shell/session-async-evaluation' doesn't test correctly, but it looks right to me (I could certainly be mistaken).  Therefore, I see only two options to fix it: remove a space from the concat expression (which I did in my latest patch) or remove a space from `org-babel-sh-prompt'.

Am I missing something?

 > >  > `md5' will be slightly faster compared to `org-id-uuid'. But it should
 > >  > not matter.
 > >  > 
 > >  > If we want use `org-id-uuid', lets move it to org-macs.el. Requiring the
 > >  > whole org-id.el must not be done. It has side effects of defining id:
 > >  > links.
 > >
 > > In the next revision (once we figure out the regex), I can create a separate commit moving `org-id-uuid' to org-macs.el and updating ob-R and ob-python from `md5' to `org-id-uuid' (assuming that's not an issue for the maintainers of those).  If you think speed is a concern, however, I can switch ob-shell.el to use plain `md5'.
 > 
 > I am in favour of using `org-id-uuid'. It might also be a useful generic
 > function for other purposes.
 > 
 > A slight concern is that some third-party code might depend on the
 > current pattern used for UUID string in ob-python. But we made no
 > promises here.
 > 
 > To be a bit safer, we can also refactor `org-uuidgen-p' exposing the
 > regexp used to match UUID. Also, it will make sense to move
 > `org-uuidgen-p' to org-macs.el.

I'm okay with all that.  I wonder, do Jack Kamm (of ob-python fame) and Jeremie Juste (of ob-R fame) have any thoughts on the matter.  I ask out of courtesy since they're the maintainers of those packages and I don't want to cross any boundaries by changing their implementations beneath them.  

Re: [PATCH] Async evaluation in ob-shell

[Max Nikulin]

On 10/03/2023 00:36, Matt wrote:
> (concat "^" (regexp-quote org-babel-sh-prompt) " *") -> "^org_babel_sh_prompt>  *"
> 
> Currently, the two combine via the concat to give*two*  spaces in the regexp.

"*" means zero or more, so "  *" with two spaces acts like " +" one or 
more space characters. It is unsafe to append just "*" or "+" to another 
regexp since you can not ensure what is the trailing pattern of the 
preceding regexp.

Re: [PATCH] Async evaluation in ob-shell

[Jack Kamm]

Matt <matt@excalamus.com> writes:

> Hi Jack and Jeremie!  I'm curious your thoughts about what Ihor and I are discussing at the end of this message about `md5'.

Thanks for checking in. I don't see any problems with switching to
org-id-uuid or similar.

Feel free to update ob-python to do this, or I can also do it later
following ob-shell's example.

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> Way back in https://list.orgmode.org/87sfeyc7qr.fsf@localhost/, we had this exchange:
>
>  ---- On Mon, 20 Feb 2023 11:24:52 +0000  Ihor Radchenko  wrote --- 
>> Matt <matt@excalamus.com> writes:
>>
>> > +(defun ob-shell-async-chunk-callback (string)
>> > +  "Filter applied to results before insertion.
>> > +See `org-babel-comint-async-chunk-callback'."
>> > +  (replace-regexp-in-string (concat org-babel-sh-prompt "*") "" string))
>>
>  > Why not using `comint-prompt-regexp'?
>
> I switched out `org-babel-sh-prompt'  with `comint-prompt-regexp' so that the expression looks like:
>
> +(defun ob-shell-async-chunk-callback (string)
> +  "Filter applied to results before insertion.
> +See `org-babel-comint-async-chunk-callback'."
> +  (replace-regexp-in-string comint-prompt-regexp "" string))
>
> This causes the new test `test-ob-shell/session-async-evaluation' to fail, as you pointed out: https://list.orgmode.org/87bkl96g6e.fsf@localhost/
>
> The test fails when we switch out the prompt in the callback because `comint-prompt-regexp' has two spaces in it.  The second space causes a prompt to not be filtered (by the callback).  The output becomes ": 1\n: 2\n: org_babel_sh_prompt>\n" instead of  ": 1\n: 2\n" .  This looks like a bug in the `comint-prompt-regexp''.

No, this looks like a bug in comint.el. ob-shell correctly sets
PROMPT to be org-babel-sh-prompt, which is "org_babel_sh_prompt> ", with
space! The fact that the comint output does not, in fact, contain space
is wrong.

We can probably remove the space in org-babel-sh-prompt to work around
this (likely, Emacs) bug.

> It could be that `test-ob-shell/session-async-evaluation' doesn't test correctly, but it looks right to me (I could certainly be mistaken).  Therefore, I see only two options to fix it: remove a space from the concat expression (which I did in my latest patch) or remove a space from `org-babel-sh-prompt'.

Removing space from the concat expression in plain wrong, as Max
explained. 

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

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


 > Matt matt@excalamus.com> writes:
 >
 > I see only two options to fix it: remove a space from the concat expression (which I did in my latest patch) or remove a space from `org-babel-sh-prompt'.

Unfortunately, I was mistaken and the second option (removing the space from `org-babel-sh-prompt') doesn't fix the issue.  The TLDR is that the code in `org-babel-comint-async-filter' which grabs the region between the indicators (incorrectly) fails to include the prompt's trailing space.

#+begin_longwinded_explanation
I'll first explain why removing the space from `org-babel-sh-prompt' doesn't fix the issue because it well also highlight the underlying problem.

If we remove the space from the `org-babel-sh-prompt', then `comint-prompt-regexp' becomes "^org_babel_sh_prompt> *" (with one space).   This would work if the string passed to the `ob-shell-async-chunk-callback' stayed the same.  It doesn't (this is where my reasoning and testing failed).  Changing the `org-babel-sh-prompt' to "org_babel_sh_prompt>" (without a space) causes the following string to be passed to the callback:

"org_babel_sh_prompt>1
org_babel_sh_prompt>2
org_babel_sh_prompt"

Note that the final prompt doesn't have a ">" and therefore the `comint-prompt-regexp' (which becomes "^org_babel_sh_prompt> * (with one space)) used in the callback fails to match it.  When we remove the space from the `org-babel-sh-prompt', the session buffer looks like this:

"sh-5.1$ PROMPT_COMMAND=;PS1="org_babel_sh_prompt>";PS2=
org_babel_sh_prompt>echo 'ob_comint_async_shell_start_39610981-1020-4baf-9dfb-f96d10af1cf8'
echo 1
echo 2
echo 'ob_comint_async_shell_end_39610981-1020-4baf-9dfb-f96d10af1cf8'
ob_comint_async_shell_start_39610981-1020-4baf-9dfb-f96d10af1cf8
org_babel_sh_prompt>1
org_babel_sh_prompt>2
org_babel_sh_prompt>ob_comint_async_shell_end_39610981-1020-4baf-9dfb-f96d10af1cf8
org_babel_sh_prompt>"

The `org-babel-comint-async-filter' is what calls the `ob-shell-async-chunk-callback' (ob-comint.el:284).  It monitors for the end indicator.  When that appears, it passes the region between the beginning of the end indicator **less 1** and the character after the end of the start indicator to the callback.  For a clean run of `test-ob-shell/session-async-evaluation', the beginning of the end indicator is at 361 and the character after the end of the start indicator is at 298.  This is the string I gave above which is missing the ">".  

In order to make the second option work, we'd need to change the "less 1" part of `org-babel-comint-async-filter' from (- (match-beginning 0) 1) to (match-beginning 0).   It turns out that's actually all we need to do.

When `org-babel-sh-prompt' is "org_babel_sh_prompt> " (with one space), then the session buffer looks like:

"sh-5.1$ PROMPT_COMMAND=;PS1="org_babel_sh_prompt> ";PS2=
org_babel_sh_prompt> echo 'ob_comint_async_shell_start_3270ed43-a99b-423f-a5fa-b15fb2e4ae26'
echo 1
echo 2
echo 'ob_comint_async_shell_end_3270ed43-a99b-423f-a5fa-b15fb2e4ae26'
ob_comint_async_shell_start_3270ed43-a99b-423f-a5fa-b15fb2e4ae26
org_babel_sh_prompt> 1
org_babel_sh_prompt> 2
org_babel_sh_prompt> ob_comint_async_shell_end_3270ed43-a99b-423f-a5fa-b15fb2e4ae26
org_babel_sh_prompt> "

The region passed to the callback is then defined as 366 to 300, or

"org_babel_sh_prompt> 1
org_babel_sh_prompt> 2
org_babel_sh_prompt>"  (<-- no space)

This looks okay at first glance.  However, **the last line is not a valid prompt**.  A prompt must end in a space!  When the `org-babel-sh-prompt' is set to  "org_babel_sh_prompt> " (with one space), the `comint-prompt-regexp' is "^org_babel_sh_prompt>  *" (with two spaces).  This means that the `comint-prompt-regexp' matches on a trailing space which the **region passed to the callback doesn't have**.  Therefore, the match fails.

Instead, if we modify the `org-babel-comint-async-filter' like

modified   lisp/ob-comint.el
@@ -273,7 +273,7 @@ STRING contains the output originally inserted into the comint buffer."
 		   (res-str-raw
 		    (buffer-substring
 		     ;; move point to beginning of indicator
-                     (- (match-beginning 0) 1)
+                     (match-beginning 0)
 		     ;; find the matching start indicator
 		     (cl-loop
                       do (re-search-backward indicator)

then the region passed to the callback will be from 367 to 300, or

"org_babel_sh_prompt> 1
org_babel_sh_prompt> 2
org_babel_sh_prompt> " (<-- with one space)

The `comint-prompt-regexp' will now match the last prompt in the region.

With this change, the `org-babel-sh-prompt' keeps the trailing space (like it should), the `comint-prompt-regexp' becomes "^org_babel_sh_prompt>  *" (with two spaces, requiring a prompt to have a trailing space like it should), the `ob-shell-async-chunk-callback' can use `comint-prompt-regexp' without modification, and the tests all pass.
#+end_longwinded_explanation

I've attached an updated diff.  If everyone is satisfied with this, I'll do a proper commit and then handle moving the uuid code like we talked about earlier in the thread.

[-- Attachment #2: 0004-ob-shell-Add-async-evaluation.diff --]
[-- Type: application/octet-stream, Size: 6818 bytes --]

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 54bf5127e..86c2bf7a7 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -273,7 +273,7 @@ STRING contains the output originally inserted into the comint buffer."
 		   (res-str-raw
 		    (buffer-substring
 		     ;; move point to beginning of indicator
-                     (- (match-beginning 0) 1)
+                     (match-beginning 0)
 		     ;; find the matching start indicator
 		     (cl-loop
                       do (re-search-backward indicator)
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 9e7b45a89..eab8ea935 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -269,12 +269,22 @@ var of the same value."
 	    (set-marker comint-last-output-start (point))
 	    (get-buffer (current-buffer)))))))
 
+(defconst ob-shell-async-indicator "echo 'ob_comint_async_shell_%s_%s'"
+  "Session output delimiter template.
+See `org-babel-comint-async-indicator'.")
+
+(defun ob-shell-async-chunk-callback (string)
+  "Filter applied to results before insertion.
+See `org-babel-comint-async-chunk-callback'."
+  (replace-regexp-in-string comint-prompt-regexp "" string))
+
 (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
   "Pass BODY to the Shell process in BUFFER.
 If RESULT-TYPE equals `output' then return a list of the outputs
 of the statements in BODY, if RESULT-TYPE equals `value' then
 return the value of the last statement in BODY."
   (let* ((shebang (cdr (assq :shebang params)))
+         (async (org-babel-comint-use-async params))
 	 (results-params (cdr (assq :result-params params)))
 	 (value-is-exit-status
 	  (or (and
@@ -306,19 +316,38 @@ return the value of the last statement in BODY."
                                 (concat (file-local-name script-file)  " " cmdline)))))
 		(buffer-string))))
 	   (session			; session evaluation
-	    (mapconcat
-	     #'org-babel-sh-strip-weird-long-prompt
-	     (mapcar
-	      #'org-trim
-	      (butlast ; Remove eoe indicator
-	       (org-babel-comint-with-output
-		   (session org-babel-sh-eoe-output t body)
-                 (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"))
+            (if async
+                (progn
+                  (let ((uuid (org-id-uuid)))
+                    (org-babel-comint-async-register
+                     session
+                     (current-buffer)
+                     "ob_comint_async_shell_\\(.+?\\)_\\(.+\\)"
+                     ;; "ob_comint_async_shell_\\(.+\\)_\\(.+\\)"
+                     'ob-shell-async-chunk-callback
+                     nil)
+                    (org-babel-comint-async-delete-dangling-and-eval
+                        session
+                      (insert (format ob-shell-async-indicator "start" uuid))
+                      (comint-send-input nil t)
+                      (insert (org-trim body))
+                      (comint-send-input nil t)
+                      (insert (format ob-shell-async-indicator "end" uuid))
+                      (comint-send-input nil t))
+                    uuid))
+	      (mapconcat
+	       #'org-babel-sh-strip-weird-long-prompt
+	       (mapcar
+	        #'org-trim
+	        (butlast ; Remove eoe indicator
+	         (org-babel-comint-with-output
+		     (session org-babel-sh-eoe-output t body)
+                   (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.
 	   ((org-string-nw-p shebang)
diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index 8366f9dbe..c56a76acf 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -33,6 +33,9 @@
 
 (org-test-for-executable "sh")
 
+(defconst test-ob-shell/uuid-regex
+  "[0-9a-fA-F]\\{8\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{4\\}\\b-[0-9a-fA-F]\\{12\\}")
+
 \f
 ;;; Code:
 (ert-deftest test-ob-shell/dont-insert-spaces-on-expanded-bodies ()
@@ -75,6 +78,59 @@ the body of the tangled block does."
     (if (should (equal '((1) (2)) result))
         (kill-buffer session-name))))
 
+(ert-deftest test-ob-shell/session-async-valid-header-arg-values ()
+  "Test that session runs asynchronously for certain :async values."
+  (let ((session-name "test-ob-shell/session-async-valid-header-arg-values")
+        (kill-buffer-query-functions nil))
+    (dolist (arg-val '("t" ""))
+     (org-test-with-temp-text
+         (concat "#+begin_src sh :session " session-name " :async " arg-val "
+echo 1<point>
+#+end_src")
+       (if (should
+            (string-match
+             test-ob-shell/uuid-regex
+             (org-trim (org-babel-execute-src-block))))
+           (kill-buffer session-name))))))
+
+(ert-deftest test-ob-shell/session-async-inserts-uuid-before-results-are-returned ()
+  "Test that a uuid placeholder is inserted before results are inserted."
+  (let ((session-name "test-ob-shell/session-async-inserts-uuid-before-results-are-returned")
+        (kill-buffer-query-functions nil))
+    (org-test-with-temp-text
+        (concat "#+begin_src sh :session " session-name " :async t
+echo 1<point>
+#+end_src")
+      (if (should
+           (string-match
+            test-ob-shell/uuid-regex
+            (org-trim (org-babel-execute-src-block))))
+          (kill-buffer session-name)))))
+
+(ert-deftest test-ob-shell/session-async-evaluation ()
+  "Test the async evaluation process."
+  (let* ((session-name "test-ob-shell/session-async-evaluation")
+         (kill-buffer-query-functions nil)
+         (start-time (current-time))
+         (wait-time (time-add start-time 3))
+         uuid-placeholder)
+    (org-test-with-temp-text
+        (concat "#+begin_src sh :session " session-name " :async t
+echo 1
+echo 2<point>
+#+end_src")
+      (setq uuid-placeholder (org-trim (org-babel-execute-src-block)))
+      (catch 'too-long
+        (while (string-match uuid-placeholder (buffer-string))
+          (progn
+            (sleep-for 0.01)
+            (when (time-less-p wait-time (current-time))
+              (throw 'too-long (ert-fail "Took too long to get result from callback"))))))
+    (search-forward "#+results")
+    (beginning-of-line 2)
+    (if (should (string= ": 1\n: 2\n" (buffer-substring-no-properties (point) (point-max))))
+          (kill-buffer session-name)))))
+
 (ert-deftest test-ob-shell/generic-uses-no-arrays ()
   "Test generic serialization of array into a single string."
   (org-test-with-temp-text

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> Unfortunately, I was mistaken and the second option (removing the space from `org-babel-sh-prompt') doesn't fix the issue.  The TLDR is that the code in `org-babel-comint-async-filter' which grabs the region between the indicators (incorrectly) fails to include the prompt's trailing space.
> --- a/lisp/ob-comint.el
> +++ b/lisp/ob-comint.el
> @@ -273,7 +273,7 @@ STRING contains the output originally inserted into the comint buffer."
>  		   (res-str-raw
>  		    (buffer-substring
>  		     ;; move point to beginning of indicator
> -                     (- (match-beginning 0) 1)
> +                     (match-beginning 0)
>  		     ;; find the matching start indicator
>  		     (cl-loop
>                        do (re-search-backward indicator)

This change looks reasonable.
If it does not break tests, I see not problem with pushing it as a
separate commit (to make things easier in case if that -1 did matter at
the end).

-- 
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: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
> index 9e7b45a89..eab8ea935 100644
> --- a/lisp/ob-shell.el
> +++ b/lisp/ob-shell.el
> @@ -269,12 +269,22 @@ var of the same value."
>  	    (set-marker comint-last-output-start (point))
>  	    (get-buffer (current-buffer)))))))
>  
> +(defconst ob-shell-async-indicator "echo 'ob_comint_async_shell_%s_%s'"
> +  "Session output delimiter template.
> +See `org-babel-comint-async-indicator'.")
> ...

I see that you pushed this onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=f7aa8c19f5170dbf09538686fb569f9b60acbd6c
May you also document this new feature in ORG-NEWS and in
https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html ?

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

 ---- On Thu, 23 Mar 2023 07:48:44 -0400  Ihor Radchenko  wrote --- 
 > May you also document this new feature in ORG-NEWS and in
 > https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html ?

Done.

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  ---- On Thu, 23 Mar 2023 07:48:44 -0400  Ihor Radchenko  wrote --- 
>  > May you also document this new feature in ORG-NEWS and in
>  > https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html ?
>
> Done.

A small note on the WORG page: it may be more natural to use :async yes
rather than :async t. Both are viable - in fact, anything other than
:async no and :async none will be treated as "t".

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

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


 ---- On Fri, 24 Mar 2023 05:13:34 -0400  Ihor Radchenko  wrote --- 

 > A small note on the WORG page: it may be more natural to use :async yes
 > rather than :async t. Both are viable - in fact, anything other than
 > :async no and :async none will be treated as "t".
 
Ah, okay.  I'll make that more clear.

Somewhat related, I had this left over from when I was working out the async code.  It prevents the user from running :async without the (required) :session header.    The :async header runs the default (blocking) process when :session is missing.

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 86c2bf7a7..384bfcda8 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -206,11 +206,12 @@ comint outputs due to buffering.")
 PARAMS are the header arguments as passed to
 `org-babel-execute:lang'."
   (let ((async (assq :async params))
-        (session (assq :session params)))
+        (sessionp (not (member (cdr (assq :session params)) '("no" "none")))))
+    (if (and async (not sessionp))
+          (error (org-babel-eval-error-notify 1 "ERROR: must use 'async' with 'session'")))
     (and async
-	 (not org-babel-exp-reference-buffer)
-         (not (equal (cdr async) "no"))
-         (not (equal (cdr session) "none")))))
+         sessionp
+         (not org-babel-exp-reference-buffer))))
 
 (defun org-babel-comint-async-filter (string)
   "Captures Babel async output from comint buffer back to Org mode buffers.

It's really just a nicety.  The user can cancel the accidental blocking process with C-g.  However, the block is run in a different shell than expected and it's jarring to have Emacs freeze when you expect async.

Thoughts on including it or something similar?

[-- Attachment #2: error-on-async-session-mismatch.diff --]
[-- Type: application/octet-stream, Size: 894 bytes --]

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 86c2bf7a7..384bfcda8 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -206,11 +206,12 @@ comint outputs due to buffering.")
 PARAMS are the header arguments as passed to
 `org-babel-execute:lang'."
   (let ((async (assq :async params))
-        (session (assq :session params)))
+        (sessionp (not (member (cdr (assq :session params)) '("no" "none")))))
+    (if (and async (not sessionp))
+          (error (org-babel-eval-error-notify 1 "ERROR: must use 'async' with 'session'")))
     (and async
-	 (not org-babel-exp-reference-buffer)
-         (not (equal (cdr async) "no"))
-         (not (equal (cdr session) "none")))))
+         sessionp
+         (not org-babel-exp-reference-buffer))))
 
 (defun org-babel-comint-async-filter (string)
   "Captures Babel async output from comint buffer back to Org mode buffers.

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> -        (session (assq :session params)))
> +        (sessionp (not (member (cdr (assq :session params)) '("no" "none")))))
> +    (if (and async (not sessionp))
> +          (error (org-babel-eval-error-notify 1 "ERROR: must use 'async' with 'session'")))

This is reasonable, but please do not use `org-babel-eval-error-notify'.
Just a simple `user-error'.

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

 ---- On Fri, 24 Mar 2023 05:11:38 -0400  Ihor Radchenko  wrote --- 
 > Matt matt@excalamus.com> writes:
 > 
 > >  ---- On Thu, 23 Mar 2023 07:48:44 -0400  Ihor Radchenko  wrote --- 
 > >  > May you also document this new feature in ORG-NEWS and in
 > >  > https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html ?
 > >
 > > Done.
 > 
 > A small note on the WORG page: it may be more natural to use :async yes
 > rather than :async t. Both are viable - in fact, anything other than
 > :async no and :async none will be treated as "t".

Updated WORG with commits 9d153ea4, 754c72cb, and 18333299.  Updated ORG-NEWS with 6c9104f59.

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  > A small note on the WORG page: it may be more natural to use :async yes
>  > rather than :async t. Both are viable - in fact, anything other than
>  > :async no and :async none will be treated as "t".
>
> Updated WORG with commits 9d153ea4, 754c72cb, and 18333299.  Updated ORG-NEWS with 6c9104f59.

Thanks, but I am not seeing 6c9104f59.

-- 
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: [PATCH] Async evaluation in ob-shell

[Matt]

 ---- On Mon, 17 Apr 2023 14:53:18 -0400  Ihor Radchenko  wrote --- 
 > Matt matt@excalamus.com> writes:
 > 
 > >  > A small note on the WORG page: it may be more natural to use :async yes
 > >  > rather than :async t. Both are viable - in fact, anything other than
 > >  > :async no and :async none will be treated as "t".
 > >
 > > Updated WORG with commits 9d153ea4, 754c72cb, and 18333299.  Updated ORG-NEWS with 6c9104f59.
 > 
 > Thanks, but I am not seeing 6c9104f59.

I'm sorry, I was unclear about which repo these commits were in.  ORG-NEWS was updated here: https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=6c9104f59ca8085abe477a81857548461bf88f23

Re: [PATCH] Async evaluation in ob-shell

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  > Thanks, but I am not seeing 6c9104f59.
>
> I'm sorry, I was unclear about which repo these commits were in.  ORG-NEWS was updated here: https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=6c9104f59ca8085abe477a81857548461bf88f23

Now, see. I was again confused by savannah interface...
Thanks for the clarification!

-- 
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>

[SUGGESTION] ob-shell async result output should not contains shell prompt

[Christopher M. Miles]

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


The ob-shell async result output contains the shell prompt. I think it
should not be captured.

#+begin_src shell :session "test2" :async t
sleep 30
echo "hello, world"
#+end_src

#+RESULTS[(2023-03-23 11:19:22) 461ed5de684f6e619890709175ec73e80b67b2d6]:
: bash-5.2$ hello, world

-- 

[ stardiviner ]
I try to make every word tell the meaning that I want to express without misunderstanding.

Blog: https://stardiviner.github.io/
IRC(libera.chat, freenode): stardiviner, Matrix: stardiviner
GPG: F09F650D7D674819892591401B5DF1C95AE89AC3

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

Re: [SUGGESTION] ob-shell async result output should not contains shell prompt

[Matt]

 ---- On Wed, 22 Mar 2023 23:25:50 -0400  Christopher M. Miles  wrote --- 
 > 
 > The ob-shell async result output contains the shell prompt. I think it
 > should not be captured.
 > 
 > #+begin_src shell :session "test2" :async t
 > sleep 30
 > echo "hello, world"
 > #+end_src
 > 
 > #+RESULTS[(2023-03-23 11:19:22) 461ed5de684f6e619890709175ec73e80b67b2d6]:
 > : bash-5.2$ hello, world

Thanks for reporting this.

Try using for the babel language whatever shell the variable `shell-file-name' gives.  For example, if `shell-file-name' is /bin/bash, do this:

#+begin_src bash :session "test2" :async t
sleep 1
echo "hello, world"
#+end_src

Is there a reason you're using "shell" instead of one of the shells listed in `org-babel-shell-names'?

Re: [SUGGESTION] ob-shell async result output should not contains shell prompt

[Christopher M. Miles]

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


Matt <matt@excalamus.com> writes:

>  ---- On Wed, 22 Mar 2023 23:25:50 -0400  Christopher M. Miles  wrote --- 
>  > 
>  > The ob-shell async result output contains the shell prompt. I think it
>  > should not be captured.
>  > 
>  > #+begin_src shell :session "test2" :async t
>  > sleep 30
>  > echo "hello, world"
>  > #+end_src
>  > 
>  > #+RESULTS[(2023-03-23 11:19:22) 461ed5de684f6e619890709175ec73e80b67b2d6]:
>  > : bash-5.2$ hello, world
>
> Thanks for reporting this.
>
> Try using for the babel language whatever shell the variable `shell-file-name' gives.  For example, if `shell-file-name' is /bin/bash, do this:
>
> #+begin_src bash :session "test2" :async t
> sleep 1
> echo "hello, world"
> #+end_src
>
> Is there a reason you're using "shell" instead of one of the shells listed in `org-babel-shell-names'?

Using language identities like bellowing:

#+begin_src sh :session "*ob-shell*" :async t
sleep 30
echo "hello, world"
#+end_src

#+RESULTS[(2023-03-23 19:14:12) dd777a237986481833c08eb5eceac717576eddb7]:
: org_babel_sh_prompt> hello, world

#+begin_src bash :session "*ob-shell-bash*" :async t
sleep 30
echo "hello, world"
#+end_src

#+RESULTS[(2023-03-23 19:14:15) 23f9ad130f7a1268e21821c6baaea2b057c70d3e]:
: org_babel_sh_prompt> hello, world

#+begin_src zsh :session "*ob-shell-zsh*" :async t
sleep 30
echo "hello, world"
#+end_src

#+RESULTS[(2023-03-23 19:14:17) 2bb44d96c2e482a90c5a89bdde0b64d0319663a1]:
: %                                                                                                                                                                      
:  
: %                                                                                                                                                                      
:  
: hello, world
: %                                                                                                                                                                      
:  

Still got a prompt. Is this intended? I think the output should be kept clean because the result
output might be used as input for other source blocks as data.

Maybe error in my Org babel settings? Bellowing is my system and variable values:

#+begin_src emacs-lisp
system-type
#+end_src

#+RESULTS[(2023-03-23 19:27:13) 7df8395169a77d83cb6a5a6efc2223d412813efa]:
: darwin

#+begin_src emacs-lisp
shell-file-name
#+end_src

#+RESULTS[(2023-03-23 19:26:33) e6fed18a9a543dd6320385ee715d9ee68b464a04]:
: /opt/homebrew/bin/bash

#+begin_src emacs-lisp
org-babel-sh-prompt
#+end_src

#+RESULTS[(2023-03-23 19:30:12) f6efc29dba5be2171eba0a25abec19908fb1c6be]:
: org_babel_sh_prompt> 

#+begin_src emacs-lisp
org-babel-shell-names
#+end_src

#+RESULTS[(2023-03-23 19:27:27) 360d6d35db3eb48deb664349eed34b7541923ca2]:
| sh | bash | zsh | fish | csh | ash | dash | ksh | mksh | posh |

#+begin_src emacs-lisp :results pp
org-babel-shell-set-prompt-commands
#+end_src

#+RESULTS[(2023-03-23 19:27:44) 910fbbafc6fea4a1846f5a31f8b7dd102eca4928]:
: (("fish" . "function fish_prompt\n	echo \"%s\"\nend")
:  ("csh" . "set prompt=\"%s\"\nset prompt2=\"\"")
:  ("posh" . "function prompt { \"%s\" }")
:  (t . "PROMPT_COMMAND=;PS1=\"%s\";PS2="))

-- 

[ stardiviner ]
I try to make every word tell the meaning that I want to express without misunderstanding.

Blog: https://stardiviner.github.io/
IRC(libera.chat, freenode): stardiviner, Matrix: stardiviner
GPG: F09F650D7D674819892591401B5DF1C95AE89AC3

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

Re: [SUGGESTION] ob-shell async result output should not contains shell prompt

[Matt]

 ---- On Thu, 23 Mar 2023 07:12:29 -0400  Christopher M. Miles  wrote --- 
 
 > #+begin_src bash :session "*ob-shell-bash*" :async t
 > sleep 30
 > echo "hello, world"
 > #+end_src
 > 
 > #+RESULTS[(2023-03-23 19:14:15) 23f9ad130f7a1268e21821c6baaea2b057c70d3e]:
 > : org_babel_sh_prompt> hello, world
 > 
 > Still got a prompt. Is this intended? I think the output should be kept clean because the result
 > output might be used as input for other source blocks as data.

I absolutely agree.  This is a bug.  

It should be an easy fix on my end but, in case there are details I'm overlooking, this specific example can use the following workaround:

#+begin_src bash :session "*ob-shell-bash*" :async t
sleep 30 && echo "hello, world"
#+end_src

To explain what's going on...

#+begin_longwinded_explanation
Shell output is filtered for prompts which should be removed before inserting the results.  The issue is that the filter assumes the prompt starts at the beginning of the line.  When sleep is called, it returns nothing and the next prompt appears on the same line:

sh-5.1$ PROMPT_COMMAND=;PS1="org_babel_sh_prompt> ";PS2=
org_babel_sh_prompt> echo 'ob_comint_async_shell_start_770d9c8f-deda-4359-aee9-a433a75a5e0d'
echo "1"
sleep 3
echo "2"
echo 'ob_comint_async_shell_end_770d9c8f-deda-4359-aee9-a433a75a5e0d'
ob_comint_async_shell_start_770d9c8f-deda-4359-aee9-a433a75a5e0d
org_babel_sh_prompt> 1
org_babel_sh_prompt> org_babel_sh_prompt> 2
org_babel_sh_prompt> ob_comint_async_shell_end_770d9c8f-deda-4359-aee9-a433a75a5e0d

Changing the `ob-shell-async-chunk-callback' like this will fix it:

@@ -276,7 +276,7 @@ See `org-babel-comint-async-indicator'.")
 (defun ob-shell-async-chunk-callback (string)
   "Filter applied to results before insertion.
 See `org-babel-comint-async-chunk-callback'."
-  (replace-regexp-in-string comint-prompt-regexp "" string))
+  (replace-regexp-in-string (concat (regexp-quote org-babel-sh-prompt) " *") "" string))
 
 (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
   "Pass BODY to the Shell process in BUFFER.
#+end_longwinded_explanation

Re: [SUGGESTION] ob-shell async result output should not contains shell prompt

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> Changing the `ob-shell-async-chunk-callback' like this will fix it:
>
> @@ -276,7 +276,7 @@ See `org-babel-comint-async-indicator'.")
>  (defun ob-shell-async-chunk-callback (string)
>    "Filter applied to results before insertion.
>  See `org-babel-comint-async-chunk-callback'."
> -  (replace-regexp-in-string comint-prompt-regexp "" string))
> +  (replace-regexp-in-string (concat (regexp-quote org-babel-sh-prompt) " *") "" string))

This is trying to replicate what `org-babel-comint-with-output' does
already and is stumbling upon the same edge cases.

May you instead factor out the filtering code from
`org-babel-comint-with-output' and reuse it in ob-shell?

-- 
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>

Remove "shell" as a supported Babel language within ob-shell.el (was Re: [SUGGESTION] ob-shell async result output should not contains shell prompt)

[Matt]

 > Matt matt@excalamus.com> writes:
 >
 > > Is there a reason you're using "shell" instead of one of the shells listed in `org-babel-shell-names'?

I'm still curious why you're using "shell".  I want to know if it's something you're using for a specific reason.  There's no wrong answer!

I ask because I have an agenda: as far as I can tell, "shell" as a Babel language is a historical accident.  

#+begin_longwinded_explanation
Originally, ob-shell.el was called ob-sh.el.  The main function was called `org-babel-execute:sh' and only /usr/bin/env sh was supported.  Over time it became clear that to support other shells, the "sh" name shouldn't be used for the package or the main function.  That is, 'sh' refers to a specific binary and, if other binaries such as bash, dash, csh, etc. were to be supported, it would be misleading for the Babel language to refer to a specific shell, 'sh'.  So, the terminology was changed to something more general, "shell".  The package was renamed to "ob-shell.el", the "namespace" updated to "shell" (for example, `org-babel-execute:shell'), and the package load call changed from (sh . t) to (shell . t).  This officially happened with Org 8.2 (ORG-NEWS noted the change in commit 1a9a6666dcb6d34bcbdff45445c827ed99dbf0b8, Tue Jan 21 09:52:31 2014).  I think this gave people the (understandable) impression that "shell" was a valid Babel language, in addition to those listed in `org-babel-shell-names'.  

And this is where the accident happened: "shell" as a Babel language only **happens**to work.  The Babel framework looks for a function prototype like "org-babel-execute:<language>".  When ob-sh.el was changed to ob-shell.el, the function `org-babel-execute:sh' became `org-babel-execute:shell'.   A call like follows is perfectly legal as far as the Babel framework is concerned:

#+begin_src shell
echo "hello, world"
#+end_src

When such a block is run, Babel looks for a function called `org-babel-execute:shell'.  Running the block prior to Org 8.2 should have failed because no `org-babel-execute:shell' function existed.  The name change happened to source Fri Dec 13 09:52:05 2013 in commit 7a6c0e35415c4a173d101336029262f3a09abb91.  After the name change, the function existed and a block using "shell" would execute!  

The "shell" language specifier, as far as I can tell, was never really intentionally supported.  Instead, it just happened to work.  It happened to work because, as far back as the first org-babel-sh.el commit, the process buffer is created using the `shell' function.  I don't know the history of `shell', but presently the documentation says,

Program used comes from variable ‘explicit-shell-file-name’,
 or (if that is nil) from the ESHELL environment variable,
 or (if that is nil) from ‘shell-file-name’.

That is, the `shell' command falls back to `shell-file-name'.  I assume that `shell' has always had that, or a similar, fallback.  The `shell-file-name' is a direct path to an executable.   This means that when "shell" is used for the language, `shell-file-name' is called and **any** startup script, such as .bash_profile or .bashrc, is called.  The prompt could be set to **anything** and Emacs will never know, and can never know, what the prompt is without the user explicitly informing Emacs.

Aside from the code change which allowed "shell" to work, "official" support of "shell" comes from Org manual commit 9d072ad67517875069f913315d762c9bb1e9c3ec, Sun Dec 17 11:06:05 2017 (for example, https://git.savannah.gnu.org/cgit/emacs/org-mode.git/tree/doc/org-manual.org?id=f7aa8c19f5170dbf09538686fb569f9b60acbd6c#n18410).  This appears unconnected with the code change.  The addition to the manual happened 4 years after the code name change and none of the commit messages around the time of code change suggest that "shell" was intended to work as a language.  In fact, I found this email from Eric Schulte (creator of Babel and maintainer at the time of the code change) which suggests that "shell" is in fact not supported or intented as a language (https://lists.gnu.org/archive/html/emacs-orgmode/2013-12/msg00294.html):

In response to the statement,

"a coworker used "#+BEGIN_SRC shell" where he should have written "#+BEGIN_SRC sh"

Eric says,

"[The suggested work around] would protect against this particular error"

#+end_longwinded_explanation

Regardless of whether "shell" was intended to work as a Babel language, the fact remains that it does work and that it's been advertised in the manual (at least) for 6 years.  What are the pros and cons of "shell"?

What benefit does "shell" provide?

- The "shell" language allows an arbitrary executable to be run.  This means that shells other than those given in `org-babel-shell-names' can be run.  People using a non-supported shell could still benefit from ob-shell.

What downsides does "shell" bring?

- "shell" falls back to `shell-file-name' which can be an arbitrary executable.  Whenever I hear "runs an arbitrary executable", my ears perk up and I start to sweat.  There may be security considerations.  
- If that executable is a shell, then the prompt gets set independently from Emacs.  For the prompt to be filtered from the output, users would need to provide Emacs with the correct regexp.  A recent thread discussed creating a header arg to address this: https://list.orgmode.org/87ttzgeg3w.fsf@localhost/
- We would get bug reports about non-supported shells which kind of work, but have issues because they're not supported
- Maintence associated with supporting arbitrary (shell) executables

As the current maintainer of ob-shell, I'm in favor of removing "shell" as a Babel language.  The cons appear to far outweigh the pros.  However, I'm aware others may have good use for it.  It's been a part of Org for nearly a decade.  I'm sure it's part of people's workflow, especially since it's been in the manual for 6 years.  Are there any pros, cons, use-cases, or considerations I've overlooked?

Re: Remove "shell" as a supported Babel language within ob-shell.el

[Christopher M. Miles]

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


At first, thanks for this long parsing and explanation.

Matt <matt@excalamus.com> writes:

>  > Matt matt@excalamus.com> writes:
>  >
>  > > Is there a reason you're using "shell" instead of one of the shells listed in `org-babel-shell-names'?
>
> I'm still curious why you're using "shell".  I want to know if it's something you're using for a specific reason.  There's no wrong answer!
>
> I ask because I have an agenda: as far as I can tell, "shell" as a Babel language is a historical accident.  
>
> #+begin_longwinded_explanation
> Originally, ob-shell.el was called ob-sh.el.  The main function was called `org-babel-execute:sh' and only /usr/bin/env sh was supported.  Over time it became clear that to support other shells, the "sh" name shouldn't be used for the package or the main function.  That is, 'sh' refers to a specific binary and, if other binaries such as bash, dash, csh, etc. were to be supported, it would be misleading for the Babel language to refer to a specific shell, 'sh'.  So, the terminology was changed to something more general, "shell".  The package was renamed to "ob-shell.el", the "namespace" updated to "shell" (for example, `org-babel-execute:shell'), and the package load call changed from (sh . t) to (shell . t).  This officially happened with Org 8.2 (ORG-NEWS noted the change in commit 1a9a6666dcb6d34bcbdff45445c827ed99dbf0b8, Tue Jan 21 09:52:31 2014).  I think this gave people the (understandable) impression that "shell" was a valid Babel language, in addition to those listed in `org-babel-shell-names'.  
>
> And this is where the accident happened: "shell" as a Babel language only **happens**to work.  The Babel framework looks for a function prototype like "org-babel-execute:<language>".  When ob-sh.el was changed to ob-shell.el, the function `org-babel-execute:sh' became `org-babel-execute:shell'.   A call like follows is perfectly legal as far as the Babel framework is concerned:
>
> #+begin_src shell
> echo "hello, world"
> #+end_src
>
> When such a block is run, Babel looks for a function called `org-babel-execute:shell'. Running the
> block prior to Org 8.2 should have failed because no `org-babel-execute:shell' function existed. The
> name change happened to source Fri Dec 13 09:52:05 2013 in commit
> 7a6c0e35415c4a173d101336029262f3a09abb91. After the name change, the function existed and a block
> using "shell" would execute!

Yes, I originally use "sh" too. But at some time point, I saw an article
somewhere, then I switched to "shell". I forget the specific reason
already.

> The "shell" language specifier, as far as I can tell, was never really intentionally supported.
> Instead, it just happened to work. It happened to work because, as far back as the first
> org-babel-sh.el commit, the process buffer is created using the `shell' function. I don't know the
> history of `shell', but presently the documentation says,
>
> Program used comes from variable ‘explicit-shell-file-name’,
>  or (if that is nil) from the ESHELL environment variable,
>  or (if that is nil) from ‘shell-file-name’.
>
> That is, the `shell' command falls back to `shell-file-name'. I assume that `shell' has always had
> that, or a similar, fallback. The `shell-file-name' is a direct path to an executable. This means
> that when "shell" is used for the language, `shell-file-name' is called and **any** startup script,
> such as .bash_profile or .bashrc, is called. The prompt could be set to **anything** and Emacs will
> never know, and can never know, what the prompt is without the user explicitly informing Emacs.
>
> Aside from the code change which allowed "shell" to work, "official" support of "shell" comes from
> Org manual commit 9d072ad67517875069f913315d762c9bb1e9c3ec, Sun Dec 17 11:06:05 2017 (for example,
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/tree/doc/org-manual.org?id=f7aa8c19f5170dbf09538686fb569f9b60acbd6c#n18410).
> This appears unconnected with the code change. The addition to the manual happened 4 years after the
> code name change and none of the commit messages around the time of code change suggest that "shell"
> was intended to work as a language. In fact, I found this email from Eric Schulte (creator of Babel
> and maintainer at the time of the code change) which suggests that "shell" is in fact not supported
> or intented as a language (https://lists.gnu.org/archive/html/emacs-orgmode/2013-12/msg00294.html):
>
> In response to the statement,
>
> "a coworker used "#+BEGIN_SRC shell" where he should have written "#+BEGIN_SRC sh"
>
> Eric says,
>
> "[The suggested work around] would protect against this particular error"
>
> #+end_longwinded_explanation
>
> Regardless of whether "shell" was intended to work as a Babel language, the fact remains that it
> does work and that it's been advertised in the manual (at least) for 6 years. What are the pros and
> cons of "shell"?
>
> What benefit does "shell" provide?
>
> - The "shell" language allows an arbitrary executable to be run. This means that shells other than
> those given in `org-babel-shell-names' can be run. People using a non-supported shell could still
> benefit from ob-shell.
>
> What downsides does "shell" bring?
>
> - "shell" falls back to `shell-file-name' which can be an arbitrary executable. Whenever I hear
> "runs an arbitrary executable", my ears perk up and I start to sweat. There may be security
> considerations.

This arbitrary executable fallback mechanism indeed is a con side.

> - If that executable is a shell, then the prompt gets set independently from Emacs. For the prompt
> to be filtered from the output, users would need to provide Emacs with the correct regexp. A recent
> thread discussed creating a header arg to address this:
> https://list.orgmode.org/87ttzgeg3w.fsf@localhost/
> - We would get bug reports about non-supported shells which kind of work, but have issues because they're not supported
> - Maintence associated with supporting arbitrary (shell) executables
>
> As the current maintainer of ob-shell, I'm in favor of removing "shell" as a Babel language. The
> cons appear to far outweigh the pros. However, I'm aware others may have good use for it. It's been
> a part of Org for nearly a decade. I'm sure it's part of people's workflow, especially since it's
> been in the manual for 6 years. Are there any pros, cons, use-cases, or considerations I've
> overlooked?

If the "shell" language will be removed, I'm ok with that. I hope this
library can warn user "shell" is deprecated. Because I have a lot of
already written Org mode files using "shell" as source block language.
Replacing it with command-line tools like "sed" etc is ok. Like adding a
line warning code:

#+begin_src emacs-lisp
(warn "The 'shell' language is deprecated already, use 'sh' instead.")
#+end_src

-- 

[ stardiviner ]
I try to make every word tell the meaning that I want to express without misunderstanding.

Blog: https://stardiviner.github.io/
IRC(libera.chat, freenode): stardiviner, Matrix: stardiviner
GPG: F09F650D7D674819892591401B5DF1C95AE89AC3

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

Re: Remove "shell" as a supported Babel language within ob-shell.el (was Re: [SUGGESTION] ob-shell async result output should not contains shell prompt)

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

> What benefit does "shell" provide?
>
> - The "shell" language allows an arbitrary executable to be run.  This means that shells other than those given in `org-babel-shell-names' can be run.  People using a non-supported shell could still benefit from ob-shell.
>
> What downsides does "shell" bring?
>
> - "shell" falls back to `shell-file-name' which can be an arbitrary executable.  Whenever I hear "runs an arbitrary executable", my ears perk up and I start to sweat.  There may be security considerations.  
> - If that executable is a shell, then the prompt gets set independently from Emacs.  For the prompt to be filtered from the output, users would need to provide Emacs with the correct regexp.  A recent thread discussed creating a header arg to address this: https://list.orgmode.org/87ttzgeg3w.fsf@localhost/
> - We would get bug reports about non-supported shells which kind of work, but have issues because they're not supported
> - Maintence associated with supporting arbitrary (shell) executables
>
> As the current maintainer of ob-shell, I'm in favor of removing "shell" as a Babel language.  The cons appear to far outweigh the pros.  However, I'm aware others may have good use for it.  It's been a part of Org for nearly a decade.  I'm sure it's part of people's workflow, especially since it's been in the manual for 6 years.  Are there any pros, cons, use-cases, or considerations I've overlooked?

I would not see arbitrary executable to be such a big deal. At the end,
if SHELL is set to something fishy, the user is likely in serious
trouble anyway. SHELL is a part POSIX standard at the end.

Yet, the problem with unsupported shells is indeed real.
Moreover, "shell" code blocks are currently not portable to different
environments.

I suggest the following:
1. Introduce a new customization `org-babel-default-shell', defaulting
   to (or (executable-find "sh") (executable-find "cmd.exe")).
2. Use the value as default shell in "shell" code blocks.
3. Document and announce the change.
4. Create org-lint checker that will mark "shell" code blocks as not
   desired.

The above steps will ensure minimal breakage for existing uses of
"shell" blocks. Only users who wrote shell blocks for non-standard shell
will have to adapt.

-- 
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: Remove "shell" as a supported Babel language within ob-shell.el (was Re: [SUGGESTION] ob-shell async result output should not contains shell prompt)

[Samuel Wales]

i have a vague memory of having used sh and then we were told to use
shell.  can we all use begin src sh now?

On 3/24/23, Ihor Radchenko <yantar92@posteo.net> wrote:
> Matt <matt@excalamus.com> writes:
>
>> What benefit does "shell" provide?
>>
>> - The "shell" language allows an arbitrary executable to be run.  This
>> means that shells other than those given in `org-babel-shell-names' can be
>> run.  People using a non-supported shell could still benefit from
>> ob-shell.
>>
>> What downsides does "shell" bring?
>>
>> - "shell" falls back to `shell-file-name' which can be an arbitrary
>> executable.  Whenever I hear "runs an arbitrary executable", my ears perk
>> up and I start to sweat.  There may be security considerations.
>> - If that executable is a shell, then the prompt gets set independently
>> from Emacs.  For the prompt to be filtered from the output, users would
>> need to provide Emacs with the correct regexp.  A recent thread discussed
>> creating a header arg to address this:
>> https://list.orgmode.org/87ttzgeg3w.fsf@localhost/
>> - We would get bug reports about non-supported shells which kind of work,
>> but have issues because they're not supported
>> - Maintence associated with supporting arbitrary (shell) executables
>>
>> As the current maintainer of ob-shell, I'm in favor of removing "shell" as
>> a Babel language.  The cons appear to far outweigh the pros.  However, I'm
>> aware others may have good use for it.  It's been a part of Org for nearly
>> a decade.  I'm sure it's part of people's workflow, especially since it's
>> been in the manual for 6 years.  Are there any pros, cons, use-cases, or
>> considerations I've overlooked?
>
> I would not see arbitrary executable to be such a big deal. At the end,
> if SHELL is set to something fishy, the user is likely in serious
> trouble anyway. SHELL is a part POSIX standard at the end.
>
> Yet, the problem with unsupported shells is indeed real.
> Moreover, "shell" code blocks are currently not portable to different
> environments.
>
> I suggest the following:
> 1. Introduce a new customization `org-babel-default-shell', defaulting
>    to (or (executable-find "sh") (executable-find "cmd.exe")).
> 2. Use the value as default shell in "shell" code blocks.
> 3. Document and announce the change.
> 4. Create org-lint checker that will mark "shell" code blocks as not
>    desired.
>
> The above steps will ensure minimal breakage for existing uses of
> "shell" blocks. Only users who wrote shell blocks for non-standard shell
> will have to adapt.
>
> --
> 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>
>
>


-- 
The Kafka Pandemic

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

Re: Remove "shell" as a supported Babel language within ob-shell.el (was Re: [SUGGESTION] ob-shell async result output should not contains shell prompt)

[Ihor Radchenko]

Samuel Wales <samologist@gmail.com> writes:

> i have a vague memory of having used sh and then we were told to use
> shell.  can we all use begin src sh now?

You can use anything listed in `org-babel-shell-names'.
Also, see https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-shell.html

-- 
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: Remove "shell" as a supported Babel language within ob-shell.el (was Re: [SUGGESTION] ob-shell async result output should not contains shell prompt)

[Matt]

 ---- On Fri, 24 Mar 2023 07:38:58 -0400  Ihor Radchenko  wrote --- 

  > I suggest the following:
 > 1. Introduce a new customization `org-babel-default-shell', defaulting
 >    to (or (executable-find "sh") (executable-find "cmd.exe")).
 > 2. Use the value as default shell in "shell" code blocks.
 > 3. Document and announce the change.
 > 4. Create org-lint checker that will mark "shell" code blocks as not
 >    desired.
 > 
 > The above steps will ensure minimal breakage for existing uses of
 > "shell" blocks. Only users who wrote shell blocks for non-standard shell
 > will have to adapt.
 
These are good suggestions.  Thank you!

Re: [PATCH] Async evaluation in ob-shell

[jackkamm]

Matt <matt@excalamus.com> writes:

> It's not clear to me if that's something that a better regexp would handle or if it should be cleaned up in the callback function.  I'm still figuring out how it's done in ob-python and ob-R.

ob-python and ob-R call out to python.el or ESS to evaluate code.

IIRC, they will write code to tempfile and then source the tempfile
within python or R session. So then we don't need to worry about
cleaning up prompts between the lines.

org-babel-comint-async-filter is capable of taking a similar approach,
and reading/writing to tempfile. I believe this approach would be
generally more robust, but the major weakness is that it would break if
you SSH'd to a remote host in the middle of the session.

There was an interesting patch to ob-shell that was never applied, that
took the approach of wrapping the code block in a bash function before
executing; I think it might be a promising approach:

https://lists.gnu.org/archive/html/emacs-orgmode/2020-02/msg00923.html

By the way, I took a look at ob-shell for the first time in awhile, and
noticed that ob-shell now forces the prompt to be like:

org_babel_sh_prompt>

Which I think makes cleaning up the prompt markers a lot more
robust. But it is a bit ugly, and also seems to break working with shell
sessions started outside of Babel with M-x shell.

Re: [PATCH] Async evaluation in ob-shell

[Matt]

 ---- On Sat, 11 Feb 2023 15:56:00 -0500    wrote --- 
 > org-babel-comint-async-filter is capable of taking a similar approach,
 > and reading/writing to tempfile.

There is some precedence in ob-shell for this.  Currently, the cmdline, stdin, and shebang headers use temp files.  It may be that implementing async versions of these could use this part of the async API.  However, cmdline, stdin, and shebang each use a temporary shell process rather than a dedicated comint, even when the :session header is present.  The async implementation requires a comint buffer.

 > I believe this approach would be
 > generally more robust, but the major weakness is that it would break if
 > you SSH'd to a remote host in the middle of the session.

Good point.

 > There was an interesting patch to ob-shell that was never applied, that
 > took the approach of wrapping the code block in a bash function before
 > executing; I think it might be a promising approach:
 > 
 > https://lists.gnu.org/archive/html/emacs-orgmode/2020-02/msg00923.html
 
This is interesting.  Thanks for sharing!  Instead of writing to a temp file, it could use a here document.  Here documents, AFAIK, are POSIX portable, although function definitions aren't.  Of course, if we're considering Windows cmd (as we did in a recent bug report), there are all sorts of problems: batch syntax (executed from a file) is different than CLI syntax, functions will implemented using labels (I'm not sure how robust that is), and no here documents.  A comint would probably be best for the Windows use-case.

ahab@pequod ~$ guix shell dash
ahab@pequod ~ [env]$ dash <<- EOF 
my_function() { echo "hello, world!"; echo "the end"; }
my_function;
EOF
hello, world!
the end

ahab@pequod ~$ guix shell fish
ahab@pequod ~ [env]$ fish <<- EOF
> function my_function
>   echo "hello, world!"
>   echo "the end"
> end
> my_function
> EOF
hello, world!
the end

 > By the way, I took a look at ob-shell for the first time in awhile, and
 > noticed that ob-shell now forces the prompt to be like:
 > 
 > org_babel_sh_prompt>
 > 
 > Which I think makes cleaning up the prompt markers a lot more
 > robust. But it is a bit ugly, and also seems to break working with shell
 > sessions started outside of Babel with M-x shell.

Is this something you consider a bug?

Re: [PATCH] Async evaluation in ob-shell

[Jack Kamm]

Matt <matt@excalamus.com> writes:

> Is this something you consider a bug?

Yes I think so. When I was testing ob-shell today I initially tried
with an M-x shell session, and I was surprised it didn't work.

But I also noticed another prompt-related issue: conda doesn't seem to
work in ob-shell sessions anymore. That is a bigger problem for my use
case.

>  > I believe this approach would be
>  > generally more robust, but the major weakness is that it would break if
>  > you SSH'd to a remote host in the middle of the session.

I guess, SSH doesn't work with the current implementation either.

This isn't a feature I need, but I have seen others make use of it in
the past, e.g. [1].

> Instead of writing to a temp file, it could use a here document.
> Here documents, AFAIK, are POSIX portable, although function
> definitions aren't.

That is a really neat idea.

Sadly, "conda activate" doesn't seem to work in here-documents :(

ob-shell is really tricky, with so many cases
(bash/zsh/fish/Windows/etc). Perhaps ob-shell could have a main
"batch-like" implementation (either here-documents, or source'ing
tempfile, or wrapper function), but also have a fallback line-by-line
implementation for certain edge cases?

[1] https://howardism.org/Technical/Emacs/literate-devops.html

[BUG] shell sessions started outside of Babel broken

[Matt]

 ---- On Sat, 11 Feb 2023 15:56:00 -0500  Jack Kamm wrote --- 

 > By the way, I took a look at ob-shell for the first time in awhile, and
 > noticed that ob-shell now forces the prompt to be like:
 > 
 > org_babel_sh_prompt>
 > 
 > Which I think makes cleaning up the prompt markers a lot more
 > robust. But it is a bit ugly, and also seems to break working with shell
 > sessions started outside of Babel with M-x shell.

Can you please elaborate?

The following does what I expect it to:

1. M-x shell
2. Call `org-ctrl-c-ctrl-c' on the following block in a separate Org buffer:

#+begin_src sh :session *shell*
echo "hello, world!"
#+end_src

#+RESULTS:
| hello | world! |

Are you doing something different?

Re: [BUG] shell sessions started outside of Babel broken

[Jack Kamm]

Matt <matt@excalamus.com> writes:

> Can you please elaborate?
>
> The following does what I expect it to:

Sorry -- I'm unable to reproduce the problem I was having before. Very
confused...anyways, thanks for looking into it.

In particular, ob-shell works with M-x shell sessions for me again now,
though I do experience some mangling of the results.

In particular here's the output I get now for:

0. emacs -Q -L ~/src/org-mode/lisp
1. M-x shell
2. Call `org-ctrl-c-ctrl-c' on the following block in a separate Org buffer:

#+begin_src sh :session *shell*
  echo hello world!
#+end_src

#+RESULTS:
| hello            | world! |
|                  |        |
| org_babel_sh_eoe |        |

So, ob-shell mostly works, but the result is a bit mangled with the
eoe token leaking through. This might not be a new bug, my memory is
that ob-shell sessions has long had problems with leaky results.

I am using zsh with following emacs and Org versions:
 
#+begin_src emacs-lisp
(emacs-version)
#+end_src

#+RESULTS:
: GNU Emacs 28.1 (build 2, x86_64-pc-linux-gnu, X toolkit, cairo version 1.17.6, Xaw3d scroll bars)
:  of 2023-01-20

#+begin_src emacs-lisp
(org-version nil t)
#+end_src

#+RESULTS:
: Org mode version 9.6.1 (release_9.6.1-250-ge6353d @ /home/jack/src/org-mode/lisp/)

Re: [BUG] shell sessions started outside of Babel broken

[Ihor Radchenko]

Jack Kamm <jackkamm@tatersworld.org> writes:

> In particular here's the output I get now for:
>
> 0. emacs -Q -L ~/src/org-mode/lisp
> 1. M-x shell
> 2. Call `org-ctrl-c-ctrl-c' on the following block in a separate Org buffer:
>
> #+begin_src sh :session *shell*
>   echo hello world!
> #+end_src
>
> #+RESULTS:
> | hello            | world! |
> |                  |        |
> | org_babel_sh_eoe |        |
>
> So, ob-shell mostly works, but the result is a bit mangled with the
> eoe token leaking through. This might not be a new bug, my memory is
> that ob-shell sessions has long had problems with leaky results.

This example is mostly an implementation detail of Org babel.
Org babel assumes that session buffer name is the same as session name
and also assumes that session is properly initialized if that buffer
exists.

In the example above, you manually create *shell* buffer and Org
believes that it is properly initialized, making certain assumptions.
The assumptions do not hold in that manually created buffer.

I am not sure if we need to do something about this situation.

I guess, we may set some buffer-local variable in actual Org babel
buffers and then make `org-babel-comint-buffer-livep' check that
buffer-local variable to distinguish Org-created buffers from manually
created. But I do not see this as a big problem.

-- 
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: [BUG] shell sessions started outside of Babel broken

[Jack Kamm]

Ihor Radchenko <yantar92@posteo.net> writes:

> Org babel assumes ... session is properly initialized if that buffer
> exists.

Forgive me for opining a moment -- but I think it's worthwhile for Org
Babel to support external sessions where feasible.

ob-R and ob-python have long supported this, though recent changes to
ob-python seem to have broken it there (I'm looking into fixing that).

Having the extra flexibility to start the session manually can be
handy. For example, conda is currently broken with ob-shell sessions (as
discussed in this thread), but not M-x shell sessions, so it provides a
useful workaround.

For ob-python, in the past I found that manually invoking "M-x
run-python" was convenient for starting different types of python
sessions, such as with IPython, or with conda environments.

That being said, I do appreciate that supporting this use case adds
extra headaches for maintenance, and is difficult to test. In the end, I
think it should be up to the individual Babel maintainer whether they
want to support it for their language.

Re: [BUG] shell sessions started outside of Babel broken

[Ihor Radchenko]

Jack Kamm <jackkamm@tatersworld.org> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> Org babel assumes ... session is properly initialized if that buffer
>> exists.
> ...
> Having the extra flexibility to start the session manually can be
> handy. For example, conda is currently broken with ob-shell sessions (as
> discussed in this thread), but not M-x shell sessions, so it provides a
> useful workaround.

Makes sense. In fact, ob-core does define a number of interactive
functions to work with sessions manually:
- `org-babel-load-in-session'
- `org-babel-initiate-session'
- `org-babel-switch-to-session'
- `org-babel-switch-to-session-with-code'

What is confusing is that `org-babel-execute-src-block' does not use
generic `org-babel-initiate-session'.

As for ob-shell, ssh/conda not working are because they change the
prompt. And the fact that default Emacs' heuristic regexp to search for
prompts is accidentally matching the prompt in ssh/conda is just a
co-incidence.

The default Emacs' heuristics, on the other hand, never matches
multi-line prompts. So, the current approach is more reliable for "true"
shell source blocks.

We may, however, allow an extra header arg to set the prompt regexp
manually. Would it make sense?

-- 
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: [BUG] shell sessions started outside of Babel broken

[Jack Kamm]

Ihor Radchenko <yantar92@posteo.net> writes:

> What is confusing is that `org-babel-execute-src-block' does not use
> generic `org-babel-initiate-session'.

I usually find myself confused whenever I am trying to remember
where org-babel-LANG-initiate-session is run and what it's doing...

In ob-shell's case, when I read the lisp code it seems like
org-babel-prompt-command would get called every time
org-babel-execute:shell runs (via org-babel-sh-initiate-session).

But that's not how the behavior seems when I use it, e.g. if using a
M-x shell session, the prompt isn't changed after executing a src
block.

Not sure what I'm missing...

> We may, however, allow an extra header arg to set the prompt regexp
> manually. Would it make sense?

It does feel a little confusing, but I don't have any better ideas at
the moment.

To clarify -- would the header arg be for setting
comint-prompt-regexp, org-babel-prompt-command, both, or something
else?

Re: [BUG] shell sessions started outside of Babel broken

[Ihor Radchenko]

Jack Kamm <jackkamm@tatersworld.org> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> What is confusing is that `org-babel-execute-src-block' does not use
>> generic `org-babel-initiate-session'.
>
> I usually find myself confused whenever I am trying to remember
> where org-babel-LANG-initiate-session is run and what it's doing...

AFAIU, org-babel-LANG-initiate-session is ran the first time the session
is started. Its job is setting the comint to work with babel.

org-babel-LANG-prep-session is ran before a code block is executed
within a session. Its purpose is applying header args (like variable
bindings) and performing other immediate preparations to actually
execute the src block.

> In ob-shell's case, when I read the lisp code it seems like
> org-babel-prompt-command would get called every time
> org-babel-execute:shell runs (via org-babel-sh-initiate-session).

No. `org-babel-sh-initiate-session' has
   (or (org-babel-comint-buffer-livep session)
       ...)

So, it does nothing when the session buffer is already present.

> But that's not how the behavior seems when I use it, e.g. if using a
> M-x shell session, the prompt isn't changed after executing a src
> block.

Because `org-babel-comint-buffer-livep' will return t for a buffer
created by M-x shell. It may or may not be a bug.

We can, for example, make `org-babel-comint-buffer-livep' return nil
unless a special buffer-local indicator is set upon evaluating
`org-babel-sh-initiate-session'.

However, it may be tricky in general case because what we have to do
when creating a new session buffer may or may not be the same with what
we have to do when M-x shell buffer exists already.

>> We may, however, allow an extra header arg to set the prompt regexp
>> manually. Would it make sense?
>
> It does feel a little confusing, but I don't have any better ideas at
> the moment.
>
> To clarify -- would the header arg be for setting
> comint-prompt-regexp, org-babel-prompt-command, both, or something
> else?

I imagine that the header arg will be setting comint-prompt-regexp.

-- 
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: [BUG] shell sessions started outside of Babel broken

[Jack Kamm]

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

Jack Kamm <jackkamm@tatersworld.org> writes:

> I think it's worthwhile for Org Babel to support external sessions
> where feasible.  ob-R and ob-python have long supported this, though
> recent changes to ob-python seem to have broken it there (I'm looking
> into fixing that).

I'm attaching a patch that restores ob-python's ability to work with
externally started sessions.

I'll plan to push this commit in a few days, unless any problems with
the patch are noticed.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ob-python-Allow-working-with-externally-started-sess.patch --]
[-- Type: text/x-patch, Size: 3839 bytes --]

From 47bf309b9af4cd342d3e939442504a9d34bf6592 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sat, 25 Mar 2023 07:53:28 -0700
Subject: [PATCH] ob-python: Allow working with externally started sessions
 again

* lisp/ob-python.el (python-shell-buffer-name): Remove unneeded
defvar.
(org-babel-python-initiate-session-by-key): Check if session already
existed before run-python.  Only wait for initialization if it's a
newly started session.  Also simplify the code a bit by combining
multiple setq and let statements into a single let statement.  Also
add a comment about why adding to `python-shell-first-prompt-hook'
after `run-python' should be safe from race conditions.
---
 lisp/ob-python.el | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 456f2d489..0e0539d7a 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -169,7 +169,6 @@ (defun org-babel-python-without-earmuffs (session)
 	(substring name 1 (- (length name) 1))
       name)))
 
-(defvar python-shell-buffer-name)
 (defvar-local org-babel-python--initialized nil
   "Flag used to mark that python session has been initialized.")
 (defun org-babel-python-initiate-session-by-key (&optional session)
@@ -178,27 +177,33 @@ (defun org-babel-python-initiate-session-by-key (&optional session)
 then create.  Return the initialized session."
   (save-window-excursion
     (let* ((session (if session (intern session) :default))
-           (py-buffer (org-babel-python-session-buffer session))
+           (py-buffer (or (org-babel-python-session-buffer session)
+                          (org-babel-python-with-earmuffs session)))
 	   (cmd (if (member system-type '(cygwin windows-nt ms-dos))
 		    (concat org-babel-python-command " -i")
-		  org-babel-python-command)))
-      (unless py-buffer
-	(setq py-buffer (org-babel-python-with-earmuffs session)))
-      (let ((python-shell-buffer-name
-	     (org-babel-python-without-earmuffs py-buffer)))
-	(run-python cmd)
-        (with-current-buffer py-buffer
-          (add-hook
-           'python-shell-first-prompt-hook
-           (lambda () (setq-local org-babel-python--initialized t))
-           nil 'local)))
-      ;; Wait until Python initializes.
-      ;; This is more reliable compared to
-      ;; `org-babel-comint-wait-for-output' as python may emit
-      ;; multiple prompts during initialization.
+		  org-babel-python-command))
+           (python-shell-buffer-name
+	    (org-babel-python-without-earmuffs py-buffer))
+           (existing-session-p (comint-check-proc py-buffer)))
+      (run-python cmd)
       (with-current-buffer py-buffer
-        (while (not org-babel-python--initialized)
-          (org-babel-comint-wait-for-output py-buffer)))
+        ;; Adding to `python-shell-first-prompt-hook' immediately
+        ;; after `run-python' should be safe from race conditions,
+        ;; because subprocess output only arrives when Emacs is
+        ;; waiting (see elisp manual, "Output from Processes")
+        (add-hook
+         'python-shell-first-prompt-hook
+         (lambda () (setq-local org-babel-python--initialized t))
+         nil 'local))
+      ;; Don't hang if session was started externally
+      (unless existing-session-p
+        ;; Wait until Python initializes
+        ;; This is more reliable compared to
+        ;; `org-babel-comint-wait-for-output' as python may emit
+        ;; multiple prompts during initialization.
+        (with-current-buffer py-buffer
+          (while (not org-babel-python--initialized)
+            (org-babel-comint-wait-for-output py-buffer))))
       (setq org-babel-python-buffers
 	    (cons (cons session py-buffer)
 		  (assq-delete-all session org-babel-python-buffers)))
-- 
2.39.2

[PATCH] Fix externally started sessions with ob-python

[Jack Kamm]

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

Jack Kamm <jackkamm@tatersworld.org> writes:

> Jack Kamm <jackkamm@tatersworld.org> writes:
>
>> I think it's worthwhile for Org Babel to support external sessions
>> where feasible.  ob-R and ob-python have long supported this, though
>> recent changes to ob-python seem to have broken it there (I'm looking
>> into fixing that).
>
> I'm attaching a patch that restores ob-python's ability to work with
> externally started sessions.
>
> I'll plan to push this commit in a few days, unless any problems with
> the patch are noticed.

Ugh, sorry, I meant to start a thread with a new Subject for this.

Re-sending the patch here, with the updated email Subject this time.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ob-python-Allow-working-with-externally-started-sess.patch --]
[-- Type: text/x-patch, Size: 3839 bytes --]

From 47bf309b9af4cd342d3e939442504a9d34bf6592 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sat, 25 Mar 2023 07:53:28 -0700
Subject: [PATCH] ob-python: Allow working with externally started sessions
 again

* lisp/ob-python.el (python-shell-buffer-name): Remove unneeded
defvar.
(org-babel-python-initiate-session-by-key): Check if session already
existed before run-python.  Only wait for initialization if it's a
newly started session.  Also simplify the code a bit by combining
multiple setq and let statements into a single let statement.  Also
add a comment about why adding to `python-shell-first-prompt-hook'
after `run-python' should be safe from race conditions.
---
 lisp/ob-python.el | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 456f2d489..0e0539d7a 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -169,7 +169,6 @@ (defun org-babel-python-without-earmuffs (session)
 	(substring name 1 (- (length name) 1))
       name)))
 
-(defvar python-shell-buffer-name)
 (defvar-local org-babel-python--initialized nil
   "Flag used to mark that python session has been initialized.")
 (defun org-babel-python-initiate-session-by-key (&optional session)
@@ -178,27 +177,33 @@ (defun org-babel-python-initiate-session-by-key (&optional session)
 then create.  Return the initialized session."
   (save-window-excursion
     (let* ((session (if session (intern session) :default))
-           (py-buffer (org-babel-python-session-buffer session))
+           (py-buffer (or (org-babel-python-session-buffer session)
+                          (org-babel-python-with-earmuffs session)))
 	   (cmd (if (member system-type '(cygwin windows-nt ms-dos))
 		    (concat org-babel-python-command " -i")
-		  org-babel-python-command)))
-      (unless py-buffer
-	(setq py-buffer (org-babel-python-with-earmuffs session)))
-      (let ((python-shell-buffer-name
-	     (org-babel-python-without-earmuffs py-buffer)))
-	(run-python cmd)
-        (with-current-buffer py-buffer
-          (add-hook
-           'python-shell-first-prompt-hook
-           (lambda () (setq-local org-babel-python--initialized t))
-           nil 'local)))
-      ;; Wait until Python initializes.
-      ;; This is more reliable compared to
-      ;; `org-babel-comint-wait-for-output' as python may emit
-      ;; multiple prompts during initialization.
+		  org-babel-python-command))
+           (python-shell-buffer-name
+	    (org-babel-python-without-earmuffs py-buffer))
+           (existing-session-p (comint-check-proc py-buffer)))
+      (run-python cmd)
       (with-current-buffer py-buffer
-        (while (not org-babel-python--initialized)
-          (org-babel-comint-wait-for-output py-buffer)))
+        ;; Adding to `python-shell-first-prompt-hook' immediately
+        ;; after `run-python' should be safe from race conditions,
+        ;; because subprocess output only arrives when Emacs is
+        ;; waiting (see elisp manual, "Output from Processes")
+        (add-hook
+         'python-shell-first-prompt-hook
+         (lambda () (setq-local org-babel-python--initialized t))
+         nil 'local))
+      ;; Don't hang if session was started externally
+      (unless existing-session-p
+        ;; Wait until Python initializes
+        ;; This is more reliable compared to
+        ;; `org-babel-comint-wait-for-output' as python may emit
+        ;; multiple prompts during initialization.
+        (with-current-buffer py-buffer
+          (while (not org-babel-python--initialized)
+            (org-babel-comint-wait-for-output py-buffer))))
       (setq org-babel-python-buffers
 	    (cons (cons session py-buffer)
 		  (assq-delete-all session org-babel-python-buffers)))
-- 
2.39.2

[BUG] conda doesn't work in ob-shell sessions

[Matt]

 ---- On Sun, 12 Feb 2023 22:16:16 -0500  Jack Kamm  wrote --- 
 
 > But I also noticed another prompt-related issue: conda doesn't seem to
 > work in ob-shell sessions anymore. That is a bigger problem for my use
 > case.

Could you elaborate?

It looks like conda has a new init (from what I remember) which requires users to run a `conda init <shell>' command in order to create a new environment.  For example, I needed to run `conda init bash.'  This added garbage to my .bashrc which automatically enabled the base environment.  I was able to do this from ob-shell, however it required me to reload the shell (I reloaded Emacs to be sure I got the latest environment variables).  After this, I was able to create and activate a new conda environment.

1. M-x shell
2. Executing the following in sequence:

#+begin_src emacs-lisp
(org-version nil t)
#+end_src

#+RESULTS:
: Org mode version 9.6.1 (release_9.6.1-250-ge6353d @ /home/user/Projects/org-mode/lisp/)

#+begin_src emacs-lisp
(org-babel-do-load-languages
  'org-babel-load-languages
  '((shell . t)))
#+end_src

#+begin_src sh :session *shell* :results output
conda create --yes --name myenv python=3.9
#+end_src

#+RESULTS:
#+begin_example
<results removed for brevity>

Downloading and Extracting Packages

Preparing transaction: done
Verifying transaction: done
Executing transaction: done

To activate this environment, use

conda activate myenv

To deactivate an active environment, use

conda deactivate
#+end_example

#+begin_src sh :session *shell* :results output
conda activate myenv
#+end_src

#+RESULTS:

#+begin_src sh :session *shell* :results output
which python
#+end_src

#+RESULTS:
: /home/user/miniconda3/envs/myenv/bin/python

The *shell* buffer's prompt also showed the expected [myenv] prefix.

Re: [BUG] conda doesn't work in ob-shell sessions

[Jack Kamm]

Matt <matt@excalamus.com> writes:

> 1. M-x shell

When I follow your example, in particular starting the session with
M-x shell, then conda works as expected, and behavior is same as you
report.

But when I don't start with M-x shell, instead letting ob-shell create
the session, then conda gives the error about needing to run conda init
(despite the fact that conda init had already set up my zshrc).

Example when _NOT_ using the M-x shell:

#+begin_src emacs-lisp
(org-version nil t)
#+end_src

#+RESULTS:
: Org mode version 9.6.1 (release_9.6.1-250-ge6353d @ /home/jack/devel/org-mode/lisp/)

#+begin_src emacs-lisp
(org-babel-do-load-languages
  'org-babel-load-languages
  '((shell . t)))
#+end_src

#+begin_src sh :session *myshell* :results output
conda activate ledger
#+end_src

#+RESULTS:
#+begin_example
CommandNotFoundError: Your shell has not been properly configured to use 'conda activate'.
To initialize your shell, run

    $ conda init <SHELL_NAME>

Currently supported shells are:
  - bash
  - fish
  - tcsh
  - xonsh
  - zsh
  - powershell

See 'conda init --help' for more information and options.

IMPORTANT: You may need to close and restart your shell after running 'conda init'.
#+end_example

Re: [BUG] conda doesn't work in ob-shell sessions

[Ihor Radchenko]

Jack Kamm <jackkamm@tatersworld.org> writes:

>> 1. M-x shell
>
> When I follow your example, in particular starting the session with
> M-x shell, then conda works as expected, and behavior is same as you
> report.
>
> But when I don't start with M-x shell, instead letting ob-shell create
> the session, then conda gives the error about needing to run conda init
> (despite the fact that conda init had already set up my zshrc).

>
> #+begin_src sh :session *myshell* :results output
> conda activate ledger
> #+end_src

> #+RESULTS:
> #+begin_example
> CommandNotFoundError: Your shell has not been properly configured to use 'conda activate'.
> To initialize your shell, run
>
>     $ conda init <SHELL_NAME>

I believe that is it because you explicitly specified "sh" shell, not
zsh.

When you use M-x shell, Emacs will pick up the system shell. In
contrast, ob-shell will let-bind `shell-file-name' to the shell name you
specify in the src block. The only exception is #+begin_src shell, which
will use the default system shell.

So, I believe that the following should work for you

#+begin_src shell :session *myshell* :results output
conda activate ledger
#+end_src

-- 
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: [BUG] conda doesn't work in ob-shell sessions

[Jack Kamm]

Ihor Radchenko <yantar92@posteo.net> writes:

> So, I believe that the following should work for you
>
> #+begin_src shell :session *myshell* :results output
> conda activate ledger
> #+end_src

No, it is still broken after switching to "shell" block, though the
error is different now. (Though I agree the previous error message was
due to using "sh" block and not ob-shell's fault).

Here is my minimal example. In init.el:

(add-to-list 'load-path "/path/to/org-mode/lisp")
(require 'org)
(org-babel-do-load-languages
   'org-babel-load-languages
   '((emacs-lisp . t)
     (shell . t)))

Then do:
emacs -q --load init.el test.org

With test.org like:

#+begin_src shell :session *shell* :results output
  conda activate some-conda-env
  echo test
#+end_src

Then, on main branch, trying to execute the block hangs. It is because
conda changes the prompt in a way that breaks the new ob-shell
implementation.

On bugfix branch, ob-shell still works with conda. I do observe a
separate bug on the first evaluation ("org-babel-execute:shell: Symbol’s
value as variable is void: org-babel-prompt-command"), but subsequent
evaluations work, and the error seems unrelated to conda.

IMO breaking conda is a pretty major regression, as conda is a critical
tool in many areas of scientific computing, including in my own
workflows.  Although I was never a major ob-shell user, I did
occasionally find it useful albeit buggy. But I've stopped using it
altogether over the past year because of this change.

Re: [BUG] conda doesn't work in ob-shell sessions

[Jack Kamm]

Jack Kamm <jackkamm@tatersworld.org> writes:

> IMO breaking conda is a pretty major regression, as conda is a critical
> tool in many areas of scientific computing, including in my own
> workflows.  Although I was never a major ob-shell user, I did
> occasionally find it useful albeit buggy. But I've stopped using it
> altogether over the past year because of this change.

Sorry, it turns out I was badly misremembering things, and overly
harsh here.

In particular, going back to older commits on 9.5 and 9.6, ob-shell was
always horribly mangled with conda, and not very useable. So I was
wrong to say the current implementation is a regression.

Actually, it seems like current version of ob-shell (on both main and
bugfix) works _better_ with conda than before -- as long as the shell
session is started with "M-x shell", instead of with
`org-babel-execute-src-block'. In particular, Ihor's commit f2949d4d1
was particularly helpful in making ob-shell play more nicely with conda.
That commit was added to bugfix around Org 9.6.4.

The reason conda doesn't work well when session is started via
`org-babel-execute-src-block' is because of the way the prompt is
set. However, when session is started by "M-x shell", ob-shell doesn't
mess with the prompt so much, and conda seems to work smoothly. Using
"M-x shell" to start the session seems like an acceptable workaround to
me, especially considering how conda never worked well with ob-shell
before.

Re: [BUG] conda doesn't work in ob-shell sessions

[Ihor Radchenko]

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

Jack Kamm <jackkamm@tatersworld.org> writes:

> The reason conda doesn't work well when session is started via
> `org-babel-execute-src-block' is because of the way the prompt is
> set. However, when session is started by "M-x shell", ob-shell doesn't
> mess with the prompt so much, and conda seems to work smoothly. Using
> "M-x shell" to start the session seems like an acceptable workaround to
> me, especially considering how conda never worked well with ob-shell
> before.

What about the attached patch?


[-- Attachment #2: 0001-lisp-ob-comint.el-Introduce-a-fallback-prompt-regexp.patch --]
[-- Type: text/x-patch, Size: 8797 bytes --]

From 45ddb20665f6124a5b08a9fbfed02ee2961ac9e3 Mon Sep 17 00:00:00 2001
Message-ID: <45ddb20665f6124a5b08a9fbfed02ee2961ac9e3.1705924712.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Mon, 22 Jan 2024 12:55:09 +0100
Subject: [PATCH] lisp/ob-comint.el: Introduce a fallback prompt regexp

* lisp/ob-comint.el (org-babel-comint-prompt-regexp-old): New variable
storing the default value of `comint-prompt-regexp' to be used when
the prompt set by Org mode changes for some reason.
(org-babel-comint-fallback-regexp-threshold): New customization to set
the time Org babel waits for comint command to finish until trying
fallback prompt regexp.
(org-babel-comint--set-fallback-prompt): New internal function that
sets the fallback prompt regexp, if there is any available.
(org-babel-comint-with-output):
(org-babel-comint-wait-for-output): Try fallback prompt regexp when we
cannot find comint command end for too long.
* lisp/ob-haskell.el (org-babel-interpret-haskell):
* lisp/ob-ruby.el (org-babel-ruby-initiate-session):
* lisp/ob-shell.el (org-babel-sh-initiate-session):
* lisp/ob-clojure.el (ob-clojure-eval-with-inf-clojure): Set
`org-babel-comint-prompt-regexp-old' when initializing the inferior
shell.

Reported-by: Jack Kamm <jackkamm@tatersworld.org>
Link: https://orgmode.org/list/87sf2q9ubd.fsf@gmail.com
---
 lisp/ob-clojure.el |  4 ++-
 lisp/ob-comint.el  | 65 ++++++++++++++++++++++++++++++++++++----------
 lisp/ob-haskell.el |  6 +++--
 lisp/ob-ruby.el    |  4 ++-
 lisp/ob-shell.el   |  8 +++---
 5 files changed, 67 insertions(+), 20 deletions(-)

diff --git a/lisp/ob-clojure.el b/lisp/ob-clojure.el
index ddcfcd9fb..4a54acc51 100644
--- a/lisp/ob-clojure.el
+++ b/lisp/ob-clojure.el
@@ -237,7 +237,9 @@ (defun ob-clojure-eval-with-inf-clojure (expanded params)
 			     "clojure" (format "clojure -A%s" alias)
 			     cmd0)
 		    cmd0)))
-	(setq comint-prompt-regexp inf-clojure-comint-prompt-regexp)
+	(setq
+         org-babel-comint-prompt-regexp-old comint-prompt-regexp
+         comint-prompt-regexp inf-clojure-comint-prompt-regexp)
 	(funcall-interactively #'inf-clojure cmd)
 	(goto-char (point-max))))
     (sit-for 1))
diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 7d258ea0e..316848b6b 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -58,6 +58,22 @@ (defmacro org-babel-comint-in-buffer (buffer &rest body)
 	   (let ((comint-input-filter (lambda (_input) nil)))
 	     ,@body))))))
 
+(defvar-local org-babel-comint-prompt-regexp-old nil
+  "Fallback regexp used to detect prompt.")
+
+(defcustom org-babel-comint-fallback-regexp-threshold 5.0
+  "Waiting time until trying to use fallback regexp to detect prompt.
+This is useful when prompt unexpectedly changes."
+  :type 'float
+  :group 'org-babel)
+
+(defun org-babel-comint--set-fallback-prompt ()
+  "Swap `comint-prompt-regexp' and `org-babel-comint-prompt-regexp-old'."
+  (when org-babel-comint-prompt-regexp-old
+    (let ((tmp comint-prompt-regexp))
+      (setq comint-prompt-regexp org-babel-comint-prompt-regexp-old
+            org-babel-comint-prompt-regexp-old tmp))))
+
 (defmacro org-babel-comint-with-output (meta &rest body)
   "Evaluate BODY in BUFFER and return process output.
 Will wait until EOE-INDICATOR appears in the output, then return
@@ -96,14 +112,27 @@ (defmacro org-babel-comint-with-output (meta &rest body)
 	 ;; pass FULL-BODY to process
 	 ,@body
 	 ;; wait for end-of-evaluation indicator
-	 (while (progn
-		  (goto-char comint-last-input-end)
-		  (not (save-excursion
-		       (and (re-search-forward
-			     (regexp-quote ,eoe-indicator) nil t)
-			    (re-search-forward
-			     comint-prompt-regexp nil t)))))
-	   (accept-process-output (get-buffer-process (current-buffer))))
+         (let ((start-time (current-time)))
+	   (while (progn
+		    (goto-char comint-last-input-end)
+		    (not (save-excursion
+		         (and (re-search-forward
+			       (regexp-quote ,eoe-indicator) nil t)
+			      (re-search-forward
+			       comint-prompt-regexp nil t)))))
+	     (accept-process-output (get-buffer-process (current-buffer)))
+             (when (and org-babel-comint-prompt-regexp-old
+                        (> (float-time (time-since start-time))
+                           org-babel-comint-fallback-regexp-threshold)
+                        (progn
+		          (goto-char comint-last-input-end)
+		          (save-excursion
+                            (and
+                             (re-search-forward
+			      (regexp-quote ,eoe-indicator) nil t)
+			     (re-search-forward
+                              org-babel-comint-prompt-regexp-old nil t)))))
+               (org-babel-comint--set-fallback-prompt))))
 	 ;; replace cut dangling text
 	 (goto-char (process-mark (get-buffer-process (current-buffer))))
 	 (insert dangling-text)
@@ -148,11 +177,21 @@ (defun org-babel-comint-wait-for-output (buffer)
 Note: this is only safe when waiting for the result of a single
 statement (not large blocks of code)."
   (org-babel-comint-in-buffer buffer
-    (while (progn
-             (goto-char comint-last-input-end)
-             (not (and (re-search-forward comint-prompt-regexp nil t)
-                     (goto-char (match-beginning 0)))))
-      (accept-process-output (get-buffer-process buffer)))))
+    (let ((start-time (current-time)))
+      (while (progn
+               (goto-char comint-last-input-end)
+               (not (and (re-search-forward comint-prompt-regexp nil t)
+                       (goto-char (match-beginning 0)))))
+        (accept-process-output (get-buffer-process buffer))
+        (when (and org-babel-comint-prompt-regexp-old
+                   (> (float-time (time-since start-time))
+                      org-babel-comint-fallback-regexp-threshold)
+                   (progn
+		     (goto-char comint-last-input-end)
+		     (save-excursion
+		       (re-search-forward
+                        org-babel-comint-prompt-regexp-old nil t))))
+          (org-babel-comint--set-fallback-prompt))))))
 
 (defun org-babel-comint-eval-invisibly-and-wait-for-file
     (buffer file string &optional period)
diff --git a/lisp/ob-haskell.el b/lisp/ob-haskell.el
index a9ed772dd..00be6d52c 100644
--- a/lisp/ob-haskell.el
+++ b/lisp/ob-haskell.el
@@ -152,8 +152,10 @@ (defun org-babel-interpret-haskell (body params)
   (org-require-package 'inf-haskell "haskell-mode")
   (add-hook 'inferior-haskell-hook
             (lambda ()
-              (setq-local comint-prompt-regexp
-                          (concat haskell-prompt-regexp "\\|^λ?> "))))
+              (setq-local
+               org-babel-comint-prompt-regexp-old comint-prompt-regexp
+               comint-prompt-regexp
+               (concat haskell-prompt-regexp "\\|^λ?> "))))
   (org-babel-haskell-with-session session params
     (cl-labels
         ((send-txt-to-ghci (txt)
diff --git a/lisp/ob-ruby.el b/lisp/ob-ruby.el
index 79b33940d..d920fb585 100644
--- a/lisp/ob-ruby.el
+++ b/lisp/ob-ruby.el
@@ -191,7 +191,9 @@ (defun org-babel-ruby-initiate-session (&optional session params)
             ;; uniquely by regexp.
             (when new-session?
               (with-current-buffer session-buffer
-                (setq-local comint-prompt-regexp (concat "^" org-babel-ruby-prompt))
+                (setq-local
+                 org-babel-comint-prompt-regexp-old comint-prompt-regexp
+                 comint-prompt-regexp (concat "^" org-babel-ruby-prompt))
                 (insert org-babel-ruby-define-prompt ";")
                 (insert "_org_prompt_mode=conf.prompt_mode;conf.prompt_mode=:CUSTOM;")
                 (insert "conf.echo=false\n")
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 31135b5fb..20d1d5e33 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -265,9 +265,11 @@ (defun org-babel-sh-initiate-session (&optional session _params)
                               org-babel-shell-set-prompt-commands))
                   (alist-get t org-babel-shell-set-prompt-commands))
               org-babel-sh-prompt))
-            (setq-local comint-prompt-regexp
-                        (concat "^" (regexp-quote org-babel-sh-prompt)
-                                " *"))
+            (setq-local
+             org-babel-comint-prompt-regexp-old comint-prompt-regexp
+             comint-prompt-regexp
+             (concat "^" (regexp-quote org-babel-sh-prompt)
+                     " *"))
 	    ;; Needed for Emacs 23 since the marker is initially
 	    ;; undefined and the filter functions try to use it without
 	    ;; checking.
-- 
2.43.0


[-- Attachment #3: Type: text/plain, Size: 224 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: [BUG] conda doesn't work in ob-shell sessions

[Jack Kamm]

Ihor Radchenko <yantar92@posteo.net> writes:

> What about the attached patch?

I applied the patch to main, but the behavior was the same as before.

More specifically, I am testing with the following block (with minimal
init file from my previous email):

#+begin_src shell :session *shell* :results output
  conda activate test-env
  echo test
#+end_src

When I execute the block before session exists, emacs hangs. But if I
start session with "M-x shell", and then execute the block, it works.

Here is how the full *shell* buffer looks in the former case that hangs,
after I quit the hanging with C-g:

$ PROMPT_COMMAND=;PS1="org_babel_sh_prompt> ";PS2=
org_babel_sh_prompt> conda activate test-env
echo test
echo 'org_babel_sh_eoe'
(test-env) org_babel_sh_prompt> test
(test-env) org_babel_sh_prompt> org_babel_sh_eoe
(test-env) org_babel_sh_prompt>

And here is how the *shell* buffer looks in the latter case which works,
when session is started with "M-x shell" instead:

$ conda activate test-env
echo test
echo 'org_babel_sh_eoe'
(test-env) $ test
(test-env) $ org_babel_sh_eoe
(test-env) $

Re: [BUG] conda doesn't work in ob-shell sessions

[Ihor Radchenko]

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

Jack Kamm <jackkamm@tatersworld.org> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> What about the attached patch?
>
> I applied the patch to main, but the behavior was the same as before.

What about the attached second version of the patch?


[-- Attachment #2: v2-0001-lisp-ob-comint.el-Introduce-a-fallback-prompt-reg.patch --]
[-- Type: text/x-patch, Size: 8939 bytes --]

From 0d1331c75ad7a09d88bc5bf38d00488980b6a510 Mon Sep 17 00:00:00 2001
Message-ID: <0d1331c75ad7a09d88bc5bf38d00488980b6a510.1706109720.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Mon, 22 Jan 2024 12:55:09 +0100
Subject: [PATCH v2] lisp/ob-comint.el: Introduce a fallback prompt regexp

* lisp/ob-comint.el (org-babel-comint-prompt-regexp-old): New variable
storing the default value of `comint-prompt-regexp' to be used when
the prompt set by Org mode changes for some reason.
(org-babel-comint-fallback-regexp-threshold): New customization to set
the time Org babel waits for comint command to finish until trying
fallback prompt regexp.
(org-babel-comint--set-fallback-prompt): New internal function that
sets the fallback prompt regexp, if there is any available.
(org-babel-comint-with-output):
(org-babel-comint-wait-for-output): Try fallback prompt regexp when we
cannot find comint command end for too long.
* lisp/ob-haskell.el (org-babel-interpret-haskell):
* lisp/ob-ruby.el (org-babel-ruby-initiate-session):
* lisp/ob-shell.el (org-babel-sh-initiate-session):
* lisp/ob-clojure.el (ob-clojure-eval-with-inf-clojure): Set
`org-babel-comint-prompt-regexp-old' when initializing the inferior
shell.

Reported-by: Jack Kamm <jackkamm@tatersworld.org>
Link: https://orgmode.org/list/87sf2q9ubd.fsf@gmail.com
---
 lisp/ob-clojure.el |  4 ++-
 lisp/ob-comint.el  | 69 +++++++++++++++++++++++++++++++++++++---------
 lisp/ob-haskell.el |  6 ++--
 lisp/ob-ruby.el    |  4 ++-
 lisp/ob-shell.el   |  8 ++++--
 5 files changed, 71 insertions(+), 20 deletions(-)

diff --git a/lisp/ob-clojure.el b/lisp/ob-clojure.el
index ddcfcd9fb..4a54acc51 100644
--- a/lisp/ob-clojure.el
+++ b/lisp/ob-clojure.el
@@ -237,7 +237,9 @@ (defun ob-clojure-eval-with-inf-clojure (expanded params)
 			     "clojure" (format "clojure -A%s" alias)
 			     cmd0)
 		    cmd0)))
-	(setq comint-prompt-regexp inf-clojure-comint-prompt-regexp)
+	(setq
+         org-babel-comint-prompt-regexp-old comint-prompt-regexp
+         comint-prompt-regexp inf-clojure-comint-prompt-regexp)
 	(funcall-interactively #'inf-clojure cmd)
 	(goto-char (point-max))))
     (sit-for 1))
diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 7d258ea0e..26f0c3bf7 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -58,6 +58,22 @@ (defmacro org-babel-comint-in-buffer (buffer &rest body)
 	   (let ((comint-input-filter (lambda (_input) nil)))
 	     ,@body))))))
 
+(defvar-local org-babel-comint-prompt-regexp-old nil
+  "Fallback regexp used to detect prompt.")
+
+(defcustom org-babel-comint-fallback-regexp-threshold 5.0
+  "Waiting time until trying to use fallback regexp to detect prompt.
+This is useful when prompt unexpectedly changes."
+  :type 'float
+  :group 'org-babel)
+
+(defun org-babel-comint--set-fallback-prompt ()
+  "Swap `comint-prompt-regexp' and `org-babel-comint-prompt-regexp-old'."
+  (when org-babel-comint-prompt-regexp-old
+    (let ((tmp comint-prompt-regexp))
+      (setq comint-prompt-regexp org-babel-comint-prompt-regexp-old
+            org-babel-comint-prompt-regexp-old tmp))))
+
 (defmacro org-babel-comint-with-output (meta &rest body)
   "Evaluate BODY in BUFFER and return process output.
 Will wait until EOE-INDICATOR appears in the output, then return
@@ -96,14 +112,29 @@ (defmacro org-babel-comint-with-output (meta &rest body)
 	 ;; pass FULL-BODY to process
 	 ,@body
 	 ;; wait for end-of-evaluation indicator
-	 (while (progn
-		  (goto-char comint-last-input-end)
-		  (not (save-excursion
-		       (and (re-search-forward
-			     (regexp-quote ,eoe-indicator) nil t)
-			    (re-search-forward
-			     comint-prompt-regexp nil t)))))
-	   (accept-process-output (get-buffer-process (current-buffer))))
+         (let ((start-time (current-time)))
+	   (while (progn
+		    (goto-char comint-last-input-end)
+		    (not (save-excursion
+		         (and (re-search-forward
+			       (regexp-quote ,eoe-indicator) nil t)
+			      (re-search-forward
+			       comint-prompt-regexp nil t)))))
+	     (accept-process-output
+              (get-buffer-process (current-buffer))
+              org-babel-comint-fallback-regexp-threshold)
+             (when (and org-babel-comint-prompt-regexp-old
+                        (> (float-time (time-since start-time))
+                           org-babel-comint-fallback-regexp-threshold)
+                        (progn
+		          (goto-char comint-last-input-end)
+		          (save-excursion
+                            (and
+                             (re-search-forward
+			      (regexp-quote ,eoe-indicator) nil t)
+			     (re-search-forward
+                              org-babel-comint-prompt-regexp-old nil t)))))
+               (org-babel-comint--set-fallback-prompt))))
 	 ;; replace cut dangling text
 	 (goto-char (process-mark (get-buffer-process (current-buffer))))
 	 (insert dangling-text)
@@ -148,11 +179,23 @@ (defun org-babel-comint-wait-for-output (buffer)
 Note: this is only safe when waiting for the result of a single
 statement (not large blocks of code)."
   (org-babel-comint-in-buffer buffer
-    (while (progn
-             (goto-char comint-last-input-end)
-             (not (and (re-search-forward comint-prompt-regexp nil t)
-                     (goto-char (match-beginning 0)))))
-      (accept-process-output (get-buffer-process buffer)))))
+    (let ((start-time (current-time)))
+      (while (progn
+               (goto-char comint-last-input-end)
+               (not (and (re-search-forward comint-prompt-regexp nil t)
+                       (goto-char (match-beginning 0)))))
+        (accept-process-output
+         (get-buffer-process buffer)
+         org-babel-comint-fallback-regexp-threshold)
+        (when (and org-babel-comint-prompt-regexp-old
+                   (> (float-time (time-since start-time))
+                      org-babel-comint-fallback-regexp-threshold)
+                   (progn
+		     (goto-char comint-last-input-end)
+		     (save-excursion
+		       (re-search-forward
+                        org-babel-comint-prompt-regexp-old nil t))))
+          (org-babel-comint--set-fallback-prompt))))))
 
 (defun org-babel-comint-eval-invisibly-and-wait-for-file
     (buffer file string &optional period)
diff --git a/lisp/ob-haskell.el b/lisp/ob-haskell.el
index a9ed772dd..00be6d52c 100644
--- a/lisp/ob-haskell.el
+++ b/lisp/ob-haskell.el
@@ -152,8 +152,10 @@ (defun org-babel-interpret-haskell (body params)
   (org-require-package 'inf-haskell "haskell-mode")
   (add-hook 'inferior-haskell-hook
             (lambda ()
-              (setq-local comint-prompt-regexp
-                          (concat haskell-prompt-regexp "\\|^λ?> "))))
+              (setq-local
+               org-babel-comint-prompt-regexp-old comint-prompt-regexp
+               comint-prompt-regexp
+               (concat haskell-prompt-regexp "\\|^λ?> "))))
   (org-babel-haskell-with-session session params
     (cl-labels
         ((send-txt-to-ghci (txt)
diff --git a/lisp/ob-ruby.el b/lisp/ob-ruby.el
index 79b33940d..d920fb585 100644
--- a/lisp/ob-ruby.el
+++ b/lisp/ob-ruby.el
@@ -191,7 +191,9 @@ (defun org-babel-ruby-initiate-session (&optional session params)
             ;; uniquely by regexp.
             (when new-session?
               (with-current-buffer session-buffer
-                (setq-local comint-prompt-regexp (concat "^" org-babel-ruby-prompt))
+                (setq-local
+                 org-babel-comint-prompt-regexp-old comint-prompt-regexp
+                 comint-prompt-regexp (concat "^" org-babel-ruby-prompt))
                 (insert org-babel-ruby-define-prompt ";")
                 (insert "_org_prompt_mode=conf.prompt_mode;conf.prompt_mode=:CUSTOM;")
                 (insert "conf.echo=false\n")
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 31135b5fb..20d1d5e33 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -265,9 +265,11 @@ (defun org-babel-sh-initiate-session (&optional session _params)
                               org-babel-shell-set-prompt-commands))
                   (alist-get t org-babel-shell-set-prompt-commands))
               org-babel-sh-prompt))
-            (setq-local comint-prompt-regexp
-                        (concat "^" (regexp-quote org-babel-sh-prompt)
-                                " *"))
+            (setq-local
+             org-babel-comint-prompt-regexp-old comint-prompt-regexp
+             comint-prompt-regexp
+             (concat "^" (regexp-quote org-babel-sh-prompt)
+                     " *"))
 	    ;; Needed for Emacs 23 since the marker is initially
 	    ;; undefined and the filter functions try to use it without
 	    ;; checking.
-- 
2.43.0


[-- Attachment #3: Type: text/plain, Size: 224 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: [BUG] conda doesn't work in ob-shell sessions

[Matt]

 ---- On Wed, 24 Jan 2024 16:20:16 +0100  Ihor Radchenko  wrote --- 
 > Jack Kamm jackkamm@tatersworld.org> writes:
 > 
 > > Ihor Radchenko yantar92@posteo.net> writes:
 > >
 > >> What about the attached patch?
 > >
 > > I applied the patch to main, but the behavior was the same as before.
 > 
 > What about the attached second version of the patch?

I spent way too long trying to test it but I'm not sure I applied the patch correctly.

I did the following, then realized that the patch I applied undid changes (I assume) from a previous patch and so it must have been applied on top of another one.  I then wasted a bunch of time trying to apply various combinations of the four patches I found in the thread.  One of the patches has trailing whitespace yet the various --ignore options of git am didn't seem to ignore it.  I guess I could have looked at the timestamps and used git apply in sequence?

Should make autoloads also be run after applying the patches?

Here's what I was able to do:

In a VM, installed conda as described by their page.  Specifically, my .bashrc was modified to activate the base environment.   Closed terminal, opened it into the base environment, and created a new environment, emacs-test.

Downloaded the latest org-mode by git-clone and did make autoloads.  Added the org-mode git repo's lisp directory to my early-init.el.

Applied patch with

  git apply --cache --ignore-space-changes --ignore-whitespace 2-v2-0001-lisp-ob-comint.el-Introduce-a-fallback-prompt-reg.patch

Doing plain git apply failed.

With the changes in the org-mode index, the base environment activated, I run 'emacs' and confirm I'm using the org-mode git with the patch applied (ob-shell doesn't set org-babel-comint-prompt in ).

Then

(org-babel-do-load-languages 'org-babel-load-languages '((shell . t)))

#+begin_src shell :results output :session *shell*
conda activate emacs-test
#+end_src

It hangs.  C-g and switch to shell buffer shows the following (note, this was typed in and not copy-pasted because VM):

(base) user@debian:~$ PROMPT_COMMAND=;PS1="org_babel_sh_prompt> ";PS2=
org_babel_sh_prompt> conda activate emacs-test
echo 'org_babel_sh_eoe'
(emacs-test) org_babel_sh_prompt> 'org_babel_sh_eoe'
(emacs-test) org_babel_sh_prompt> 


--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: [BUG] conda doesn't work in ob-shell sessions

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  > What about the attached second version of the patch?
>
> I spent way too long trying to test it but I'm not sure I applied the patch correctly.
>
> I did the following, then realized that the patch I applied undid
> changes (I assume) from a previous patch and so it must have been
> applied on top of another one. I then wasted a bunch of time trying to
> apply various combinations of the four patches I found in the thread.
> One of the patches has trailing whitespace yet the various --ignore
> options of git am didn't seem to ignore it. I guess I could have
> looked at the timestamps and used git apply in sequence?

The patch should apply cleanly on the latest main.
No previous patches are needed.

To simplify working with email patches, you may consider
https://docs.kyleam.com/piem/Overview.html (Emacs package)
It can apply patches right from inside Emacs email client.

You can also use magit.

> Should make autoloads also be run after applying the patches?

Yes. Or make. Or you can use make repro to run clean Emacs using the
current git branch.

> Here's what I was able to do:
>
> In a VM, installed conda as described by their page.  Specifically, my .bashrc was modified to activate the base environment.   Closed terminal, opened it into the base environment, and created a new environment, emacs-test.
>
> Downloaded the latest org-mode by git-clone and did make autoloads.  Added the org-mode git repo's lisp directory to my early-init.el.
>
> Applied patch with
>
>   git apply --cache --ignore-space-changes --ignore-whitespace 2-v2-0001-lisp-ob-comint.el-Introduce-a-fallback-prompt-reg.patch
>
> Doing plain git apply failed.
>
> With the changes in the org-mode index, the base environment activated, I run 'emacs' and confirm I'm using the org-mode git with the patch applied (ob-shell doesn't set org-babel-comint-prompt in ).
>
> Then
>
> (org-babel-do-load-languages 'org-babel-load-languages '((shell . t)))
>
> #+begin_src shell :results output :session *shell*
> conda activate emacs-test
> #+end_src
>
> It hangs.  C-g and switch to shell buffer shows the following (note, this was typed in and not copy-pasted because VM):

Note that the patch should only make Emacs unhang after 5 seconds delay.

I tested the patch with the following (conda does not available for my system):

#+begin_src bash :results output :session *shell*
PS1="prompt> "
ls
#+end_src

-- 
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: [BUG] conda doesn't work in ob-shell sessions

[Jack Kamm]

Ihor Radchenko <yantar92@posteo.net> writes:

> Jack Kamm <jackkamm@tatersworld.org> writes:
>
>> Ihor Radchenko <yantar92@posteo.net> writes:
>>
>>> What about the attached patch?
>>
>> I applied the patch to main, but the behavior was the same as before.
>
> What about the attached second version of the patch?

Second version of the patch works on my test example now. The initial
block hangs for a few seconds but then finishes. Subsequent blocks seem
to work without any noticeable hanging.

Thanks!

Re: [BUG] conda doesn't work in ob-shell sessions

[Matt]

 ---- On Fri, 26 Jan 2024 01:42:59 +0100  Jack Kamm  wrote --- 

 > Second version of the patch works on my test example now. The initial
 > block hangs for a few seconds but then finishes. Subsequent blocks seem
 > to work without any noticeable hanging.

I was able to confirm this as well.

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode

Re: [BUG] conda doesn't work in ob-shell sessions

[Ihor Radchenko]

Matt <matt@excalamus.com> writes:

>  ---- On Fri, 26 Jan 2024 01:42:59 +0100  Jack Kamm  wrote --- 
>
>  > Second version of the patch works on my test example now. The initial
>  > block hangs for a few seconds but then finishes. Subsequent blocks seem
>  > to work without any noticeable hanging.
>
> I was able to confirm this as well.

Thanks for checking!
Applied, onto main.
I also added ORG-NEWS entry about the new custom variable.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=27d6f8305

Fixed.

-- 
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: [BUG] conda doesn't work in ob-shell sessions

[Suhail Singh]

Jack Kamm <jackkamm@tatersworld.org> writes:

> #+begin_src shell :session *shell* :results output
>   conda activate some-conda-env
>   echo test
> #+end_src
>
> Then, on main branch, trying to execute the block hangs. It is because
> conda changes the prompt in a way that breaks the new ob-shell
> implementation.

This isn't a fix, but a workaround I've used for a while is to define a
function set_PS1 which I invoke after "conda activate" within org-babel
blocks.  set_PS1 (among other things that aren't relevant) compares PS1
against "(${CONDA_DEFAULT_ENV:-base}) org_babel_sh_prompt> ", and in
those cases sets PS1 to "org_babel_sh_prompt> ".  set_PS1 thus defined,
ensures that the follow block doesn't hang on first invocation.

#+begin_src bash :session conda-test :results replace
  conda activate test && set_PS1
  echo "${CONDA_DEFAULT_ENV}"
#+end_src

#+RESULTS:
: test

> ... on the first evaluation ("org-babel-execute:shell: Symbol’s value
> as variable is void: org-babel-prompt-command")

On org-version as recent as 9.6.17, when performing some actions such as
invoking org-metadown on a block with :session defined, I encounter the
same unless I have the below in my init.el:

#+begin_src elisp
  (defvar org-babel-prompt-command nil)
#+end_src

-- 
Suhail