correct remote path handling

correct remote path handling

[Felipe Lema]

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

Hello, there

I bumped into a problem running src blocks using a remote (tramp) :dir. I've 
looked into it and found that the problem is that a temporary file is passed 
as a remote path to the remote process (temp file should be local to remote 
process).

I'm attaching fixes for python and shell src blocks. I didn't add any tests 
because that would require more than 15 LOC. I'm willing to add them, but 
directly to git repo.

I've signed the necessary papers from (to?) the FSF involving org mode, so I'm 
ready on my side to add tests and maybe add support for other tramp-related 
stuff.

Thanks
Felipe

[-- Attachment #2: 0001-Squashed-commit-of-the-following.patch --]
[-- Type: text/x-patch, Size: 1235 bytes --]

From e902f40842a20baa0c4a2ca462b83d9ce949a19f Mon Sep 17 00:00:00 2001
From: Felipe Lema <1232306+FelipeLema@users.noreply.github.com>
Date: Fri, 21 Feb 2020 14:34:53 -0300
Subject: [PATCH 1/2] Squashed commit of the following:

commit 6ea888f432b5eeb3559706e336a752791f48d7fb
Author: Felipe Lema <1232306+FelipeLema@users.noreply.github.com>
Date:   Fri Feb 21 11:25:51 2020 -0300

    fix evaluate python code in remote directory

    Evaluating an "AST python code" should be local to the process /
    directory.

    `file-local-name` will do just this (strip the tramp prefix in path)
---
 lisp/ob-python.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index dbcfac08d..85c9644c4 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -345,7 +345,8 @@ last statement in BODY, as elisp."
 					      "python-")))
 			   (with-temp-file tmp-src-file (insert body))
 			   (format org-babel-python--eval-ast
-				   tmp-src-file))))
+				   (file-local-name
+				    tmp-src-file)))))
                (org-babel-comint-with-output
                    (session org-babel-python-eoe-indicator nil body)
                  (let ((comint-process-echoes nil))
-- 
2.16.4


[-- Attachment #3: 0002-use-this-script-file-should-be-local-to-interpreter-.patch --]
[-- Type: text/x-patch, Size: 854 bytes --]

From d9d1c4180c86f38653ebdb0eb2e0a9d5865df0de Mon Sep 17 00:00:00 2001
From: Felipe Lema <1232306+FelipeLema@users.noreply.github.com>
Date: Fri, 21 Feb 2020 14:35:35 -0300
Subject: [PATCH 2/2] "use this script file" should be local to interpreter
 we're using

---
 lisp/ob-shell.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 6c8ca9652..8dce9c7dd 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -240,7 +240,8 @@ return the value of the last statement in BODY."
 	      (with-temp-buffer
 		(call-process-shell-command
 		 (concat (if shebang script-file
-			   (format "%s %s" shell-file-name script-file))
+			   (format "%s %s" shell-file-name
+				   (file-local-name script-file)))
 			 (and cmdline (concat " " cmdline)))
 		 stdin-file
 		 (current-buffer))
-- 
2.16.4

Re: correct remote path handling

[Jack Kamm]

Hi Felipe,

Felipe Lema <felipelema@mortemale.org> writes:

> I bumped into a problem running src blocks using a remote (tramp) :dir. I've 
> looked into it and found that the problem is that a temporary file is passed 
> as a remote path to the remote process (temp file should be local to remote 
> process).

Thanks for finding this bug and submitting a patch to fix it.

> I'm attaching fixes for python and shell src blocks. I didn't add any tests 
> because that would require more than 15 LOC.

I think it would be difficult to test for this, so I wouldn't worry too
much about it, but if you have ideas for how to do it that'd be great.

> I've signed the necessary papers from (to?) the FSF involving org mode, so I'm 
> ready on my side to add tests and maybe add support for other tramp-related 
> stuff.

Great, we should add you to the list of copyrighted contributors:
https://orgmode.org/worg/org-contribute.html

CC'ing Bastien in case he wants to verify everything by private message
first.

>     fix evaluate python code in remote directory
>
>     Evaluating an "AST python code" should be local to the process /
>     directory.
>
>     `file-local-name` will do just this (strip the tramp prefix in path)

Please see https://orgmode.org/worg/org-contribute.html#commit-messages
for information on how to properly format commit messages. You can also
take a look at the git log for examples.

> -				   tmp-src-file))))
> +				   (file-local-name
> +				    tmp-src-file)))))

