[Buildbot-devel] Removing the "lone pipe exception"

[Dustin J. Mitchell]

OK, that helped quite a bit - thanks for the detail.

So, this started in http://trac.buildbot.net/ticket/1799 where Nate
wanted to escape | within an argument, but carved out an exception for
'|' as a standalone argument, so that ['a', '|', 'b'] would actually
pipe a to b.  This is unlike POSIX, where ['a', '|', 'b'] will pass
'|' and 'b' as arguments to a.  There's no clear rationale for this in
the bug or the commit messages, except the implication that it seems
sensible.

Roman, you made some improvements back in March, but kept this
particular piece of "functionality".

So I think it makes sense to remove this before 0.9.0, with a suitable
warning in the release notes (since it does change current behavior).

As for subprocess against spawnProcess, spawnProcess is designed to
work well with the reactor, with callbacks for stdout, stderr, and
process termination.  If we switched to subprocess, we'd need to build
all of that ourselves.

Dustin

On Wed, Sep 24, 2014 at 2:10 PM, Роман Донченко <dpb at corrigendum.ru> wrote:
> Dustin J. Mitchell <dustin at v.igoro.us> писал в своём письме Wed, 24 Sep 2014
> 04:00:48 +0400:
>
>> Well, we do use Twisted's `spawnProcess`, but when given a string we
>> want to replicate the behavior on POSIX, which is to pass it to sh -c
>> '..'.  Although I see (in `slave/buildslave/runprocess.py`) that we're
>> still using COMSPEC even when given a list.
>
>
> Which is explained by this comment:
> <https://github.com/buildbot/buildbot/blob/v0.8.9/slave/buildslave/runprocess.py#L465>.
>
>>
>> Maybe a good way to start to get into this is to assemble a set of
>> desired behaviors (run an EXE, and pipe output between two EXEs might
>> be two of them), and then a set of existing configurations so we know
>> if and when we're breaking backward compatibliity (something like
>> ShellCommand('echo "hello" | NUL')?).
>
>
> IMO, the only sane behavior is:
>
> 1) When a list is passed, the elements of the list are used as the command's
> argv;
> 2) When a string is passed, it's executed via the shell.
>
> This is already the case on Unix-likes, and is almost the case on Windows,
> except for the titular exception: if you do ShellCommand(['a', '|', 'b']),
> instead of calling 'a' with arguments '|' and 'b', it creates a pipe between
> 'a' and 'b'. This is the only case I want to change.
>
>> That would help to define the
>> problem we're trying to solve.  Once that's done, if switching from
>> `spawnProcess` to `subprocess.Popen()` gets us more behaviors with
>> minimal impact to compatibility, great!  If dumping the command into
>> $temp.bat and running that batch file is better, then let's do that.
>
>
> From briefly experimenting with subprocess, it looks like it escapes well,
> and it also searches for the program in %PATH% (so the workaround with the
> batch file isn't needed). However, I'm not familiar with Twisted; perhaps
> spawnProcess has some advantage that I don't know about? Otherwise, why
> would it be necessary in the first place?
>
> One thing to note about this is that using subprocess will also remove the
> pipe exception.
>
>>>> Couldn't you pass "^|" explicitly?
>
>
> It would become ^^^| in the batch file and then unescaped back to ^|.
>
> Roman.