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

[Bruno Barbier]

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

0001-ob-shell-Use-process-file-when-stdin-or-cmdline.patch
Description: patch