GN files for running Closure Compiler

GN files for running Closure Compiler

BUG=632206
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[aberent, 2016-08-17 13:24:19]

Description was changed from

==========
GN files for running Closure Compiler

BUG=632206
==========

to

==========
GN files for running Closure Compiler

BUG=632206
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[aberent, 2016-08-17 13:28:44]

aberent@chromium.org changed reviewers:
+ dbeam@chromium.org

[aberent, 2016-08-17 13:28:45]

Mainly for information at this stage, although I think compile_js2.gni is
probably quite close to what we want.

What I have done to src/BUILD.gn is clearly wrong, but I still need to work out
how to incorporate this into the GN tree of build files.

[Dan Beam, 2016-08-17 18:24:22]

https://codereview.chromium.org/2247353004/diff/1/third_party/closure_compile...
File third_party/closure_compiler/compile_js2.gni (right):

https://codereview.chromium.org/2247353004/diff/1/third_party/closure_compile...
third_party/closure_compiler/compile_js2.gni:62: if (defined(exters)) {
externs

[aberent, 2016-08-18 10:30:37]

In view of the discussions on dependencies and source files this will need a
rethink. Updated version may or may not follow before I go on holiday at the end
of today.

[aberent, 2016-08-18 16:08:52]

Patch set 2 solves the dependency problem using data dependencies (as suggested
by dprank@). I am not sure I particularly like the solution, but it is not as
bad as
I feared.

As you can see I have tested this with braille_ime, which is small but seems to
cover most of the cases. We could, in theory, land this and then land all the
other uses of it in different CLs.

https://codereview.chromium.org/2247353004/diff/1/third_party/closure_compile...
File third_party/closure_compiler/compile_js2.gni (right):

https://codereview.chromium.org/2247353004/diff/1/third_party/closure_compile...
third_party/closure_compiler/compile_js2.gni:62: if (defined(exters)) {
On 2016/08/17 18:24:21, Dan Beam wrote:
> externs

Done.

[Dan Beam, 2016-08-19 01:55:19]

https://codereview.chromium.org/2247353004/diff/40001/third_party/closure_com...
File third_party/closure_compiler/compile_js2.gni (right):

https://codereview.chromium.org/2247353004/diff/40001/third_party/closure_com...
third_party/closure_compiler/compile_js2.gni:38: # dependency tree
can actions have dependencies?  if so, we just kind of need to do a dependency
order set union here i think

[Dan Beam, 2016-08-31 23:47:02]

Description was changed from

==========
GN files for running Closure Compiler

BUG=632206
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
GN files for running Closure Compiler

BUG=632206
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-08-31 23:47:09]

/cc brettw@

[aberent, 2016-09-06 13:22:29]

https://codereview.chromium.org/2247353004/diff/40001/third_party/closure_com...
File third_party/closure_compiler/compile_js2.gni (right):

https://codereview.chromium.org/2247353004/diff/40001/third_party/closure_com...
third_party/closure_compiler/compile_js2.gni:38: # dependency tree
On 2016/08/19 01:55:19, Dan Beam wrote:
> can actions have dependencies?  if so, we just kind of need to do a dependency
> order set union here i think

Yes, actions can have dependencies. The problem is that, as I understand it, GN
is single pass. As such it does not know the full dependency tree of any target
until the GN run is finished, so one cannot read that dependency tree within GN.
 The best it can do is create a file containing a data dependency list that can
be read at ninja time (see the discussion in the email thread).

Dirk may be able to correct me, or comment further.

I have just realised, however that this comment is totally the wrong place! It
should be a comment on line 80, 'data = sources'.

[Dan Beam, 2016-09-06 16:46:46]

overall I think this will work, and I'd be happy to get some people to help
convert when this system lands/is ready

just a few questions before we go forward

https://codereview.chromium.org/2247353004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/braille_ime/BUILD.gn (right):

https://codereview.chromium.org/2247353004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/braille_ime/BUILD.gn:17: source_files = [
"externs.js" ]
this should probably be "externs =" instead of "source_files =", right?

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

https://codereview.chromium.org/2247353004/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile2.py:194: source_list_file=None,
can we call this like gn_source_list_file to make it clearer it's coming from a
format GN controls (and doesn't apply to the GYP version of this for the short
time it exists)?

