emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [PATCH] Async evaluation in ob-shell
@ 2023-02-06 19:39 Matt
  2023-02-07 11:40 ` Ihor Radchenko
  2023-02-11 20:56 ` [PATCH] Async evaluation in ob-shell jackkamm
  0 siblings, 2 replies; 49+ messages in thread
From: Matt @ 2023-02-06 19:39 UTC (permalink / raw)
  To: emacs-orgmode

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

^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-06 19:39 [PATCH] Async evaluation in ob-shell Matt
@ 2023-02-07 11:40 ` Ihor Radchenko
  2023-02-09  4:33   ` Matt
  2023-02-11 20:56 ` [PATCH] Async evaluation in ob-shell jackkamm
  1 sibling, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-02-07 11:40 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-07 11:40 ` Ihor Radchenko
@ 2023-02-09  4:33   ` Matt
  2023-02-09 11:24     ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Matt @ 2023-02-09  4:33 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode

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

^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-09  4:33   ` Matt
@ 2023-02-09 11:24     ` Ihor Radchenko
  2023-02-10 22:19       ` Matt
  2023-03-23  3:25       ` [SUGGESTION] ob-shell async result output should not contains shell prompt Christopher M. Miles
  0 siblings, 2 replies; 49+ messages in thread
From: Ihor Radchenko @ 2023-02-09 11:24 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-09 11:24     ` Ihor Radchenko
@ 2023-02-10 22:19       ` Matt
  2023-02-11 11:44         ` Ihor Radchenko
  2023-03-23  3:25       ` [SUGGESTION] ob-shell async result output should not contains shell prompt Christopher M. Miles
  1 sibling, 1 reply; 49+ messages in thread
From: Matt @ 2023-02-10 22:19 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode

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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-10 22:19       ` Matt
@ 2023-02-11 11:44         ` Ihor Radchenko
  2023-02-12 19:32           ` Matt
  0 siblings, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-02-11 11:44 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-06 19:39 [PATCH] Async evaluation in ob-shell Matt
  2023-02-07 11:40 ` Ihor Radchenko
@ 2023-02-11 20:56 ` jackkamm
  2023-02-12 19:02   ` Matt
  1 sibling, 1 reply; 49+ messages in thread
From: jackkamm @ 2023-02-11 20:56 UTC (permalink / raw)
  To: Matt, emacs-orgmode

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.


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-11 20:56 ` [PATCH] Async evaluation in ob-shell jackkamm
@ 2023-02-12 19:02   ` Matt
  2023-02-13  3:16     ` Jack Kamm
  0 siblings, 1 reply; 49+ messages in thread
From: Matt @ 2023-02-12 19:02 UTC (permalink / raw)
  To: jackkamm; +Cc: emacs-orgmode


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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-11 11:44         ` Ihor Radchenko
@ 2023-02-12 19:32           ` Matt
  2023-02-15 15:08             ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Matt @ 2023-02-12 19:32 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode


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



^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-12 19:02   ` Matt
@ 2023-02-13  3:16     ` Jack Kamm
  2023-02-13 17:07       ` [BUG] shell sessions started outside of Babel broken Matt
  2023-02-13 20:11       ` [BUG] conda doesn't work in ob-shell sessions Matt
  0 siblings, 2 replies; 49+ messages in thread
From: Jack Kamm @ 2023-02-13  3:16 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [BUG] shell sessions started outside of Babel broken
  2023-02-13  3:16     ` Jack Kamm
@ 2023-02-13 17:07       ` Matt
  2023-02-15  6:19         ` Jack Kamm
  2023-02-13 20:11       ` [BUG] conda doesn't work in ob-shell sessions Matt
  1 sibling, 1 reply; 49+ messages in thread
From: Matt @ 2023-02-13 17:07 UTC (permalink / raw)
  To: Jack Kamm; +Cc: emacs-orgmode

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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [BUG] conda doesn't work in ob-shell sessions
  2023-02-13  3:16     ` Jack Kamm
  2023-02-13 17:07       ` [BUG] shell sessions started outside of Babel broken Matt
