[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [BUG] ob-shell: cmdline and stdin broken when used with TRAMP
From: |
Ihor Radchenko |
Subject: |
Re: [BUG] ob-shell: cmdline and stdin broken when used with TRAMP |
Date: |
Thu, 23 Jun 2022 20:46:32 +0800 |
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