From: Bruno Barbier <brubar.cs@gmail.com>
To: Ihor Radchenko <yantar92@gmail.com>
Cc: Felix Freeman <libsys@hacktivista.org>, emacs-orgmode@gnu.org
Subject: Re: [BUG] ob-shell: cmdline and stdin broken when used with TRAMP
Date: Sat, 03 Sep 2022 19:20:17 +0200 [thread overview]
Message-ID: <E1oUWlR-0000jc-G0@lists.gnu.org> (raw)
In-Reply-To: <87k097a6on.fsf@localhost>
[-- Attachment #1: Type: text/plain, Size: 2335 bytes --]
Ihor Radchenko <yantar92@gmail.com> writes:
> Thanks for the patch!
>
Thanks for the review!
>> I've also included a test, as the problem is reproducible with TRAMP
>> "/mock::" connection. But, that test will only work on GNU/Linux
>> systems.
> Then you also need to guard the tests against system-type variable
> value. If we cannot tests things on Windows, we should at least make the
> tests not fail when they should not.
Done. I've told ERT to skip this new test for ms-dos and windows-nt
systems. Thanks for the variable to use.
> You patch is >15LOC so we do need your copyright assignment before
> merging. Let me know if you face any difficulties with the copyright
> process. Note that FSF should reply within 5 working days.
Done. I just learnt a few days ago the process is done.
> Note that we quote symbols like `symbols'.
> See https://orgmode.org/worg/org-contribute.html#commit-messages
> Also, please link to the bug report in the commit message for future
> reference.
Should be now OK too.
> > + (list shell-command-switch
> > + (concat (file-local-name script-file) " " cmdline)))
>
> Probably you do not need concat here.
> AFAIU, (list shell-command-switch (file-local-name script-file) cmdline)
> should be good enough as ARGS argument of `process-file'.
The shell process should receive 2 arguments: the switch and the
command to execute. I think the 'concat' is mandatory here.
Am I missing something?
>> + (:stdin t :shebang t)
>> + (:cmdline t :stdin t :shebang t)
>> + ))
>
> Please do not leave closing parenthesis at a separate line. See D.1
> Emacs Lisp Coding Conventions section of Elisp manual for details.
Oops. Fixed.
>> +(defconst org-test-tramp-remote-dir "/mock::/tmp/"
>> + "Remote tramp directory.
>> +We really should use 'tramp-test-temporary-file-directory', but that would require TRAMP sources.")
>
> Since TRAMP sources are not normally available, we can add this variable
> as defined in tramp-tests.el somewhere into testing/org-test.el, for
> example.
I've copy/pasted the logic used in GNU Emacs Tramp and added a macro.
I've added (require 'tramp) in the body; should I move it to the top
of the file ?
Thanks again for the review,
Bruno
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 6939 bytes --]
From d9abfb423bff620dee15d204f4bab48e2ec8dc4e Mon Sep 17 00:00:00 2001
From: Bruno BARBIER <brubar.cs@gmail.com>
Date: Sat, 18 Jun 2022 09:48:01 +0200
Subject: [PATCH] ob-shell: Use 'process-file' when stdin or cmdline
lib/ob-shell.el (org-babel-sh-evaluate): Use `process-file' (instead
of `call-process-shell-command') so that `org-babel-sh-evaluate' will
invoke file name handlers based on `default-directory', if needed,
like when using a remote directory.
testing/lisp/test-ob-shell.el (ob-shell/remote-with-stdin-or-cmdline):
New test.
testing/org-test.el (org-test-with-tramp-remote-dir): New macro.
Fixes https://list.orgmode.org/CKMOBWBK709F.1RUN69SRWB64U@laptop/.
---
lisp/ob-shell.el | 16 +++++++-----
testing/lisp/test-ob-shell.el | 49 +++++++++++++++++++++++++++++++++++
testing/org-test.el | 29 +++++++++++++++++++++
3 files changed, 88 insertions(+), 6 deletions(-)
diff --git a/lisp/ob-shell.el b/lisp/ob-shell.el
index 44efb4ea1..51071d40a 100644
--- a/lisp/ob-shell.el
+++ b/lisp/ob-shell.el
@@ -279,12 +279,16 @@ (defun org-babel-sh-evaluate (session body &optional params stdin cmdline)
(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))
+ (with-connection-local-variables
+ (apply #'process-file
+ (if shebang (file-local-name script-file)
+ shell-file-name)
+ stdin-file
+ (current-buffer)
+ nil
+ (if shebang (when cmdline (list cmdline))
+ (list shell-command-switch
+ (concat (file-local-name script-file) " " cmdline)))))
(buffer-string))))
(session ; session evaluation
(mapconcat
diff --git a/testing/lisp/test-ob-shell.el b/testing/lisp/test-ob-shell.el
index 2f346f699..442e70372 100644
--- a/testing/lisp/test-ob-shell.el
+++ b/testing/lisp/test-ob-shell.el
@@ -106,6 +106,55 @@ (ert-deftest ob-shell/simple-list ()
"#+BEGIN_SRC sh :results output :var l='(1 2)\necho ${l}\n#+END_SRC"
(org-trim (org-babel-execute-src-block))))))
+(ert-deftest ob-shell/remote-with-stdin-or-cmdline ()
+ "Test :stdin and :cmdline with a remote directory."
+ ;; We assume 'default-directory' is a local directory.
+ (skip-unless (not (memq system-type '(ms-dos windows-nt))))
+ (org-test-with-tramp-remote-dir remote-dir
+ (dolist (spec `( ()
+ (:dir ,remote-dir)
+ (:dir ,remote-dir :cmdline t)
+ (:dir ,remote-dir :stdin t)
+ (:dir ,remote-dir :cmdline t :shebang t)
+ (:dir ,remote-dir :stdin t :shebang t)
+ (:dir ,remote-dir :cmdline t :stdin t :shebang t)
+ (:cmdline t)
+ (:stdin t)
+ (:cmdline t :shebang t)
+ (:stdin t :shebang t)
+ (:cmdline t :stdin t :shebang t)))
+ (let ((default-directory (or (plist-get spec :dir) default-directory))
+ (org-confirm-babel-evaluate nil)
+ (params-line "")
+ (who-line " export who=tramp")
+ (args-line " echo ARGS: --verbose 23 71"))
+ (when-let ((dir (plist-get spec :dir)))
+ (setq params-line (concat params-line " " ":dir " dir)))
+ (when (plist-get spec :stdin)
+ (setq who-line " read -r who")
+ (setq params-line (concat params-line " :stdin input")))
+ (when (plist-get spec :cmdline)
+ (setq args-line " echo \"ARGS: $*\"")
+ (setq params-line (concat params-line " :cmdline \"--verbose 23 71\"")))
+ (when (plist-get spec :shebang)
+ (setq params-line (concat params-line " :shebang \"#!/bin/sh\"")))
+ (let* ((result (org-test-with-temp-text
+ (mapconcat #'identity
+ (list "#+name: input"
+ "tramp"
+ ""
+ (concat "<point>"
+ "#+begin_src sh :results output " params-line)
+ args-line
+ who-line
+ " echo \"hello $who from $(pwd)/\""
+ "#+end_src")
+ "\n")
+ (org-trim (org-babel-execute-src-block))))
+ (expected (concat "ARGS: --verbose 23 71"
+ "\nhello tramp from " (file-local-name default-directory))))
+ (should (equal result expected)))))))
+
(provide 'test-ob-shell)
;;; test-ob-shell.el ends here
diff --git a/testing/org-test.el b/testing/org-test.el
index 0520e82f9..7e5d60e63 100644
--- a/testing/org-test.el
+++ b/testing/org-test.el
@@ -284,6 +284,35 @@ (defun org-test-table-target-expect (target &optional expect laps
;; on multiple lines in the ERT results buffer.
(setq pp-escape-newlines back)))))
+(defun org-test-with-tramp-remote-dir--worker (body)
+ "Worker for 'org-test-with-tramp-remote-dir'."
+ (let ((env-def (getenv "REMOTE_TEMPORARY_FILE_DIRECTORY")))
+ (cond
+ (env-def (funcall body env-def))
+ ((eq system-type 'windows-nt) (funcall body null-device))
+ (t (require 'tramp)
+ (let ((tramp-methods
+ (cons '("mock"
+ (tramp-login-program "sh")
+ (tramp-login-args (("-i")))
+ (tramp-remote-shell "/bin/sh")
+ (tramp-remote-shell-args ("-c"))
+ (tramp-connection-timeout 10))
+ tramp-methods))
+ (tramp-default-host-alist
+ `(("\\`mock\\'" nil ,(system-name)))))
+ (funcall body (format "/mock::%s" temporary-file-directory)))))))
+
+(defmacro org-test-with-tramp-remote-dir (dir &rest body)
+ "Bind the symbol DIR to a remote directory and execute BODY.
+Return the value of the last form in BODY. The directory DIR
+will be something like \"/mock::/tmp/\", which allows to test
+Tramp related features. We mostly follow
+`tramp-test-temporary-file-directory' from GNU Emacs tests."
+ (declare (debug (sexp body)) (indent 2))
+ `(org-test-with-tramp-remote-dir--worker (lambda (,dir) ,@body)))
+
+
\f
;;; Navigation Functions
--
2.35.1
next prev parent reply other threads:[~2022-09-03 17:17 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-10 18:42 [BUG] ob-shell: cmdline and stdin broken when used with TRAMP Felix Freeman via General discussions about Org-mode.
2022-06-18 18:54 ` Bruno Barbier
2022-06-23 12:46 ` Ihor Radchenko
2022-09-03 17:20 ` Bruno Barbier [this message]
2022-09-04 9:49 ` Ihor Radchenko
2022-09-04 17:08 ` Bruno Barbier
2022-09-05 10:33 ` Ihor Radchenko
2022-09-06 17:04 ` Bruno Barbier
2022-09-05 8:16 ` Bastien Guerry
2022-09-06 17:03 ` Bruno Barbier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E1oUWlR-0000jc-G0@lists.gnu.org \
--to=brubar.cs@gmail.com \
--cc=emacs-orgmode@gnu.org \
--cc=libsys@hacktivista.org \
--cc=yantar92@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).