@ 2023-02-13 20:11       ` Matt
  2023-02-15  6:21         ` Jack Kamm
  1 sibling, 1 reply; 49+ messages in thread
From: Matt @ 2023-02-13 20:11 UTC (permalink / raw)
  To: Jack Kamm; +Cc: emacs-orgmode


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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [BUG] shell sessions started outside of Babel broken
  2023-02-13 17:07       ` [BUG] shell sessions started outside of Babel broken Matt
@ 2023-02-15  6:19         ` Jack Kamm
  2023-02-16 12:53           ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Jack Kamm @ 2023-02-15  6:19 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [BUG] conda doesn't work in ob-shell sessions
  2023-02-13 20:11       ` [BUG] conda doesn't work in ob-shell sessions Matt
@ 2023-02-15  6:21         ` Jack Kamm
  0 siblings, 0 replies; 49+ messages in thread
From: Jack Kamm @ 2023-02-15  6:21 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-12 19:32           ` Matt
@ 2023-02-15 15:08             ` Ihor Radchenko
  2023-02-16  4:02               ` Matt
  0 siblings, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-02-15 15:08 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-15 15:08             ` Ihor Radchenko
@ 2023-02-16  4:02               ` Matt
  2023-02-17 10:44                 ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Matt @ 2023-02-16  4:02 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode


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





^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [BUG] shell sessions started outside of Babel broken
  2023-02-15  6:19         ` Jack Kamm
@ 2023-02-16 12:53           ` Ihor Radchenko
  2023-02-19 15:04             ` Jack Kamm
  0 siblings, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-02-16 12:53 UTC (permalink / raw)
  To: Jack Kamm; +Cc: Matt, emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-16  4:02               ` Matt
@ 2023-02-17 10:44                 ` Ihor Radchenko
  2023-02-19 23:14                   ` Matt
  0 siblings, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-02-17 10:44 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [BUG] shell sessions started outside of Babel broken
  2023-02-16 12:53           ` Ihor Radchenko
@ 2023-02-19 15:04             ` Jack Kamm
  2023-02-20 11:22               ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Jack Kamm @ 2023-02-19 15:04 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: Matt, emacs-orgmode

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.


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-17 10:44                 ` Ihor Radchenko
@ 2023-02-19 23:14                   ` Matt
  2023-02-20 11:24                     ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Matt @ 2023-02-19 23:14 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode

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


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [BUG] shell sessions started outside of Babel broken
  2023-02-19 15:04             ` Jack Kamm
@ 2023-02-20 11:22               ` Ihor Radchenko
  2023-02-21  5:23                 ` Jack Kamm
  0 siblings, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-02-20 11:22 UTC (permalink / raw)
  To: Jack Kamm; +Cc: Matt, emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-19 23:14                   ` Matt
@ 2023-02-20 11:24                     ` Ihor Radchenko
  2023-02-20 17:24                       ` Matt
  0 siblings, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-02-20 11:24 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-20 11:24                     ` Ihor Radchenko
@ 2023-02-20 17:24                       ` Matt
  2023-02-22 10:30                         ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Matt @ 2023-02-20 17:24 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode

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


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [BUG] shell sessions started outside of Babel broken
  2023-02-20 11:22               ` Ihor Radchenko
@ 2023-02-21  5:23                 ` Jack Kamm
  2023-02-22 10:38                   ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Jack Kamm @ 2023-02-21  5:23 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: Matt, emacs-orgmode

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?


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-20 17:24                       ` Matt
@ 2023-02-22 10:30                         ` Ihor Radchenko
  2023-03-02  1:36                           ` Matt
  0 siblings, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-02-22 10:30 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [BUG] shell sessions started outside of Babel broken
  2023-02-21  5:23                 ` Jack Kamm
@ 2023-02-22 10:38                   ` Ihor Radchenko
  0 siblings, 0 replies; 49+ messages in thread
