366607 - MozBuild::Util::RunShellCommand needs some work

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[J. Paul Reed [:preed]]

MozBuild::Util::RunShellCommand has a couple of deficiencies that it would be nice to have:

* The way shell arguments are handled is... bad. If any of the arguments have escaped characters (spaces in file names, mostly), then you have to deal with this in the caller by quoting your arguments. What we really need is the ability to pass an array of arguments, and each element can have spaces/escaped charaters that will correctly make it into the program's ARGC.

* We sorta do a hand wavy thing handling stdout/stderr. Right now, the callers will often do "2>&1" in the command. This should be a passed in option, and should be handled correctly.

* As a part of that, we should handle these output streams separately, so we can do useful things with them and keep them separate.

* It should be possible to "background" a command, and check on it periodically outside of RunShellCommand.

* Some of the return semantics don't make sense; we'd return a PID of a dead process, which isn't useful. We'd return a -1 if we timed out, which arguably isn't correct (the comment says callers expect it, but callers should check timedOut if they want to know if it timedOut.

* We always append to the given logfile; that should be an option.

Comment 1

[J. Paul Reed [:preed]]

Attachment: Address some of these issues

Comment 2

[J. Paul Reed [:preed]]

Attachment: Address these issues, v2

IO::Handle->getline() can return undef if there's nothing to read (in the case of stderr); so, don't append that to output, logfiles, and the screen if there's nothing to output.

Also, a couple of variable name typo fixes (perl -c now works ;-)

Comment 3

[Benjamin Smedberg]

Comment on attachment 251115 [details] [diff] [review]
Address these issues, v2

>Index: Util.pm

>+        my @commandParts = split(/\s+/, $shellCommand);

Boy, does that feel wrong. oh well

Comment 4

[J. Paul Reed [:preed]]

Attachment: v3

Like v3, except... performant.

The problem with v2 is that because the streams are in non-blocking mode, on long running commands, all the getline() calls will cause perl to spin, waiting for input; there's no sleep() or anything else to slow this down, so we hammer the CPU.

This is bad.

This version fixes that by using the IO::Select module to do this more intelligently. Also did a few optimizations around the case where we throw stderr away.

This version also includes some comments plus some *gasp* documentation up top about the features.

Comment 5

[J. Paul Reed [:preed]]

Comment on attachment 251253 [details] [diff] [review]
v3

Benjamin: Sorry to make you look again, but the performance of v2 was awful; general concept, of course, is the same, but v3 is actually usable in a tinderbox scenario.

Comment 6

[Robert Helmer [:rhelmer]]

Comment on attachment 251253 [details] [diff] [review]
v3

>+    # This is a compatibility check for the old calling convention; if we
>+    # find spaces in the command, turn it into the proper command [ args]-type
>+    # call. This will break callers that "escape" their args by quoting them,
>+    # i.e. foo "bar baz" buh, expecting the args to foo to be "bar baz" and
>+    # "buh". They will turn out to be "\"bar", "baz\" and "buh". These callers
>+    # just need to be fixed.
>+    if ($shellCommand =~ /\s/) {
>+        $shellCommand =~ s/^\s+//;
>+        $shellCommand =~ s/\s+$//;
>+
>+        my @commandParts = split(/\s+/, $shellCommand);
>+
>+        $shellCommand = shift(@commandParts);
>+        if (defined($commandArgs)) {
>+            push(@{$commandArgs}, @commandParts);
>+        } else {
>+            $commandArgs = \@commandParts;
>+        }
>+    }

I wouldn't bother with this; just make callers use the new calling convention (passing an array instead of a string). Having callers be subtly broken will be much more annoying to track down than just fixing them to use arrays. How many apps use this class? Just bootstrap right now right?

Looks good besides that.

Comment 7

[J. Paul Reed [:preed]]

(In reply to comment #6)

> I wouldn't bother with this; just make callers use the new calling convention
> (passing an array instead of a string). Having callers be subtly broken will be
> much more annoying to track down than just fixing them to use arrays. How many
> apps use this class? Just bootstrap right now right?

Ok; that makes it easier for me. :-)

How about something like this, to help out old callers:

if ($shellCommand =~ /\s/) {
       $shellCommand =~ s/^\s+//;
       $shellCommand =~ s/\s+$//;

       if ($shellCommand =~ /\s/) {
           die 'ASSERT: RunShellCommand(): old calling convention detected';
       }
}

Comment 8

[J. Paul Reed [:preed]]

Checking in Util.pm;
/cvsroot/mozilla/tools/release/MozBuild/Util.pm,v  <--  Util.pm
new revision: 1.7; previous revision: 1.6
done