* correct remote path handling
@ 2020-02-25 1:36 Felipe Lema
2020-02-25 15:54 ` Jack Kamm
0 siblings, 1 reply; 5+ messages in thread
From: Felipe Lema @ 2020-02-25 1:36 UTC (permalink / raw)
To: emacs-orgmode
[-- 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: correct remote path handling 2020-02-25 1:36 correct remote path handling Felipe Lema @ 2020-02-25 15:54 ` Jack Kamm 2020-02-25 19:14 ` Bastien 0 siblings, 1 reply; 5+ messages in thread From: Jack Kamm @ 2020-02-25 15:54 UTC (permalink / raw) To: Felipe Lema, emacs-orgmode; +Cc: Bastien 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: correct remote path handling 2020-02-25 15:54 ` Jack Kamm @ 2020-02-25 19:14 ` Bastien 2020-02-27 1:58 ` Felipe Lema 0 siblings, 1 reply; 5+ messages in thread From: Bastien @ 2020-02-25 19:14 UTC (permalink / raw) To: Jack Kamm; +Cc: Felipe Lema, emacs-orgmode 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: correct remote path handling 2020-02-25 19:14 ` Bastien @ 2020-02-27 1:58 ` Felipe Lema 2020-02-29 16:22 ` Jack Kamm 0 siblings, 1 reply; 5+ messages in thread From: Felipe Lema @ 2020-02-27 1:58 UTC (permalink / raw) To: Bastien; +Cc: Jack Kamm, emacs-orgmode [-- 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." ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: correct remote path handling 2020-02-27 1:58 ` Felipe Lema @ 2020-02-29 16:22 ` Jack Kamm 0 siblings, 0 replies; 5+ messages in thread From: Jack Kamm @ 2020-02-29 16:22 UTC (permalink / raw) To: Felipe Lema, Bastien; +Cc: emacs-orgmode 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-29 16:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-25 1:36 correct remote path handling Felipe Lema 2020-02-25 15:54 ` Jack Kamm 2020-02-25 19:14 ` Bastien 2020-02-27 1:58 ` Felipe Lema 2020-02-29 16:22 ` 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).