https://codereview.chromium.org/2247353004/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile2.py:221: if (source_list_file):
if source_list_file:

https://codereview.chromium.org/2247353004/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile2.py:347: source_list_file =
opts.source_list_file,
no spaces between =, i.e. source_list_file=opts.source_list_file,

https://codereview.chromium.org/2247353004/diff/60001/third_party/closure_com...
File third_party/closure_compiler/run_compiler (right):

https://codereview.chromium.org/2247353004/diff/60001/third_party/closure_com...
third_party/closure_compiler/run_compiler:18: gn gen
--args='root_extra_deps=["//third_party/closure_compiler:compile_js_all"]'
out/Default
i don't think we want to:

a) run both GN and GYP duplicatively
b) ask everybody to use GN quite yet

let's either make a new script or just wait to swap the use of GYP in this file.
 lots of folks use this script daily, I don't want to break their work flows. 
i'd say we can switch this in a week or two, or whenever this work settles down.

https://codereview.chromium.org/2247353004/diff/60001/third_party/jstemplate/...
File third_party/jstemplate/BUILD.gn (right):

https://codereview.chromium.org/2247353004/diff/60001/third_party/jstemplate/...
third_party/jstemplate/BUILD.gn:6: compile_js("compile_util") {
why are we prefixing with compile_ everywhere?  what's the value?  this is
mainly typechecking right now, and this differs from the old system (as it adds
a lot of fluff to every target name).

this already reads as "compile JS <name>".  with the "compile_" prefix it reads
as "compile JS compile <name>", i.e. "compile the compiled thing" (IMO)

[aberent, 2016-09-06 17:30:22]

Answers to your questions. I will look at your editorial comments tomorrow.

https://codereview.chromium.org/2247353004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/chromeos/braille_ime/BUILD.gn (right):

https://codereview.chromium.org/2247353004/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/braille_ime/BUILD.gn:17: source_files = [
"externs.js" ]
On 2016/09/06 16:46:46, Dan Beam wrote:
> this should probably be "externs =" instead of "source_files =", right?

Actually no. The externs are for things in the closure compiler's externs
folder. braille_ime, however, has its own externs.js, which here, as in the GYP
file, has its own compile target.

https://codereview.chromium.org/2247353004/diff/60001/third_party/closure_com...
File third_party/closure_compiler/run_compiler (right):

https://codereview.chromium.org/2247353004/diff/60001/third_party/closure_com...
third_party/closure_compiler/run_compiler:18: gn gen
--args='root_extra_deps=["//third_party/closure_compiler:compile_js_all"]'
out/Default
On 2016/09/06 16:46:46, Dan Beam wrote:
> i don't think we want to:
> 
> a) run both GN and GYP duplicatively
> b) ask everybody to use GN quite yet
> 
> let's either make a new script or just wait to swap the use of GYP in this
file.
>  lots of folks use this script daily, I don't want to break their work flows. 
> i'd say we can switch this in a week or two, or whenever this work settles
down.

My thought was to take individual dependencies out of compiled_resources2.gyp as
I add them to BUILD.gn. This isn't yet in this patch set because I wanted to get
agreement on the direction before I did much more.

I don't, however, feel strongly about how we make the changeover, so I am happy
to do create a separate build file if you think this is better.

https://codereview.chromium.org/2247353004/diff/60001/third_party/jstemplate/...
File third_party/jstemplate/BUILD.gn (right):

