Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2800833004:
Create js_library and js_binary templates for closure compiling. (Closed)
|
Created:
3 years, 8 months ago by damargulis Modified:
3 years, 8 months ago CC:
aberent, brettw, chromium-reviews, dbeam+watch-closure_chromium.org, dpapad, Dirk Pranke, jlklein+watch-closure_chromium.org, vitalyp+closure_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate js_library and js_binary templates for closure compiling.
This creates the js_library and js_binary templates to be used within
chromium. These templates simulate their matching partners in Blaze.
The js_library template is used to create an ordering for .js files.
The js_binary template is used to compile .js files and js_library targets.
BUG=632206
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2800833004
Cr-Commit-Position: refs/heads/master@{#466471}
Committed: https://chromium.googlesource.com/chromium/src/+/5c5337d8b474a6d07320536d0bab8bfbf548c219
Patch Set 1 #Patch Set 2 : Remove unused import #Patch Set 3 : fix comments #Patch Set 4 : clean #Patch Set 5 : add common_closure_args #Patch Set 6 : format #Patch Set 7 : use get_label_info #
Total comments: 32
Patch Set 8 : fixes #
Total comments: 10
Patch Set 9 : . #
Total comments: 2
Patch Set 10 : nits #Patch Set 11 : fixes #Patch Set 12 : format #
Total comments: 9
Patch Set 13 : fixes #
Total comments: 44
Patch Set 14 : fixes #
Total comments: 8
Patch Set 15 : glint #Patch Set 16 : fixes #
Total comments: 2
Patch Set 17 : use compile2 #
Total comments: 1
Patch Set 18 : use minifying_closure_args #Patch Set 19 : update comments #
Messages
Total messages: 49 (13 generated)
Description was changed from ========== Create js_library and js_binary templates for closure compiling. This creates the js_library and js_binary templates to be used within chromium. These templates simulate their matching partners in Blaze. The js_library template is used to create an ordering for .js files. The js_binary template is used to compile .js files and js_library targets. BUG= ========== to ========== Create js_library and js_binary templates for closure compiling. This creates the js_library and js_binary templates to be used within chromium. These templates simulate their matching partners in Blaze. The js_library template is used to create an ordering for .js files. The js_binary template is used to compile .js files and js_library targets. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
damargulis@chromium.org changed reviewers: + alokp@chromium.org, slan@chromium.org
Hi Stephen,
Alok mentioned that you know alot about gn actions, so I'd really appreciate you
taking a look at this patch and letting me know what you think!
This patch creates js_library and js_binary templates that can be used to
compile javascript, similarly to the way those rules work in Blaze. This will
be useful anywhere in chromium where javascript needs to be compiled, and
particularly in creating the crypto bindings on cast.__platform__. I tried to
keep the usage of the rules as similar to their implementations in Blaze as
possible, changing mostly just for syntax. I believe I included all of the
variables used in Blaze that might still be relevant in chromium, but if I
missed any then Id be happy to add them.
The js_library rule takes in sources and deps and creates a text file listing
them out so they can later be consumed by a js_binary rule. The format of the
text file is:
sources:
<list of sources>
deps:
<list of deps>
The js_binary then parses the dependency tree created by these files, and passes
all arguments into the closure compiler.
There are still two issues that I don't know how to work around, but I'm not
sure if they are necessarily a problem. First, the library text files are not
cleaned up and remain in the /gen/ directory. I could clean them all up after
the js_binary rule, but that would create errors if two js_binary rules relied
on the same js_library rules. Is there some sort of post-build rule I can
create to clean up those files?
The second issue comes from using get_target_outputs() in the two rules. This
function requires that the target it is looking for is defined previously in the
file. This means that it enforces an ordering of targets that is not enforced
in Blaze. For example,
js_library(
name = "apple_tree",
srcs = ["tree_main.js"],
deps = [
":branch",
":trunk",
":root",
],
)
js_library(
name = "branch",
srcs = [
"branch.js",
"knot.js",
],
msgs = [
"branch_msg__en.js",
"branch_msg__de.js",
],
deps = [
":bark",
":trunk",
":leaf",
":apple",
],
)
works fine in blaze, but would throw an error here. To properly compile them,
the branch target would need to be defined before the apple_tree target.
I really appreciate any help with these issues, or any other comments you have
before I send this out to a larger group for review.
Thanks!
mbjorge@chromium.org changed reviewers: + mbjorge@chromium.org
Description was changed from ========== Create js_library and js_binary templates for closure compiling. This creates the js_library and js_binary templates to be used within chromium. These templates simulate their matching partners in Blaze. The js_library template is used to create an ordering for .js files. The js_binary template is used to compile .js files and js_library targets. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Create js_library and js_binary templates for closure compiling. This creates the js_library and js_binary templates to be used within chromium. These templates simulate their matching partners in Blaze. The js_library template is used to create an ordering for .js files. The js_binary template is used to compile .js files and js_library targets. BUG=632206 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Some unsolicited feedback :D I think you would be better off finding an alternative to get_target_outputs, since it cannot reference targets in other files (https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...). I think this will end up being quite an inconvenience if this gets wider spread use. As an alternative, you could use get_label_info(dep, "target_gen_dir") + get_label_info(dep, "name") [https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/reference.md#get_label_info] and re-build the output name yourself (you control what it will be, since you are making the template). Then you can depend on targets from anywhere in the codebase. And as noted offline, you probably want to incorporatae the args defined in //third_party/closure_compiler/closure_args.gni
On 2017/04/06 22:42:53, mbjorge wrote: > Some unsolicited feedback :D > > I think you would be better off finding an alternative to get_target_outputs, > since it cannot reference targets in other files > (https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...). > I think this will end up being quite an inconvenience if this gets wider spread > use. > > As an alternative, you could use get_label_info(dep, "target_gen_dir") + > get_label_info(dep, "name") > > [https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/reference.md#get_label_info] > and re-build the output name yourself (you control what it will be, since you > are making the template). Then you can depend on targets from anywhere in the > codebase. > > > > And as noted offline, you probably want to incorporatae the args defined in > //third_party/closure_compiler/closure_args.gni Thanks for the advice! Using get_label_info worked perfectly!
On 2017/04/06 22:57:52, damargulis wrote: > On 2017/04/06 22:42:53, mbjorge wrote: > > Some unsolicited feedback :D > > > > I think you would be better off finding an alternative to get_target_outputs, > > since it cannot reference targets in other files > > > (https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...). > > I think this will end up being quite an inconvenience if this gets wider > spread > > use. > > > > As an alternative, you could use get_label_info(dep, "target_gen_dir") + > > get_label_info(dep, "name") > > > > > [https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/reference.md#get_label_info] > > and re-build the output name yourself (you control what it will be, since you > > are making the template). Then you can depend on targets from anywhere in the > > codebase. > > > > > > > > And as noted offline, you probably want to incorporatae the args defined in > > //third_party/closure_compiler/closure_args.gni > > Thanks for the advice! Using get_label_info worked perfectly! I suppose this brings up a new issue however. Since ordering is no longer enforced, it is possible for someone to declare a circular dependency. If that happens right now, my python script would go into an infinite loop. I don't see anywhere in the Blaze documentation detailing what happens in this case. Should I throw an error, or just silently ignore any repeated dependencies?
On 2017/04/06 at 23:01:56, damargulis wrote: > On 2017/04/06 22:57:52, damargulis wrote: > > On 2017/04/06 22:42:53, mbjorge wrote: > > > Some unsolicited feedback :D > > > > > > I think you would be better off finding an alternative to get_target_outputs, > > > since it cannot reference targets in other files > > > > > (https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...). > > > I think this will end up being quite an inconvenience if this gets wider > > spread > > > use. > > > > > > As an alternative, you could use get_label_info(dep, "target_gen_dir") + > > > get_label_info(dep, "name") > > > > > > > > [https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/reference.md#get_label_info] > > > and re-build the output name yourself (you control what it will be, since you > > > are making the template). Then you can depend on targets from anywhere in the > > > codebase. > > > > > > > > > > > > And as noted offline, you probably want to incorporatae the args defined in > > > //third_party/closure_compiler/closure_args.gni > > > > Thanks for the advice! Using get_label_info worked perfectly! > > I suppose this brings up a new issue however. Since ordering is no longer enforced, it is possible for someone to declare a circular dependency. If that happens right now, my python script would go into an infinite loop. I don't see anywhere in the Blaze documentation detailing what happens in this case. Should I throw an error, or just silently ignore any repeated dependencies? GN should protect you from dependency cycles. It will error out with 'ERROR Dependency cycle:' and list the bad dep
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
cc'd some folks that I've talked about this with in the past
imo, this looks pretty good! Mostly just style comments from me https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:12: # Usage is similar but adapted to fit the .gn syntax. I don't think the references to the corresponding Blaze targets are necessary/helpful. The other documentatiin here makes the usage clear. -Based off of the js_library rule in Blaze. Usage is similar but adapted to fit the .gn syntax https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:19: # List of js_library targets to depend on Do you know what would happen if an non-js_library target was added as a dep? https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:22: # in Blaze: remove Blaze example https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:63: target_path = get_label_info(dep, "target_gen_dir") + "/" + I think if you mirror the format of the outputs target above, it's even more clear you are building the same path. I think a comment to highlight explicitly this is supposed to match the output would be good too. e.g.: dep_gen_dir = get_label_info(dep, "target_gen_dir") dep_name = get_label_info(dep, "name") target_path = "$dep_gen_dir/$dep_name.txt" https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:71: # Defines a target that compiles javascript files using the closure compiler Would be good to add a sentence that tells someone why they might want to do use this target: error checking? syntax checking? minified / optimized output file? https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:72: # Based off of the js_binary rule in Blaze. Usage is similar but adapted -Blaze refenenes https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:97: # in Blaze: remove blaze example https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:120: outputs = [ if the generated .js file is actually useful, then you may want to let the user specify an output parameter so they can control where it goes. But if it's not a file that someone can use, then this is fine. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:25: def FindCommand(command): add one line doc strings for each of the methods https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:26: fpath, _ = os.path.split(command) rename: fpath -> filepath or command_name https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:58: def postOrder(deps, sources): nit: PostOrder https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:60: with file(dep, 'r') as dep_list: with open(dep, 'r') as dep_list When opening a file, it’s preferable to use open() instead of invoking [file()] directly https://docs.python.org/2/library/functions.html#file https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:62: split = lines.index('deps:') Maybe pull this out into a helper method? new_sources, new_deps = ParseDepList(dep) def ParseDepList(dep): with open(dep, 'r') as dep_list: lines = [line.strip() for line in dep_list.readlines()] split = lines.index('deps:') return lines[1:split], lines[split+1:] Or maybe just give some nice names to the different parts: source_lines, dep_lines = lines[1:split], lines[split+1:] https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:97: 'SIMPLE_OPTIMIZATIONS', does it make sense for this to be in the GN template instead? This is defined in //third_party/clouser_compiler/closure_args.gni :: checking_clouser_args, maybe you want to just include all of those by default also? Possibly with a boolean option (that defaults true) that can be set to false if someone doesn't want to check their target (the ios version has checked and unchecked targets, so presumably there is a usecase for not checking? https://cs.chromium.org/chromium/src/ios/web/js_compile.gni?q=js_comp&l=100)
https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:12: # Usage is similar but adapted to fit the .gn syntax. On 2017/04/07 19:23:18, mbjorge wrote: > I don't think the references to the corresponding Blaze targets are > necessary/helpful. The other documentatiin here makes the usage clear. > > -Based off of the js_library rule in Blaze. > Usage is similar but adapted to fit the .gn syntax Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:19: # List of js_library targets to depend on On 2017/04/07 19:23:18, mbjorge wrote: > Do you know what would happen if an non-js_library target was added as a dep? Right now, the python script will error out. It will fail when it attempts to read the output file, unless the dependency also happens to have created an output file with the same format name that the js_library action is expecting. If it did happen to create a file like that, the python script will still likely error out when it is reading the data in that file, as it won't fit the format it is expecting. This could potentially be solved by checking in the python script if a file exists before attempting to read it, but it is still possible that another rule may just happen to create a file with the same name that js_library is expecting, which would still create problems. I think it makes sense to only include js_librarys as dependencies, as I don't think there will be any use cases where this is a problem. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:22: # in Blaze: On 2017/04/07 19:23:18, mbjorge wrote: > remove Blaze example Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:63: target_path = get_label_info(dep, "target_gen_dir") + "/" + On 2017/04/07 19:23:18, mbjorge wrote: > I think if you mirror the format of the outputs target above, it's even more > clear you are building the same path. I think a comment to highlight explicitly > this is supposed to match the output would be good too. > > e.g.: > dep_gen_dir = get_label_info(dep, "target_gen_dir") > dep_name = get_label_info(dep, "name") > target_path = "$dep_gen_dir/$dep_name.txt" Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:71: # Defines a target that compiles javascript files using the closure compiler On 2017/04/07 19:23:18, mbjorge wrote: > Would be good to add a sentence that tells someone why they might want to do use > this target: error checking? syntax checking? minified / optimized output file? Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:72: # Based off of the js_binary rule in Blaze. Usage is similar but adapted On 2017/04/07 19:23:17, mbjorge wrote: > -Blaze refenenes Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:97: # in Blaze: On 2017/04/07 19:23:18, mbjorge wrote: > remove blaze example Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:120: outputs = [ On 2017/04/07 19:23:17, mbjorge wrote: > if the generated .js file is actually useful, then you may want to let the user > specify an output parameter so they can control where it goes. > > But if it's not a file that someone can use, then this is fine. Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:25: def FindCommand(command): On 2017/04/07 19:23:18, mbjorge wrote: > add one line doc strings for each of the methods Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:26: fpath, _ = os.path.split(command) On 2017/04/07 19:23:18, mbjorge wrote: > rename: fpath -> filepath or command_name Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:58: def postOrder(deps, sources): On 2017/04/07 19:23:18, mbjorge wrote: > nit: PostOrder Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:60: with file(dep, 'r') as dep_list: On 2017/04/07 19:23:18, mbjorge wrote: > with open(dep, 'r') as dep_list > > When opening a file, it’s preferable to use open() instead of invoking [file()] > directly > https://docs.python.org/2/library/functions.html#file Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:62: split = lines.index('deps:') On 2017/04/07 19:23:18, mbjorge wrote: > Maybe pull this out into a helper method? > > new_sources, new_deps = ParseDepList(dep) > > def ParseDepList(dep): > with open(dep, 'r') as dep_list: > lines = [line.strip() for line in dep_list.readlines()] > split = lines.index('deps:') > return lines[1:split], lines[split+1:] > > > Or maybe just give some nice names to the different parts: > source_lines, dep_lines = lines[1:split], lines[split+1:] Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/js_binary.py:97: 'SIMPLE_OPTIMIZATIONS', On 2017/04/07 19:23:18, mbjorge wrote: > does it make sense for this to be in the GN template instead? > > This is defined in //third_party/clouser_compiler/closure_args.gni :: > checking_clouser_args, maybe you want to just include all of those by default > also? > > Possibly with a boolean option (that defaults true) that can be set to false if > someone doesn't want to check their target (the ios version has checked and > unchecked targets, so presumably there is a usecase for not checking? > https://cs.chromium.org/chromium/src/ios/web/js_compile.gni?q=js_comp&l=100) I think it makes sense to add this in from the GN template, but Im not sure if we should create an option to leave it unchecked. In ios, the unchecked target is only used for legacy code that doesn't pass the checks, and there is currently a TODO to fix all the code and remove the unchecked target. I think it makes more sense to require checking when using this template to ensure that code always can pass the checks. Ideally, whenever anyone uses this template they will not commit the code until it passes all these checks anyway. Also, checking_closure_args includes polymer_pass, so I will need to include the polymer externs by default, but this already exists in closure_compiler/externs/polymer-1.0.js, so this is simple to do.
This patch looks really solid to me, especially after Mike's comments. Nice work. lgtm. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:53: outputs = [ super-nit: This needs to be an array, but your python script won't parse correctly if this ever grows to be more than one element (because of the --output flags config). I would either do something like this: output_file = "$target_gen_dir/$target_name.txt" outputs = [ output_file ] args = [ "--output" ] + rebase_path(output_file, root_build_dir) or make your script handle this error case. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:107: # js_binary("tree") { nit: put bootstrap and config files in the example. https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... third_party/closure_compiler/js_binary.py:29: if filepath: nit: Use "and" here.
https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... third_party/closure_compiler/js_binary.py:30: if IsExecutable(command): should this be IsExecutable(filepath)?
just a few more nits If you wanted, you could use a .js_library extension instead of .txt, and then your js_binary.py script could error out if it finds non-*.js_library deps. Not sure if it's necessary; main advantage is you can give better error messaging in the event someone is holding it wrong https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:117: target_path = get_label_info(dep, "target_gen_dir") + "/" + copy from above; makes it clear it's doing the same thing https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:131: args += [ "--defs" ] + default_closure_args Add a comment above here: # |default_closure_args| from //third_party/closure_compiler/closure_args.gni https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... third_party/closure_compiler/js_binary.py:37: if not ext: exts = [ext] if ext else os.environ['PATHEXT'].split(os.path.pathsep)
I changed the extension from .txt -> .js_library, but I was not able to check for .js_library extensions on the deps inside of the python script, since the .js_library extension is added to each name as part of the GN action. Instead I was able to put an assertion that each file exists, and if the file does not exist, throw an error saying that it the target is not a js_library action. I think its safe to assume that no other actions will create files with a .js_library extension, so this should work. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:53: outputs = [ On 2017/04/07 23:08:39, slan wrote: > super-nit: This needs to be an array, but your python script won't parse > correctly if this ever grows to be more than one element (because of the > --output flags config). I would either do something like this: > > output_file = "$target_gen_dir/$target_name.txt" > outputs = [ output_file ] > args = [ "--output" ] + rebase_path(output_file, root_build_dir) > > or make your script handle this error case. Done. https://codereview.chromium.org/2800833004/diff/120001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:107: # js_binary("tree") { On 2017/04/07 23:08:39, slan wrote: > nit: put bootstrap and config files in the example. Done. https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:117: target_path = get_label_info(dep, "target_gen_dir") + "/" + On 2017/04/07 23:30:19, mbjorge wrote: > copy from above; makes it clear it's doing the same thing Done. https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:131: args += [ "--defs" ] + default_closure_args On 2017/04/07 23:30:19, mbjorge wrote: > Add a comment above here: > > # |default_closure_args| from //third_party/closure_compiler/closure_args.gni Done. https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... third_party/closure_compiler/js_binary.py:29: if filepath: On 2017/04/07 23:08:39, slan wrote: > nit: Use "and" here. Done. https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... third_party/closure_compiler/js_binary.py:30: if IsExecutable(command): On 2017/04/07 23:11:19, mbjorge wrote: > should this be IsExecutable(filepath)? I don't think so. I borrowed most of this code from https://cs.corp.google.com/eureka_internal/chromium/src/build/util/java_actio... with some minor changes. They have it as IsExecutable(command) there. I think the idea is that if filepath exists, then it means that command was passed in as a full path, and may lead directly to an executable. filepath would just be the directory that the executable lives in. https://codereview.chromium.org/2800833004/diff/140001/third_party/closure_co... third_party/closure_compiler/js_binary.py:37: if not ext: On 2017/04/07 23:30:19, mbjorge wrote: > exts = [ext] if ext else os.environ['PATHEXT'].split(os.path.pathsep) Done.
lgtm % last couple nits https://codereview.chromium.org/2800833004/diff/160001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/160001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:73: # Only takes in a single file, but should be placed in a list must be https://codereview.chromium.org/2800833004/diff/160001/third_party/closure_co... File third_party/closure_compiler/js_library.py (right): https://codereview.chromium.org/2800833004/diff/160001/third_party/closure_co... third_party/closure_compiler/js_library.py:23: with file(args.output, 'w') as out: with open()
I noticed a few errors while attempting to use this, so I uploaded a couple quick fixes. default_closure_args included the argument "checks_only", which prevented any output from being created. I now take that away from the array before sending to the python script, but the user still has the option to include that in their self defined defs attribute, if they only want to use the target to check and not compile. I also noticed that the script was not properly ordering the scripts when a js_binary rule included a source as well as deps (it was putting the source at the beginning of the file instead of the end). This has now been fixed. Finally, externs_list wasn't being read by the compiler properly. The argument does not accept a list, so each extern list need to be explicitly listed as such, or only the first extern file was read. I changed: if(args.externs): compiler_args += ['--externs'] + args.externs to: for extern in args.externs: compiler_args += ['--externs=' + extern] which solved this problem.
On 2017/04/11 at 22:45:28, damargulis wrote: > I noticed a few errors while attempting to use this, so I uploaded a couple quick fixes. > > default_closure_args included the argument "checks_only", which prevented any output from being created. I now take that away from the array before sending to the python script, but the user still has the option to include that in their self defined defs attribute, if they only want to use the target to check and not compile. > > I also noticed that the script was not properly ordering the scripts when a js_binary rule included a source as well as deps (it was putting the source at the beginning of the file instead of the end). This has now been fixed. > > Finally, externs_list wasn't being read by the compiler properly. The argument does not accept a list, so each extern list need to be explicitly listed as such, or only the first extern file was read. > > I changed: > if(args.externs): > compiler_args += ['--externs'] + args.externs > > to: > for extern in args.externs: > compiler_args += ['--externs=' + extern] > > which solved this problem. lgtm
Friendly ping for OWNER review!
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
Nice! This is more or less what I had in mind but never got around to implementing :). lgtm w/ comments addressed. https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:32: "Need sources or deps in $target_name for js_library") or, not and? https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:112: forward_variables_from(invoker, "*") Is the forward_variables_from() really necessary? It seems like referring to invoker.sources, invoker.deps, etc is a little safer. https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... third_party/closure_compiler/js_binary.py:10: optional --defs argument which will add custom flags to the comiler. Any extern s/comiler/compiler/ https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... third_party/closure_compiler/js_binary.py:23: """Returns whether file at |path| exists and is executable.""" Generally I discourage docstrings that tend to be obvious, which is what this and the next two are :).
Thanks for the help Dirk! I made some of the changes you mentioned, and left a couple comments. I also noticed that in my old implementation, GN would not rerun after changes to any files listed as bootstrap, config, or externs, so I now add them to the sources variable so GN knows to monitor them as well. https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:32: "Need sources or deps in $target_name for js_library") On 2017/04/14 02:01:44, Dirk Pranke wrote: > or, not and? I think it should be or. It definitely needs to be valid to have a js_library that has only sources, as this is the bottom of the dependency tree. It should also be ok to have only deps but no sources, if you want to create a target that aggregates together several other targets without adding any additional files. As long as either deps or sources is present than the rule should be valid. https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:112: forward_variables_from(invoker, "*") On 2017/04/14 02:01:44, Dirk Pranke wrote: > Is the forward_variables_from() really necessary? It seems like referring to > invoker.sources, invoker.deps, etc is a little safer. The action needs to be explicitly told what its sources/deps/outputs are so that ninja knows when to rerun the build. However, I can explicitly label the variables to forward instead of using "*", which should make this safer to use. https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... third_party/closure_compiler/js_binary.py:10: optional --defs argument which will add custom flags to the comiler. Any extern On 2017/04/14 02:01:44, Dirk Pranke wrote: > s/comiler/compiler/ Done. https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... third_party/closure_compiler/js_binary.py:23: """Returns whether file at |path| exists and is executable.""" On 2017/04/14 02:01:44, Dirk Pranke wrote: > Generally I discourage docstrings that tend to be obvious, which is what this > and the next two are :). Done.
lgtm! https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/220001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:32: "Need sources or deps in $target_name for js_library") ok, just checking.
The CQ bit was checked by damargulis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org, mbjorge@chromium.org Link to the patchset: https://codereview.chromium.org/2800833004/#ps240001 (title: "fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
hey Daniel, I think this generally looks good and I think this could be a great replacement for the GYP-based system. Have you tried this on any real code to see whether the dependency order is useful/correct? Is there a way you could write some tests for the existing code? Maybe the dependency order traversal? Here's an example of what I tested when changing the dependency order in GYP: https://codereview.chromium.org/1182323007/diff/80001/test/dependencies/adso/... https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:61: # Defines a target that compiles javascript files using the closure compiler. nit: I don't think it matters which compiler is used, but if you really want to keep the name, capitalize "Closure"? https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:63: # peform error, syntax, and type checking. Additional checks and options perform https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:77: # bootstrap_file: is there a concrete need for this right now? https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:80: # config_files: would it make sense to name this "dependencies" or "deps" or something? it's not clear to me that without these files compilation might fail (meaning you /depend/ on them being there) https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:84: # defs: closure_flags is the name we've used previously and is less blaze-specific (and truer to what these actually are, IMO) https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:16: import subprocess it seems more common in chrome to use subprocess2 and Popen() and communicate(). is there a reason why you're not doing that? https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:19: EXIT_SUCCESS = 0 unused https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:24: file-level globals should have 2 \n between them https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:52: return subprocess.check_call([java_path, '-jar'] + args) can you share this code across compile.py and compile2.py as well? https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:56: assert os.path.isfile(dep), dep[:-11] + ' is not a js_library target' what does -11 mean here? https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:58: lines = [line.strip() for line in dep_list.readlines()] use dep_list.splitlines() instead of readlines() + strip() https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:59: split = lines.index('deps:') what if this fails? https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:62: def PostOrder(deps, sources): can we name this something else? this sounds like you're buying a sandwich online https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:100: ] + defs what is doing this formatting? this would fit on one line... https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:103: compiler_args += ['--externs=' + extern] compiler_args += ['--externs=%s' % e for e in args.externs] https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:103: compiler_args += ['--externs=' + extern] the python style guide discourages use of + for string concatenation in favor of '%s' % arg https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:110: if(args.bootstrap): no () (this ain't C) https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:110: if(args.bootstrap): space after if https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:117: sys.exit(main()) it doesn't look like main() actually returns anything on success? sys.exit() ends the program early, so no real reason to pass the return value, IMO, just crash the program in the middle (just make sure to clean up first) https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... File third_party/closure_compiler/js_library.py (right): https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_library.py:24: out.write('sources:\n') could this be: out.write('sources:\n%s\ndeps:%s' % ('\n'.join(sources), '\n'.join(deps))) https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_library.py:32: sys.exit(main()) i don't see a reason to do this sys.exit() nor to import sys after you've removed it
https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:103: compiler_args += ['--externs=' + extern] On 2017/04/18 02:05:50, Dan Beam wrote: > the python style guide discourages use of + for string concatenation in favor of > '%s' % arg oh, i guess it's a little more nuanced than that: https://google.github.io/styleguide/pyguide.html#Strings
Thanks for all the help Dan! I made most of the changes you suggested, and left a couple comments. As of now, I had been testing this by manually creating arbitrary js_library and js_binary targets and checking the order the source files were compiled in. For example, I used the example given in the blaze js_library (https://g3doc.corp.google.com/devtools/blaze/g3doc/be/javascript.html#js_library) and ensured that my compilation completed in the same order. I can also write some tests for these files. Is there a module for .gn similar to the TestGyp module you shared that I could use? https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:61: # Defines a target that compiles javascript files using the closure compiler. On 2017/04/18 02:05:49, Dan Beam wrote: > nit: I don't think it matters which compiler is used, but if you really want to > keep the name, capitalize "Closure"? Done. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:63: # peform error, syntax, and type checking. Additional checks and options On 2017/04/18 02:05:49, Dan Beam wrote: > perform Done. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:77: # bootstrap_file: On 2017/04/18 02:05:49, Dan Beam wrote: > is there a concrete need for this right now? One use case for this would be in chromecast for implementing the cast.__platform__ object. We would create js_library targets for each part of the cast.__platform__ object, (ex. cast.__platform__.crypto), and a single js_binary target to compile all of them together. However, we would need a file before all of the library targets which initializes the cast.__platform__ object https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:80: # config_files: On 2017/04/18 02:05:49, Dan Beam wrote: > would it make sense to name this "dependencies" or "deps" or something? > > it's not clear to me that without these files compilation might fail (meaning > you /depend/ on them being there) There is already a deps variable that holds the js_library rules that the binary depends on. I think calling this dependencies would confuse those two variables. I took the name config_files from the blaze rule which uses that name. I am open to changing it, but I am not sure what to. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:84: # defs: On 2017/04/18 02:05:49, Dan Beam wrote: > closure_flags is the name we've used previously and is less blaze-specific (and > truer to what these actually are, IMO) Done. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:16: import subprocess On 2017/04/18 02:05:50, Dan Beam wrote: > it seems more common in chrome to use subprocess2 and Popen() and communicate(). > > is there a reason why you're not doing that? I based most of the code to run the jar from java_action.py (https://cs.chromium.org/chromium/src/build/util/java_action.py?q=java_action....) Is there any advantage of switching from check_call() to Popen()? https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:19: EXIT_SUCCESS = 0 On 2017/04/18 02:05:50, Dan Beam wrote: > unused Done. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:24: On 2017/04/18 02:05:49, Dan Beam wrote: > file-level globals should have 2 \n between them Done. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:52: return subprocess.check_call([java_path, '-jar'] + args) On 2017/04/18 02:05:49, Dan Beam wrote: > can you share this code across compile.py and compile2.py as well? compile.py and compile2.py use the Checker class, which runs the compiler through the _run_jar method. Since this is a private method, I don't want to call that directly, and I don't want to use the exposed check method, as this does a lot of additional work and modifications that I don't want done. I could change compile.py and compile2.py to use the RunCompiler method here, but I'm not sure how useful that would be. They are both run through .gyp commands, and I thought that .gyp was being deprecated, so I assumed that these files were being deprecated as well. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:56: assert os.path.isfile(dep), dep[:-11] + ' is not a js_library target' On 2017/04/18 02:05:50, Dan Beam wrote: > what does -11 mean here? This removes ".js_library" from the name of the file before printing it to the console. The name of the dep is created in the js_library template, and ".js_library" is automatically appended. If the file doesn't exist however, it means that it was not actually a js_library target. I thought it would be confusing to get an error message saying "target_name.js_library is not a js_library target", since "target_name.js_library" doesn't exist. This will instead just print "target_name is not a js_library target", which I think will be a more helpful error message. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:58: lines = [line.strip() for line in dep_list.readlines()] On 2017/04/18 02:05:49, Dan Beam wrote: > use dep_list.splitlines() instead of readlines() + strip() splitlines() is only available on strings, not files, but I was able to use dep_list.read().splitlines() https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:59: split = lines.index('deps:') On 2017/04/18 02:05:49, Dan Beam wrote: > what if this fails? If the file exists, it should never fail, because the js_library rule that creates it should always write the file in the form sources: <list of sources> deps: <list of deps> But I can put a check in to ensure this, and have it error out otherwise. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:62: def PostOrder(deps, sources): On 2017/04/18 02:05:50, Dan Beam wrote: > can we name this something else? this sounds like you're buying a sandwich > online Changed to CrawlDepsTree https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:100: ] + defs On 2017/04/18 02:05:49, Dan Beam wrote: > what is doing this formatting? this would fit on one line... Done. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:103: compiler_args += ['--externs=' + extern] On 2017/04/18 02:07:42, Dan Beam wrote: > On 2017/04/18 02:05:50, Dan Beam wrote: > > the python style guide discourages use of + for string concatenation in favor > of > > '%s' % arg > > oh, i guess it's a little more nuanced than that: > https://google.github.io/styleguide/pyguide.html#Strings I switched it to use substitution, as it turns it into a one-liner and I think looks a little cleaner. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:110: if(args.bootstrap): On 2017/04/18 02:05:49, Dan Beam wrote: > space after if Done. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:110: if(args.bootstrap): On 2017/04/18 02:05:49, Dan Beam wrote: > no () (this ain't C) Done. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:117: sys.exit(main()) On 2017/04/18 02:05:49, Dan Beam wrote: > it doesn't look like main() actually returns anything on success? > > sys.exit() ends the program early, so no real reason to pass the return value, > IMO, just crash the program in the middle (just make sure to clean up first) Done. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... File third_party/closure_compiler/js_library.py (right): https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_library.py:24: out.write('sources:\n') On 2017/04/18 02:05:50, Dan Beam wrote: > could this be: > > out.write('sources:\n%s\ndeps:%s' % ('\n'.join(sources), '\n'.join(deps))) Done. https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_library.py:32: sys.exit(main()) On 2017/04/18 02:05:50, Dan Beam wrote: > i don't see a reason to do this sys.exit() nor to import sys after you've > removed it Done.
https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:85: # A list of custom flags to pass to the closure compiler. Do not include Closure https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... third_party/closure_compiler/js_binary.py:57: assert os.path.isfile(dep), dep[:-11] + ' is not a js_library target' I think dpream@ was getting at -11 looks random for a person reading it. Something like os.path.splitext(dep)[0] (or possibly dep[:-len('.js_library')]) would be more obvious at a glance https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... third_party/closure_compiler/js_binary.py:60: assert 'deps:' in lines, dep + ' is not formated correctly' nit: formatted add something a bit more specific about what isn't correct +': missing "deps:" line' https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... third_party/closure_compiler/js_binary.py:100: flags = ['--' + flag for flag in args.flags] nit: match the style below '--%s' % flag
As talked about with Mike, I also ran glint on the python files and made a few small style changes from that. https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:85: # A list of custom flags to pass to the closure compiler. Do not include On 2017/04/19 01:15:42, mbjorge wrote: > Closure Done. https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... third_party/closure_compiler/js_binary.py:57: assert os.path.isfile(dep), dep[:-11] + ' is not a js_library target' On 2017/04/19 01:15:42, mbjorge wrote: > I think dpream@ was getting at -11 looks random for a person reading it. > Something like os.path.splitext(dep)[0] (or possibly dep[:-len('.js_library')]) > would be more obvious at a glance Done. https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... third_party/closure_compiler/js_binary.py:60: assert 'deps:' in lines, dep + ' is not formated correctly' On 2017/04/19 01:15:42, mbjorge wrote: > nit: formatted > > add something a bit more specific about what isn't correct +': missing "deps:" > line' Done. https://codereview.chromium.org/2800833004/diff/260001/third_party/closure_co... third_party/closure_compiler/js_binary.py:100: flags = ['--' + flag for flag in args.flags] On 2017/04/19 01:15:42, mbjorge wrote: > nit: match the style below '--%s' % flag Done.
https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:16: import subprocess On 2017/04/19 00:42:12, damargulis wrote: > On 2017/04/18 02:05:50, Dan Beam wrote: > > it seems more common in chrome to use subprocess2 and Popen() and > communicate(). > > > > is there a reason why you're not doing that? > > I based most of the code to run the jar from java_action.py > (https://cs.chromium.org/chromium/src/build/util/java_action.py?q=java_action....) > > Is there any advantage of switching from check_call() to Popen()? why are you duplicating the code rather than just importing it and using it, then? https://codereview.chromium.org/2800833004/diff/240001/third_party/closure_co... third_party/closure_compiler/js_binary.py:52: return subprocess.check_call([java_path, '-jar'] + args) On 2017/04/19 00:42:12, damargulis wrote: > On 2017/04/18 02:05:49, Dan Beam wrote: > > can you share this code across compile.py and compile2.py as well? > > compile.py and compile2.py use the Checker class, which runs the compiler > through the _run_jar method. Since this is a private method, that's obviously easy to change > I don't want to > call that directly, and I don't want to use the exposed check method, as this > does a lot of additional work and modifications that I don't want done. why not just make it public? > > I could change compile.py and compile2.py to use the RunCompiler method here, > but I'm not sure how useful that would be. They are both run through .gyp > commands, and I thought that .gyp was being deprecated, so I assumed that these > files were being deprecated as well. what do you think we're going to replace them with, if not the code you're writing right now? :) https://codereview.chromium.org/2800833004/diff/300001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/300001/third_party/closure_co... third_party/closure_compiler/js_binary.py:73: sources += [source for source in new_sources if source not in sources] why is sources not simply a set()?
I changed js_binary.py to import compile2 and use its run_jar method, which I changed to a public method. If you think there is a better way of going about this let me know! https://codereview.chromium.org/2800833004/diff/300001/third_party/closure_co... File third_party/closure_compiler/js_binary.py (right): https://codereview.chromium.org/2800833004/diff/300001/third_party/closure_co... third_party/closure_compiler/js_binary.py:73: sources += [source for source in new_sources if source not in sources] On 2017/04/19 17:59:45, Dan Beam wrote: > why is sources not simply a set()? A set won't maintain proper ordering of the sources
https://codereview.chromium.org/2800833004/diff/320001/third_party/closure_co... File third_party/closure_compiler/compile_js.gni (right): https://codereview.chromium.org/2800833004/diff/320001/third_party/closure_co... third_party/closure_compiler/compile_js.gni:155: args += [ "--flags" ] + default_closure_args - [ "checks_only" ] you should be using minifying_closure_args instead of default_closure_args I think
otherwise this lgtm
The CQ bit was checked by damargulis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org, mbjorge@chromium.org, dpranke@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2800833004/#ps360001 (title: "update comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/21 at 00:33:40, dbeam wrote: > https://codereview.chromium.org/2800833004/diff/320001/third_party/closure_co... > File third_party/closure_compiler/compile_js.gni (right): > > https://codereview.chromium.org/2800833004/diff/320001/third_party/closure_co... > third_party/closure_compiler/compile_js.gni:155: args += [ "--flags" ] + default_closure_args - [ "checks_only" ] > you should be using minifying_closure_args instead of default_closure_args I think Why minifying_closure_args? I thought the main benefit of these targets was to get all the checking? minifying_closure_args is very bare bones, and doesn't even include common_closure_args which describe themselves as "for all uses of the closure compiler".
On 2017/04/21 21:13:58, mbjorge wrote: > On 2017/04/21 at 00:33:40, dbeam wrote: > > > https://codereview.chromium.org/2800833004/diff/320001/third_party/closure_co... > > File third_party/closure_compiler/compile_js.gni (right): > > > > > https://codereview.chromium.org/2800833004/diff/320001/third_party/closure_co... > > third_party/closure_compiler/compile_js.gni:155: args += [ "--flags" ] + > default_closure_args - [ "checks_only" ] > > you should be using minifying_closure_args instead of default_closure_args I > think > > Why minifying_closure_args? I thought the main benefit of these targets was to > get all the checking? minifying_closure_args is very bare bones, and doesn't > even include common_closure_args which describe themselves as "for all uses of > the closure compiler". it didn't make sense to me to use a bunch of flags and remove one, but we should instead use the right bunch to begin with. the CL description is unclear what these rules are being added for. the "checks_only" flag is being /removed/, which means that the author cares about the output. right now only the minification path cares about the output, which is why I felt it's closer to that. the "default" args really just mean "typechecking" when that was the only thing we used closure for (which predates minification on android). we should rename "default" to "typechecking" in many cases. damargulis@: is the goal to do both typechecking and minification (separately or combined) in the future with this code?
On 2017/04/21 22:12:51, Dan Beam wrote: > On 2017/04/21 21:13:58, mbjorge wrote: > > On 2017/04/21 at 00:33:40, dbeam wrote: > > > > > > https://codereview.chromium.org/2800833004/diff/320001/third_party/closure_co... > > > File third_party/closure_compiler/compile_js.gni (right): > > > > > > > > > https://codereview.chromium.org/2800833004/diff/320001/third_party/closure_co... > > > third_party/closure_compiler/compile_js.gni:155: args += [ "--flags" ] + > > default_closure_args - [ "checks_only" ] > > > you should be using minifying_closure_args instead of default_closure_args I > > think > > > > Why minifying_closure_args? I thought the main benefit of these targets was to > > get all the checking? minifying_closure_args is very bare bones, and doesn't > > even include common_closure_args which describe themselves as "for all uses of > > the closure compiler". > > it didn't make sense to me to use a bunch of flags and remove one, but we should > instead use the right bunch to begin with. the CL description is unclear what > these rules are being added for. > > the "checks_only" flag is being /removed/, which means that the author cares > about the output. right now only the minification path cares about the output, > which is why I felt it's closer to that. > > the "default" args really just mean "typechecking" when that was the only thing > we used closure for (which predates minification on android). we should rename > "default" to "typechecking" in many cases. > > damargulis@: is the goal to do both typechecking and minification (separately or > combined) in the future with this code? I think I agree that using minifying_closure_args is better. Although the use cases I had in mind for this would be for both typechecking and minification, I think there is no reason why we should limit others to using that. By using minifying_closure_args, the default will be to simply remove whitespace, but checking and optimizing can be accomplished by passing in the right flags. If we using default_closure_args as the default choice, then anyone using the target will be forced to use typechecking and optimizing. I think using minifying_closure_args makes this more general.
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1492808969553640,
"parent_rev": "37861ef99134d28395b1891a25edb0b750a7011e", "commit_rev":
"5c5337d8b474a6d07320536d0bab8bfbf548c219"}
Message was sent while issue was closed.
Description was changed from ========== Create js_library and js_binary templates for closure compiling. This creates the js_library and js_binary templates to be used within chromium. These templates simulate their matching partners in Blaze. The js_library template is used to create an ordering for .js files. The js_binary template is used to compile .js files and js_library targets. BUG=632206 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Create js_library and js_binary templates for closure compiling. This creates the js_library and js_binary templates to be used within chromium. These templates simulate their matching partners in Blaze. The js_library template is used to create an ordering for .js files. The js_binary template is used to compile .js files and js_library targets. BUG=632206 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2800833004 Cr-Commit-Position: refs/heads/master@{#466471} Committed: https://chromium.googlesource.com/chromium/src/+/5c5337d8b474a6d07320536d0bab... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001) as https://chromium.googlesource.com/chromium/src/+/5c5337d8b474a6d07320536d0bab... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
