emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Ihor Radchenko <yantar92@gmail.com>
To: Bruno Barbier <brubar.cs@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: Thu, 23 Jun 2022 20:46:32 +0800	[thread overview]
Message-ID: <87k097a6on.fsf@localhost> (raw)
In-Reply-To: <E1o2dZt-0006wh-Gt@lists.gnu.org>

Bruno Barbier <brubar.cs@gmail.com> writes:

> From what I understand, the function 'org-babel-sh-evaluate' relies on
> 'call-process'; and that function ignores file name handlers; as TRAMP
> relies on those file name handlers, it just cannot do the right thing.
>
> Using 'process-file' instead works for me.
>
> See the attached patch.

Thanks for the patch!

I am not very familiar with TRAMP, but since you supplied tests and they
are also passing on my side, everything looks good.

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

> Warning: that's my first attempt to write a patch, and I don't have
> (yet) signed the copyright papers.

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.

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

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.

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

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

> +                   (: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.

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

Best,
Ihor


  reply	other threads:[~2022-06-23 12:51 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 [this message]
2022-09-03 17:20     ` Bruno Barbier
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=87k097a6on.fsf@localhost \
    --to=yantar92@gmail.com \
    --cc=brubar.cs@gmail.com \
    --cc=emacs-orgmode@gnu.org \
    --cc=libsys@hacktivista.org \
    /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).