From: Ihor Radchenko @ 2023-02-22 10:38 UTC (permalink / raw)
  To: Jack Kamm; +Cc: Matt, emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-02-22 10:30                         ` Ihor Radchenko
@ 2023-03-02  1:36                           ` Matt
  2023-03-03 14:52                             ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Matt @ 2023-03-02  1:36 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode

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


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-02  1:36                           ` Matt
@ 2023-03-03 14:52                             ` Ihor Radchenko
  2023-03-03 17:53                               ` Matt
  0 siblings, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-03-03 14:52 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-03 14:52                             ` Ihor Radchenko
@ 2023-03-03 17:53                               ` Matt
  2023-03-05 12:15                                 ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Matt @ 2023-03-03 17:53 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode

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


^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-03 17:53                               ` Matt
@ 2023-03-05 12:15                                 ` Ihor Radchenko
  2023-03-06  6:45                                   ` Matt
  0 siblings, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-03-05 12:15 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-05 12:15                                 ` Ihor Radchenko
@ 2023-03-06  6:45                                   ` Matt
  2023-03-07 12:45                                     ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Matt @ 2023-03-06  6:45 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode


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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-06  6:45                                   ` Matt
@ 2023-03-07 12:45                                     ` Ihor Radchenko
  2023-03-09 17:36                                       ` Matt
  0 siblings, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-03-07 12:45 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-07 12:45                                     ` Ihor Radchenko
@ 2023-03-09 17:36                                       ` Matt
  2023-03-10  1:52                                         ` Max Nikulin
                                                           ` (2 more replies)
  0 siblings, 3 replies; 49+ messages in thread
From: Matt @ 2023-03-09 17:36 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode, jeremiejuste, jackkamm

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.  


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-09 17:36                                       ` Matt
@ 2023-03-10  1:52                                         ` Max Nikulin
  2023-03-12 16:28                                         ` Jack Kamm
  2023-03-18 10:48                                         ` Ihor Radchenko
  2 siblings, 0 replies; 49+ messages in thread
From: Max Nikulin @ 2023-03-10  1:52 UTC (permalink / raw)
  To: emacs-orgmode

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.



^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-09 17:36                                       ` Matt
  2023-03-10  1:52                                         ` Max Nikulin
@ 2023-03-12 16:28                                         ` Jack Kamm
  2023-03-18 10:48                                         ` Ihor Radchenko
  2 siblings, 0 replies; 49+ messages in thread
From: Jack Kamm @ 2023-03-12 16:28 UTC (permalink / raw)
  To: Matt, Ihor Radchenko; +Cc: emacs-orgmode, jeremiejuste

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.


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-09 17:36                                       ` Matt
  2023-03-10  1:52                                         ` Max Nikulin
  2023-03-12 16:28                                         ` Jack Kamm
@ 2023-03-18 10:48                                         ` Ihor Radchenko
  2023-03-21 20:29                                           ` Matt
  2 siblings, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-03-18 10:48 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode, jeremiejuste, jackkamm

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-18 10:48                                         ` Ihor Radchenko
@ 2023-03-21 20:29                                           ` Matt
  2023-03-22 12:12                                             ` Ihor Radchenko
  2023-03-23 11:50                                             ` Ihor Radchenko
  0 siblings, 2 replies; 49+ messages in thread
From: Matt @ 2023-03-21 20:29 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode

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

^ permalink raw reply related	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-21 20:29                                           ` Matt
@ 2023-03-22 12:12                                             ` Ihor Radchenko
  2023-03-23 11:50                                             ` Ihor Radchenko
  1 sibling, 0 replies; 49+ messages in thread
From: Ihor Radchenko @ 2023-03-22 12:12 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [SUGGESTION] ob-shell async result output should not contains shell prompt
  2023-02-09 11:24     ` Ihor Radchenko
  2023-02-10 22:19       ` Matt
@ 2023-03-23  3:25       ` Christopher M. Miles
  2023-03-23  4:21         ` Matt
  1 sibling, 1 reply; 49+ messages in thread
From: Christopher M. Miles @ 2023-03-23  3:25 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: Matt, emacs-orgmode

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

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [SUGGESTION] ob-shell async result output should not contains shell prompt
  2023-03-23  3:25       ` [SUGGESTION] ob-shell async result output should not contains shell prompt Christopher M. Miles