Instead of `file-local-name', the preferred function we use for this is
`org-babel-process-file-name'. It's used in a few other places in
ob-python.el as well.

It looks like this patch only handles the ":session :results value"
case, but ":session :results output" also suffers from this problem,
could you add a fix for that case as well?

For example, I tried to execute the following ":session :results output"
block within a remote Org file, with the following result:

#+begin_src python :session :results output
  x = 1+1
  x
#+end_src

#+RESULTS:
: Traceback (most recent call last):
:   File "<stdin>", line 1, in <module>
: IOError: [Errno 2] No such file or directory: '/scp:pi:/tmp/python-NJwf2U'


> -			   (format "%s %s" shell-file-name script-file))
> +			   (format "%s %s" shell-file-name
> +				   (file-local-name script-file)))

As noted above, use `org-babel-process-file-name' instead of
`file-local-name'.

Also, I'm not sure the shell case is totally fixed by this, for example
trying to execute the following shell block within a remote Org file
still yields the error, even after applying your patch:

#+begin_src shell :shebang "#!/usr/bin/bash"
  echo foo
#+end_src

/bin/sh: 1: /scp:pi:/tmp/sh-script-8Z5fw7: not found

Re: correct remote path handling

[Bastien]

Hi Felipe and Jack,

> Felipe Lema <felipelema@mortemale.org> writes:
>
>> I've signed the necessary papers from (to?) the FSF involving org
>> mode, so I'm ready on my side to add tests and maybe add support
>> for other tramp-related stuff.

I've checked and the papers are not yet processed by the FSF.

> Great, we should add you to the list of copyrighted contributors:
> https://orgmode.org/worg/org-contribute.html

Done -  https://orgmode.org/worg/org-contribute.html#org7c578f2

Let's iterate on this patch and get it ready for 9.5.

Thanks,

-- 
 Bastien

Re: correct remote path handling

[Felipe Lema]

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

Hey, y'all

I found out that ob-shell was more broken than I thought so I had to upgrade 
my sword and get a new shield to fix it.

Tramp support for ob-python and ob-shell look OK now, although still no tests. 
I'll wait until this review is done until I start working on the tests. Will 
probably take a while because I'm thinking of some cross-section code that 
will do the a set of tests (all current? some marked? some new instead?) using 
a remote directory.

Using `ssh localhost` should suffice, but should these remote tests be done by 
default? It's most likely that everyone running tests will require a 
passwordless setup. While I've automatized this for a separate package, it may 
be bothersome to someone else (see [1]).

Anyway, I hope the patch file is correct now. I messed up my mirror repo and I 
/think/ I got it right.

BTW I just got a mail that FSF papers are approved now.

Gards, Felipe

[1]: https://github.com/FelipeLema/emacs-counsel-gtags/blame/
5d2a8c2c2d358e374a576cf8a3a67f7997a8839b/.travis.yml#L6

On Tuesday, 25 February 2020 16:14:05 -03 Bastien wrote:
> Hi Felipe and Jack,
> 
> > Felipe Lema <felipelema@mortemale.org> writes:
> >> I've signed the necessary papers from (to?) the FSF involving org
> >> mode, so I'm ready on my side to add tests and maybe add support
> >> for other tramp-related stuff.
> 
> I've checked and the papers are not yet processed by the FSF.
> 
> > Great, we should add you to the list of copyrighted contributors:
> > https://orgmode.org/worg/org-contribute.html
> 
> Done -  https://orgmode.org/worg/org-contribute.html#org7c578f2
> 
> Let's iterate on this patch and get it ready for 9.5.
> 
> Thanks,