https://codereview.chromium.org/2247353004/diff/60001/third_party/jstemplate/...
third_party/jstemplate/BUILD.gn:6: compile_js("compile_util") {
On 2016/09/06 16:46:46, Dan Beam wrote:
> why are we prefixing with compile_ everywhere?  what's the value?  this is
> mainly typechecking right now, and this differs from the old system (as it
adds
> a lot of fluff to every target name).
> 
> this already reads as "compile JS <name>".  with the "compile_" prefix it
reads
> as "compile JS compile <name>", i.e. "compile the compiled thing" (IMO)

The issue is that one can't have multiple BUILD.gn files in a single directory,
so target names have to be unique within a directory. If, as is the case with
braille_ime, the folder already has (or could in future contain) targets that
are part of the main build we want to be sure that our target names wont
conflict with them. It seemed to me that using just the file name root as the
target name (as we do in GYP) was quite likely to conflict, so I invented the
convention of adding compile_ to the front of these target names.

[aberent, 2016-09-07 09:31:18]

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

https://codereview.chromium.org/2247353004/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile2.py:194: source_list_file=None,
On 2016/09/06 16:46:46, Dan Beam wrote:
> can we call this like gn_source_list_file to make it clearer it's coming from
a
> format GN controls (and doesn't apply to the GYP version of this for the short
> time it exists)?

Done.

https://codereview.chromium.org/2247353004/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile2.py:221: if (source_list_file):
On 2016/09/06 16:46:46, Dan Beam wrote:
> if source_list_file:

Done.

https://codereview.chromium.org/2247353004/diff/60001/third_party/closure_com...
third_party/closure_compiler/compile2.py:347: source_list_file =
opts.source_list_file,
On 2016/09/06 16:46:46, Dan Beam wrote:
> no spaces between =, i.e. source_list_file=opts.source_list_file,

Done.

[Dan Beam, 2016-09-07 18:18:05]

getting closer

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

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/compile2.py:347:
gn_source_list_file=opts.gn_source_list_file,
nit: wrap at 80 cols

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
File third_party/closure_compiler/compile_js2.gni (right):

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/compile_js2.gni:34: action(target_name) {
can we just automatically pref targets if we're doing to do that?  i.e.
"compile_js" + target_name here?

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/compile_js2.gni:50: out_file =
"${target_gen_dir}/closure/${target_name}.js"
why is this /closure/ here?  was this in the old system?

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/compile_js2.gni:55:
"${CLOSURE_DIR}/build/inputs.py",
where are inputs.py or outputs.py invoked?  they used to be invoked as actions
to generate a list of dependent source files.  i don't see where that's
happening now...

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/compile_js2.gni:61: sources = source_files
can we not use sources directly rather than "source_files" => "sources"?

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
File third_party/closure_compiler/run_compiler (right):

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/run_compiler:18: gn gen
--args='root_extra_deps=["//third_party/closure_compiler:compile_js_all"]'
out/Default
80 col wrap

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/run_compiler:18: gn gen
--args='root_extra_deps=["//third_party/closure_compiler:compile_js_all"]'
out/Default
can we change the name of where these are generated to?  i.e.

gyp -Goutput_dir=closure_compilation
ninja -C closure_compilation/Default

gn gen --args='...' closure_compilation/gn
ninja -C closure_compilation/gn

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/run_compiler:18: gn gen
--args='root_extra_deps=["//third_party/closure_compiler:compile_js_all"]'
out/Default
nit: store "third_party/closure_compiler:compile_js_all" in variable and reuse
it in both places, i.e.

GN_ALL_TARGET="third_party/closure_compiler:compile_js_all"

gn gen --args="root_extra_deps=[""//${GN_ALL_TARGET}""]"
ninja -C closure_compilation/gn "${GN_ALL_TARGET}"

[aberent, 2016-09-08 10:29:19]

Some answers, but no new upload yet.

I don't think it is worth doing much more than this until I have an answer on
the ordering of data dependencies (see my email from this morning).

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
File third_party/closure_compiler/compile_js2.gni (right):

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/compile_js2.gni:34: action(target_name) {
On 2016/09/07 18:18:05, Dan Beam wrote:
> can we just automatically pref targets if we're doing to do that?  i.e.
> "compile_js" + target_name here?

We could, and I wondered about this, but given that the dependencies have to be
on the full target name I thought that might confuse things more than clarifying
them.

The issue is that the BUILD.gn file would end up containing:


compile_js("a") {
 ...
}

compile_js("b") {
...
  deps = [":compile_js_a"]

}


Unless one knows then it might not be entirely obvious that the target
"compile_js_a" is created by the template (and, in particular, a text search
won't find it). In general the convention for template usage in GN seems to be
to pass the primary target name as the argument.

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/compile_js2.gni:55:
"${CLOSURE_DIR}/build/inputs.py",
On 2016/09/07 18:18:05, Dan Beam wrote:
> where are inputs.py or outputs.py invoked?  they used to be invoked as actions
> to generate a list of dependent source files.  i don't see where that's
> happening now...

Unless I have missed something inputs.py isn't actually used by
compile_js2.gypi, only by compile_js.gypi. We clearly can't use inputs.py in its
current form since it is reading the gyp files to find dependencies, but, in
principle, we should be able to use GN runtime_dependencies to get the file list
(although see my email of this morning).

output.py is doing some simple file name manipulation which can be done (I
think) by GN directly.

Although exec_script exists in GN it is generally a bad idea to run scripts at
GN time, since a slow script slows down GN for everybody (remember that one can
only have one BUILD.gn file per directory).

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/compile_js2.gni:61: sources = source_files
On 2016/09/07 18:18:05, Dan Beam wrote:
> can we not use sources directly rather than "source_files" => "sources"?

I tried this originally, and it didn't seem to work. I am not exactly sure why,
but "sources" has a special meaning in actions, and assigning to it more than
once (or modifying it) seemed to confuse GN.

[Dan Beam, 2016-09-08 17:53:21]

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
File third_party/closure_compiler/compile_js2.gni (right):

https://codereview.chromium.org/2247353004/diff/80001/third_party/closure_com...
third_party/closure_compiler/compile_js2.gni:55:
"${CLOSURE_DIR}/build/inputs.py",
On 2016/09/08 10:29:19, aberent wrote:
> On 2016/09/07 18:18:05, Dan Beam wrote:
> > where are inputs.py or outputs.py invoked?  they used to be invoked as
actions
> > to generate a list of dependent source files.  i don't see where that's
> > happening now...
> 
> Unless I have missed something inputs.py isn't actually used by
> compile_js2.gypi, only by compile_js.gypi. We clearly can't use inputs.py in
its
> current form since it is reading the gyp files to find dependencies, but, in
> principle, we should be able to use GN runtime_dependencies to get the file
list
> (although see my email of this morning).
> 
> output.py is doing some simple file name manipulation which can be done (I
> think) by GN directly.
> 
> Although exec_script exists in GN it is generally a bad idea to run scripts at
> GN time, since a slow script slows down GN for everybody (remember that one
can
> only have one BUILD.gn file per directory).

yeah, I just want to remove from inputs = [] if we don't need them there

[aberent, 2016-09-15 10:27:30]

I just added my draft version of tools/polymer/generate_compiled_resources_gn.py
to the CL, since Dan is, at least for now, taking this over. It still needs
tidying up to pass presubmit checks, but seems to work.

[Dirk Pranke, 2016-09-16 01:34:11]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[aberent, 2016-12-05 14:25:29]

On 2016/09/15 10:27:30, aberent wrote:
> I just added my draft version of
tools/polymer/generate_compiled_resources_gn.py
> to the CL, since Dan is, at least for now, taking this over. It still needs
> tidying up to pass presubmit checks, but seems to work.

Dan, did you actually take this over and finish it? If so then I would like to
close this CL.

[aberent, 2016-12-05 14:25:31]

On 2016/09/15 10:27:30, aberent wrote:
> I just added my draft version of
tools/polymer/generate_compiled_resources_gn.py
> to the CL, since Dan is, at least for now, taking this over. It still needs
> tidying up to pass presubmit checks, but seems to work.

Dan, did you actually take this over and finish it? If so then I would like to
close this CL.

[Dan Beam, 2016-12-05 20:53:14]

On 2016/12/05 14:25:31, aberent wrote:
> On 2016/09/15 10:27:30, aberent wrote:
> > I just added my draft version of
> tools/polymer/generate_compiled_resources_gn.py
> > to the CL, since Dan is, at least for now, taking this over. It still needs
> > tidying up to pass presubmit checks, but seems to work.
> 
> Dan, did you actually take this over and finish it? If so then I would like to
> close this CL.

Dirk keeps threatening to work on this :)

[Dirk Pranke, 2016-12-06 02:40:03]

On 2016/12/05 20:53:14, Dan Beam wrote:
> On 2016/12/05 14:25:31, aberent wrote:
> > On 2016/09/15 10:27:30, aberent wrote:
> > > I just added my draft version of
> > tools/polymer/generate_compiled_resources_gn.py
> > > to the CL, since Dan is, at least for now, taking this over. It still
needs
> > > tidying up to pass presubmit checks, but seems to work.
> > 
> > Dan, did you actually take this over and finish it? If so then I would like
to
> > close this CL.
> 
> Dirk keeps threatening to work on this :)

Yes ... I promise to have a real update soon. The last couple weeks have been
crazy for me but things are settling down now, I think.