Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2354263003:
[inspector] add presubmit.py to compile inspector-related scripts (Closed)
Description[inspector] add presubmit.py to compile inspector-related scripts
BUG=chromium:635948
R=dgozman@chromium.org,alph@chromium.org
Committed: https://crrev.com/e9ceb376e4a7f796544707d148bb62aa68fe8bc0
Cr-Commit-Position: refs/heads/master@{#39846}
Patch Set 1 #
Total comments: 8
Patch Set 2 : addressed comments #Patch Set 3 : a #
Total comments: 2
Patch Set 4 : addressed comments #
Total comments: 30
Patch Set 5 : addressed comments #
Messages
Total messages: 30 (17 generated)
Dmitry, please take a look.
https://codereview.chromium.org/2354263003/diff/1/src/inspector/PRESUBMIT.py File src/inspector/PRESUBMIT.py (right): https://codereview.chromium.org/2354263003/diff/1/src/inspector/PRESUBMIT.py#... src/inspector/PRESUBMIT.py:21: any("debugger_script_exters.js" in path for path in local_paths) or typo: exter_n_s https://codereview.chromium.org/2354263003/diff/1/src/inspector/PRESUBMIT.py#... src/inspector/PRESUBMIT.py:45: results.extend(_CompileScripts(input_api, output_api)) Running on commit requires bots to have java, right? I don't think we do that for blink. https://codereview.chromium.org/2354263003/diff/1/src/inspector/build/compile... File src/inspector/build/compile-scripts.py (right): https://codereview.chromium.org/2354263003/diff/1/src/inspector/build/compile... src/inspector/build/compile-scripts.py:102: closure_compiler_jar = path.join(v8_inspector_path, os.pardir, os.pardir, 'tools', 'closure-compiler', 'closure-compiler.jar') I thought we decided to fetch it locally into gitignore'd folder? https://codereview.chromium.org/2354263003/diff/1/src/inspector/debugger_scri... File src/inspector/debugger_script_externs.js (right): https://codereview.chromium.org/2354263003/diff/1/src/inspector/debugger_scri... src/inspector/debugger_script_externs.js:123: var ScriptCompilationType = { Host: 0, const?
Patchset #3 (id:40001) has been deleted
all done. please take a look! https://codereview.chromium.org/2354263003/diff/1/src/inspector/PRESUBMIT.py File src/inspector/PRESUBMIT.py (right): https://codereview.chromium.org/2354263003/diff/1/src/inspector/PRESUBMIT.py#... src/inspector/PRESUBMIT.py:21: any("debugger_script_exters.js" in path for path in local_paths) or On 2016/09/22 16:50:00, dgozman wrote: > typo: exter_n_s Done. https://codereview.chromium.org/2354263003/diff/1/src/inspector/PRESUBMIT.py#... src/inspector/PRESUBMIT.py:45: results.extend(_CompileScripts(input_api, output_api)) On 2016/09/22 16:50:00, dgozman wrote: > Running on commit requires bots to have java, right? I don't think we do that > for blink. Removed. https://codereview.chromium.org/2354263003/diff/1/src/inspector/build/compile... File src/inspector/build/compile-scripts.py (right): https://codereview.chromium.org/2354263003/diff/1/src/inspector/build/compile... src/inspector/build/compile-scripts.py:102: closure_compiler_jar = path.join(v8_inspector_path, os.pardir, os.pardir, 'tools', 'closure-compiler', 'closure-compiler.jar') On 2016/09/22 16:50:00, dgozman wrote: > I thought we decided to fetch it locally into gitignore'd folder? Done. https://codereview.chromium.org/2354263003/diff/1/src/inspector/debugger_scri... File src/inspector/debugger_script_externs.js (right): https://codereview.chromium.org/2354263003/diff/1/src/inspector/debugger_scri... src/inspector/debugger_script_externs.js:123: var ScriptCompilationType = { Host: 0, On 2016/09/22 16:50:00, dgozman wrote: > const? Done.
The CQ bit was checked by kozyatinskiy@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.
lgtm. Nice stuff! https://codereview.chromium.org/2354263003/diff/60001/src/inspector/build/com... File src/inspector/build/compile-scripts.py (right): https://codereview.chromium.org/2354263003/diff/60001/src/inspector/build/com... src/inspector/build/compile-scripts.py:21: scripts_path = path.dirname(path.abspath(__file__)) inline this
All done! https://codereview.chromium.org/2354263003/diff/60001/src/inspector/build/com... File src/inspector/build/compile-scripts.py (right): https://codereview.chromium.org/2354263003/diff/60001/src/inspector/build/com... src/inspector/build/compile-scripts.py:21: scripts_path = path.dirname(path.abspath(__file__)) On 2016/09/23 23:36:06, dgozman wrote: > inline this Done.
The CQ bit was checked by kozyatinskiy@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...
kozyatinskiy@chromium.org changed reviewers: + machenbach@chromium.org
Michael, please take a look! I need owner lgtm for presubmit.py.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with comments and suggestions. I didn't review js files nor files excluded from license checks as I assume they are copied from somewhere. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT.py File src/inspector/PRESUBMIT.py (right): https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:17: local_paths = [f.LocalPath() for f in input_api.AffectedFiles()] nit: two space indents. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:18: if (any("js_protocol.json" in path for path in local_paths) or nit: too much indentation. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:24: any("debugger-script.js" in path for path in local_paths)): Can you maybe rewrite this with less duplication? E.g. one flat list of files that gets processed by the same code? https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:25: lint_path = input_api.os_path.join(input_api.PresubmitLocalPath(), nit: Is lint an appropriate name in this context? https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:31: if "ERROR" in out or "WARNING" in out: How about also checking the return code of the process? Needs a local variable of the popen result. Not that it failed with an error and no output. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:45: results = [] Why not on commit? We'd like the CQ to actually ensure this, especially if folks bypass hooks on upload. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/che... File src/inspector/build/check_injected_script_source.py (right): https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/che... src/inspector/build/check_injected_script_source.py:1: #!/usr/bin/env python Could you add a note where those file are copied from and if you made any changes to the originals or if this is 1:1? https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... File src/inspector/build/compile-scripts.py (right): https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:3: # Copyright 2016 the V8 project authors. All rights reserved. nit: If this is not copied but a new file, please fix 80 char line limits in this file. We also usually have indentation of two spaces. The file is also inconsistent in python style names with _ and camel case names. We usually prefer _ style for new files. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:18: def popen(arguments): nit: move method down to other methods and don't mix with toplevel constants. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:23: protocol_externs_file = path.join(v8_inspector_path, 'protocol_externs.js') Is this robust to the file already existing? E.g. if the script bailed out early once or got Ctrl-C'ed and the file kept lying around? https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:63: required_major = 1 Maybe make these toplevel constants at the top of this script for better visibility? https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:101: closure_compiler_jar = path.join(v8_inspector_path, 'build', 'closure-compiler', 'closure-compiler.jar') nit: move constant up to other toplevel constants https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:140: print 'injected-script-source.js compilation output:%s' % os.linesep, injectedScriptCompileOut Does this work? One %s but two parameters? Thought this might need %s%s and parentheses around the two parameters. Also below. https://codereview.chromium.org/2354263003/diff/80001/tools/presubmit.py File tools/presubmit.py (right): https://codereview.chromium.org/2354263003/diff/80001/tools/presubmit.py#newc... tools/presubmit.py:508: success &= CheckStatusFiles(workspace) This script also runs continuously on our bots. If somebody breaks presubmit e.g. by submitting a CL with PRESUBMIT=false or manual commit or a bad revert, we want the bot to go red: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20presubmit Maybe add your check here too?
https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT.py File src/inspector/PRESUBMIT.py (right): https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:45: results = [] On 2016/09/28 09:54:05, machenbach (slow) wrote: > Why not on commit? We'd like the CQ to actually ensure this, especially if folks > bypass hooks on upload. AFAIU, commit hooks are run on bots, which require them to have Java. Our team member is working on enabling this for DevTools frontend on Chromium bots now, but it's not finished yet.
https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT.py File src/inspector/PRESUBMIT.py (right): https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:45: results = [] On 2016/09/28 13:50:32, dgozman wrote: > On 2016/09/28 09:54:05, machenbach (slow) wrote: > > Why not on commit? We'd like the CQ to actually ensure this, especially if > folks > > bypass hooks on upload. > > AFAIU, commit hooks are run on bots, which require them to have Java. Our team > member is working on enabling this for DevTools frontend on Chromium bots now, > but it's not finished yet. Just checked a random bot that runs our presubmit and it has java. And as I understood, this auto-detects if java is installed and just does nothing if it isn't?
The CQ bit was checked by kozyatinskiy@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...
Addressed all comments. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT.py File src/inspector/PRESUBMIT.py (right): https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:17: local_paths = [f.LocalPath() for f in input_api.AffectedFiles()] On 2016/09/28 09:54:04, machenbach (slow) wrote: > nit: two space indents. Done. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:18: if (any("js_protocol.json" in path for path in local_paths) or On 2016/09/28 09:54:04, machenbach (slow) wrote: > nit: too much indentation. Done. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:24: any("debugger-script.js" in path for path in local_paths)): On 2016/09/28 09:54:04, machenbach (slow) wrote: > Can you maybe rewrite this with less duplication? E.g. one flat list of files > that gets processed by the same code? Done. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:25: lint_path = input_api.os_path.join(input_api.PresubmitLocalPath(), On 2016/09/28 09:54:04, machenbach (slow) wrote: > nit: Is lint an appropriate name in this context? Done. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:31: if "ERROR" in out or "WARNING" in out: On 2016/09/28 09:54:04, machenbach (slow) wrote: > How about also checking the return code of the process? Needs a local variable > of the popen result. Not that it failed with an error and no output. Done. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/PRESUBMIT... src/inspector/PRESUBMIT.py:45: results = [] On 2016/09/28 14:27:27, machenbach (slow) wrote: > On 2016/09/28 13:50:32, dgozman wrote: > > On 2016/09/28 09:54:05, machenbach (slow) wrote: > > > Why not on commit? We'd like the CQ to actually ensure this, especially if > > folks > > > bypass hooks on upload. > > > > AFAIU, commit hooks are run on bots, which require them to have Java. Our team > > member is working on enabling this for DevTools frontend on Chromium bots now, > > but it's not finished yet. > > Just checked a random bot that runs our presubmit and it has java. And as I > understood, this auto-detects if java is installed and just does nothing if it > isn't? Added for commit. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/che... File src/inspector/build/check_injected_script_source.py (right): https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/che... src/inspector/build/check_injected_script_source.py:1: #!/usr/bin/env python On 2016/09/28 09:54:05, machenbach (slow) wrote: > Could you add a note where those file are copied from and if you made any > changes to the originals or if this is 1:1? I added a comment and will remove this as soon as script is removed from blink. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... File src/inspector/build/compile-scripts.py (right): https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:3: # Copyright 2016 the V8 project authors. All rights reserved. On 2016/09/28 09:54:05, machenbach (slow) wrote: > nit: If this is not copied but a new file, please fix 80 char line limits in > this file. We also usually have indentation of two spaces. The file is also > inconsistent in python style names with _ and camel case names. We usually > prefer _ style for new files. Done. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:18: def popen(arguments): On 2016/09/28 09:54:05, machenbach (slow) wrote: > nit: move method down to other methods and don't mix with toplevel constants. Done. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:23: protocol_externs_file = path.join(v8_inspector_path, 'protocol_externs.js') On 2016/09/28 09:54:05, machenbach (slow) wrote: > Is this robust to the file already existing? E.g. if the script bailed out early > once or got Ctrl-C'ed and the file kept lying around? Yes, it is. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:63: required_major = 1 On 2016/09/28 09:54:05, machenbach (slow) wrote: > Maybe make these toplevel constants at the top of this script for better > visibility? Done. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:101: closure_compiler_jar = path.join(v8_inspector_path, 'build', 'closure-compiler', 'closure-compiler.jar') On 2016/09/28 09:54:05, machenbach (slow) wrote: > nit: move constant up to other toplevel constants Done. https://codereview.chromium.org/2354263003/diff/80001/src/inspector/build/com... src/inspector/build/compile-scripts.py:140: print 'injected-script-source.js compilation output:%s' % os.linesep, injectedScriptCompileOut On 2016/09/28 09:54:05, machenbach (slow) wrote: > Does this work? One %s but two parameters? Thought this might need %s%s and > parentheses around the two parameters. Also below. It print first formatted string and then second string without any formatting. Split into to print. https://codereview.chromium.org/2354263003/diff/80001/tools/presubmit.py File tools/presubmit.py (right): https://codereview.chromium.org/2354263003/diff/80001/tools/presubmit.py#newc... tools/presubmit.py:508: success &= CheckStatusFiles(workspace) On 2016/09/28 09:54:05, machenbach (slow) wrote: > This script also runs continuously on our bots. If somebody breaks presubmit > e.g. by submitting a CL with PRESUBMIT=false or manual commit or a bad revert, > we want the bot to go red: > https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20presubmit > > Maybe add your check here too? SGTM. I'll add it later after testing of PRESUBMIT.py on try bots for some time.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2354263003/#ps100001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] add presubmit.py to compile inspector-related scripts BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org ========== to ========== [inspector] add presubmit.py to compile inspector-related scripts BUG=chromium:635948 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/e9ceb376e4a7f796544707d148bb62aa68fe8bc0 Cr-Commit-Position: refs/heads/master@{#39846} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e9ceb376e4a7f796544707d148bb62aa68fe8bc0 Cr-Commit-Position: refs/heads/master@{#39846} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