[-- Attachment #2: tramp-support.patch --]
[-- Type: text/x-patch, Size: 9929 bytes --]

fix tramp support for ob-shell & ob-python

* ob-python.el (org-babel-python-evaluate-session): use file path local to (maybe) remote process.
* ob-shell.el (org-babel-execute:shell, org-babel-sh-evaluate): call-process-…→ process-file-… (tramp aware), wrap :session block in a function, separate "ob overhead" from "eval block", add documentation.

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index dbcfac08d..1afea9cb3 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -327,7 +327,8 @@ last statement in BODY, as elisp."
 						  "python-")))
 			       (with-temp-file tmp-src-file (insert body))
 			       (format org-babel-python--exec-tmpfile
-				       tmp-src-file))
+				       (org-babel-local-file-name
+					tmp-src-file)))
 			   body)))
 	       (mapconcat
 		#'org-trim
@@ -345,9 +346,10 @@ last statement in BODY, as elisp."
 					      "python-")))
 			   (with-temp-file tmp-src-file (insert body))
 			   (format org-babel-python--eval-ast
-				   tmp-src-file))))
-               (org-babel-comint-with-output
-                   (session org-babel-python-eoe-indicator nil body)
+				   (org-babel-local-file-name
+				    tmp-src-file)))))
+	       (org-babel-comint-with-output
+		   (session org-babel-python-eoe-indicator nil body)
                  (let ((comint-process-echoes nil))
                    (funcall input-body body)
 		   (dolist
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 347ffedd1..66cdfb94c 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -82,12 +82,17 @@ This function is called by `org-babel-execute-src-block'."
 	 (value-is-exit-status
 	  (member "value" (cdr (assq :result-params params))))
 	 (cmdline (cdr (assq :cmdline params)))
+	 (shebang (cdr (assq :shebang params)))
+	 (padline (not (equal "no" (cdr (assq :padline params)))))
+	 (result-params (cdr (assq :result-params params)))
          (full-body (concat
 		     (org-babel-expand-body:generic
 		      body params (org-babel-variable-assignments:shell params))
 		     (when value-is-exit-status "\necho $?"))))
     (org-babel-reassemble-table
-     (org-babel-sh-evaluate session full-body params stdin cmdline)
+     (org-babel-sh-evaluate session full-body
+			    stdin cmdline shebang value-is-exit-status padline
+			    result-params)
      (org-babel-pick-name
       (cdr (assq :colname-names params)) (cdr (assq :colnames params)))
      (org-babel-pick-name
@@ -206,76 +211,114 @@ var of the same value."
   "String to indicate that evaluation has completed.")
 (defvar org-babel-sh-eoe-output "org_babel_sh_eoe"
   "String to indicate that evaluation has completed.")
+(defvar org-babel-sh-block-function-name "org_babel_block"
+  "Name of the shell function that will hold the code to be executed.")
 
-(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)))
-	 (value-is-exit-status
-	  (member "value" (cdr (assq :result-params params))))
-	 (results
-	  (cond
-	   ((or stdin cmdline)	       ; external shell script w/STDIN
-	    (let ((script-file (org-babel-temp-file "sh-script-"))
-		  (stdin-file (org-babel-temp-file "sh-stdin-"))
-		  (padline (not (string= "no" (cdr (assq :padline params))))))
-	      (with-temp-file script-file
-		(when shebang (insert shebang "\n"))
-		(when padline (insert "\n"))
-		(insert body))
-	      (set-file-modes script-file #o755)
-	      (with-temp-file stdin-file (insert (or stdin "")))
-	      (with-temp-buffer
-		(call-process-shell-command
-		 (concat (if shebang script-file
-			   (format "%s %s" shell-file-name script-file))
-			 (and cmdline (concat " " cmdline)))
-		 stdin-file
-		 (current-buffer))
-		(buffer-string))))
-	   (session			; session evaluation
-	    (mapconcat
-	     #'org-babel-sh-strip-weird-long-prompt
-	     (mapcar
-	      #'org-trim
-	      (butlast
-	       (org-babel-comint-with-output
-		   (session org-babel-sh-eoe-output t body)
-		 (dolist (line (append (split-string (org-trim body) "\n")
-				       (list org-babel-sh-eoe-indicator)))
-		   (insert line)
-		   (comint-send-input nil t)
-		   (while (save-excursion
-			    (goto-char comint-last-input-end)
-			    (not (re-search-forward
-				  comint-prompt-regexp nil t)))
-		     (accept-process-output
-		      (get-buffer-process (current-buffer))))))
-	       2))
-	     "\n"))
+(defun org-babel-sh-evaluate (session body
+				      &optional stdin cmdline
+				      shebang value-is-exit-status padline
+				      result-params)
+  "Pass BODY to the Shell process in SESSION.
+
+Optional arguments:
+
+- Send STDIN as stdin in.
+- Extra commandline arguments (such as -f -x -v…) in CMDLINE.
+- Use SHEBANG as interpreter for block (such as \"#!/usr/bin/bash\")
+- When VALUE-IS-EXIT-STATUS, returns the exit status value as a string.
+- Add an extra end-of-line when PADLINE
+- forward RESULT-PARAMS to `org-babel-result-cond'."
+  (let* ((results
+          (cond
+           ((or stdin cmdline)         ; external shell script w/STDIN
+            (let ((script-file (org-babel-temp-file "sh-script-"))
+                  (stdin-file (org-babel-temp-file "sh-stdin-")))
+              (with-temp-file script-file
+                (when shebang (insert shebang "\n"))
+                (when padline (insert "\n"))
+                (insert body))
+              (set-file-modes script-file #o755)
+              (with-temp-file stdin-file (insert (or stdin "")))
+              (with-temp-buffer
+                (process-file-shell-command
+                 (concat
+                  (let ((local-script-file (org-babel-local-file-name
+                                            script-file)))
+                    (if shebang local-script-file
+                      (format "%s %s" shell-file-name
+                              local-script-file)))
+                  (and cmdline (concat " " cmdline)))
+                 stdin-file
+                 (current-buffer))
+                (buffer-string))))
+           (session                     ; session evaluation
+	    (let* ((block-output-lines
+		    (let ((fun-body
+			   (format "%s(){\n%s\n}\n%s"
+				    org-babel-sh-block-function-name
+				    ;; function block
+				    (concat
+				     body
+				     "\n"
+				     org-babel-sh-eoe-indicator) ;; mark eoe
+				    ;; mark "function has been input"
+				    org-babel-sh-eoe-indicator)))
+		      (cl-flet ((send-and-wait ()
+					       (comint-send-input nil t)
+					       (while (save-excursion
+							(goto-char comint-last-input-end)
+							(not (re-search-forward
+							      comint-prompt-regexp nil t)))
+						 (accept-process-output
+						  (get-buffer-process (current-buffer))))))
+			;; define a function with the code we want to eval
+			(org-babel-comint-with-output
+			    (session org-babel-sh-eoe-output t fun-body)
+			  (insert fun-body)
+			  (send-and-wait)) ;; wait until function code is accepted
+			;; now, actually eval the code by calling it as a function
+			(org-babel-comint-with-output
+			    (session org-babel-sh-eoe-output t
+				     org-babel-sh-block-function-name)
+			  (insert org-babel-sh-block-function-name)
+			  (send-and-wait)))))
+		   (block-output-lines-sans-marker-sans-prompt
+		    (thread-first
+			block-output-lines
+		      (car) ;; (list ouput-str prompt-str)
+		      (split-string "\n" t) ;; string lines
+		      (butlast 1))) ;; remove last line with prompt
+		   (block-output-clean-lines
+		    (mapcar #'org-trim
+			    block-output-lines-sans-marker-sans-prompt))
+		   (block-output
+		    (mapconcat
+		     #'org-babel-sh-strip-weird-long-prompt
+		     block-output-clean-lines "\n")))
+	      block-output))
 	   ;; External shell script, with or without a predefined
-	   ;; shebang.
-	   ((org-string-nw-p shebang)
-	    (let ((script-file (org-babel-temp-file "sh-script-"))
-		  (padline (not (equal "no" (cdr (assq :padline params))))))
-	      (with-temp-file script-file
-		(insert shebang "\n")
-		(when padline (insert "\n"))
-		(insert body))
-	      (set-file-modes script-file #o755)
-	      (org-babel-eval script-file "")))
-	   (t (org-babel-eval shell-file-name (org-trim body))))))
+           ;; shebang.
+           ((org-string-nw-p shebang)
+            (let ((script-file (org-babel-temp-file "sh-script-")))
+              (with-temp-file script-file
+                (insert shebang "\n")
+                (when padline (insert "\n"))
+                (insert body))
+              (set-file-modes script-file #o755)
+              ;; (maybe remotely) run this script as a command
+              (org-babel-eval (org-babel-local-file-name script-file) "")))
+           (t (org-babel-eval
+               shell-file-name
+               (org-trim body))))))
     (when value-is-exit-status
+      ;; last line is the output of "echo $?"
       (setq results (car (reverse (split-string results "\n" t)))))
     (when results
-      (let ((result-params (cdr (assq :result-params params))))
-        (org-babel-result-cond result-params
+      (org-babel-result-cond result-params
           results
           (let ((tmp-file (org-babel-temp-file "sh-")))
             (with-temp-file tmp-file (insert results))
-            (org-babel-import-elisp-from-file tmp-file)))))))
+            (org-babel-import-elisp-from-file tmp-file))))))
 
 (defun org-babel-sh-strip-weird-long-prompt (string)
   "Remove prompt cruft from a string of shell output."

Re: correct remote path handling

[Jack Kamm]

Hi Felipe,

It looks like you've made some quite substantial changes to ob-shell.

I think it would be a good idea to split this up into 2 patches, and to
start a new thread for the ob-shell patch.

I'm not as familiar with ob-shell, and it's also had some work
lately. So it'd be good to get some more visibility for these changes
with a new thread for it.

Also, I think it'd be a good idea to provide some explanation of what
you've changed in ob-shell, the reason for these changes, and some
examples of how the behavior has changed.

If possible, it'd also be good to split up the ob-shell changes into a
few smaller patches, each addressing a specific issue.

Before starting a new thread, I would recommend waiting until Org 9.4 is
released, which should be any day now.

Below are some specific feedback for your updated patch:

> diff --git a/lisp/ob-python.el b/lisp/ob-python.el
> index dbcfac08d..1afea9cb3 100644
> --- a/lisp/ob-python.el
> +++ b/lisp/ob-python.el
> @@ -327,7 +327,8 @@ last statement in BODY, as elisp."
>  						  "python-")))
>  			       (with-temp-file tmp-src-file (insert body))
>  			       (format org-babel-python--exec-tmpfile
> -				       tmp-src-file))
> +				       (org-babel-local-file-name
> +					tmp-src-file)))

