Re: [BUG] ob-shell: cmdline and stdin broken when used with TRAMP

[Ihor Radchenko <yantar92@gmail.com>]

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