Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2930773003:
Closure compilation: Modify compile_js2.py to not swallow errors. (Closed)
DescriptionClosure compilation: Modify compile_js2.py to not swallow errors.
Previously certain types of errors would not cause compile_js2.py to exit
with an error code. Only type-checking related errors would trigger an
error code, for example a InvalidOptionsException would erroneously not
be considered an error.
BUG=730866
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2930773003
Cr-Commit-Position: refs/heads/master@{#478463}
Committed: https://chromium.googlesource.com/chromium/src/+/9a670d56a08acfc570ce624a52f80237c2552e7c
Patch Set 1 #Patch Set 2 : Respect error code #Patch Set 3 : More changes. #
Total comments: 1
Patch Set 4 : Smaller diff. #
Total comments: 3
Patch Set 5 : Don't write to file on failure. #Messages
Total messages: 26 (20 generated)
Description was changed from ========== DO NOT SUBMIT, compile_js2.py swallows some errors. BUG= ========== to ========== DO NOT SUBMIT, compile_js2.py swallows some errors. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== DO NOT SUBMIT, compile_js2.py swallows some errors. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== DO NOT SUBMIT, compile_js2.py swallows some errors. Execute ./third_party/closure_compiler/run_compiler search_settings Observe errors in verbose output, but run_compiler thinks it succeeded. Re-running the command results in "nothing to do". BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== DO NOT SUBMIT, compile_js2.py swallows some errors. Execute ./third_party/closure_compiler/run_compiler search_settings Observe errors in verbose output, but run_compiler thinks it succeeded. Re-running the command results in "nothing to do". BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Closure compilation: Modify compile_js2.py to always respect exit code. Previously certain types of errors would not cause compile_js2.py to exit with an error code. Only type-checking related errors would trigger an error code, for example a InvalidOptionsException would erroneously not be considered an error. BUG=730866 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Closure compilation: Modify compile_js2.py to always respect exit code. Previously certain types of errors would not cause compile_js2.py to exit with an error code. Only type-checking related errors would trigger an error code, for example a InvalidOptionsException would erroneously not be considered an error. BUG=730866 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Closure compilation: Modify compile_js2.py to not swallow errors. Previously certain types of errors would not cause compile_js2.py to exit with an error code. Only type-checking related errors would trigger an error code, for example a InvalidOptionsException would erroneously not be considered an error. BUG=730866 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2930773003/diff/40001/third_party/closure_com... File third_party/closure_compiler/compile2.py (right): https://codereview.chromium.org/2930773003/diff/40001/third_party/closure_com... third_party/closure_compiler/compile2.py:287: return return_code, bool(errors), stderr An alternative to adding a new return variable, is to do something like return bool(errors) or return_code > 0, stderr Let me know if you think that's better (simpler, cleaner etc).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2930773003/diff/80001/third_party/closure_com... File third_party/closure_compiler/compile2.py (right): https://codereview.chromium.org/2930773003/diff/80001/third_party/closure_com... third_party/closure_compiler/compile2.py:270: f.write('') this still writes a file in the event of an error, do you want that?
https://codereview.chromium.org/2930773003/diff/80001/third_party/closure_com... File third_party/closure_compiler/compile2.py (right): https://codereview.chromium.org/2930773003/diff/80001/third_party/closure_com... third_party/closure_compiler/compile2.py:270: f.write('') On 2017/06/09 at 03:10:39, Dan Beam wrote: > this still writes a file in the event of an error, do you want that? I guess not. Added an extra condition. Having said that, even with writing a file when return_code indicated a failure, re-running run_compiler re-triggers compilation as expected, which I can't fully explain.
lgtm https://codereview.chromium.org/2930773003/diff/80001/third_party/closure_com... File third_party/closure_compiler/compile2.py (right): https://codereview.chromium.org/2930773003/diff/80001/third_party/closure_com... third_party/closure_compiler/compile2.py:270: f.write('') On 2017/06/09 22:50:35, dpapad wrote: > On 2017/06/09 at 03:10:39, Dan Beam wrote: > > this still writes a file in the event of an error, do you want that? > > I guess not. Added an extra condition. Having said that, even with writing a > file when return_code indicated a failure, re-running run_compiler re-triggers > compilation as expected, which I can't fully explain. I'd guess that ninja doesn't stamp with a non-zero exit code
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497050427401140,
"parent_rev": "8b6b2938ac9f3a090d2cf8bafbe8ca73dcbf7945", "commit_rev":
"6c8993f34702f4721ff706e5403600e74be5ae4c"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497050427401140,
"parent_rev": "3defd8d25abf15d8e5ff7963323b269347c2b0cc", "commit_rev":
"9a670d56a08acfc570ce624a52f80237c2552e7c"}
Message was sent while issue was closed.
Description was changed from ========== Closure compilation: Modify compile_js2.py to not swallow errors. Previously certain types of errors would not cause compile_js2.py to exit with an error code. Only type-checking related errors would trigger an error code, for example a InvalidOptionsException would erroneously not be considered an error. BUG=730866 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Closure compilation: Modify compile_js2.py to not swallow errors. Previously certain types of errors would not cause compile_js2.py to exit with an error code. Only type-checking related errors would trigger an error code, for example a InvalidOptionsException would erroneously not be considered an error. BUG=730866 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2930773003 Cr-Commit-Position: refs/heads/master@{#478463} Committed: https://chromium.googlesource.com/chromium/src/+/9a670d56a08acfc570ce624a52f8... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/9a670d56a08acfc570ce624a52f8... |
