From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp10.migadu.com ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms5.migadu.com with LMTPS id aB7UMSqME2NKYAAAbAwnHQ (envelope-from ) for ; Sat, 03 Sep 2022 19:17:30 +0200 Received: from aspmx1.migadu.com ([2001:41d0:2:bcc0::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp10.migadu.com with LMTPS id kD8EMSqME2MvbQAAG6o9tA (envelope-from ) for ; Sat, 03 Sep 2022 19:17:30 +0200 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 50E6F1452F for ; Sat, 3 Sep 2022 19:17:30 +0200 (CEST) Received: from localhost ([::1]:56960 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oUWlt-0000mV-Gz for larch@yhetil.org; Sat, 03 Sep 2022 13:17:29 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:39252) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oUWlR-0000jc-G0 for emacs-orgmode@gnu.org; Sat, 03 Sep 2022 13:17:01 -0400 Received: from smtp6-g21.free.fr ([212.27.42.6]:60938) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oUWlP-00020c-Ah for emacs-orgmode@gnu.org; Sat, 03 Sep 2022 13:17:01 -0400 Received: from tosh-laptop (unknown [IPv6:2a01:e0a:505:3460:1a16:a0c4:3f89:c0d9]) by smtp6-g21.free.fr (Postfix) with ESMTPS id 392D978050A; Sat, 3 Sep 2022 19:16:53 +0200 (CEST) Received: by tosh-laptop (sSMTP sendmail emulation); Sat, 03 Sep 2022 19:20:18 +0200 From: Bruno Barbier To: Ihor Radchenko Cc: Felix Freeman , emacs-orgmode@gnu.org Subject: Re: [BUG] ob-shell: cmdline and stdin broken when used with TRAMP In-Reply-To: <87k097a6on.fsf@localhost> References: <87k097a6on.fsf@localhost> Date: Sat, 03 Sep 2022 19:20:17 +0200 MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Received-SPF: softfail client-ip=212.27.42.6; envelope-from=brubar.cs@gmail.com; helo=smtp6-g21.free.fr X-Spam_score_int: 12 X-Spam_score: 1.2 X-Spam_bar: + X-Spam_report: (1.2 / 5.0 requ) BAYES_00=-1.9, DKIM_ADSP_CUSTOM_MED=0.001, FORGED_GMAIL_RCVD=1, FREEMAIL_FROM=0.001, MISSING_MID=0.497, NML_ADSP_CUSTOM_MED=0.9, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_SOFTFAIL=0.665, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=no autolearn_force=no X-Spam_action: no action Message-Id: X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: "Emacs-orgmode" X-Migadu-Flow: FLOW_IN X-Migadu-To: larch@yhetil.org X-Migadu-Country: US ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1662225450; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references:list-id:list-help:list-unsubscribe: list-subscribe:list-post; bh=KaR795oLnp7AZsiHQSujqS5AHO07gyM0PCumCSGQ3nQ=; b=p49OCXyoE0Me4gXDSdBNPwTSbRQTB2ffHH0KdKwKgU7rXk5ErG7Uvhc9Nni67pjdAxLvM5 Ne/31KfoegMi/4em9SyvYopbln5/NjhoCeh84oeivyDUkI4oJGWAjsDUxaT+H5N2un4W3k hRJzZqwll+HYsDVmQ3bQATJojN4PDZdsoS8hPpyRwAXa5T7HBe8SiISbkc1cdIZVUZi+Ol Av8G1eK1SMZO46vPEsrTWlRy7p1G/sPH2GfOoSzxzahbFOcaKJKdJaqtM4iMQ7oBdi6Hu/ 2hkDcWPnNwzJ56l1RxOF2z9ul8ng+obUc7LyK2JUjj1FXiHkuXZNWZWN09AWvw== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1662225450; a=rsa-sha256; cv=none; b=r1RErQPxm+g9IMc2DUw3mjrim2wjYOXIJJEHw2MUoYKBFQX3pnav1lFyldr72XKk+su4hT k/l7Y1FU/4kmtHP5u/Z3CPSL9x1shBxfXf4Kj+4lCNMXjZcVVLWPf/8F9KsATM/OuvYFpp n8yAMUf4R5p3TverjYXcBY3SvalcISW2S+n51/8byPR43qTA7m2LT7/CRvsnfOdPz3HlsB gxFnzmAgVZQAtm/YdYN3u8Ae/YdBFAbT4nk8Mbd4YNe1voKXdEeGuKBjRtisjDxjLl917E M1H4SQgBPVJveNQkFOCFxJxA7lxKaNPKc/AkkjzQzJJWCBYfx7yhdn+WMMuw+A== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=gmail.com (policy=none); spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org" X-Migadu-Spam-Score: 1.73 Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=gmail.com (policy=none); spf=pass (aspmx1.migadu.com: domain of "emacs-orgmode-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="emacs-orgmode-bounces+larch=yhetil.org@gnu.org" X-Migadu-Queue-Id: 50E6F1452F X-Spam-Score: 1.73 X-Migadu-Scanner: scn0.migadu.com X-TUID: 3XS/KRRxYutm --=-=-= Content-Type: text/plain Ihor Radchenko 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 --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0001-ob-shell-Use-process-file-when-stdin-or-cmdline.patch Content-Description: patch >From d9abfb423bff620dee15d204f4bab48e2ec8dc4e Mon Sep 17 00:00:00 2001 From: Bruno BARBIER 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 "" + "#+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))) + + ;;; Navigation Functions -- 2.35.1 --=-=-=--