You should use `org-babel-process-file-name' here, not
`org-babel-local-file-name'.

> -				   tmp-src-file))))
> -               (org-babel-comint-with-output
> -                   (session org-babel-python-eoe-indicator nil body)
> +				   (org-babel-local-file-name
> +				    tmp-src-file)))))
> +	       (org-babel-comint-with-output
> +		   (session org-babel-python-eoe-indicator nil body)

Same comment as above.

> diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
> index 347ffedd1..66cdfb94c 100644
> --- a/lisp/ob-shell.el
> +++ b/lisp/ob-shell.el
> @@ -82,12 +82,17 @@ This function is called by `org-babel-execute-src-block'."
>  	 (value-is-exit-status
>  	  (member "value" (cdr (assq :result-params params))))
>  	 (cmdline (cdr (assq :cmdline params)))
> +	 (shebang (cdr (assq :shebang params)))
> +	 (padline (not (equal "no" (cdr (assq :padline params)))))
> +	 (result-params (cdr (assq :result-params params)))
>           (full-body (concat
>  		     (org-babel-expand-body:generic
>  		      body params (org-babel-variable-assignments:shell params))
>  		     (when value-is-exit-status "\necho $?"))))
>      (org-babel-reassemble-table
> -     (org-babel-sh-evaluate session full-body params stdin cmdline)
> +     (org-babel-sh-evaluate session full-body
> +			    stdin cmdline shebang value-is-exit-status padline
> +			    result-params)

These variables used to be let-bound inside `org-babel-sh-evaluate', but
now you've changed the signature of that function to pass them in from
the outside.

I don't see a good reason for this change, since as far as I can tell,
those variables aren't used outside `org-babel-sh-evaluate'.

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

It looks like you've changed the indentation of this function, which
causes a hard-to-read diff. Please stick to the original indentation so
we can more easily review the changes here.

> +	    (let* ((block-output-lines
> +		    (let ((fun-body
> +			   (format "%s(){\n%s\n}\n%s"
> +				    org-babel-sh-block-function-name
> +				    ;; function block
> +				    (concat
> +				     body
> +				     "\n"
> +				     org-babel-sh-eoe-indicator) ;; mark eoe
> +				    ;; mark "function has been input"
> +				    org-babel-sh-eoe-indicator)))

This is a very interesting idea, to wrap the shell block inside a
function.

I think it's a good idea and would fix some issues with leaky prompts,
among other things.

As an aside, I've been working on developing an async evaluation feature
for org-babel blocks, but we were struggling with ob-shell. This idea to
wrap the block in a function might be the solution.

Anyways, it would be good to discuss this change further with motivation
and examples.