@ 2023-03-23  4:21         ` Matt
  2023-03-23 11:12           ` Christopher M. Miles
  0 siblings, 1 reply; 49+ messages in thread
From: Matt @ 2023-03-23  4:21 UTC (permalink / raw)
  To: numbchild; +Cc: Ihor Radchenko, emacs-orgmode


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




^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [SUGGESTION] ob-shell async result output should not contains shell prompt
  2023-03-23  4:21         ` Matt
@ 2023-03-23 11:12           ` Christopher M. Miles
  2023-03-23 16:23             ` Matt
  2023-03-23 16:26             ` 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
  0 siblings, 2 replies; 49+ messages in thread
From: Christopher M. Miles @ 2023-03-23 11:12 UTC (permalink / raw)
  To: Matt; +Cc: numbchild, Ihor Radchenko, emacs-orgmode

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

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-21 20:29                                           ` Matt
  2023-03-22 12:12                                             ` Ihor Radchenko
@ 2023-03-23 11:50                                             ` Ihor Radchenko
  2023-03-23 19:35                                               ` Matt
  1 sibling, 1 reply; 49+ messages in thread
From: Ihor Radchenko @ 2023-03-23 11:50 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [SUGGESTION] ob-shell async result output should not contains shell prompt
  2023-03-23 11:12           ` Christopher M. Miles
@ 2023-03-23 16:23             ` Matt
  2023-03-24 11:20               ` Ihor Radchenko
  2023-03-23 16:26             ` 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
  1 sibling, 1 reply; 49+ messages in thread
From: Matt @ 2023-03-23 16:23 UTC (permalink / raw)
  To: numbchild; +Cc: Ihor Radchenko, emacs-orgmode


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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Remove "shell" as a supported Babel language within ob-shell.el (was Re: [SUGGESTION] ob-shell async result output should not contains shell prompt)
  2023-03-23 11:12           ` Christopher M. Miles
  2023-03-23 16:23             ` Matt
@ 2023-03-23 16:26             ` Matt
  2023-03-24  1:53               ` Remove "shell" as a supported Babel language within ob-shell.el Christopher M. Miles
  2023-03-24 11:38               ` 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
  1 sibling, 2 replies; 49+ messages in thread
From: Matt @ 2023-03-23 16:26 UTC (permalink / raw)
  To: numbchild; +Cc: Ihor Radchenko, emacs-orgmode

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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-23 11:50                                             ` Ihor Radchenko
@ 2023-03-23 19:35                                               ` Matt
  2023-03-24  9:13                                                 ` Ihor Radchenko
  0 siblings, 1 reply; 49+ messages in thread
From: Matt @ 2023-03-23 19:35 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: emacs-orgmode


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


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: Remove "shell" as a supported Babel language within ob-shell.el
  2023-03-23 16:26             ` 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
@ 2023-03-24  1:53               ` Christopher M. Miles
  2023-03-24 11:38               ` 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
  1 sibling, 0 replies; 49+ messages in thread
From: Christopher M. Miles @ 2023-03-24  1:53 UTC (permalink / raw)
  To: Matt; +Cc: numbchild, Ihor Radchenko, emacs-orgmode

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

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] Async evaluation in ob-shell
  2023-03-23 19:35                                               ` Matt
@ 2023-03-24  9:13                                                 ` Ihor Radchenko
  0 siblings, 0 replies; 49+ messages in thread
From: Ihor Radchenko @ 2023-03-24  9:13 UTC (permalink / raw)
  To: Matt; +Cc: emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [SUGGESTION] ob-shell async result output should not contains shell prompt
  2023-03-23 16:23             ` Matt
