Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1152583011:
Refactor compile_js.gypi to support script_args and closure_args (Closed)
|
Created:
5 years, 7 months ago by Theresa Modified:
5 years, 6 months ago CC:
chromium-reviews, vitalyp+closure_chromium.org, jlklein+watch-closure_chromium.org, dbeam+watch-closure_chromium.org, Jamie Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis refactor makes third_party/closure_compiler/compile_js.gypi more versatile by allowing other gypi's that include it to define script_args and additional closure_args. remoting/ and components/resources/enhanced_bookmarks/ have been refactored to use compile_js.gypi (rather than calling compile.py directly).
BUG=496276
Committed: https://crrev.com/9899f157e5d5088f9500e7c3e8365e0d9a63412c
Cr-Commit-Position: refs/heads/master@{#335692}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use script_args and closure_args #
Total comments: 3
Patch Set 3 : Move all closure args into gypi files #
Total comments: 3
Patch Set 4 : Added remoting/compile_js.gypi and decoupled target_name from source_files #
Total comments: 28
Patch Set 5 : Cleanup and fix documentation #
Total comments: 2
Patch Set 6 : Fix closure_args in gypi, introduce closure_strictness_args% #
Total comments: 1
Patch Set 7 : Finish remoting gypi's, enhanced_bookmarks gn, compile_test.py #Patch Set 8 : Cleanup from self review #Patch Set 9 : Fix Android GN build #
Total comments: 11
Patch Set 10 : Remove --success_stamp, use gypi closure args in compile_test.py #
Total comments: 8
Patch Set 11 : compile_test.py cleanup #
Total comments: 6
Patch Set 12 : GN for remoting, extract closure_args to gpyi/gni #Patch Set 13 : Fix remoting builds #Patch Set 14 : Add type back to remoting_key_tester_jscompile #Patch Set 15 : More remoting gyp #Patch Set 16 : Rebase #Patch Set 17 : app_remoting_webapp gyp stuff #
Total comments: 26
Patch Set 18 : Changes from last dbeam@ review #Patch Set 19 : Fix inputs.py #
Total comments: 6
Patch Set 20 : Rebase, changes from last dbeam, garykac reviews #Patch Set 21 : Another rebase #Patch Set 22 : Rebase #Patch Set 23 : Yet another rebase #Messages
Total messages: 82 (20 generated)
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1152583011/diff/1/third_party/closure_compile... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/1/third_party/closure_compile... third_party/closure_compiler/compile_js.gypi:39: '<@(extra_args)', we'd probably want to pass these through to closure directly
twellington@chromium.org changed reviewers: + jlklein@chromium.org, tbreisacher@chromium.org
Merging conversations between the email thread and the CL. jlklein@ said: >My personal preference would be to limit extra_args to >"extra_flags" or "extra_defs" and explicitly allow multiple >source files if that's a use case that's in high demand. I >think that would keep things a bit cleaner. I don't know if multiple source files is in high demand. I need it for some stuff I'm doing with enhanced bookmarks, but I'm not sure if others will. I'd be fine with explicitly allowing multiple source files and keeping "extra_flags" limited to things besides additional source files. https://codereview.chromium.org/1152583011/diff/1/third_party/closure_compile... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/1/third_party/closure_compile... third_party/closure_compiler/compile_js.gypi:39: '<@(extra_args)', On 2015/05/27 00:57:03, Dan Beam wrote: > we'd probably want to pass these through to closure directly One of the things I wanted to use extra args for was --no-single-file, which controls which method compile.py initially calls into (check or check_multiple).
why can't we just send the args separately? e.g., in compile_js.gypi:
'script_args': [],
'closure_args': [
'--accept_const_keyword',
'--language_in=ECMASCRIPT5_STRICT',
...
],
'action': [
'<(CLOSURE_DIR)/compile.py', '<(@script_args)', '--', '<(@closure_args)'
],
from targets:
{
'target_name': 'js_file',
'variables': {
'closure_args': ['--output_wrapper=(function(){%s}())'],
'script_args': ['--polymer_pass']
},
'includes': ['compile_js.gypi'],
},
I think this would make it clearer what's happening and let us remove some code
from compile.py.
what does everybody thing?
> 'script_args': ['--polymer_pass'] whoops, meant 'script_args': ['--no-single-file'] (see how easy it is to understand when you mess up with the proposed, explicit system? :P)
On 2015/05/28 21:52:55, Dan Beam wrote: > > 'script_args': ['--polymer_pass'] > > whoops, meant 'script_args': ['--no-single-file'] > > (see how easy it is to understand when you mess up with the proposed, explicit > system? :P) +1 I like that approach, and agree that it's more clear what's affecting compile.py vs being passed to the closure compiler directly.
On 2015/05/28 22:35:54, Theresa Wellington wrote: > On 2015/05/28 21:52:55, Dan Beam wrote: > > > 'script_args': ['--polymer_pass'] > > > > whoops, meant 'script_args': ['--no-single-file'] > > > > (see how easy it is to understand when you mess up with the proposed, explicit > > system? :P) > > +1 I like that approach, and agree that it's more clear what's affecting > compile.py vs being passed to the closure compiler directly. in talking with jlklein@, it might also make sense to make 'source_file': '<(_target_name.js)', into 'source_files': ['<(_target_name.js)'], so you can just append sources to it (rather than using script_args for this)
Uploaded a second patchset with script_args, closure_args and source_files. Is this the gist of what you were thinking? https://codereview.chromium.org/1152583011/diff/20001/third_party/closure_com... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/20001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:22: '--accept_const_keyword', Would it make more sense to keep these in compile.py and just make this extra closure_args passed in by client .gypi files. Or should the polymer_extra_annotations and common_jscomp_errors move here?
https://codereview.chromium.org/1152583011/diff/20001/third_party/closure_com... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/20001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:22: '--accept_const_keyword', On 2015/05/28 23:12:18, Theresa Wellington wrote: > Would it make more sense to keep these in compile.py and just make this extra > closure_args passed in by client .gypi files. Or should the > polymer_extra_annotations and common_jscomp_errors move here? tbreisacher@ & jlklein@: wdyt?
https://codereview.chromium.org/1152583011/diff/20001/third_party/closure_com... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/20001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:22: '--accept_const_keyword', On 2015/05/28 23:15:11, Dan Beam wrote: > On 2015/05/28 23:12:18, Theresa Wellington wrote: > > Would it make more sense to keep these in compile.py and just make this extra > > closure_args passed in by client .gypi files. Or should the > > polymer_extra_annotations and common_jscomp_errors move here? > > tbreisacher@ & jlklein@: wdyt? I think we should put them here if possible. the only thing we might want to keep inside (or here with a note) is --summary_detail_level=3 which compile.py depends on for output parsing. For instance, I'd think we'd change 'script_args': ['--strict'] to 'closure_args': [ '--jscomp_error=reportUnknownTypes', '--jscomp_error=duplicate', '--jscomp_error=misplacedTypeAnnotation', ], this allows callers to better understand what's going on and not accidentally have their args overridden in some other place in the code.
https://codereview.chromium.org/1152583011/diff/40001/remoting/remoting_key_t... File remoting/remoting_key_tester.gypi (right): https://codereview.chromium.org/1152583011/diff/40001/remoting/remoting_key_t... remoting/remoting_key_tester.gypi:46: 'target_name': 'remoting_key_tester_jscompile', Do we want to decouple the target_name from the source_file/out_file name? This current patchset (#3) expects remoting_key_tester_jscompile to be a source file and the --out_file arg included in script_args below probably causes issues because compile.py will get two different --out_file args. https://codereview.chromium.org/1152583011/diff/40001/third_party/closure_com... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/40001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:54: "--jscomp_off=misplacedTypeAnnotation", I'm not sure if/how compiler flags override each other. If --jscomp_off=duplicate is added here, but --jscomp_error=duplicate is appended (either before or after) this set of flags, which one does the compiler respect? Since the "strict" vs "disabled" closure args contradict each other, I think they might need to stay in compile.py. Or at least be split out here, in compile_js.gypi, so their inclusion can be controlled by a variable.
https://codereview.chromium.org/1152583011/diff/40001/third_party/closure_com... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/40001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:54: "--jscomp_off=misplacedTypeAnnotation", On 2015/06/01 15:39:26, Theresa Wellington wrote: > I'm not sure if/how compiler flags override each other. If > --jscomp_off=duplicate is added here, but --jscomp_error=duplicate is appended > (either before or after) this set of flags, which one does the compiler respect? > > Since the "strict" vs "disabled" closure args contradict each other, I think > they might need to stay in compile.py. Or at least be split out here, in > compile_js.gypi, so their inclusion can be controlled by a variable. we shouldn't send both at once. we probably also don't need to only use 1 compile_js.gypi for all projects. if remoting or files.app wants to customize their flags, we can probably can just make a remoting/compile_js.gypi and mix in options with third_party/closure_compiler/compile_js.gypi like this: # remoting/compile_js.gypi { 'variables': [ 'closure_args!': [ '--jscomp_off=duplicate', ], 'closure_args': [ '--jscomp_error=duplicate', ], ], 'include': 'compile_js.gypi', } (note that if we don't wanna do 'list_exclusion!', we could make a compile_js.gypi and compile_js_strict.gypi like iOS does or a couple per-codebase) alternatively we could do a one-off for now for strict specifically, but I don't like that as much 'strict%': 0, 'conditions': [ {'strict == 1': { 'variables': [ ... more stricter ... ], }, { # else strict != 1 'variables': [ ... loosening ... ], } ]
added remoting/compile_js.gypi and decoupled target_name from source_files
did you actually run the code with this patch? doesn't seem like it'll work as is... https://codereview.chromium.org/1152583011/diff/60001/components/resources/en... File components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi (right): https://codereview.chromium.org/1152583011/diff/60001/components/resources/en... components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi:5: 'targets' :[ 'targets': [ https://codereview.chromium.org/1152583011/diff/60001/components/resources/en... components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi:13: 'script_args' : [ 'script_args': [ https://codereview.chromium.org/1152583011/diff/60001/components/resources/en... components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi:16: 'closure_args' : [ 'closure_args': [ https://codereview.chromium.org/1152583011/diff/60001/remoting/compile_js.gypi File remoting/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/60001/remoting/compile_js.gyp... remoting/compile_js.gypi:5: # remoting/compile_js.gypi can remove this as from the path it's clear (I just added it to make it clear in the really free-form text area that we publish comments in) https://codereview.chromium.org/1152583011/diff/60001/remoting/compile_js.gyp... remoting/compile_js.gypi:17: 'script_args' : [ 'script_args': ['--no-single-file'], https://codereview.chromium.org/1152583011/diff/60001/remoting/compile_js.gyp... remoting/compile_js.gypi:21: 'includes': [ '../third_party/closure_compiler/compile_js.gypi' ], i'd say mild preference to do: 'list_name': ['one_item'], # no extra \s or \n when there's only 1 item in a list and 'list_name': [ '--flag1', '--flag2', ], when there's 2+. whatever we land on, we should be consistent https://codereview.chromium.org/1152583011/diff/60001/remoting/remoting_key_t... File remoting/remoting_key_tester.gypi (right): https://codereview.chromium.org/1152583011/diff/60001/remoting/remoting_key_t... remoting/remoting_key_tester.gypi:57: 'script_args' : [ 'script_args': [ https://codereview.chromium.org/1152583011/diff/60001/remoting/remoting_key_t... remoting/remoting_key_tester.gypi:64: ], 'includes': ['compile_js.gypi'], https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... File third_party/closure_compiler/compile.py (right): https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile.py:333: parser.add_argument("-c", "--closure_args", i'd say all of these args should be --dash-form for consistency (instead of --under_scores) https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile.py:337: parser.add_argument("--strict", action="store_true", kill --strict https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:28: "--jscomp_error=accessControls", " -> ' https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:54: "--jscomp_off=misplacedTypeAnnotation", alphabetize this list https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:73: '<(source_files)', where does <(_target_name).js actually get sent to compile.py now? https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:78: '<@(closure_args)', hmmmm, shouldn't this have a -c or --closure(_-)args for it to actually work?
On 2015/06/01 22:57:40, Dan Beam wrote: > did you actually run the code with this patch? doesn't seem like it'll work as > is... Nope, I did not actually run anything (past patchset 1) primarily because components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi was added in a different CL that just landed today so this patchset previously would not compile in isolation. I think the general approach is pretty solid now, so I can go ahead and clean up the code (thank you for all of the comments!) and send out a real review if that sounds good to everyone. I'd prefer to start a separate issue (without the back and forth discussion on overall design), but am okay with keeping just this one if that's easier for reviewers.
On 2015/06/01 23:09:11, Theresa Wellington wrote: > On 2015/06/01 22:57:40, Dan Beam wrote: > > did you actually run the code with this patch? doesn't seem like it'll work > as > > is... > > Nope, I did not actually run anything (past patchset 1) primarily because > components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi was added in a > different CL that just landed today so this patchset previously would not > compile in isolation. > > I think the general approach is pretty solid now, so I can go ahead and clean up > the code (thank you for all of the comments!) and send out a real review if that > sounds good to everyone. I'd prefer to start a separate issue (without the back > and forth discussion on overall design), but am okay with keeping just this one > if that's easier for reviewers. It's good to keep a rationale for code changes so folks can find (or remember) it later, so I'd vote for keeping it. I'm pretty sure your reviewers will not feel obligated to read the entire stack of old review comments in their "free time".
https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:73: '<(source_files)', On 2015/06/01 22:57:39, Dan Beam wrote: > where does <(_target_name).js actually get sent to compile.py now? I decoupled _target_name and source_files, because the .gypi clients in remoting/ use a target name that does not correspond to an actual source file name. In enhanced_bookmarks.gypi, I included get_salient_image_url.js and dom_initializer.js in the respective source_files lists, so the clients of closure_compiler/compile_js.gypi would be responsible for including all source files in said list. The biggest downside to this approach is that it requires changing all files that include compile_js.gypi, but I am not opposed to doing that. Would an alternative be doing something like this in remoting/compile_js.gypi: 'variables': { ... 'source_files!': [<(_target_name).js] ... }, Or do we want to enforce a requirement that the target_name is a source file? https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:78: '<@(closure_args)', On 2015/06/01 22:57:40, Dan Beam wrote: > hmmmm, shouldn't this have a -c or --closure(_-)args for it to actually work? Yes, I think it does.
https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:73: '<(source_files)', On 2015/06/01 23:46:55, Theresa Wellington wrote: > On 2015/06/01 22:57:39, Dan Beam wrote: > > where does <(_target_name).js actually get sent to compile.py now? > > I decoupled _target_name and source_files, because the .gypi clients in > remoting/ use a target name that does not correspond to an actual source file > name. > > In enhanced_bookmarks.gypi, I included get_salient_image_url.js and > dom_initializer.js in the respective source_files lists, so the clients of > closure_compiler/compile_js.gypi would be responsible for including all source > files in said list. The biggest downside to this approach is that it requires > changing all files that include compile_js.gypi, but I am not opposed to doing > that. > > Would an alternative be doing something like this in remoting/compile_js.gypi: > > 'variables': { > ... > 'source_files!': [<(_target_name).js] > ... > }, > > Or do we want to enforce a requirement that the target_name is a source file? 'source_files%': ['<(_target_name.js)'], https://chromium.googlesource.com/external/gyp/+/master/docs/InputFormatRefer...
https://codereview.chromium.org/1152583011/diff/60001/components/resources/en... File components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi (right): https://codereview.chromium.org/1152583011/diff/60001/components/resources/en... components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi:5: 'targets' :[ On 2015/06/01 22:57:39, Dan Beam wrote: > 'targets': [ Done. https://codereview.chromium.org/1152583011/diff/60001/components/resources/en... components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi:13: 'script_args' : [ On 2015/06/01 22:57:39, Dan Beam wrote: > 'script_args': [ Done. https://codereview.chromium.org/1152583011/diff/60001/components/resources/en... components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi:16: 'closure_args' : [ On 2015/06/01 22:57:39, Dan Beam wrote: > 'closure_args': [ Done. https://codereview.chromium.org/1152583011/diff/60001/remoting/compile_js.gypi File remoting/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/60001/remoting/compile_js.gyp... remoting/compile_js.gypi:17: 'script_args' : [ On 2015/06/01 22:57:39, Dan Beam wrote: > 'script_args': ['--no-single-file'], Done. https://codereview.chromium.org/1152583011/diff/60001/remoting/compile_js.gyp... remoting/compile_js.gypi:21: 'includes': [ '../third_party/closure_compiler/compile_js.gypi' ], On 2015/06/01 22:57:39, Dan Beam wrote: > i'd say mild preference to do: > > 'list_name': ['one_item'], # no extra \s or \n > > when there's only 1 item in a list and > > 'list_name': [ > '--flag1', > '--flag2', > ], > > when there's 2+. whatever we land on, we should be consistent Done. https://codereview.chromium.org/1152583011/diff/60001/remoting/remoting_key_t... File remoting/remoting_key_tester.gypi (right): https://codereview.chromium.org/1152583011/diff/60001/remoting/remoting_key_t... remoting/remoting_key_tester.gypi:57: 'script_args' : [ On 2015/06/01 22:57:39, Dan Beam wrote: > 'script_args': [ Done. https://codereview.chromium.org/1152583011/diff/60001/remoting/remoting_key_t... remoting/remoting_key_tester.gypi:64: ], On 2015/06/01 22:57:39, Dan Beam wrote: > 'includes': ['compile_js.gypi'], Done. https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... File third_party/closure_compiler/compile.py (right): https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile.py:333: parser.add_argument("-c", "--closure_args", On 2015/06/01 22:57:39, Dan Beam wrote: > i'd say all of these args should be --dash-form for consistency (instead of > --under_scores) Done. https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile.py:337: parser.add_argument("--strict", action="store_true", On 2015/06/01 22:57:39, Dan Beam wrote: > kill --strict Done. https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:28: "--jscomp_error=accessControls", On 2015/06/01 22:57:39, Dan Beam wrote: > " -> ' Done. https://codereview.chromium.org/1152583011/diff/60001/third_party/closure_com... third_party/closure_compiler/compile_js.gypi:54: "--jscomp_off=misplacedTypeAnnotation", On 2015/06/01 22:57:40, Dan Beam wrote: > alphabetize this list Done.
Getting closer, still need to: - finish modifying the .gypi files for remoting/ - update enhanced_bookmarks .gn files - update closure_compiler's unit tests
https://codereview.chromium.org/1152583011/diff/100001/third_party/closure_co... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/100001/third_party/closure_co... third_party/closure_compiler/compile_js.gypi:63: 'closure_strictness_args%': [ exclusion lists for user-defined variables don't appear to work, will try to think of a better solution over night: https://codereview.chromium.org/26608002 https://code.google.com/p/gyp/issues/detail?id=373
twellington@chromium.org changed reviewers: + kelvinp@chromium.org
ptal - still need to fix the GN build for Android, but otherwise this is ready for review +kelvinp for remoting/ -- The tryjobs passed, but can you please try to manually compile the patch and make sure the compiled scripts look right as a sanity check? Or point me toward instructions on how to build with remoting=1? I set the flag in GYP_DEFINES, but didn't succeed in compiling due to other errors (starting with '../remoting/remoting.gyp:remoting_android_resources' not being defined anywhere)
https://codereview.chromium.org/1152583011/diff/80001/third_party/closure_com... File third_party/closure_compiler/compile.py (right): https://codereview.chromium.org/1152583011/diff/80001/third_party/closure_com... third_party/closure_compiler/compile.py:215: args += ["--%s" % arg] nit: if closure_args: args += ["--%s" % arg for arg in closure_args] https://codereview.chromium.org/1152583011/diff/150001/remoting/remoting_weba... File remoting/remoting_webapp_compile.gypi (right): https://codereview.chromium.org/1152583011/diff/150001/remoting/remoting_weba... remoting/remoting_webapp_compile.gypi:28: '--success-stamp', '<(success_stamp)', i don't think we need --success-stamp now that we can change out_file? https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... third_party/closure_compiler/compile_js.gypi:63: 'closure_strictness_args%': [ the strictness args are... looser? https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... third_party/closure_compiler/compiler_test.py:23: _COMMON_CLOSURE_ARGS = [ can we share this in gyp/gn/here?
https://codereview.chromium.org/1152583011/diff/80001/third_party/closure_com... File third_party/closure_compiler/compile.py (right): https://codereview.chromium.org/1152583011/diff/80001/third_party/closure_com... third_party/closure_compiler/compile.py:215: args += ["--%s" % arg] On 2015/06/03 23:39:11, Dan Beam wrote: > nit: > > if closure_args: > args += ["--%s" % arg for arg in closure_args] Done. https://codereview.chromium.org/1152583011/diff/150001/remoting/remoting_weba... File remoting/remoting_webapp_compile.gypi (right): https://codereview.chromium.org/1152583011/diff/150001/remoting/remoting_weba... remoting/remoting_webapp_compile.gypi:28: '--success-stamp', '<(success_stamp)', On 2015/06/03 23:39:11, Dan Beam wrote: > i don't think we need --success-stamp now that we can change out_file? I think you're right. Should I kill it in compile.py as well? https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... third_party/closure_compiler/compile_js.gypi:63: 'closure_strictness_args%': [ On 2015/06/03 23:39:11, Dan Beam wrote: > the strictness args are... looser? I think this could use a better name. The defaults here are looser, with the intention that other gypi's can override with more strict args. Maybe it makes more sense to keep the "disabled_closure_args" and "strict_closure_args" concepts from the current implementation of compile.py. The set of jscomp_off flags could be the default for disabled_closure_args, and the default for "strict_closure_args" could be []. gypi's that want more strict compilation could override disabled_closure_args to be [] and set strict_closure_args with the flags they want. Or I could reintroduce a "strict" variable that determines whether disabled_closure_args or strict_closure_args are included. https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... third_party/closure_compiler/compiler_test.py:23: _COMMON_CLOSURE_ARGS = [ On 2015/06/03 23:39:11, Dan Beam wrote: > can we share this in gyp/gn/here? For sharing with here, not that I know of, which isn't to say there isn't a way. How does run_tests.py get invoked? Is it just something set up on the FYI bot or is there a gyp file somewhere in our code base that has an action to run it (I did a file search but I'm not seeing anything)? Or is importing a gypi file into python a thing? For sharing with gn, probably. I think the enhanced_bookmarks gn is a one-off and that most people using the Closure compiler won't need to create gn files, so I'd prefer not to invest a lot of time in gn (also, gn is even more of a mystery to me than gyp). If others think gn builds dependent on compile.py will be a typical use case, then I can create more gn files that can be shared.
https://codereview.chromium.org/1152583011/diff/150001/remoting/remoting_weba... File remoting/remoting_webapp_compile.gypi (right): https://codereview.chromium.org/1152583011/diff/150001/remoting/remoting_weba... remoting/remoting_webapp_compile.gypi:28: '--success-stamp', '<(success_stamp)', On 2015/06/04 17:44:05, Theresa Wellington wrote: > On 2015/06/03 23:39:11, Dan Beam wrote: > > i don't think we need --success-stamp now that we can change out_file? > > I think you're right. Should I kill it in compile.py as well? plz https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... third_party/closure_compiler/compiler_test.py:23: _COMMON_CLOSURE_ARGS = [ On 2015/06/04 17:44:05, Theresa Wellington wrote: > On 2015/06/03 23:39:11, Dan Beam wrote: > > can we share this in gyp/gn/here? > > For sharing with here, not that I know of, which isn't to say there isn't a way. > > How does run_tests.py get invoked? Is it just something set up on the FYI bot or > is there a gyp file somewhere in our code base that has an action to run it (I > did a file search but I'm not seeing anything)? > > Or is importing a gypi file into python a thing? pretty sure GYP is written in Python, btw... > > For sharing with gn, probably. I think the enhanced_bookmarks gn is a one-off > and that most people using the Closure compiler won't need to create gn files, > so I'd prefer not to invest a lot of time in gn (also, gn is even more of a > mystery to me than gyp). If others think gn builds dependent on compile.py will > be a typical use case, then I can create more gn files that can be shared. that's fine
https://codereview.chromium.org/1152583011/diff/150001/remoting/remoting_weba... File remoting/remoting_webapp_compile.gypi (right): https://codereview.chromium.org/1152583011/diff/150001/remoting/remoting_weba... remoting/remoting_webapp_compile.gypi:28: '--success-stamp', '<(success_stamp)', On 2015/06/04 18:35:45, Dan Beam wrote: > On 2015/06/04 17:44:05, Theresa Wellington wrote: > > On 2015/06/03 23:39:11, Dan Beam wrote: > > > i don't think we need --success-stamp now that we can change out_file? > > > > I think you're right. Should I kill it in compile.py as well? > > plz Done. https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... third_party/closure_compiler/compile_js.gypi:63: 'closure_strictness_args%': [ On 2015/06/04 17:44:05, Theresa Wellington wrote: > On 2015/06/03 23:39:11, Dan Beam wrote: > > the strictness args are... looser? > > I think this could use a better name. The defaults here are looser, with the > intention that other gypi's can override with more strict args. > > Maybe it makes more sense to keep the "disabled_closure_args" and > "strict_closure_args" concepts from the current implementation of compile.py. > The set of jscomp_off flags could be the default for disabled_closure_args, and > the default for "strict_closure_args" could be []. gypi's that want more strict > compilation could override disabled_closure_args to be [] and set > strict_closure_args with the flags they want. > > Or I could reintroduce a "strict" variable that determines whether > disabled_closure_args or strict_closure_args are included. I was making this more complexly than necessary. I renamed this variable to "disabled_closure_args". In remoting/compile_js.gypi, disabled_closure_args is set to [] and closure_args is extended with the additional jscomp_error flags. https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1152583011/diff/150001/third_party/closure_co... third_party/closure_compiler/compiler_test.py:23: _COMMON_CLOSURE_ARGS = [ On 2015/06/04 18:35:45, Dan Beam wrote: > On 2015/06/04 17:44:05, Theresa Wellington wrote: > > On 2015/06/03 23:39:11, Dan Beam wrote: > > > can we share this in gyp/gn/here? > > > > For sharing with here, not that I know of, which isn't to say there isn't a > way. > > > > How does run_tests.py get invoked? Is it just something set up on the FYI bot > or > > is there a gyp file somewhere in our code base that has an action to run it (I > > did a file search but I'm not seeing anything)? > > > > Or is importing a gypi file into python a thing? > > pretty sure GYP is written in Python, btw... > > > > > For sharing with gn, probably. I think the enhanced_bookmarks gn is a one-off > > and that most people using the Closure compiler won't need to create gn files, > > so I'd prefer not to invest a lot of time in gn (also, gn is even more of a > > mystery to me than gyp). If others think gn builds dependent on compile.py > will > > be a typical use case, then I can create more gn files that can be shared. > > that's fine Done.
https://codereview.chromium.org/1152583011/diff/170001/third_party/closure_co... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1152583011/diff/170001/third_party/closure_co... third_party/closure_compiler/compiler_test.py:23: _GYPI_DICT = eval(open(os.path.join(os.path.dirname(__file__)) + '/compile_js.gypi').read()) os.path.join(_SCRIPT_DIR, 'compile_js.gypi') https://codereview.chromium.org/1152583011/diff/170001/third_party/closure_co... third_party/closure_compiler/compiler_test.py:23: _GYPI_DICT = eval(open(os.path.join(os.path.dirname(__file__)) + '/compile_js.gypi').read()) can we use gyp.input.Load()? https://codereview.chromium.org/1152583011/diff/170001/third_party/closure_co... third_party/closure_compiler/compiler_test.py:49: args += closure_args args = _COMMON_CLOSURE_ARGS + (closure_args or [])
https://codereview.chromium.org/1152583011/diff/170001/remoting/compile_js.gypi File remoting/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/170001/remoting/compile_js.gy... remoting/compile_js.gypi:7: 'externs': ['<(DEPTH)/third_party/closure_compiler/externs/chrome_extensions.js'], nit: <(DEPTH) -> ..
https://codereview.chromium.org/1152583011/diff/170001/remoting/compile_js.gypi File remoting/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/170001/remoting/compile_js.gy... remoting/compile_js.gypi:7: 'externs': ['<(DEPTH)/third_party/closure_compiler/externs/chrome_extensions.js'], On 2015/06/04 20:45:56, Dan Beam wrote: > nit: <(DEPTH) -> .. Done. https://codereview.chromium.org/1152583011/diff/170001/third_party/closure_co... File third_party/closure_compiler/compiler_test.py (right): https://codereview.chromium.org/1152583011/diff/170001/third_party/closure_co... third_party/closure_compiler/compiler_test.py:23: _GYPI_DICT = eval(open(os.path.join(os.path.dirname(__file__)) + '/compile_js.gypi').read()) On 2015/06/04 20:44:30, Dan Beam wrote: > os.path.join(_SCRIPT_DIR, 'compile_js.gypi') Done. https://codereview.chromium.org/1152583011/diff/170001/third_party/closure_co... third_party/closure_compiler/compiler_test.py:23: _GYPI_DICT = eval(open(os.path.join(os.path.dirname(__file__)) + '/compile_js.gypi').read()) On 2015/06/04 20:44:30, Dan Beam wrote: > can we use gyp.input.Load()? Discussed on hangouts; gyp.input.Load() requires more code and complexity and a simple literal_eval works in this case. https://codereview.chromium.org/1152583011/diff/170001/third_party/closure_co... third_party/closure_compiler/compiler_test.py:49: args += closure_args On 2015/06/04 20:44:30, Dan Beam wrote: > args = _COMMON_CLOSURE_ARGS + (closure_args or []) Done.
kelvinp@chromium.org changed reviewers: + garykac@chromium.org
https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gypi File remoting/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gy... remoting/compile_js.gypi:7: 'externs': ['../third_party/closure_compiler/externs/chrome_extensions.js'], You probably need to update the GN version as well at remoting/webapp/build_template.gni
https://codereview.chromium.org/1152583011/diff/190001/remoting/remoting_weba... File remoting/remoting_webapp_compile.gypi (left): https://codereview.chromium.org/1152583011/diff/190001/remoting/remoting_weba... remoting/remoting_webapp_compile.gypi:26: '<@(remoting_webapp_js_proto_files)', are these no longer necessary because |source_files| are now all sent to/resolved in inputs.py? https://codereview.chromium.org/1152583011/diff/190001/third_party/closure_co... File third_party/closure_compiler/build/inputs.py (right): https://codereview.chromium.org/1152583011/diff/190001/third_party/closure_co... third_party/closure_compiler/build/inputs.py:82: source = opts.sources[0] shouldn't we be resolving dependencies for all sources?
https://codereview.chromium.org/1152583011/diff/190001/remoting/remoting_weba... File remoting/remoting_webapp_compile.gypi (left): https://codereview.chromium.org/1152583011/diff/190001/remoting/remoting_weba... remoting/remoting_webapp_compile.gypi:26: '<@(remoting_webapp_js_proto_files)', On 2015/06/08 21:25:45, Dan Beam wrote: > are these no longer necessary because |source_files| are now all sent > to/resolved in inputs.py? Yep. https://codereview.chromium.org/1152583011/diff/190001/third_party/closure_co... File third_party/closure_compiler/build/inputs.py (right): https://codereview.chromium.org/1152583011/diff/190001/third_party/closure_co... third_party/closure_compiler/build/inputs.py:82: source = opts.sources[0] On 2015/06/08 21:25:45, Dan Beam wrote: > shouldn't we be resolving dependencies for all sources? Since compile.py's check_multiple doesn't even take a depends variable, I don't think we need to call resolve_recursive_dependencies for multiple sources. Would it make sense to introduce a --no-single-file arg and do the resolve_recursive_dependencies call if that flag is not set?
https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gypi File remoting/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gy... remoting/compile_js.gypi:7: 'externs': ['../third_party/closure_compiler/externs/chrome_extensions.js'], On 2015/06/08 19:54:26, kelvinp wrote: > You probably need to update the GN version as well at > remoting/webapp/build_template.gni Done.
On 2015/06/10 00:36:41, Theresa Wellington wrote: > https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gypi > File remoting/compile_js.gypi (right): > > https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gy... > remoting/compile_js.gypi:7: 'externs': > ['../third_party/closure_compiler/externs/chrome_extensions.js'], > On 2015/06/08 19:54:26, kelvinp wrote: > > You probably need to update the GN version as well at > > remoting/webapp/build_template.gni > > Done. Sure let me take a look.
On 2015/06/10 22:29:04, kelvinp wrote: > On 2015/06/10 00:36:41, Theresa Wellington wrote: > > > https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gypi > > File remoting/compile_js.gypi (right): > > > > > https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gy... > > remoting/compile_js.gypi:7: 'externs': > > ['../third_party/closure_compiler/externs/chrome_extensions.js'], > > On 2015/06/08 19:54:26, kelvinp wrote: > > > You probably need to update the GN version as well at > > > remoting/webapp/build_template.gni > > > > Done. > > Sure let me take a look. gclient runhooks fails after applying your latest patch Traceback (most recent call last): File "src/build/gyp_chromium", line 323, in <module> gyp_rc = gyp.main(args) File "/usr/local/google/home/kelvinp/enlistments/chromium/src/tools/gyp/pylib/gyp/__init__.py", line 538, in main return gyp_main(args) File "/usr/local/google/home/kelvinp/enlistments/chromium/src/tools/gyp/pylib/gyp/__init__.py", line 523, in gyp_main generator.GenerateOutput(flat_list, targets, data, params) File "/usr/local/google/home/kelvinp/enlistments/chromium/src/tools/gyp/pylib/gyp/generator/ninja.py", line 2384, in GenerateOutput pool.map(CallGenerateOutputForConfig, arglists) File "/usr/lib/python2.7/multiprocessing/pool.py", line 251, in map return self.map_async(func, iterable, chunksize).get() File "/usr/lib/python2.7/multiprocessing/pool.py", line 558, in get raise self._value KeyError: 'action' To build the remoting webapp, For GYP 1. GYP_DEFINES=run_jscompile=1 && ninja -C out/Debug -j500 remoting_webapp For gn, 1. Modify src/remoting/remoting_options.gni to set run_jscompile = true 2. ninja -C out_gn/Debug -j500 /remoting/webapp 3. The patch fails with compile.py: error: too few arguments ninja: build stopped: subcommand failed. Let me know if that works for you.
On 2015/06/11 01:04:07, kelvinp wrote: > On 2015/06/10 22:29:04, kelvinp wrote: > > On 2015/06/10 00:36:41, Theresa Wellington wrote: > > > > > > https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gypi > > > File remoting/compile_js.gypi (right): > > > > > > > > > https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gy... > > > remoting/compile_js.gypi:7: 'externs': > > > ['../third_party/closure_compiler/externs/chrome_extensions.js'], > > > On 2015/06/08 19:54:26, kelvinp wrote: > > > > You probably need to update the GN version as well at > > > > remoting/webapp/build_template.gni > > > > > > Done. > > > > Sure let me take a look. > > gclient runhooks fails after applying your latest patch > Traceback (most recent call last): > File "src/build/gyp_chromium", line 323, in <module> > gyp_rc = gyp.main(args) > File > "/usr/local/google/home/kelvinp/enlistments/chromium/src/tools/gyp/pylib/gyp/__init__.py", > line 538, in main > return gyp_main(args) > File > "/usr/local/google/home/kelvinp/enlistments/chromium/src/tools/gyp/pylib/gyp/__init__.py", > line 523, in gyp_main > generator.GenerateOutput(flat_list, targets, data, params) > File > "/usr/local/google/home/kelvinp/enlistments/chromium/src/tools/gyp/pylib/gyp/generator/ninja.py", > line 2384, in GenerateOutput > pool.map(CallGenerateOutputForConfig, arglists) > File "/usr/lib/python2.7/multiprocessing/pool.py", line 251, in map > return self.map_async(func, iterable, chunksize).get() > File "/usr/lib/python2.7/multiprocessing/pool.py", line 558, in get > raise self._value > KeyError: 'action' > > To build the remoting webapp, > > For GYP > 1. GYP_DEFINES=run_jscompile=1 && ninja -C out/Debug -j500 remoting_webapp > > For gn, > 1. Modify src/remoting/remoting_options.gni to set run_jscompile = true > 2. ninja -C out_gn/Debug -j500 /remoting/webapp > 3. The patch fails with > compile.py: error: too few arguments > ninja: build stopped: subcommand failed. > > Let me know if that works for you. Thank you for the instructions! They were both failing in the same way for me. For the GYP - including a gypi with an actions inside an action block was causing issues (so including compile_js.gypi, inside the action blocks in remoting_webapp_compile.gypi). I changed them from actions to targets, and changed the include in third_party/closure_compiler/compiled_resources.gyp to a dependency. My local build is successful - I need do some more testing but I think that should work. For the GN build, the problem was that the list of sources has to come before the list of closure_args or compile.py assumes they're part of closure_args. That's working locally now as well. Are there any trybots that have run_jscompile set to true? Or bots in the main waterfall? Or are the tests primarily run locally?
On 2015/06/11 16:32:21, Theresa Wellington wrote: > On 2015/06/11 01:04:07, kelvinp wrote: > > On 2015/06/10 22:29:04, kelvinp wrote: > > > On 2015/06/10 00:36:41, Theresa Wellington wrote: > > > > > > > > > > https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gypi > > > > File remoting/compile_js.gypi (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1152583011/diff/190001/remoting/compile_js.gy... > > > > remoting/compile_js.gypi:7: 'externs': > > > > ['../third_party/closure_compiler/externs/chrome_extensions.js'], > > > > On 2015/06/08 19:54:26, kelvinp wrote: > > > > > You probably need to update the GN version as well at > > > > > remoting/webapp/build_template.gni > > > > > > > > Done. > > > > > > Sure let me take a look. > > > > gclient runhooks fails after applying your latest patch > > Traceback (most recent call last): > > File "src/build/gyp_chromium", line 323, in <module> > > gyp_rc = gyp.main(args) > > File > > > "/usr/local/google/home/kelvinp/enlistments/chromium/src/tools/gyp/pylib/gyp/__init__.py", > > line 538, in main > > return gyp_main(args) > > File > > > "/usr/local/google/home/kelvinp/enlistments/chromium/src/tools/gyp/pylib/gyp/__init__.py", > > line 523, in gyp_main > > generator.GenerateOutput(flat_list, targets, data, params) > > File > > > "/usr/local/google/home/kelvinp/enlistments/chromium/src/tools/gyp/pylib/gyp/generator/ninja.py", > > line 2384, in GenerateOutput > > pool.map(CallGenerateOutputForConfig, arglists) > > File "/usr/lib/python2.7/multiprocessing/pool.py", line 251, in map > > return self.map_async(func, iterable, chunksize).get() > > File "/usr/lib/python2.7/multiprocessing/pool.py", line 558, in get > > raise self._value > > KeyError: 'action' > > > > To build the remoting webapp, > > > > For GYP > > 1. GYP_DEFINES=run_jscompile=1 && ninja -C out/Debug -j500 remoting_webapp > > > > For gn, > > 1. Modify src/remoting/remoting_options.gni to set run_jscompile = true > > 2. ninja -C out_gn/Debug -j500 /remoting/webapp > > 3. The patch fails with > > compile.py: error: too few arguments > > ninja: build stopped: subcommand failed. > > > > Let me know if that works for you. > > Thank you for the instructions! They were both failing in the same way for me. > > For the GYP - including a gypi with an actions inside an action block was > causing issues (so including compile_js.gypi, inside the action blocks in > remoting_webapp_compile.gypi). I changed them from actions to targets, and > changed the include in third_party/closure_compiler/compiled_resources.gyp to a > dependency. My local build is successful - I need do some more testing but I > think that should work. > > For the GN build, the problem was that the list of sources has to come before > the list of closure_args or compile.py assumes they're part of closure_args. > That's working locally now as well. > > Are there any trybots that have run_jscompile set to true? Or bots in the main > waterfall? Or are the tests primarily run locally? I think dbeam@ is working on a closure bots that needs dependency on Java. For our team, those test are primarily run locally. Let me know when the next patch is ready for review.
ptal - remoting stuff looks like it's all passing now (and works locally for me as well)
On 2015/06/11 20:50:17, Theresa Wellington wrote: > ptal - remoting stuff looks like it's all passing now (and works locally for me > as well) The GN build looks good! I forget to mention that there is another gyp target that you would need to take care of. ninja -C out/Debug ar_sample_app You would need to modify app_remoting_webapp.gyp to use the new syntax.
On 2015/06/11 21:40:12, kelvinp wrote: > The GN build looks good! > > I forget to mention that there is another gyp target that you would need to take > care of. > > ninja -C out/Debug ar_sample_app > > You would need to modify app_remoting_webapp.gyp to use the new syntax. Thanks! Not sure how I missed that one. I'll ping the CL again once that's done.
On 2015/06/11 21:40:12, kelvinp wrote: > The GN build looks good! > > I forget to mention that there is another gyp target that you would need to take > care of. > > ninja -C out/Debug ar_sample_app > > You would need to modify app_remoting_webapp.gyp to use the new syntax. Thanks! Not sure how I missed that one. I'll ping the CL again once that's done.
ptal - app_remoting_webapp.gyp was changed to use the new syntax. It's not totally done (I have a couple of questions). https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... File remoting/app_remoting_webapp.gyp (right): https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp.gyp:9: ], will fix whitespace https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp.gyp:37: # '<(zip_path)', ninja is not at all happy with this variable. It complains that it's undefined, even though it's set in the target_defaults section of app_remoting_webapp_build.gypi, which is in the includes list above. Any thoughts on why it's not picking it up? Is this a critical piece to keep (why does this set of actions need to run after the build? after the build of what?)? https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... File remoting/app_remoting_webapp_compile.gypi (right): https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp_compile.gypi:12: 'target_name': 'verify_main.html', Previously, the action name had >(ar_app_name) in the middle. Including app_remoting_webapp_build.gypi was causing cascading problems around app_name not being defined, then app_description, then app_id, etc. I don't think any of those variables actually need to be defined to compile JS. Is it okay to drop the app_name form these target names or should I figure out a way to get it in there?
looks pretty good to me https://codereview.chromium.org/1152583011/diff/310001/components/resources/e... File components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi (right): https://codereview.chromium.org/1152583011/diff/310001/components/resources/e... components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi:13: 'script_args': ['--no-single-file'], it'd be preferable to just detect this via len(options.sources) > 1 in input.py, but you could do that later https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... File third_party/closure_compiler/compile.py (left): https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... third_party/closure_compiler/compile.py:85: # Avoiding parse-time warnings needs 2 pass compiling. crbug.com/421562. where did these comments go? https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... File third_party/closure_compiler/compile.py (right): https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... third_party/closure_compiler/compile.py:216: args += ["--%s" % arg] args += ["--%s" % arg for arg in closure_args] https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... third_party/closure_compiler/compile_js.gypi:25: # Non-strict defaults are provided that can be overriden. nit: 80 col wrap when you can https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... third_party/closure_compiler/compile_js.gypi:33: 'disabled_closure_args%': [ why can't these live in closure_args.gypi? 'disabled_closure_args%': <(default_disabled_closure_args), (something like this?) https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... File third_party/closure_compiler/compiled_resources.gyp (right): https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... third_party/closure_compiler/compiled_resources.gyp:28: '../../remoting/remoting_webapp_compile.gypi:*', hmmm, i didn't know .gypi:* worked. great.
https://codereview.chromium.org/1152583011/diff/310001/components/resources/e... File components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi (right): https://codereview.chromium.org/1152583011/diff/310001/components/resources/e... components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi:13: 'script_args': ['--no-single-file'], On 2015/06/12 21:45:06, Dan Beam wrote: > it'd be preferable to just detect this via len(options.sources) > 1 in input.py, > but you could do that later I added a TODO for myself note in compile.py. https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... File remoting/app_remoting_webapp.gyp (right): https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp.gyp:9: ], On 2015/06/12 20:36:50, Theresa Wellington wrote: > will fix whitespace Done. https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... File third_party/closure_compiler/compile.py (left): https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... third_party/closure_compiler/compile.py:85: # Avoiding parse-time warnings needs 2 pass compiling. crbug.com/421562. On 2015/06/12 21:45:06, Dan Beam wrote: > where did these comments go? Added back in closure_args.gypi. https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... File third_party/closure_compiler/compile.py (right): https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... third_party/closure_compiler/compile.py:216: args += ["--%s" % arg] On 2015/06/12 21:45:06, Dan Beam wrote: > args += ["--%s" % arg for arg in closure_args] Done. https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... third_party/closure_compiler/compile_js.gypi:25: # Non-strict defaults are provided that can be overriden. On 2015/06/12 21:45:06, Dan Beam wrote: > nit: 80 col wrap when you can Done. https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... third_party/closure_compiler/compile_js.gypi:33: 'disabled_closure_args%': [ On 2015/06/12 21:45:06, Dan Beam wrote: > why can't these live in closure_args.gypi? > > 'disabled_closure_args%': <(default_disabled_closure_args), > > (something like this?) They can. Moved. https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... File third_party/closure_compiler/compiled_resources.gyp (right): https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... third_party/closure_compiler/compiled_resources.gyp:28: '../../remoting/remoting_webapp_compile.gypi:*', On 2015/06/12 21:45:06, Dan Beam wrote: > hmmm, i didn't know .gypi:* worked. great. It works in other dependency lists in our code (and in my local tests building the ar_sample_app target in remoting_webapp.gyp) so I'm 99% sure it works here too. I haven't actually tried to build this target locally... that would be a good test to run before landing.
On 2015/06/12 22:12:15, Theresa Wellington wrote: > https://codereview.chromium.org/1152583011/diff/310001/components/resources/e... > File components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi (right): > > https://codereview.chromium.org/1152583011/diff/310001/components/resources/e... > components/resources/enhanced_bookmarks/enhanced_bookmarks.gypi:13: > 'script_args': ['--no-single-file'], > On 2015/06/12 21:45:06, Dan Beam wrote: > > it'd be preferable to just detect this via len(options.sources) > 1 in > input.py, > > but you could do that later > > I added a TODO for myself note in compile.py. > > https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... > File remoting/app_remoting_webapp.gyp (right): > > https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... > remoting/app_remoting_webapp.gyp:9: ], > On 2015/06/12 20:36:50, Theresa Wellington wrote: > > will fix whitespace > > Done. > > https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... > File third_party/closure_compiler/compile.py (left): > > https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... > third_party/closure_compiler/compile.py:85: # Avoiding parse-time warnings needs > 2 pass compiling. crbug.com/421562. > On 2015/06/12 21:45:06, Dan Beam wrote: > > where did these comments go? > > Added back in closure_args.gypi. > > https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... > File third_party/closure_compiler/compile.py (right): > > https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... > third_party/closure_compiler/compile.py:216: args += ["--%s" % arg] > On 2015/06/12 21:45:06, Dan Beam wrote: > > args += ["--%s" % arg for arg in closure_args] > > Done. > > https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... > File third_party/closure_compiler/compile_js.gypi (right): > > https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... > third_party/closure_compiler/compile_js.gypi:25: # > Non-strict defaults are provided that can be overriden. > On 2015/06/12 21:45:06, Dan Beam wrote: > > nit: 80 col wrap when you can > > Done. > > https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... > third_party/closure_compiler/compile_js.gypi:33: 'disabled_closure_args%': [ > On 2015/06/12 21:45:06, Dan Beam wrote: > > why can't these live in closure_args.gypi? > > > > 'disabled_closure_args%': <(default_disabled_closure_args), > > > > (something like this?) > > They can. Moved. > > https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... > File third_party/closure_compiler/compiled_resources.gyp (right): > > https://codereview.chromium.org/1152583011/diff/310001/third_party/closure_co... > third_party/closure_compiler/compiled_resources.gyp:28: > '../../remoting/remoting_webapp_compile.gypi:*', > On 2015/06/12 21:45:06, Dan Beam wrote: > > hmmm, i didn't know .gypi:* worked. great. > > It works in other dependency lists in our code (and in my local tests building > the ar_sample_app target in remoting_webapp.gyp) so I'm 99% sure it works here > too. I haven't actually tried to build this target locally... that would be a > good test to run before landing. For your questions regarding chromoting, garykac@ will be a better person to answer them :)
https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... File remoting/app_remoting_webapp.gyp (right): https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp.gyp:20: 'type': 'none', This change shouldn't be needed since it comes from app_remoting_webapp_build's target_defaults. If it's not already defined here, then something odd is going on. https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp.gyp:28: 'conditions': [ This needs to be in 'target_defaults' (where it was) because there is more than one AppRemoting target and they need to share the build rules and conditions rather than repeat them in each target. The 'target' should contain the minimal set of info that differs for each target. Most of the targets are internal - ar_sample_app is the public sample app that acts as a sanity check to make sure that the builds don't break. You should be building at least one of the internal apps to verify your changes. I can give you more info separately. https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp.gyp:31: 'app_remoting_webapp_compile.gypi:*' Note that use of the '*' has been deprecated in GYP because this feature does not exist in GN. The workaround is to explicitly list each target. https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp.gyp:37: # '<(zip_path)', On 2015/06/12 20:36:50, Theresa Wellington wrote: > ninja is not at all happy with this variable. It complains that it's undefined, > even though it's set in the target_defaults section of > app_remoting_webapp_build.gypi, which is in the includes list above. > > Any thoughts on why it's not picking it up? No. But the previous code was working when it was in the target_defaults section. Since this needs to go back there, you should do that before investigating further. > Is this a critical piece to keep > (why does this set of actions need to run after the build? after the build of > what?)? I don't think that part is critical anymore, and we may need to update how we jscompile anyway (since we our build script writes into the JS files - we want to modify it to check the output of that rather than the checked-in files). So you can omit this. https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... File remoting/app_remoting_webapp_compile.gypi (right): https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp_compile.gypi:12: 'target_name': 'verify_main.html', On 2015/06/12 20:36:51, Theresa Wellington wrote: > Previously, the action name had >(ar_app_name) in the middle. Including > app_remoting_webapp_build.gypi was causing cascading problems around app_name > not being defined, then app_description, then app_id, etc. I don't think any of > those variables actually need to be defined to compile JS. > > Is it okay to drop the app_name form these target names or should I figure out a > way to get it in there? It's OK to drop that. We had that because we were running jscompile after each app was built and needed a different action name. Having it depend on just the source files is fine.
lgtm though you need to merge with jhawkins@' recent changes https://codereview.chromium.org/1152583011/diff/350001/third_party/closure_co... File third_party/closure_compiler/build/inputs.py (right): https://codereview.chromium.org/1152583011/diff/350001/third_party/closure_co... third_party/closure_compiler/build/inputs.py:83: if (len(opts.sources) == 1): nit: no parens if len(opts.sources) == 1: https://codereview.chromium.org/1152583011/diff/350001/third_party/closure_co... third_party/closure_compiler/build/inputs.py:93: for file in set(opts.sources) | set(depends) | set(externs): is doing set() on something that's already a set() cheap? https://codereview.chromium.org/1152583011/diff/350001/third_party/closure_co... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/350001/third_party/closure_co... third_party/closure_compiler/compile_js.gypi:35: 'disabled_closure_args%': ['<@(default_disabled_closure_args)'], nit: seems weird that we have a variable that's a list... but then we stringify and re-expand it or something (via ['<@()'])
https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... File remoting/app_remoting_webapp.gyp (right): https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp.gyp:20: 'type': 'none', On 2015/06/16 00:32:42, garykac wrote: > This change shouldn't be needed since it comes from app_remoting_webapp_build's > target_defaults. If it's not already defined here, then something odd is going > on. Done. https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp.gyp:28: 'conditions': [ On 2015/06/16 00:32:42, garykac wrote: > This needs to be in 'target_defaults' (where it was) because there is more than > one AppRemoting target and they need to share the build rules and conditions > rather than repeat them in each target. The 'target' should contain the minimal > set of info that differs for each target. > > Most of the targets are internal - ar_sample_app is the public sample app that > acts as a sanity check to make sure that the builds don't break. > > You should be building at least one of the internal apps to verify your changes. > I can give you more info separately. Moved to target_defaults. Yes, please send instructions for an internal app that is easy to test. I work on Chrome for Android, so assume my setup is from scratch wrt to testing remoting. https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp.gyp:31: 'app_remoting_webapp_compile.gypi:*' On 2015/06/16 00:32:42, garykac wrote: > Note that use of the '*' has been deprecated in GYP because this feature does > not exist in GN. The workaround is to explicitly list each target. Done. https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp.gyp:37: # '<(zip_path)', On 2015/06/16 00:32:42, garykac wrote: > On 2015/06/12 20:36:50, Theresa Wellington wrote: > > ninja is not at all happy with this variable. It complains that it's > undefined, > > even though it's set in the target_defaults section of > > app_remoting_webapp_build.gypi, which is in the includes list above. > > > > Any thoughts on why it's not picking it up? > > No. But the previous code was working when it was in the target_defaults > section. Since this needs to go back there, you should do that before > investigating further. > > > Is this a critical piece to keep > > (why does this set of actions need to run after the build? after the build of > > what?)? > > I don't think that part is critical anymore, and we may need to update how we > jscompile anyway (since we our build script writes into the JS files - we want > to modify it to check the output of that rather than the checked-in files). So > you can omit this. It works now that it's back in target_defaults. Would you prefer if I leave it in or omit it? https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... File remoting/app_remoting_webapp_compile.gypi (right): https://codereview.chromium.org/1152583011/diff/310001/remoting/app_remoting_... remoting/app_remoting_webapp_compile.gypi:12: 'target_name': 'verify_main.html', On 2015/06/16 00:32:42, garykac wrote: > On 2015/06/12 20:36:51, Theresa Wellington wrote: > > Previously, the action name had >(ar_app_name) in the middle. Including > > app_remoting_webapp_build.gypi was causing cascading problems around app_name > > not being defined, then app_description, then app_id, etc. I don't think any > of > > those variables actually need to be defined to compile JS. > > > > Is it okay to drop the app_name form these target names or should I figure out > a > > way to get it in there? > > It's OK to drop that. We had that because we were running jscompile after each > app was built and needed a different action name. > > Having it depend on just the source files is fine. Acknowledged. https://codereview.chromium.org/1152583011/diff/350001/third_party/closure_co... File third_party/closure_compiler/build/inputs.py (right): https://codereview.chromium.org/1152583011/diff/350001/third_party/closure_co... third_party/closure_compiler/build/inputs.py:83: if (len(opts.sources) == 1): On 2015/06/16 00:54:43, Dan Beam wrote: > nit: no parens > > if len(opts.sources) == 1: Done. https://codereview.chromium.org/1152583011/diff/350001/third_party/closure_co... third_party/closure_compiler/build/inputs.py:93: for file in set(opts.sources) | set(depends) | set(externs): On 2015/06/16 00:54:43, Dan Beam wrote: > is doing set() on something that's already a set() cheap? I don't know that it's cheap - the script fails to run if externs isn't turned into a set somewhere. I moved the set creation for externs to the else block above since resolve_recursive_dependencies already makes it a set. https://codereview.chromium.org/1152583011/diff/350001/third_party/closure_co... File third_party/closure_compiler/compile_js.gypi (right): https://codereview.chromium.org/1152583011/diff/350001/third_party/closure_co... third_party/closure_compiler/compile_js.gypi:35: 'disabled_closure_args%': ['<@(default_disabled_closure_args)'], On 2015/06/16 00:54:43, Dan Beam wrote: > nit: seems weird that we have a variable that's a list... but then we stringify > and re-expand it or something (via ['<@()']) Changed it to just '<(default_disabled_closure_args)'
ptal - tested some internal remoting apps and they all compiled successfully
lgtm
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1152583011/#ps390001 (title: "Another rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152583011/390001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_arm_compile on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, garykac@chromium.org Link to the patchset: https://codereview.chromium.org/1152583011/#ps410001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152583011/410001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by twellington@chromium.org
The CQ bit was unchecked by twellington@chromium.org
The CQ bit was checked by twellington@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152583011/410001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, garykac@chromium.org Link to the patchset: https://codereview.chromium.org/1152583011/#ps430001 (title: "Yet another rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152583011/430001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by twellington@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152583011/430001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by twellington@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152583011/430001
Message was sent while issue was closed.
Committed patchset #23 (id:430001)
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/9899f157e5d5088f9500e7c3e8365e0d9a63412c Cr-Commit-Position: refs/heads/master@{#335692} |
