Fix Closure Compiler's compile.py error checking to match compile2.py.

Fix Closure Compiler's compile.py error checking to match compile2.py.

Without this change compile.py will fail when there are no errors

Committed: https://crrev.com/8275e261f1d65af32665ea90585ded450b48dfb8
Cr-Commit-Position: refs/heads/master@{#413859}

[doloffd, 2016-08-23 19:16:58]

Description was changed from

==========
Fix Closure Compiler's compile.py error checking to match compile2.py.

Without this change compile.py will fail when there are no errors
==========

to

==========
Fix Closure Compiler's compile.py error checking to match compile2.py.

Without this change compile.py will fail when there are no errors
==========

[doloffd, 2016-08-23 19:16:58]

doloffd@amazon.com changed reviewers:
+ tbreisacher@chromium.org

[doloffd, 2016-08-23 19:18:27]

PTAL. This change stops compile.py from printing an error message when there are
no errors.

[Tyler Breisacher (Chromium), 2016-08-23 19:21:39]

Doing regex matches on the compiler output is error-prone. You may prefer the
--summary_detail_level flag:

https://github.com/google/closure-compiler/blob/master/src/com/google/javascr...

[doloffd, 2016-08-23 21:01:16]

https://codereview.chromium.org/2273813002/diff/20001/third_party/closure_com...
File third_party/closure_compiler/compile2.py (right):

https://codereview.chromium.org/2273813002/diff/20001/third_party/closure_com...
third_party/closure_compiler/compile2.py:284: if summary.group('error_count') !=
"0" and out_file:
Why does this check for errors if the script was supposed to bail out by this
point? I didn't want to remove something I don't understand, but it seems
strange to me and looks like dead code.

[Tyler Breisacher (Chromium), 2016-08-23 21:08:06]

lgtm

I'm not sure but it seems likely you're right

[doloffd, 2016-08-23 21:16:22]

The CQ bit was checked by doloffd@amazon.com

[commit-bot: I haz the power, 2016-08-23 21:17:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 22:18:54]

Description was changed from

==========
Fix Closure Compiler's compile.py error checking to match compile2.py.

Without this change compile.py will fail when there are no errors
==========

to

==========
Fix Closure Compiler's compile.py error checking to match compile2.py.

Without this change compile.py will fail when there are no errors
==========

[commit-bot: I haz the power, 2016-08-23 22:18:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-23 22:21:59]

Description was changed from

==========
Fix Closure Compiler's compile.py error checking to match compile2.py.

Without this change compile.py will fail when there are no errors
==========

to

==========
Fix Closure Compiler's compile.py error checking to match compile2.py.

Without this change compile.py will fail when there are no errors

Committed: https://crrev.com/8275e261f1d65af32665ea90585ded450b48dfb8
Cr-Commit-Position: refs/heads/master@{#413859}
==========

[commit-bot: I haz the power, 2016-08-23 22:22:00]

Patchset 2 (id:??) landed as
https://crrev.com/8275e261f1d65af32665ea90585ded450b48dfb8
Cr-Commit-Position: refs/heads/master@{#413859}

[Dan Beam, 2016-08-23 22:38:34]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2273923002/ by dbeam@chromium.org.

The reason for reverting is:
https://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Li....

[doloffd, 2016-09-01 21:12:40]

Patchset #3 (id:40001) has been deleted

[doloffd, 2016-09-01 21:31:06]

Description was changed from

==========
Fix Closure Compiler's compile.py error checking to match compile2.py.

Without this change compile.py will fail when there are no errors

Committed: https://crrev.com/8275e261f1d65af32665ea90585ded450b48dfb8
Cr-Commit-Position: refs/heads/master@{#413859}
==========

to

==========
Fix Closure Compiler's compile.py error checking to match compile2.py.

Without this change compile.py will fail when there are no errors

Committed: https://crrev.com/8275e261f1d65af32665ea90585ded450b48dfb8
Cr-Commit-Position: refs/heads/master@{#413859}
==========

[doloffd, 2016-09-01 23:52:41]

I have fixed the tests that were broken when it landed before. The code will
still sys.exit(1) if there is an error, but only after returning from the
function under test.

[doloffd, 2016-09-12 20:58:12]

doloffd@amazon.com changed reviewers:
+ dbeam@chromium.org

[doloffd, 2016-09-19 16:18:44]

Could one of you take another look at my change?

[Tyler Breisacher (Chromium), 2016-09-19 17:17:29]

On 2016/09/19 16:18:44, doloffd wrote:
> Could one of you take another look at my change?

I think you need to create a new CL since this one was already submitted.

[Dan Beam, 2016-09-19 17:36:50]

you don't really need to create a new issue (you can keep uploading to this
one), but you need to merge in newer master revs.  this diff looks like it's
based on your reverted CL.  in order for your diff to apply cleaning, it needs
to be based (more-or-less) on origin/master HEAD.

[doloffd, 2016-09-21 20:03:56]

I have rebased and now all the diffs look correct

[Dan Beam, 2016-09-21 21:13:48]

what bug is this trying to fix?  is there a bug filed on crbug.com?

if i remember correctly, the return code of the compiler can be != 0 in the case
that EITHER the jar fails to load or the compiler hits typechecking errors. 
it's good to disambiguate those because we want the script to continue if it's
just a typechecking error, but we don't really have incentive to continue if the
jar's not running correctly.

https://codereview.chromium.org/2273813002/diff/70001/third_party/closure_com...
File third_party/closure_compiler/compile2.py (right):

https://codereview.chromium.org/2273813002/diff/70001/third_party/closure_com...
third_party/closure_compiler/compile2.py:281: summary =
re.search("(?P<error_count>\d+).*error.*warning", maybe_summary)
can't this return None?

[doloffd, 2016-09-22 17:58:02]

On 2016/09/21 21:13:48, Dan Beam wrote:
> what bug is this trying to fix?  is there a bug filed on crbug.com?

Running compile.py on a valid file would result in the build failing due to
incorrect assumptions about what the output looked like. This was broken during
a roll of the closure compiler. Unfortunately, nothing in Chromium currently
uses the closure compiler so this breakage was not detected as part of the roll.

I didn't file a bug on crbug.com.

> if i remember correctly, the return code of the compiler can be != 0 in the
case
> that EITHER the jar fails to load or the compiler hits typechecking errors. 
> it's good to disambiguate those because we want the script to continue if it's
> just a typechecking error, but we don't really have incentive to continue if
the
> jar's not running correctly.

I don't believe that's part of the functionality of the scripts right now. The
only way I can think to do this would be to add an argument to the script that
says whether to fail on errors or not. Is that what you want? I thought the idea
was to break the build on typechecking errors.

>
https://codereview.chromium.org/2273813002/diff/70001/third_party/closure_com...
> File third_party/closure_compiler/compile2.py (right):
> 
>
https://codereview.chromium.org/2273813002/diff/70001/third_party/closure_com...
> third_party/closure_compiler/compile2.py:281: summary =
> re.search("(?P<error_count>\d+).*error.*warning", maybe_summary)
> can't this return None?

Yes. I can can add a check for that case

[Dan Beam, 2016-09-22 19:55:28]

On 2016/09/22 17:58:02, doloffd wrote:
> On 2016/09/21 21:13:48, Dan Beam wrote:
> > what bug is this trying to fix?  is there a bug filed on crbug.com?
> 
> Running compile.py on a valid file would result in the build failing due to
> incorrect assumptions about what the output looked like. This was broken
during
> a roll of the closure compiler. Unfortunately, nothing in Chromium currently
> uses the closure compiler so this breakage was not detected as part of the
roll.

I assume you mean nothing in Chrome uses closure compiler from HEAD, right?

We run closure compiler continuously on pretty much every change, and often
before people are allowed to change the code.

https://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compil...
https://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Linux

I hardcoded the "summary_detail_level=3" into both compile*.py scripts to ensure
the output is always there, but you're saying the format of that output has
changed?

> 
> I didn't file a bug on http://crbug.com.
> 
> > if i remember correctly, the return code of the compiler can be != 0 in the
> case
> > that EITHER the jar fails to load or the compiler hits typechecking errors. 
> > it's good to disambiguate those because we want the script to continue if
it's
> > just a typechecking error, but we don't really have incentive to continue if
> the
> > jar's not running correctly.
> 
> I don't believe that's part of the functionality of the scripts right now. The
> only way I can think to do this would be to add an argument to the script that
> says whether to fail on errors or not. Is that what you want? I thought the
idea
> was to break the build on typechecking errors.

The purpose of the sys.exit(1) was to ensure that using the wrong Java version
(or a incorrectly built .jar) would fail.  We were previously having issues with
that.

Also, I think it's possible the exit code is >0 if warnings happen, but I don't
remember for sure.

> 
> >
>
https://codereview.chromium.org/2273813002/diff/70001/third_party/closure_com...
> > File third_party/closure_compiler/compile2.py (right):
> > 
> >
>
https://codereview.chromium.org/2273813002/diff/70001/third_party/closure_com...
> > third_party/closure_compiler/compile2.py:281: summary =
> > re.search("(?P<error_count>\d+).*error.*warning", maybe_summary)
> > can't this return None?
> 
> Yes. I can can add a check for that case

[bhallam, 2016-10-22 00:17:08]

bhallam@amazon.com changed reviewers:
+ bhallam@amazon.com

[bhallam, 2016-10-22 00:17:09]

Hi!
I am taking over the CL for doloffd@.

Please look at https://codereview.chromium.org/2440123003

Thanks
bhallam@