@ 2023-03-24 11:20               ` Ihor Radchenko
  0 siblings, 0 replies; 49+ messages in thread
From: Ihor Radchenko @ 2023-03-24 11:20 UTC (permalink / raw)
  To: Matt; +Cc: numbchild, emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

* 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)
  2023-03-23 16:26             ` 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
  2023-03-24  1:53               ` Remove "shell" as a supported Babel language within ob-shell.el Christopher M. Miles
@ 2023-03-24 11:38               ` Ihor Radchenko
  1 sibling, 0 replies; 49+ messages in thread
From: Ihor Radchenko @ 2023-03-24 11:38 UTC (permalink / raw)
  To: Matt; +Cc: numbchild, emacs-orgmode

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>


^ permalink raw reply	[flat|nested] 49+ messages in thread

end of thread, other threads:[~2023-03-24 15:47 UTC | newest]

Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-06 19:39 [PATCH] Async evaluation in ob-shell Matt
2023-02-07 11:40 ` Ihor Radchenko
2023-02-09  4:33   ` Matt
2023-02-09 11:24     ` Ihor Radchenko
2023-02-10 22:19       ` Matt
2023-02-11 11:44         ` Ihor Radchenko
2023-02-12 19:32           ` Matt
2023-02-15 15:08             ` Ihor Radchenko
2023-02-16  4:02               ` Matt
2023-02-17 10:44                 ` Ihor Radchenko
2023-02-19 23:14                   ` Matt
2023-02-20 11:24                     ` Ihor Radchenko
2023-02-20 17:24                       ` Matt
2023-02-22 10:30                         ` Ihor Radchenko
2023-03-02  1:36                           ` Matt
2023-03-03 14:52                             ` Ihor Radchenko
2023-03-03 17:53                               ` Matt
2023-03-05 12:15                                 ` Ihor Radchenko
2023-03-06  6:45                                   ` Matt
2023-03-07 12:45                                     ` Ihor Radchenko
2023-03-09 17:36                                       ` Matt
2023-03-10  1:52                                         ` Max Nikulin
2023-03-12 16:28                                         ` Jack Kamm
2023-03-18 10:48                                         ` Ihor Radchenko
2023-03-21 20:29                                           ` Matt
2023-03-22 12:12                                             ` Ihor Radchenko
2023-03-23 11:50                                             ` Ihor Radchenko
2023-03-23 19:35                                               ` Matt
2023-03-24  9:13                                                 ` Ihor Radchenko
2023-03-23  3:25       ` [SUGGESTION] ob-shell async result output should not contains shell prompt Christopher M. Miles
2023-03-23  4:21         ` Matt
2023-03-23 11:12           ` Christopher M. Miles
2023-03-23 16:23             ` Matt
2023-03-24 11:20               ` Ihor Radchenko
2023-03-23 16:26             ` 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
2023-03-24  1:53               ` Remove "shell" as a supported Babel language within ob-shell.el Christopher M. Miles
2023-03-24 11:38               ` 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
2023-02-11 20:56 ` [PATCH] Async evaluation in ob-shell jackkamm
2023-02-12 19:02   ` Matt
2023-02-13  3:16     ` Jack Kamm
2023-02-13 17:07       ` [BUG] shell sessions started outside of Babel broken Matt
2023-02-15  6:19         ` Jack Kamm
2023-02-16 12:53           ` Ihor Radchenko
2023-02-19 15:04             ` Jack Kamm
2023-02-20 11:22               ` Ihor Radchenko
2023-02-21  5:23                 ` Jack Kamm
2023-02-22 10:38                   ` Ihor Radchenko
2023-02-13 20:11       ` [BUG] conda doesn't work in ob-shell sessions Matt
2023-02-15  6:21         ` Jack Kamm

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).