Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2082693002:
Plugin support for protobuf compiler (protoc) targets. (Closed)
DescriptionPlugin support for protobuf compiler (protoc) targets.
Until now the protobuf generator targets in the build files
did hard-code the unconditional generation of C++ and python
stubs. There are two new upcoming use cases, that are,
bit.ly/TracingV2 (see BUG) and gRPC, where we need to generate
C++ stubs using a custom protoc plugin.
This CL introduces the plumbing necessary to the GYP/GN
build files to be able to pass an arbitrary plugin to
the protoc invocation. See crrev.com/2083373002 as an example of
how this will be used in upcoming CLs.
BUG=622680
Committed: https://crrev.com/b241e6e5fa05e90eaa4199d4bc497869e3cf08e1
Cr-Commit-Position: refs/heads/master@{#403981}
Patch Set 1 #
Total comments: 2
Patch Set 2 : cc_generator_suffix #Patch Set 3 : more flexible targets and revert of protoc_wrapper #
Total comments: 13
Patch Set 4 : minor fixes #Patch Set 5 #
Total comments: 3
Patch Set 6 : comments #
Total comments: 1
Patch Set 7 : comment #
Total comments: 1
Patch Set 8 : hard_dependency #Patch Set 9 : rebase #Messages
Total messages: 50 (20 generated)
Update of //third_party/protobuf/README.chromium is not required because nothing changed to upstream.
Description was changed from ========== Plugin support for protobuf compiler (protoc) targets. Introduced new argument cc_generator_plugin to proto_library.gni (GN) and protoc.gypi (GYP) build recipes. BUG= ========== to ========== Plugin support for protobuf compiler (protoc) targets. Introduced new argument cc_generator_plugin to proto_library.gni (GN) and protoc.gypi (GYP) build recipes. BUG= ==========
kraynov@chromium.org changed reviewers: + primiano@chromium.org, xyzzyz@chromium.org
Please take a look. Thanks!
https://codereview.chromium.org/2082693002/diff/1/third_party/protobuf/proto_... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2082693002/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:29: # Plugin executable name. Expected .pb.h and .pb.cc as output. If you drop the requirement that the plugin outputs need to be .pb.h and .pb.cc, I think the code will be both more general and simpler at the same time. It will be more general, because e.g. gRPC plugin generates .grpc.pb.h and .grpc.pb.cc files in addition to .pb.h/.pb.cc files generated by regular protoc, and so the current code will not be usable with gRPC plugin. You will probably need to add some parameter here to let the GN/GYP know about the desired extension (and so about desired output artifacts). Also, it looks like your plugin generates the file with the same as regular "--cpp_out". Is it necessary in your case? If so, you could also add parameters that would determine if regular "--cpp_out" is passed. It will be simpler, because you will not need to make any changes to protoc_wrapper.py at all -- you will just add an additional "--plugin_out" flag alongside currently existing "--cpp_out" and "--python_out", conditional on a plugin being used.
https://codereview.chromium.org/2082693002/diff/1/third_party/protobuf/proto_... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2082693002/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:29: # Plugin executable name. Expected .pb.h and .pb.cc as output. On 2016/06/20 20:17:46, xyzzyz wrote: > If you drop the requirement that the plugin outputs need to be .pb.h and .pb.cc, > I think the code will be both more general and simpler at the same time. > > It will be more general, because e.g. gRPC plugin generates .grpc.pb.h and > .grpc.pb.cc files in addition to .pb.h/.pb.cc files generated by regular protoc, > and so the current code will not be usable with gRPC plugin. You will probably > need to add some parameter here to let the GN/GYP know about the desired > extension (and so about desired output artifacts). Also, it looks like your > plugin generates the file with the same as regular "--cpp_out". Is it necessary > in your case? If so, you could also add parameters that would determine if > regular "--cpp_out" is passed. > > It will be simpler, because you will not need to make any changes to > protoc_wrapper.py at all -- you will just add an additional "--plugin_out" flag > alongside currently existing "--cpp_out" and "--python_out", conditional on a > plugin being used. Acknowledged.
On 2016/06/20 20:19:47, kraynov wrote: > https://codereview.chromium.org/2082693002/diff/1/third_party/protobuf/proto_... > File third_party/protobuf/proto_library.gni (right): > > https://codereview.chromium.org/2082693002/diff/1/third_party/protobuf/proto_... > third_party/protobuf/proto_library.gni:29: # Plugin executable name. > Expected .pb.h and .pb.cc as output. > On 2016/06/20 20:17:46, xyzzyz wrote: > > If you drop the requirement that the plugin outputs need to be .pb.h and > .pb.cc, > > I think the code will be both more general and simpler at the same time. > > > > It will be more general, because e.g. gRPC plugin generates .grpc.pb.h and > > .grpc.pb.cc files in addition to .pb.h/.pb.cc files generated by regular > protoc, > > and so the current code will not be usable with gRPC plugin. You will probably > > need to add some parameter here to let the GN/GYP know about the desired > > extension (and so about desired output artifacts). Also, it looks like your > > plugin generates the file with the same as regular "--cpp_out". Is it > necessary > > in your case? If so, you could also add parameters that would determine if > > regular "--cpp_out" is passed. > > > > It will be simpler, because you will not need to make any changes to > > protoc_wrapper.py at all -- you will just add an additional "--plugin_out" > flag > > alongside currently existing "--cpp_out" and "--python_out", conditional on a > > plugin being used. > > Acknowledged. The change to protoc_wrapper.py was made in order to avoid significant difficulty of handling undefined variables in GYP (some targets still using it). Now trying to find a reasonable way to satisfy requirements of gRPC plugin. Last change with introduced suffixes is not yet enough for gRPC.
Description was changed from ========== Plugin support for protobuf compiler (protoc) targets. Introduced new argument cc_generator_plugin to proto_library.gni (GN) and protoc.gypi (GYP) build recipes. BUG= ========== to ========== Plugin support for protobuf compiler (protoc) targets. Introduced new arguments to proto_library.gni (GN) and protoc.gypi (GYP) build recipes. BUG= ==========
Description was changed from ========== Plugin support for protobuf compiler (protoc) targets. Introduced new arguments to proto_library.gni (GN) and protoc.gypi (GYP) build recipes. BUG= ========== to ========== Plugin support for protobuf compiler (protoc) targets. Introduced new arguments to proto_library.gni (GN) and protoc.gypi (GYP) build recipes. BUG= ==========
protoc_wrapper.py is reverted. Found the way to implement the feature in GN and GYP only.
Ok some comments. Overall seems to have a good shape. I'll leave the final round to xyzzyz. Also I suggest to add brettw@ once you get a LG form xyzzyz, as IIRC he wrote the gn part of this. https://codereview.chromium.org/2082693002/diff/40001/build/protoc.gypi File build/protoc.gypi (left): https://codereview.chromium.org/2082693002/diff/40001/build/protoc.gypi#oldco... build/protoc.gypi:122: 'hard_dependency': 1, why are you moving this and removing its comment? I think you always want hard_dependency, not just in the case of generate_cc. https://codereview.chromium.org/2082693002/diff/40001/build/protoc.gypi File build/protoc.gypi (right): https://codereview.chromium.org/2082693002/diff/40001/build/protoc.gypi#newco... build/protoc.gypi:57: 'generator_plugin%': '', # generator_plugin_suffix is required if set. you want to add a comment here explaining that this needs to be the name of the target for the plugin, otherwise it's not clear whether this needs to be a path or whatnot. https://codereview.chromium.org/2082693002/diff/40001/build/protoc.gypi#newco... build/protoc.gypi:77: 'outputs': [ IIRC in gyp, conversely to gn, the list doesn't need to be defined before doing +=. In other words, you can delete these 3 lines alltogether. https://codereview.chromium.org/2082693002/diff/40001/build/protoc.gypi#newco... build/protoc.gypi:103: 'outputs+': [ Looks like in gyp name+ means "prepend" which is NOT what you want to do here. I'm surprised that the action+ statement below worked :) https://codereview.chromium.org/2082693002/diff/40001/build/protoc.gypi#newco... build/protoc.gypi:123: '<(cc_dir)/<(RULE_INPUT_ROOT)<(generator_plugin_suffix).cc', I think you want this initialized in the variables above anyways. I don't know what the behavior of gyp would be otherwise. Gyp is ... tough. https://codereview.chromium.org/2082693002/diff/40001/build/protoc.gypi#newco... build/protoc.gypi:128: 'protoc-gen-plugin=<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)<(generator_plugin)<(EXECUTABLE_SUFFIX)', I think here you should add a dependencies: <(generator_plugin) so that this target ends up depending on your plugin target and you don't have to rely on the caller remembering to do that. https://codereview.chromium.org/2082693002/diff/40001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2082693002/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:35: # Custom plugin executable name. I think this is s/executable/target/ https://codereview.chromium.org/2082693002/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:97: if (defined(invoker.generator_plugin)) { not sure you need this (See below) https://codereview.chromium.org/2082693002/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:111: out_dir = "$root_gen_dir/" + proto_out_dir It seems this is changing the assignment of out_dir if you take the else branch? Is it intended? Before, if you take the else branch, out_dir = "{{source_gen_dir}}” now, it becomes "$root_gen_dir/{{source_root_relative_dir}}" https://codereview.chromium.org/2082693002/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:175: if (generator_plugin != "") { Why this is not if (defined(invoker.generator_plugin)) ? https://codereview.chromium.org/2082693002/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:222: if (!defined(invoker.generator_plugin)) { I think this should be really: if (generate_cc)
https://codereview.chromium.org/2082693002/diff/40001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2082693002/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:111: out_dir = "$root_gen_dir/" + proto_out_dir On 2016/06/21 14:15:09, Primiano Tucci wrote: > It seems this is changing the assignment of out_dir if you take the else branch? > Is it intended? > Before, if you take the else branch, out_dir = "{{source_gen_dir}}” > now, it becomes "$root_gen_dir/{{source_root_relative_dir}}" It is intended. These string will be the same. Basic idea is that proto_out_dir should get default value if not set. Output directories before and after the change are equal.
https://codereview.chromium.org/2082693002/diff/40001/build/protoc.gypi File build/protoc.gypi (right): https://codereview.chromium.org/2082693002/diff/40001/build/protoc.gypi#newco... build/protoc.gypi:128: 'protoc-gen-plugin=<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)<(generator_plugin)<(EXECUTABLE_SUFFIX)', On 2016/06/21 14:15:09, Primiano Tucci wrote: > I think here you should add a dependencies: <(generator_plugin) so that this > target ends up depending on your plugin target and you don't have to rely on the > caller remembering to do that. As discussed in offline chat, target name is not sufficient to put dependency into GN. Current GN approach is relying on invoker about the dependency. Should have the same in GYP.
https://codereview.chromium.org/2082693002/diff/80001/build/protoc.gypi File build/protoc.gypi (right): https://codereview.chromium.org/2082693002/diff/80001/build/protoc.gypi#newco... build/protoc.gypi:59: 'generator_plugin%': '', Could you rename it to "cc_generator_plugin" to make it clear that it only supports generating C++ output (as .cc and .h are hardcoded)? If someone needs plugin support for other languages, I'll ask them to refactor this to be even more general and support more languages, but C++ support only will be fine for now. https://codereview.chromium.org/2082693002/diff/80001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2082693002/diff/80001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:34: # generator_plugin (optional) Same as in gypi (cc_generator_plugin and same for other variables) https://codereview.chromium.org/2082693002/diff/80001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:153: "$cc_generator_options$rel_out_dir", # Separated by colon. Could you keep the original comment about colon? It's more informative.
kraynov@chromium.org changed reviewers: + brettw@chromium.org
kraynov@chromium.org changed reviewers: + brettw@chromium.org
Please review. Thanks!
Please review. Thanks!
lgtm https://codereview.chromium.org/2082693002/diff/100001/third_party/protobuf/p... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2082693002/diff/100001/third_party/protobuf/p... third_party/protobuf/proto_library.gni:35: # Target name of plugin which generates custom cc stubs. What exactly is "target name" here? By looking at the code, I understand it's the name of the plugin executable binary, not the //chrome/foo:bar. Please clarify the comment, and add a comment that the plugin needs to be in the deps clause of proto_library (as I understand this is how it's supposed to work).
The CQ bit was checked by kraynov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xyzzyz@chromium.org Link to the patchset: https://codereview.chromium.org/2082693002/#ps120001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2082693002/120001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Two people asked me to comment on this, so here's a drive-by not lgtm: It's not clear what this is for. Please file a tracking bug for what you want to do and link to that, or explain it elsewhere.
Description was changed from ========== Plugin support for protobuf compiler (protoc) targets. Introduced new arguments to proto_library.gni (GN) and protoc.gypi (GYP) build recipes. BUG= ========== to ========== Plugin support for protobuf compiler (protoc) targets. Introduced new arguments to proto_library.gni (GN) and protoc.gypi (GYP) build recipes. BUG=622680 ==========
proto_libarary targe was hardcoded to generate .cc and python stubs. As described in crbug.com/622680 there are new use cases upcoming (Tracing V2 and gRPC) where we want to pass a custom plugin to protoc. This change adds this missing capability to GN/gyp build files to pass a custom plugin to protoc.
Description was changed from
==========
Plugin support for protobuf compiler (protoc) targets.
Introduced new arguments to proto_library.gni (GN) and
protoc.gypi (GYP) build recipes.
BUG=622680
==========
to
==========
Plugin support for protobuf compiler (protoc) targets.
Introduced new arguments to 'proto_library.gni' and 'protoc.gypi'
build recipes and un-hardcoded default (cc and python) output.
Custom generator can be passed as 'generator_plugin' argument.
Default output can be adjusted with 'generate_{cc|python}' flags.
BUG=622680
==========
I am not a build file expert, nor at owner, but the changes LGTM. Plz get Nico/Brett to review them. My major comment is on the CL description, my guideline is: more code makes the project less readable. We don't want more code unless it's really necessary. When you send out a CL you want to explain and convince folks that we really need this change. The problem of this commit message is that it says what you are doing, doesn't really tell why. The way I'd have worded this commit message is: ----- Until now the protobuf generator targets in the build files did hard-code the unconditional generation of C++ and python stubs. There are two new upcoming use cases, that are, bit.ly/TracingV2 (see BUG) and gRPC, where we need to generate C++ stubs using a custom protoc plugin. This CL introduces the plumbing necessary to the GYP/GN build files to be able to pass an arbitrary plugin to the protoc invocation. See crrev.com/XXXXXX as an example of how this will be used in upcoming CLs. BUG=xxxxx ----- https://codereview.chromium.org/2082693002/diff/120001/build/protoc.gypi File build/protoc.gypi (right): https://codereview.chromium.org/2082693002/diff/120001/build/protoc.gypi#newc... build/protoc.gypi:154: 'hard_dependency': 1, Not quite sure about this, probably hard_dependency should stay unconditional as you will have side effects also when using the plugin? Defer this to thakis. I never fully understood how hard_dependency works.
Description was changed from
==========
Plugin support for protobuf compiler (protoc) targets.
Introduced new arguments to 'proto_library.gni' and 'protoc.gypi'
build recipes and un-hardcoded default (cc and python) output.
Custom generator can be passed as 'generator_plugin' argument.
Default output can be adjusted with 'generate_{cc|python}' flags.
BUG=622680
==========
to
==========
Plugin support for protobuf compiler (protoc) targets.
Until now the protobuf generator targets in the build files
did hard-code the unconditional generation of C++ and python
stubs. There are two new upcoming use cases, that are,
bit.ly/TracingV2 (see BUG) and gRPC, where we need to generate
C++ stubs using a custom protoc plugin.
This CL introduces the plumbing necessary to the GYP/GN
build files to be able to pass an arbitrary plugin to
the protoc invocation. See crrev.com/2083373002 as an example of
how this will be used in upcoming CLs.
BUG=622680
==========
Description was changed from ========== Plugin support for protobuf compiler (protoc) targets. Until now the protobuf generator targets in the build files did hard-code the unconditional generation of C++ and python stubs. There are two new upcoming use cases, that are, bit.ly/TracingV2 (see BUG) and gRPC, where we need to generate C++ stubs using a custom protoc plugin. This CL introduces the plumbing necessary to the GYP/GN build files to be able to pass an arbitrary plugin to the protoc invocation. See crrev.com/2083373002 as an example of how this will be used in upcoming CLs. BUG=622680 ========== to ========== Plugin support for protobuf compiler (protoc) targets. Until now the protobuf generator targets in the build files did hard-code the unconditional generation of C++ and python stubs. There are two new upcoming use cases, that are, bit.ly/TracingV2 (see BUG) and gRPC, where we need to generate C++ stubs using a custom protoc plugin. This CL introduces the plumbing necessary to the GYP/GN build files to be able to pass an arbitrary plugin to the protoc invocation. See crrev.com/2083373002 as an example of how this will be used in upcoming CLs. BUG=622680 ==========
Nico, can you PTAL to the revised CL? (kraynov is OOO but should be back soon)
LGTM, but you'll need to merge my recent changes to the proto_library.gni file. Hopefully it's not too hard.
On 2016/07/01 18:25:16, brettw (plz ping after 24h) wrote: > LGTM, but you'll need to merge my recent changes to the proto_library.gni file. > Hopefully it's not too hard. Took a look to PS 9 and rebase on top of crrev.com/2110233003 looks sane to me. Ship it!
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, xyzzyz@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2082693002/#ps160001 (title: "rebase")
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
A disapproval has been posted by following reviewers: thakis@chromium.org. Please make sure to get positive review before another CQ attempt.
On 2016/07/05 09:42:57, commit-bot: I haz the power wrote: > A disapproval has been posted by following reviewers: mailto:thakis@chromium.org. > Please make sure to get positive review before another CQ attempt. Nico, does it LGTY now? Your disapproval is blocking this CL.
This CL lgtm, but please send out a design doc to chromium-dev before actually a plugin.
The CQ bit was checked by kraynov@chromium.org
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 ========== Plugin support for protobuf compiler (protoc) targets. Until now the protobuf generator targets in the build files did hard-code the unconditional generation of C++ and python stubs. There are two new upcoming use cases, that are, bit.ly/TracingV2 (see BUG) and gRPC, where we need to generate C++ stubs using a custom protoc plugin. This CL introduces the plumbing necessary to the GYP/GN build files to be able to pass an arbitrary plugin to the protoc invocation. See crrev.com/2083373002 as an example of how this will be used in upcoming CLs. BUG=622680 ========== to ========== Plugin support for protobuf compiler (protoc) targets. Until now the protobuf generator targets in the build files did hard-code the unconditional generation of C++ and python stubs. There are two new upcoming use cases, that are, bit.ly/TracingV2 (see BUG) and gRPC, where we need to generate C++ stubs using a custom protoc plugin. This CL introduces the plumbing necessary to the GYP/GN build files to be able to pass an arbitrary plugin to the protoc invocation. See crrev.com/2083373002 as an example of how this will be used in upcoming CLs. BUG=622680 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Plugin support for protobuf compiler (protoc) targets. Until now the protobuf generator targets in the build files did hard-code the unconditional generation of C++ and python stubs. There are two new upcoming use cases, that are, bit.ly/TracingV2 (see BUG) and gRPC, where we need to generate C++ stubs using a custom protoc plugin. This CL introduces the plumbing necessary to the GYP/GN build files to be able to pass an arbitrary plugin to the protoc invocation. See crrev.com/2083373002 as an example of how this will be used in upcoming CLs. BUG=622680 ========== to ========== Plugin support for protobuf compiler (protoc) targets. Until now the protobuf generator targets in the build files did hard-code the unconditional generation of C++ and python stubs. There are two new upcoming use cases, that are, bit.ly/TracingV2 (see BUG) and gRPC, where we need to generate C++ stubs using a custom protoc plugin. This CL introduces the plumbing necessary to the GYP/GN build files to be able to pass an arbitrary plugin to the protoc invocation. See crrev.com/2083373002 as an example of how this will be used in upcoming CLs. BUG=622680 Committed: https://crrev.com/b241e6e5fa05e90eaa4199d4bc497869e3cf08e1 Cr-Commit-Position: refs/heads/master@{#403981} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b241e6e5fa05e90eaa4199d4bc497869e3cf08e1 Cr-Commit-Position: refs/heads/master@{#403981} |
