Parse output from xcodebuild to make it easier to spot problems with non-incremental builds.

Parse output from xcodebuild to make it easier to spot problems with non-incremental builds.

Committed: https://code.google.com/p/dart/source/detail?r=20975

[kustermann, 2013-04-02 21:13:15]

lgtm

https://codereview.chromium.org/13470019/diff/1/dart/tools/build.py
File dart/tools/build.py (right):

https://codereview.chromium.org/13470019/diff/1/dart/tools/build.py#newcode231
dart/tools/build.py:231: """
A small comment explaining the 'output format' of xcodebuild would be nice here.

https://codereview.chromium.org/13470019/diff/1/dart/tools/build.py#newcode238
dart/tools/build.py:238: bufsize=0,
I think 'bufsize=0' is the default -- you could remove this.

Why do you make it unbuffered. Shouldn't it be line buffered (i.e. bufsize=1)?

https://codereview.chromium.org/13470019/diff/1/dart/tools/build.py#newcode277
dart/tools/build.py:277: parse_xcodebuild = False
The intention is not to parse the output of xcodebuild but rather to filter
empty sections. So you could rename it to 'filter_xcodebuild_output'.

https://codereview.chromium.org/13470019/diff/1/dart/tools/build.py#newcode358
dart/tools/build.py:358: process = subprocess.Popen(args, stdin=None)
You could do something like this:

if filter_xcodebuild_output:
  process = subprocess.Popen(args,
                                               stdin=None,
                                               stdout=subprocess.PIPE,  
                                               stderr=subprocess.STDOUT)
  FilterEmptyXcodebuildSections(process.stdout)
else:
  process = subprocess.Popen(args, stdin=None)

This way the subprocess calls are near to each other and
'FilterEmptyXcodebuildSections' has one responsibility (filtering empty
sections) instead of two.

[ahe, 2013-04-05 15:03:43]

PTAL

https://codereview.chromium.org/13470019/diff/1/dart/tools/build.py
File dart/tools/build.py (right):

https://codereview.chromium.org/13470019/diff/1/dart/tools/build.py#newcode231
dart/tools/build.py:231: """
On 2013/04/02 21:13:15, kustermann wrote:
> A small comment explaining the 'output format' of xcodebuild would be nice
here.

Done.

https://codereview.chromium.org/13470019/diff/1/dart/tools/build.py#newcode238
dart/tools/build.py:238: bufsize=0,
On 2013/04/02 21:13:15, kustermann wrote:
> I think 'bufsize=0' is the default -- you could remove this.
> 
> Why do you make it unbuffered. Shouldn't it be line buffered (i.e. bufsize=1)?

Yes.  In addition it turns out that "for line in process.stdout" has its own
buffer.  I found a way to disable that.

https://codereview.chromium.org/13470019/diff/1/dart/tools/build.py#newcode277
dart/tools/build.py:277: parse_xcodebuild = False
On 2013/04/02 21:13:15, kustermann wrote:
> The intention is not to parse the output of xcodebuild but rather to filter
> empty sections. So you could rename it to 'filter_xcodebuild_output'.

Done.

https://codereview.chromium.org/13470019/diff/1/dart/tools/build.py#newcode358
dart/tools/build.py:358: process = subprocess.Popen(args, stdin=None)
On 2013/04/02 21:13:15, kustermann wrote:
> You could do something like this:
> 
> if filter_xcodebuild_output:
>   process = subprocess.Popen(args,
>                                                stdin=None,
>                                                stdout=subprocess.PIPE,  
>                                                stderr=subprocess.STDOUT)
>   FilterEmptyXcodebuildSections(process.stdout)
> else:
>   process = subprocess.Popen(args, stdin=None)
> 
> This way the subprocess calls are near to each other and
> 'FilterEmptyXcodebuildSections' has one responsibility (filtering empty
> sections) instead of two.

Done.

[kustermann, 2013-04-05 15:31:01]

lgtm

https://codereview.chromium.org/13470019/diff/8001/dart/tools/build.py
File dart/tools/build.py (right):

https://codereview.chromium.org/13470019/diff/8001/dart/tools/build.py#newcod...
dart/tools/build.py:236: === BUILD AGGREGATE TARGET samples OF PROJECT dart WITH
CONFIGURATION ReleaseIA32 ===
You could abbreviate these lines with '...' so we don't hit the 80 character
limit.

https://codereview.chromium.org/13470019/diff/8001/dart/tools/build.py#newcod...
dart/tools/build.py:258: return iter(callable, b'')
The 'b' should not be necessary here.

[ahe, 2013-04-05 15:45:44]

Thank you, Martin!

https://codereview.chromium.org/13470019/diff/8001/dart/tools/build.py
File dart/tools/build.py (right):

https://codereview.chromium.org/13470019/diff/8001/dart/tools/build.py#newcod...
dart/tools/build.py:236: === BUILD AGGREGATE TARGET samples OF PROJECT dart WITH
CONFIGURATION ReleaseIA32 ===
On 2013/04/05 15:31:01, kustermann wrote:
> You could abbreviate these lines with '...' so we don't hit the 80 character
> limit.

Done.

https://codereview.chromium.org/13470019/diff/8001/dart/tools/build.py#newcod...
dart/tools/build.py:258: return iter(callable, b'')
On 2013/04/05 15:31:01, kustermann wrote:
> The 'b' should not be necessary here.

Done.

[ahe, 2013-04-05 15:46:47]

Committed patchset #3 manually as r20975 (presubmit successful).

[Ivan Posva, 2013-04-05 20:21:27]

I really like this change. Except for the little quirk noted below.

-Ivan

https://codereview.chromium.org/13470019/diff/9002/dart/tools/build.py
File dart/tools/build.py (right):

https://codereview.chromium.org/13470019/diff/9002/dart/tools/build.py#newcod...
dart/tools/build.py:272: print '%s[2K%s\r' % (chr(27), section),
I think this is leading to lines like this on the buildbot:
=== BUILD AGGREGATE TARGET generate_async_cc_file OF PROJECT dart-runtime
WITH CONFIGURATION ReleaseIA32 ===

On another note: It looks like on my local build the filteres lines are
truncated to the terminal width. And the output gets really unhappy if the size
of the terminal window changes.

[ahe, 2013-04-08 14:09:02]

https://codereview.chromium.org/13470019/diff/9002/dart/tools/build.py
File dart/tools/build.py (right):

https://codereview.chromium.org/13470019/diff/9002/dart/tools/build.py#newcod...
dart/tools/build.py:272: print '%s[2K%s\r' % (chr(27), section),
On 2013/04/05 20:21:27, Ivan Posva wrote:
> I think this is leading to lines like this on the buildbot:
> === BUILD AGGREGATE TARGET generate_async_cc_file OF PROJECT dart-runtime
> WITH CONFIGURATION ReleaseIA32 ===
> 
> On another note: It looks like on my local build the filteres lines are
> truncated to the terminal width. And the output gets really unhappy if the
size
> of the terminal window changes.

I believe I have fixed both issues.

Cheers,
Peter