Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2064533002:
[GN] Add JSON project writer (Closed)
Description[GN] Add JSON project writer
Output is a JSON file containing information about targets. The generator
can also optionally invoke a python script on generated file.
Example:
gn gen --ide=json ./out-json --json-ide-script=//scripts/custom-ide-generator.py \
--ison-ide-script-args="additional script arguments"
Also implements --format=json for gn desc as described in https://bugs.chromium.org/p/chromium/issues/detail?id=620132
BUG=
Committed: https://crrev.com/d2edeb9a743ad5a2046e655997277d3bb630db03
Cr-Commit-Position: refs/heads/master@{#406064}
Patch Set 1 #Patch Set 2 : Keep toolchain in target label for non default toolchain targets #Patch Set 3 : Remove declared args from json build settings #Patch Set 4 : Add DescBuilder that generates base::Value that command_desc and json project writer use #Patch Set 5 : [GN] add --format=json to command_desc #Patch Set 6 : [GN] fix include order #
Total comments: 1
Patch Set 7 : Rebase, fix build, fix variable names #
Total comments: 37
Patch Set 8 : [GN] Address issues from codereview #Patch Set 9 : [GN] document json format #Patch Set 10 : [GN] Only include source_outputs in project JSON if not empty #Patch Set 11 : [GN] Add myself to AUTHORS #
Messages
Total messages: 46 (13 generated)
Description was changed from
==========
[GN] Add JSON project writer
Output is a JSON file containing information about targets. The generator
can also optionally invoke a python script on generated file.
Example:
gn gen --ide=json ./out-json --exec-script=//scripts/custom-ide-generator.py \
--exec-script-extra-args="additional script arguments"
BUG=
==========
to
==========
[GN] Add JSON project writer
Output is a JSON file containing information about targets. The generator
can also optionally invoke a python script on generated file.
Example:
gn gen --ide=json ./out-json --exec-script=//scripts/custom-ide-generator.py \
--exec-script-extra-args="additional script arguments"
Note that in current state this does not expose configs and all build properties
on targets are already merged. This should be enough for project generation, but
if there is interest for finer granularity, I could add configs to output as
well as config values set directly on target.
BUG=
==========
brettw@chromium.org changed reviewers: + brettw@chromium.org
Hi! Thanks for looking at this. I've told a number of people who have asked how I think this should be implemented, but I didn't write it down in a known place. Apologies for not doing this before you started. So I just wrote up a bug describing what I was thinking: https://bugs.chromium.org/p/chromium/issues/detail?id=620132 I haven't looked at your code. I would be interested in hearing your thoughts about the two approaches. If you still think using --ide is better, I can take a look at your code more closely and get a better opinion.
On 2016/06/14 22:37:25, brettw wrote: > Hi! Thanks for looking at this. I've told a number of people who have asked how > I think this should be implemented, but I didn't write it down in a known place. > Apologies for not doing this before you started. So I just wrote up a bug > describing what I was thinking: > https://bugs.chromium.org/p/chromium/issues/detail?id=620132 > > I haven't looked at your code. I would be interested in hearing your thoughts > about the two approaches. If you still think using --ide is better, I can take a > look at your code more closely and get a better opinion. Hi Brett, I actually think you can have both, as they do have slightly different use case. I'm mostly interested in custom IDE generators, so there are some extra things that --ide does: 1) it doesn't have to be run on existing output directory, 2) it will also write ninja files alongside the json file 3) it will output build related properties to the json file such as build_dir, root_path and possibly other ide related config values (xcode_...) 4) it can execute python script on generated json file; It's very handly to have this done during gn gen, because the invocation gets saved to rule gn, so everything gets properly rebuilt when project files change So my primary usecase when implementing this was to enable using custom generators same way as embedded one (including automatic project regeneration). That said, code that transforms target to base::value is separated in value_builder, and can be used for desc_as well (although it might currently be missing some properties not that interesting for build generation, such as configs)
I think there are already too many types of project writers in GN, and I also don't think we should be forking out to Python for every target written. I really think if people want such things in the future they should get the full dependency graph out of GN with desc.
On 2016/06/15 19:42:22, brettw wrote: > I think there are already too many types of project writers in GN, and I also > don't think we should be forking out to Python for every target written. I > really think if people want such things in the future they should get the full > dependency graph out of GN with desc. Could you maybe elaborate why would python be forked for every target written? In this CR python is forked at most once, because all targets are in same JSON file. Also I'm not sure I see how your reply addresses any of the four points I've mentioned in previous email. gn desc will only get you half-way there. It doesn't address build settings, or regenerating project files when ninja is invoked. This does, and the actual project writer is just 140 lines long, without much potential to bitrot. These 140 lines make it possible to use custom generators with same convenience as built-in ones.
I see, I saw the execute process calls and assumed you were doing it for every target. I'm OK outputting json when we run gen, but only if that code is shared with desc. We should have one backend that computes base::Values for everything that desc does today and use it for both places. I've previously thought it would be nice if desc also supported toolchains, so you could add that so we can really dump everything, and then all you need is the name of the default toochain, build dir, and source dir in the json output and I think then your generator should have all the information it needs with almost no special code. I don't see why it would need to output args, those are opaque to the generators
On 2016/06/16 05:49:39, brettw wrote: > I see, I saw the execute process calls and assumed you were doing it for every > target. > > I'm OK outputting json when we run gen, but only if that code is shared with > desc. We should have one backend that computes base::Values for everything that > desc does today and use it for both places. > > I've previously thought it would be nice if desc also supported toolchains, so > you could add that so we can really dump everything, and then all you need is > the name of the default toochain, build dir, and source dir in the json output > and I think then your generator should have all the information it needs with > almost no special code. I don't see why it would need to output args, those are > opaque to the generators Yes, the base::Values code is definitely meant to be shared. Regarding build args, I actually don't need them after there is something like you've suggested here https://codereview.chromium.org/2063243002/ set_ide_params("xcode") { ios_deployment_target = ... ... } right now I'm using build arguments as a hack to pass value to generators, that would no longer be necessary with set_ide_params.
On 2016/06/16 09:09:21, matej.knopp wrote: > On 2016/06/16 05:49:39, brettw wrote: > > I see, I saw the execute process calls and assumed you were doing it for every > > target. > > > > I'm OK outputting json when we run gen, but only if that code is shared with > > desc. We should have one backend that computes base::Values for everything > that > > desc does today and use it for both places. > > > > I've previously thought it would be nice if desc also supported toolchains, so > > you could add that so we can really dump everything, and then all you need is > > the name of the default toochain, build dir, and source dir in the json output > > and I think then your generator should have all the information it needs with > > almost no special code. I don't see why it would need to output args, those > are > > opaque to the generators > > Yes, the base::Values code is definitely meant to be shared. Regarding build > args, I actually don't need them after there is something like you've suggested > here https://codereview.chromium.org/2063243002/ > > set_ide_params("xcode") { > ios_deployment_target = ... > ... > } > > right now I'm using build arguments as a hack to pass value to generators, that > would no longer be necessary with set_ide_params. Let's treat these things separately. I don't want to add hacks around the fact that we haven't implemented that other thing. I think you can work around this limitation in your generator for now and we can incrementally improve the situation.
> > Let's treat these things separately. I don't want to add hacks around the fact > that we haven't implemented that other thing. I think you can work around this > limitation in your generator for now and we can incrementally improve the > situation. Done. Declared args are no longer exported in the json file.
Sorry if I wasn't super clear above, but I really think the code to generate the base::Values should be shared with "command_desc.cc" as I described in the bug I linked to in comment 4.
On 2016/06/20 18:06:43, brettw wrote: > Sorry if I wasn't super clear above, but I really think the code to generate the > base::Values should be shared with "command_desc.cc" as I described in the bug I > linked to in comment 4. I see. That would involve making sure that value_builder puts everything to the base::value representation that command_desc.cc needs, and rewriting command_desc.cc to to render the base::value in human readable format somewhat similar to current output. The latter might be a bit tricky. Also this would mean that the output format of command_desc will change, is that okay? does anything out there try to parse command_desc output?
On 2016/06/20 18:06:43, brettw (egregiously slow) wrote: > Sorry if I wasn't super clear above, but I really think the code to generate the > base::Values should be shared with "command_desc.cc" as I described in the bug I > linked to in comment 4. With last patchset the code that generates base::Values is in desc_builder. Both command_desc and json_project_writer use it to generate output. Format and behavior of command_desc should be very close to current behavior, including the blame, tree and all switches.
Hi, sorry this fell off my list, will refresh my memory of what's going on now.
I tried to patch this in to my tree to test it: - visibility.h needs an include for unique_ptr - Need build file updates to include the new files. The new "command_desc.cc" code looks basically like I would expect with the new design. Sorry, I didn't get a chance to look at the desc_builder code in detail yet. I'll take a look in more detail when the patch builds. https://codereview.chromium.org/2064533002/diff/100001/tools/gn/command_desc.cc File tools/gn/command_desc.cc (right): https://codereview.chromium.org/2064533002/diff/100001/tools/gn/command_desc.... tools/gn/command_desc.cc:38: bool b(false); Can you do bool bool_value = false; (the paren-style init looks very strange in this case, and "b" is a confusing name). For consistency, the list and dict above should probably be named similarly (i.e. list_value, dict_value).
On 2016/07/01 23:34:18, brettw (plz ping after 24h) wrote: > I tried to patch this in to my tree to test it: > > - visibility.h needs an include for unique_ptr > > - Need build file updates to include the new files. > > The new "command_desc.cc" code looks basically like I would expect with the new > design. Sorry, I didn't get a chance to look at the desc_builder code in detail > yet. I'll take a look in more detail when the patch builds. > > https://codereview.chromium.org/2064533002/diff/100001/tools/gn/command_desc.cc > File tools/gn/command_desc.cc (right): > > https://codereview.chromium.org/2064533002/diff/100001/tools/gn/command_desc.... > tools/gn/command_desc.cc:38: bool b(false); > Can you do > bool bool_value = false; > (the paren-style init looks very strange in this case, and "b" is a confusing > name). For consistency, the list and dict above should probably be named > similarly (i.e. list_value, dict_value). Thanks for the feedback. I'm currently building it through bootstrap and I've forgotten to update the build file. I should upload another patchset soon.
Issues should be fixed now, could you give it another go? As a sidenote, I made a proof of concept Xcode generator https://github.com/knopp/gn-project-generators i.e. this would create project file for GN (and its dependencies) gn gen --ide=json --exec-script=<path-to>/gen_xcode.py --filters="//tools/gn:gn" build-xcode On 2016/07/01 23:34:18, brettw (plz ping after 24h) wrote: > I tried to patch this in to my tree to test it: > > - visibility.h needs an include for unique_ptr > > - Need build file updates to include the new files. > > The new "command_desc.cc" code looks basically like I would expect with the new > design. Sorry, I didn't get a chance to look at the desc_builder code in detail > yet. I'll take a look in more detail when the patch builds. > > https://codereview.chromium.org/2064533002/diff/100001/tools/gn/command_desc.cc > File tools/gn/command_desc.cc (right): > > https://codereview.chromium.org/2064533002/diff/100001/tools/gn/command_desc.... > tools/gn/command_desc.cc:38: bool b(false); > Can you do > bool bool_value = false; > (the paren-style init looks very strange in this case, and "b" is a confusing > name). For consistency, the list and dict above should probably be named > similarly (i.e. list_value, dict_value).
I finally had time to go through this carefully. Sorry I took so long. This patch looks like a nice improvement and I can definitely appreciate it took a while to write. Most of these commands are fairly minor so I don't think this review will take much more time. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.cc File tools/gn/command_desc.cc (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.... tools/gn/command_desc.cc:73: void TypeHandler(const std::string& name, const base::Value* value) { I think you can delete this function if you take my advice on the "type" conversion in the other file. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.... tools/gn/command_desc.cc:140: // Outputs need special processing when output patterns are present Style: period at end of comment. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.... tools/gn/command_desc.cc:408: " Optional --format=json flag can be specified to switch output to JSON.\n" Can you just move this up to the "Shared flags" section. I'd format it like this: ALL_TOOLCHAINS_SWITCH_HELP <----(existing) "\n" " --format=json\n" " Format the output as JSON instead of text.\n" "\n" https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.cc File tools/gn/command_gen.cc (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.c... tools/gn/command_gen.cc:46: const char kSwitchExecScript[] = "exec-script"; Can we call this something like "json_ide_script" or something to make clear this is applying to the json output? There's already something called exec_script so this more general name is a bit confusing to me. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.c... tools/gn/command_gen.cc:238: bool quiet = command_line->HasSwitch(switches::kQuiet); This is duplicated from above (which already duplicates it a few times. It would be nice if your variable was just moved to the top of the function and then everybody just referenced it instead of calling HasSwitch. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.c... tools/gn/command_gen.cc:341: " Overrides default file name of generated JSON file.\n" Can you list what the default file name is here? https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.cc File tools/gn/desc_builder.cc (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:21: namespace { Somewhere (I'm thinking at the top of this file might be the best place) we should have a comment defining what the JSON/base::Value structure looks like, and reference this comment from the ide=json help. We don't need to define everything (most flags and stuff are obvious), but I'm thinking the overall structure, the build_settings dictionary, where to look up the possible values for 'type', and how you define 'source_outputs' (which doesn't correspond to anything in the BUILD file directly) should be covered. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:76: auto res = new base::ListValue(); I prefer not to have owning bare pointers floating around. In the cases where you make a new *Value, can you put it directly into unique_ptr when you make it and then return it via .Pass()? This will insulate us from leaks if somebody adds early returns later. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:101: else According to the style guide, just delete the "else" here. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:563: } else if (target_->output_type() == Target::ACTION_FOREACH) { When I tried your patch I got an assertion in the "else" case below. I realize is wasn't your fault and this existed before from this patch: https://codereview.chromium.org/2105333002/diff/20001/tools/gn/command_desc.cc I think all we need to do is make this condition handle Target::COPY_FILES also so it doesn't fall through to the else. Can you add that? https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:648: } // Namespace Normally we do lower-case "n" here. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:650: const char* DescBuilder::GetStringForOutputType(Target::OutputType type) { Here (and in the following function) we're inventing a new type of string for output type handling. Is this super necessary? The string provided by Target should be pretty standardized. In some places it's used for friendly user strings, but I'm happy also saying that those shouldn't change (if so, we should add a comment in Target::GetStringForOutputType to this effect). That should allow you to delete most of this conversion code. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:675: return Target::C If we keep this, can you indent this line one more level and put a blank after here to separate it from the CASE calls? It's all getting bunched together. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... File tools/gn/json_project_writer.cc (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... tools/gn/json_project_writer.cc:23: void add_target_dependencies(const Target* target, These local functions should use CamelCase like "AddTargetDependencies" (use the hacker_style only for inline getters and setters on classes). https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... tools/gn/json_project_writer.cc:60: // in the generated Xcode project (and thus stability of the file generated). This is a general file and probably shouldn't reference xcode. I think wanting stable output is enough reason to give in this context. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... tools/gn/json_project_writer.cc:80: // outputs need to be asked for separately Can you make this a sentence with a capital and period? https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... tools/gn/json_project_writer.cc:182: // relative path, assume the base is in build_dir Same "complete sentence" comment from above. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... File tools/gn/json_project_writer.h (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... tools/gn/json_project_writer.h:18: const std::string & file_name, Style nit: no space before & https://codereview.chromium.org/2064533002/diff/120001/tools/gn/visibility.cc File tools/gn/visibility.cc (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/visibility.cc... tools/gn/visibility.cc:90: for (const auto& pattern : patterns_) { The style guide says we can do it either way but to be consistent, and I've been trying to be consistent in GN to not use {} for these single-line conditionals and loops (we'd always use {} for multi-line ones, even if it's still only one statement). Can you delete the {} from here and in other places where you have added them?
Hi Brett, thank you very much for thorough review. I'll try to address every single point in few days. I wasn't quite sure what the rule for hacker_style vs CamelCase is, I honestly find the usage in chromium source a bit confusing. I.e. what's the point of inline getters having different naming convention than regular getters? Regarding the target type mapping - I though it would be nice if JSON output had same string value as the enum, i.e. STATIC_LIBRARY instead of "Static library", which does seem rather weird for output that's meant to be machine processed, but I also agree that current mapping back and forth is cumbersome. So if Target::GetStringForOutputType is considered stable I'll just remove the unnecessary code. -Matej On 2016/07/06 22:19:27, brettw (plz ping after 24h) wrote: > I finally had time to go through this carefully. Sorry I took so long. This > patch looks like a nice improvement and I can definitely appreciate it took a > while to write. > > Most of these commands are fairly minor so I don't think this review will take > much more time. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.cc > File tools/gn/command_desc.cc (right): > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.... > tools/gn/command_desc.cc:73: void TypeHandler(const std::string& name, const > base::Value* value) { > I think you can delete this function if you take my advice on the "type" > conversion in the other file. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.... > tools/gn/command_desc.cc:140: // Outputs need special processing when output > patterns are present > Style: period at end of comment. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.... > tools/gn/command_desc.cc:408: " Optional --format=json flag can be specified to > switch output to JSON.\n" > Can you just move this up to the "Shared flags" section. I'd format it like > this: > > ALL_TOOLCHAINS_SWITCH_HELP <----(existing) > "\n" > " --format=json\n" > " Format the output as JSON instead of text.\n" > "\n" > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.cc > File tools/gn/command_gen.cc (right): > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.c... > tools/gn/command_gen.cc:46: const char kSwitchExecScript[] = "exec-script"; > Can we call this something like "json_ide_script" or something to make clear > this is applying to the json output? There's already something called > exec_script so this more general name is a bit confusing to me. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.c... > tools/gn/command_gen.cc:238: bool quiet = > command_line->HasSwitch(switches::kQuiet); > This is duplicated from above (which already duplicates it a few times. It would > be nice if your variable was just moved to the top of the function and then > everybody just referenced it instead of calling HasSwitch. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.c... > tools/gn/command_gen.cc:341: " Overrides default file name of generated > JSON file.\n" > Can you list what the default file name is here? > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.cc > File tools/gn/desc_builder.cc (right): > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... > tools/gn/desc_builder.cc:21: namespace { > Somewhere (I'm thinking at the top of this file might be the best place) we > should have a comment defining what the JSON/base::Value structure looks like, > and reference this comment from the ide=json help. > > We don't need to define everything (most flags and stuff are obvious), but I'm > thinking the overall structure, the build_settings dictionary, where to look up > the possible values for 'type', and how you define 'source_outputs' (which > doesn't correspond to anything in the BUILD file directly) should be covered. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... > tools/gn/desc_builder.cc:76: auto res = new base::ListValue(); > I prefer not to have owning bare pointers floating around. In the cases where > you make a new *Value, can you put it directly into unique_ptr when you make it > and then return it via .Pass()? This will insulate us from leaks if somebody > adds early returns later. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... > tools/gn/desc_builder.cc:101: else > According to the style guide, just delete the "else" here. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... > tools/gn/desc_builder.cc:563: } else if (target_->output_type() == > Target::ACTION_FOREACH) { > When I tried your patch I got an assertion in the "else" case below. I realize > is wasn't your fault and this existed before from this patch: > https://codereview.chromium.org/2105333002/diff/20001/tools/gn/command_desc.cc > I think all we need to do is make this condition handle Target::COPY_FILES also > so it doesn't fall through to the else. Can you add that? > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... > tools/gn/desc_builder.cc:648: } // Namespace > Normally we do lower-case "n" here. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... > tools/gn/desc_builder.cc:650: const char* > DescBuilder::GetStringForOutputType(Target::OutputType type) { > Here (and in the following function) we're inventing a new type of string for > output type handling. > > Is this super necessary? The string provided by Target should be pretty > standardized. In some places it's used for friendly user strings, but I'm happy > also saying that those shouldn't change (if so, we should add a comment in > Target::GetStringForOutputType to this effect). That should allow you to delete > most of this conversion code. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... > tools/gn/desc_builder.cc:675: return Target::C > If we keep this, can you indent this line one more level and put a blank after > here to separate it from the CASE calls? It's all getting bunched together. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... > File tools/gn/json_project_writer.cc (right): > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... > tools/gn/json_project_writer.cc:23: void add_target_dependencies(const Target* > target, > These local functions should use CamelCase like "AddTargetDependencies" (use the > hacker_style only for inline getters and setters on classes). > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... > tools/gn/json_project_writer.cc:60: // in the generated Xcode project (and thus > stability of the file generated). > This is a general file and probably shouldn't reference xcode. I think wanting > stable output is enough reason to give in this context. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... > tools/gn/json_project_writer.cc:80: // outputs need to be asked for separately > Can you make this a sentence with a capital and period? > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... > tools/gn/json_project_writer.cc:182: // relative path, assume the base is in > build_dir > Same "complete sentence" comment from above. > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... > File tools/gn/json_project_writer.h (right): > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... > tools/gn/json_project_writer.h:18: const std::string & file_name, > Style nit: no space before & > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/visibility.cc > File tools/gn/visibility.cc (right): > > https://codereview.chromium.org/2064533002/diff/120001/tools/gn/visibility.cc... > tools/gn/visibility.cc:90: for (const auto& pattern : patterns_) { > The style guide says we can do it either way but to be consistent, and I've been > trying to be consistent in GN to not use {} for these single-line conditionals > and loops (we'd always use {} for multi-line ones, even if it's still only one > statement). Can you delete the {} from here and in other places where you have > added them?
On 2016/07/06 22:34:34, matt.k wrote: > Hi Brett, thank you very much for thorough review. I'll try to address every > single point in few days. > > I wasn't quite sure what the rule for hacker_style vs CamelCase is, I honestly > find the usage in chromium source a bit confusing. I.e. what's the point of > inline getters having different naming convention than regular getters? I think the goal for the rule (it's a very old one at Google) is if the access is as cheap as a variable, it's named like a variable. So when I do my_class->foo() I know that this will be inlined away and I shouldn't bother assigning it to a temporary or anything. When I see my_class->GetFoo() it might be doing more expensive computations or virtual function calls so I wouldn't want to call it repeatedly in performance-sensitive code. > Regarding the target type mapping - I though it would be nice if JSON output had > same string value as the enum, i.e. STATIC_LIBRARY instead of "Static library", > which does seem rather weird for output that's meant to be machine processed, > but I also agree that current mapping back and forth is cumbersome. So if > Target::GetStringForOutputType is considered stable I'll just remove the > unnecessary code. Yeah, I can see there's some attractiveness in matching the enum, but I think it's simpler to just have one thing, and I don't think there are any technical downsides. It does make me a little uneasy that these strings don't match anything else. Since we're going to say they can't change in the future, let's take this opportunity to make them more consistent. I'm now thinking we should change Target::GetStringForOutputType to return the name of the GN function that creates that type, then it will be consistent everywhere the user sees it. We can even re-use the constants for the function names in functions.h so everything stays in sync (so for example return functions::kActionForEach). Do you care to update this?
> I think the goal for the rule (it's a very old one at Google) is if the access > is as cheap as a variable, it's named like a variable. So when I do > my_class->foo() I know that this will be inlined away and I shouldn't bother > assigning it to a temporary or anything. When I see my_class->GetFoo() it might > be doing more expensive computations or virtual function calls so I wouldn't > want to call it repeatedly in performance-sensitive code. Okay, that makes sense. > It does make me a little uneasy that these strings don't match anything else. > Since we're going to say they can't change in the future, let's take this > opportunity to make them more consistent. I'm now thinking we should change > Target::GetStringForOutputType to return the name of the GN function that > creates that type, then it will be consistent everywhere the user sees it. We > can even re-use the constants for the function names in functions.h so > everything stays in sync (so for example return functions::kActionForEach). Do > you care to update this? I like this. I'll change the return values to functions::k*
All issues should be addressed except the format overview at the beginning of desc_builder.cc. I'll try to do that tomorrow. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.cc File tools/gn/command_desc.cc (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.... tools/gn/command_desc.cc:73: void TypeHandler(const std::string& name, const base::Value* value) { On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > I think you can delete this function if you take my advice on the "type" > conversion in the other file. Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.... tools/gn/command_desc.cc:140: // Outputs need special processing when output patterns are present On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > Style: period at end of comment. Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_desc.... tools/gn/command_desc.cc:408: " Optional --format=json flag can be specified to switch output to JSON.\n" On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > Can you just move this up to the "Shared flags" section. I'd format it like > this: > > ALL_TOOLCHAINS_SWITCH_HELP <----(existing) > "\n" > " --format=json\n" > " Format the output as JSON instead of text.\n" > "\n" Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.cc File tools/gn/command_gen.cc (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.c... tools/gn/command_gen.cc:46: const char kSwitchExecScript[] = "exec-script"; On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > Can we call this something like "json_ide_script" or something to make clear > this is applying to the json output? There's already something called > exec_script so this more general name is a bit confusing to me. Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.c... tools/gn/command_gen.cc:238: bool quiet = command_line->HasSwitch(switches::kQuiet); On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > This is duplicated from above (which already duplicates it a few times. It would > be nice if your variable was just moved to the top of the function and then > everybody just referenced it instead of calling HasSwitch. Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/command_gen.c... tools/gn/command_gen.cc:341: " Overrides default file name of generated JSON file.\n" On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > Can you list what the default file name is here? Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.cc File tools/gn/desc_builder.cc (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:76: auto res = new base::ListValue(); On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > I prefer not to have owning bare pointers floating around. In the cases where > you make a new *Value, can you put it directly into unique_ptr when you make it > and then return it via .Pass()? This will insulate us from leaks if somebody > adds early returns later. Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:101: else On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > According to the style guide, just delete the "else" here. Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:563: } else if (target_->output_type() == Target::ACTION_FOREACH) { On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > When I tried your patch I got an assertion in the "else" case below. I realize > is wasn't your fault and this existed before from this patch: > https://codereview.chromium.org/2105333002/diff/20001/tools/gn/command_desc.cc > I think all we need to do is make this condition handle Target::COPY_FILES also > so it doesn't fall through to the else. Can you add that? Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:648: } // Namespace On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > Normally we do lower-case "n" here. Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:650: const char* DescBuilder::GetStringForOutputType(Target::OutputType type) { On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > Here (and in the following function) we're inventing a new type of string for > output type handling. > > Is this super necessary? The string provided by Target should be pretty > standardized. In some places it's used for friendly user strings, but I'm happy > also saying that those shouldn't change (if so, we should add a comment in > Target::GetStringForOutputType to this effect). That should allow you to delete > most of this conversion code. Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/desc_builder.... tools/gn/desc_builder.cc:675: return Target::C On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > If we keep this, can you indent this line one more level and put a blank after > here to separate it from the CASE calls? It's all getting bunched together. Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... File tools/gn/json_project_writer.cc (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... tools/gn/json_project_writer.cc:23: void add_target_dependencies(const Target* target, On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > These local functions should use CamelCase like "AddTargetDependencies" (use the > hacker_style only for inline getters and setters on classes). Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... tools/gn/json_project_writer.cc:60: // in the generated Xcode project (and thus stability of the file generated). On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > This is a general file and probably shouldn't reference xcode. I think wanting > stable output is enough reason to give in this context. Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... tools/gn/json_project_writer.cc:80: // outputs need to be asked for separately On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > Can you make this a sentence with a capital and period? Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... tools/gn/json_project_writer.cc:182: // relative path, assume the base is in build_dir On 2016/07/06 22:19:26, brettw (plz ping after 24h) wrote: > Same "complete sentence" comment from above. Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... File tools/gn/json_project_writer.h (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/json_project_... tools/gn/json_project_writer.h:18: const std::string & file_name, On 2016/07/06 22:19:27, brettw (plz ping after 24h) wrote: > Style nit: no space before & Done. https://codereview.chromium.org/2064533002/diff/120001/tools/gn/visibility.cc File tools/gn/visibility.cc (right): https://codereview.chromium.org/2064533002/diff/120001/tools/gn/visibility.cc... tools/gn/visibility.cc:90: for (const auto& pattern : patterns_) { On 2016/07/06 22:19:27, brettw (plz ping after 24h) wrote: > The style guide says we can do it either way but to be consistent, and I've been > trying to be consistent in GN to not use {} for these single-line conditionals > and loops (we'd always use {} for multi-line ones, even if it's still only one > statement). Can you delete the {} from here and in other places where you have > added them? Done.
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Description was changed from
==========
[GN] Add JSON project writer
Output is a JSON file containing information about targets. The generator
can also optionally invoke a python script on generated file.
Example:
gn gen --ide=json ./out-json --exec-script=//scripts/custom-ide-generator.py \
--exec-script-extra-args="additional script arguments"
Note that in current state this does not expose configs and all build properties
on targets are already merged. This should be enough for project generation, but
if there is interest for finer granularity, I could add configs to output as
well as config values set directly on target.
BUG=
==========
to
==========
[GN] Add JSON project writer
Output is a JSON file containing information about targets. The generator
can also optionally invoke a python script on generated file.
Example:
gn gen --ide=json ./out-json --json-ide-script=//scripts/custom-ide-generator.py
\
--ison-ide-script-args="additional script arguments"
Also implements --format=json for gn desc as described in
https://bugs.chromium.org/p/chromium/issues/detail?id=620132
BUG=
==========
The CQ bit was checked by brettw@chromium.org
lgtm
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...)
Looks like you signed the CLA, but since this is your first patch, also add yourself to the src/AUTHORS file (just add that file to this patch with your name and email added).
On 2016/07/14 16:52:32, brettw (ping after 24h) wrote: > Looks like you signed the CLA, but since this is your first patch, also add > yourself to the src/AUTHORS file (just add that file to this patch with your > name and email added). I added the entry. Anything else needs to be done?
That should be it, I'll push commit for you and we'll see if the bots are happy enough to land it.
The CQ bit was checked by brettw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2064533002/#ps200001 (title: "[GN] Add myself to AUTHORS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
[GN] Add JSON project writer
Output is a JSON file containing information about targets. The generator
can also optionally invoke a python script on generated file.
Example:
gn gen --ide=json ./out-json --json-ide-script=//scripts/custom-ide-generator.py
\
--ison-ide-script-args="additional script arguments"
Also implements --format=json for gn desc as described in
https://bugs.chromium.org/p/chromium/issues/detail?id=620132
BUG=
==========
to
==========
[GN] Add JSON project writer
Output is a JSON file containing information about targets. The generator
can also optionally invoke a python script on generated file.
Example:
gn gen --ide=json ./out-json --json-ide-script=//scripts/custom-ide-generator.py
\
--ison-ide-script-args="additional script arguments"
Also implements --format=json for gn desc as described in
https://bugs.chromium.org/p/chromium/issues/detail?id=620132
BUG=
==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from
==========
[GN] Add JSON project writer
Output is a JSON file containing information about targets. The generator
can also optionally invoke a python script on generated file.
Example:
gn gen --ide=json ./out-json --json-ide-script=//scripts/custom-ide-generator.py
\
--ison-ide-script-args="additional script arguments"
Also implements --format=json for gn desc as described in
https://bugs.chromium.org/p/chromium/issues/detail?id=620132
BUG=
==========
to
==========
[GN] Add JSON project writer
Output is a JSON file containing information about targets. The generator
can also optionally invoke a python script on generated file.
Example:
gn gen --ide=json ./out-json --json-ide-script=//scripts/custom-ide-generator.py
\
--ison-ide-script-args="additional script arguments"
Also implements --format=json for gn desc as described in
https://bugs.chromium.org/p/chromium/issues/detail?id=620132
BUG=
Committed: https://crrev.com/d2edeb9a743ad5a2046e655997277d3bb630db03
Cr-Commit-Position: refs/heads/master@{#406064}
==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/d2edeb9a743ad5a2046e655997277d3bb630db03 Cr-Commit-Position: refs/heads/master@{#406064}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2160533004/ by grt@chromium.org. The reason for reverting is: Broke Win8 GYP (dbg) builder: [822/12249] LINK_EMBED gn.exe FAILED: gn.exe gn.exe.pdb E:\b\depot_tools\python276_bin\python.exe gyp-win-tool link-with-manifests environment.x86 True gn.exe "E:\b\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper environment.x86 False link.exe /nologo /OUT:gn.exe @gn.exe.rsp" 1 mt.exe rc.exe "obj\tools\gn\gn.gn.exe.intermediate.manifest" obj\tools\gn\gn.gn.exe.generated.manifest ..\..\build\win\compatibility.manifest gn_lib.lib(gn_lib.command_desc.obj) : error LNK2019: unresolved external symbol "public: static class std::unique_ptr<class base::DictionaryValue,struct std::default_delete<class base::DictionaryValue> > __cdecl DescBuilder::DescriptionForTarget(class Target const *,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,bool,bool,bool)" (?DescriptionForTarget@DescBuilder@@SA?AV?$unique_ptr@VDictionaryValue@base@@U?$default_delete@VDictionaryValue@base@@@std@@@std@@PBVTarget@@ABV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@3@_N22@Z) referenced in function "bool __cdecl commands::`anonymous namespace'::PrintTarget(class Target const *,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,bool,bool,bool,bool)" (?PrintTarget@?A0x51180473@commands@@YA_NPBVTarget@@ABV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@_N222@Z) gn_lib.lib(gn_lib.command_desc.obj) : error LNK2019: unresolved external symbol "public: static class std::unique_ptr<class base::DictionaryValue,struct std::default_delete<class base::DictionaryValue> > __cdecl DescBuilder::DescriptionForConfig(class Config const *,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)" (?DescriptionForConfig@DescBuilder@@SA?AV?$unique_ptr@VDictionaryValue@base@@U?$default_delete@VDictionaryValue@base@@@std@@@std@@PBVConfig@@ABV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@3@@Z) referenced in function "bool __cdecl commands::`anonymous namespace'::PrintConfig(class Config const *,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,bool)" (?PrintConfig@?A0x51180473@commands@@YA_NPBVConfig@@ABV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@_N@Z) gn_lib.lib(gn_lib.command_gen.obj) : error LNK2019: unresolved external symbol "public: static bool __cdecl JSONProjectWriter::RunAndWriteFiles(class BuildSettings const *,class Builder const *,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,bool,class Err *)" (?RunAndWriteFiles@JSONProjectWriter@@SA_NPBVBuildSettings@@PBVBuilder@@ABV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@222_NPAVErr@@@Z) referenced in function "bool __cdecl commands::`anonymous namespace'::RunIdeWriter(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,class BuildSettings const *,class Builder *,class Err *)" (?RunIdeWriter@?A0x1eff038c@commands@@YA_NABV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@PBVBuildSettings@@PAVBuilder@@PAVErr@@@Z) gn.exe : fatal error LNK1120: 3 unresolved externals Traceback (most recent call last): File "gyp-win-tool", line 323, in <module> sys.exit(main(sys.argv[1:])) File "gyp-win-tool", line 29, in main exit_code = executor.Dispatch(args) File "gyp-win-tool", line 71, in Dispatch return getattr(self, method)(*args[1:]) File "gyp-win-tool", line 179, in ExecLinkWithManifests subprocess.check_call(ldcmd + add_to_ld) File "E:\b\depot_tools\python276_bin\lib\subprocess.py", line 540, in check_call raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command 'E:\b\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper environment.x86 False link.exe /nologo /OUT:gn.exe @gn.exe.rsp gn.exe.manifest.res' returned non-zero exit status 1120.
Message was sent while issue was closed.
Fixing and relanding in https://codereview.chromium.org/2156173003/ . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
