Output better error messages during build

Output better error messages during build

The python build scripts use subprocess.check_call() for external
commands (aapt, javac, etc). On a failure, it prints the python
stacktrace, followed by the command being run (in a form that can be
copy-pasted and actually work), followed by the output from the command.

Previously, it was printing command output, then command, then
stacktrace which is much less useful.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191300

[cjhopman, 2013-03-28 00:53:27]

What do you guys think? The output is much nicer, but the function is kind of
ugly (and I don't really like that it calls sys.exit).

[newt (away), 2013-03-28 03:26:50]

I like this.  javac failures are currently quite messy, and this should help a
lot.

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
File build/android/pylib/build_utils.py (right):

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:67: stdout=subprocess.PIPE,
stderr=subprocess.PIPE, cwd=cwd)
How about:

child = subprocess.Popen(args,
    stdout=subprocess.PIPE, stderr=subprocess.STDOUT, cwd=cwd)

Then stdout and stderr are interleaved.  You lose the ability to distinguish
between the two... though you don't seem to be using that below.

alternatively, for long-running commands, it would be nice to print the output
as the command runs.  but then it would be duplicated when you print it below.

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:74: print ' '.join(traceback.format_stack())
use '' instead of ' '

use sys.stderr.write instead of print?

also: might it be useful to exclude the top entry from the stack trace, since
the last two lines will always be

  File "build_utils.py", line 74, in CheckCall
    print ' '.join(traceback.format_stack())

and the interesting stuff will be the preceding lines.

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:78: copyable_command = ('( cd ' +
os.path.abspath(cwd) + '; '
why parens in the command?

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:88: sys.exit(child.returncode)
instead of sys.exit(), you could redirect stdout and stderr to /dev/null and
then raise an exception. I'm not suggesting that though :)

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:93: if stderr:
don't need this if stderr > stdout

[Isaac (away), 2013-03-28 06:54:25]

Drive by comment.  This looks good, though!

Check out depot_tools/subprocess2 if you have any questions about impl details.

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
File build/android/pylib/build_utils.py (right):

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:62: def CheckCall(args, cwd=None):
Could we name this something more strongly worded like CheckCallDie to make it
clear it will terminate the calling process?

[cjhopman, 2013-03-28 23:30:20]

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
File build/android/pylib/build_utils.py (right):

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:62: def CheckCall(args, cwd=None):
On 2013/03/28 06:54:25, Isaac wrote:
> Could we name this something more strongly worded like CheckCallDie to make it
> clear it will terminate the calling process?

Done.

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:67: stdout=subprocess.PIPE,
stderr=subprocess.PIPE, cwd=cwd)
On 2013/03/28 03:26:50, newt wrote:
> How about:
> 
> child = subprocess.Popen(args,
>     stdout=subprocess.PIPE, stderr=subprocess.STDOUT, cwd=cwd)
> 
> Then stdout and stderr are interleaved.  You lose the ability to distinguish
> between the two... though you don't seem to be using that below.

Done. I didn't know you could do that.

> 
> alternatively, for long-running commands, it would be nice to print the output
> as the command runs.  but then it would be duplicated when you print it below.


Ninja already intercepts a command's output and only prints it when the command
is finished, so it's not really worth worrying about buffering it like this.

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:74: print ' '.join(traceback.format_stack())
On 2013/03/28 03:26:50, newt wrote:
> use '' instead of ' '
Done.
> 
> use sys.stderr.write instead of print?
Done. Though ninja sends it to stdout anyway -- it probably does something like
you suggested for interleaving a subprocesses output :).

> 
> also: might it be useful to exclude the top entry from the stack trace, since
> the last two lines will always be
> 
>   File "build_utils.py", line 74, in CheckCall
>     print ' '.join(traceback.format_stack())
> 
> and the interesting stuff will be the preceding lines.
Yeah, I considered this. I like that including this stack frame makes it obvious
what function failed (CheckCallDie). I extracted the extract_stack() call to its
own line so it's more obvious what that stackframe is about.

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:78: copyable_command = ('( cd ' +
os.path.abspath(cwd) + '; '
On 2013/03/28 03:26:50, newt wrote:
> why parens in the command?

Those are so that when I copy-paste it to run it, it does so in a subshell and
the 'cd' doesn't change my directory.

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:88: sys.exit(child.returncode)
On 2013/03/28 03:26:50, newt wrote:
> instead of sys.exit(), you could redirect stdout and stderr to /dev/null and
> then raise an exception. I'm not suggesting that though :)

Ha, I actually considered that but it felt a bit too magical.

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:93: if stderr:
On 2013/03/28 03:26:50, newt wrote:
> don't need this if stderr > stdout

Done.

[Yaron, 2013-03-28 23:53:10]

lgtm

https://codereview.chromium.org/12913028/diff/27002/build/android/process_res...
File build/android/process_resources.py (right):

https://codereview.chromium.org/12913028/diff/27002/build/android/process_res...
build/android/process_resources.py:88: 'crunch',
Nit: fix indent

https://codereview.chromium.org/12913028/diff/27002/build/android/pylib/build...
File build/android/pylib/build_utils.py (right):

https://codereview.chromium.org/12913028/diff/27002/build/android/pylib/build...
build/android/pylib/build_utils.py:69: (stdout, _) = child.communicate()
no parens needed

[newt (away), 2013-03-29 00:20:07]

lgtm

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
File build/android/pylib/build_utils.py (right):

https://codereview.chromium.org/12913028/diff/2001/build/android/pylib/build_...
build/android/pylib/build_utils.py:78: copyable_command = ('( cd ' +
os.path.abspath(cwd) + '; '
On 2013/03/28 23:30:20, cjhopman wrote:
> On 2013/03/28 03:26:50, newt wrote:
> > why parens in the command?
> 
> Those are so that when I copy-paste it to run it, it does so in a subshell and
> the 'cd' doesn't change my directory.

ah, makes sense

https://codereview.chromium.org/12913028/diff/27002/build/android/pylib/build...
File build/android/pylib/build_utils.py (right):

https://codereview.chromium.org/12913028/diff/27002/build/android/pylib/build...
build/android/pylib/build_utils.py:79: print >> sys.stderr, 'Command failed: ',
copyable_command, '\n'
print adds a space between each argument, so you could change 'Command failed: '
-> 'Command failed:'

[cjhopman, 2013-03-29 00:39:50]

https://codereview.chromium.org/12913028/diff/27002/build/android/process_res...
File build/android/process_resources.py (right):

https://codereview.chromium.org/12913028/diff/27002/build/android/process_res...
build/android/process_resources.py:88: 'crunch',
On 2013/03/28 23:53:10, Yaron wrote:
> Nit: fix indent

Done.

https://codereview.chromium.org/12913028/diff/27002/build/android/pylib/build...
File build/android/pylib/build_utils.py (right):

https://codereview.chromium.org/12913028/diff/27002/build/android/pylib/build...
build/android/pylib/build_utils.py:69: (stdout, _) = child.communicate()
On 2013/03/28 23:53:10, Yaron wrote:
> no parens needed

Done.

https://codereview.chromium.org/12913028/diff/27002/build/android/pylib/build...
build/android/pylib/build_utils.py:79: print >> sys.stderr, 'Command failed: ',
copyable_command, '\n'
On 2013/03/29 00:20:07, newt wrote:
> print adds a space between each argument, so you could change 'Command failed:
'
> -> 'Command failed:'

Done.

[commit-bot: I haz the power, 2013-03-29 00:40:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cjhopman@chromium.org/12913028/39001

[commit-bot: I haz the power, 2013-03-29 05:06:36]

Change committed as 191300