Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2239383002:
GN proto_libary refactoring. (Closed)
|
Created:
4 years, 4 months ago by kraynov Modified:
4 years, 4 months ago Reviewers:
Dirk Pranke, xyzzyz, brettw, oystein (OOO til 10th of July), petrcermak, Paweł Hajdan Jr., scottmg CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGN proto_libary refactoring.
Added support for sub-directories.
Builds multi-file libraries in a single pass to avoid repeated
proto parsing and to follow dependencies strictier.
BUG=637292
Committed: https://crrev.com/b72583a75181ba099159ce89b3f751054181fa62
Cr-Commit-Position: refs/heads/master@{#412970}
Patch Set 1 #Patch Set 2 : fix LITE_RUNTIME #
Total comments: 82
Patch Set 3 : review iteration #
Total comments: 13
Patch Set 4 : nit #
Total comments: 6
Patch Set 5 : public includes #Patch Set 6 : fix typo in test #
Total comments: 2
Patch Set 7 : temporary merge #Patch Set 8 : optimisation #Messages
Total messages: 60 (37 generated)
The CQ bit was checked by kraynov@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 kraynov@chromium.org
kraynov@chromium.org changed reviewers: + petrcermak@chromium.org
+petrcermak Could you please take a look? More details here: crbug.com/637292 Thank you!
The CQ bit was checked by kraynov@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 kraynov@chromium.org
Looks good overall. I've left you a bunch of inline comments (sorry for being so pedantic). In addition, s/Added/Add/, s/Builds/Build/ and s/strictier/more strictly/ in the description. Thanks, Petr https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (left): https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:13: # Targets that depend on the proto target will be able to include the Why do you remove this usage comment? https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:10: # Specifies the path relative to current BUILD.gn file where .proto files nit s/current/the current/ https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:11: # are located and directory structure of this proto library starts. s/directory structure/the directory structure/ https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:14: # assertion error if any nested directories found. s/found/are found/ https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:18: # This path will be appended to the root_gen_dir for cc or plugin output So it won't be appended in other use cases (apart from cc and plugin)? Are there any other use cases? https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:19: # and will be set as an include path as well. s/set/appended/? I don't know gn very well, but it seems strange to use a folder as both as output and include (maybe explain more?). https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:20: # Python stubs will be placed to |$root_build_dir/pyproto/$proto_out_dir|. I think that the correct notation is: |root_build_dir|/pyproto/|proto_out_dir| https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:44: # Don't specify a toolchain, host toolchain is assumed. s/Don't specify a toolchain/If a toolchain is not specified/ https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:133: # Avoid absolute path because of assumption that |proto_in_dir| is relative nit: s/assumption/the assumption/ https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:165: if (proto_dir != ".") { "positive" if statements are generally easier to read: if (proto_dir == ".") basic_path = proto_name } else { ... } This of course doesn't appear when you don't have an else clause. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:168: basic_path = proto_name Why do you need this special case? "./FILE" is the same path as "FILE", isn't it? https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:203: # may be different to |root_out_dir| of protoc built on host toolchain. nit: s/different to/different from/ https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:252: # System protoc is not used so need to build a chromium one. s/need/we need/ (or "it's necessary") https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:256: # Depends on generator plugin but for host toolchain only. What "depends on generator plugin"? "This target"? https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:294: # If using built-in cc generator the resulting headers reference headers There should be a comma after "generator". Otherwise, this sentence is ambiguous (as it's unclear where the condition ends). https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:295: # within protobuf_lite, hence dependencies require those headers too. "hence" should start a separate sentence and there should be a comma after it: within protobuf_lite. Hence, dependencies [...] https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:296: # In case of generator plugin such issues should be resolved by invoker. Again, there should be a comma after "plugin" to make it clear where the condition ends. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:296: # In case of generator plugin such issues should be resolved by invoker. nit: s/invoker/the invoker/ https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:2: # Copyright (c) 2016 The Chromium Authors. All rights reserved. The date should NOT be changed. See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.... https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:10: - Automatic include insertion. Maybe add a hash ("#include") so that it's easier to understand? https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:11: - Preventing bad proto names. s/Preventing/Prevents/ https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:24: def ProtocGenOptions(options): The name is a noun, so it suggests that it's a class (rather than a method). Something like "ConvertProtocGenOptionsToString" would be better. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:24: def ProtocGenOptions(options): If I understand correctly, all methods in this file are not meant to be used outside it. If that is indeed the case, please prefix their names with a single underscore (e.g. _ConvertProtocGenOptionsToString). https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:25: if options is not None: Don't use nested if statements unless necessary: if options is None: return '' if options.endswith(':'): return options return options + ':' https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:25: if options is not None: Unless '' (empty string) are valid |options|, you can use |not options| instead of |options is not None|. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:34: def StripProtos(protos): (Feel free to ignore) I would personally use a comprehension: def StripProtos(protos): return [StripProto(p) for p in protos) def StripProto(proto): if not proto.endswith(".proto"): ... ... return filename.rsplit('.', 1)[0] https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:40: raise RuntimeError("Proto files must not have hyphens in their names " suggestion "Proto file names must not contain hyphens" https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:41: "(see http://crbug.com/386125 for more information).") supernit: Your error ends with a period here, but not on lines 38 and 61 https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:55: if stripped_line == PROTOC_INCLUDE_POINT: you should add an if statement asserting that include_point_found is false here. if not, you will insert the include twice. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:64: for line in contents: f.writelines(contents) https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:69: parser = optparse.OptionParser() Could you use the argparse module instead? Optparse has been deprecated (https://docs.python.org/2/library/optparse.html). https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:80: help="Output directory for custom generator.") I think that the help string should contain the word "plugin" since it's in the flag name https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:85: help="Custom generator options.") I think that the help string should contain the word "plugin" since it's in the flag name https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:89: help="Name of include to put into generated headers.") supernit: s/put/insert/ https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:90: (options, args) = parser.parse_args(argv) no need for parentheses (feel free to ignore) https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:92: protoc = os.path.realpath(options.protoc) In my opinion, there's no need to define this here when you don't need it until line 119. (Feel free to ignore) https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:94: protoc_args = ["--proto_path", proto_dir] To avoid pre-pending the protoc binary at the end on line 119, why not just call this protoc_cmd: proto_cmd = [os.path.realpath(options.protoc), "--proto_path", proto_dir] https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:100: if options.cc_out_dir is not None: you could do the following: if options.cc_out_dir is None: headers = [] else: ... headers = [os.path.join(...) for name in stripped_protos] https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:117: protoc_args.append(os.path.join(proto_dir, name)) I personally prefer comprehensions over explicit loops protoc_args += [os.path.join(proto_dir, name) for name in protos] https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:123: if options.include is not None: I think you can drop "is not None" here https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:127: if __name__ == '__main__': Before this patch, this file used single quotes everywhere. Now, double quotes are used everywhere but here. Either switch back to single quotes everywhere, or change this to double quotes as well, please.
petrcermak@ Thanks for comments. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:10: # Specifies the path relative to current BUILD.gn file where .proto files On 2016/08/15 13:00:05, petrcermak wrote: > nit s/current/the current/ Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:11: # are located and directory structure of this proto library starts. On 2016/08/15 13:00:05, petrcermak wrote: > s/directory structure/the directory structure/ Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:14: # assertion error if any nested directories found. On 2016/08/15 13:00:05, petrcermak wrote: > s/found/are found/ Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:18: # This path will be appended to the root_gen_dir for cc or plugin output On 2016/08/15 13:00:05, petrcermak wrote: > So it won't be appended in other use cases (apart from cc and plugin)? Are there > any other use cases? In case of python it appends to $root_build_dir/pyproto/ https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:19: # and will be set as an include path as well. On 2016/08/15 13:00:05, petrcermak wrote: > s/set/appended/? I don't know gn very well, but it seems strange to use a folder > as both as output and include (maybe explain more?). It's "two-in-one", at first we convert .proto to .cc and .h files, then we compile these generated files as a static library. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:20: # Python stubs will be placed to |$root_build_dir/pyproto/$proto_out_dir|. On 2016/08/15 13:00:05, petrcermak wrote: > I think that the correct notation is: > > |root_build_dir|/pyproto/|proto_out_dir| Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:44: # Don't specify a toolchain, host toolchain is assumed. On 2016/08/15 13:00:05, petrcermak wrote: > s/Don't specify a toolchain/If a toolchain is not specified/ There is no condition whether toolchain is specified or not. Using any other toolchain for generator plugin is non-sense. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:133: # Avoid absolute path because of assumption that |proto_in_dir| is relative On 2016/08/15 13:00:05, petrcermak wrote: > nit: s/assumption/the assumption/ Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:165: if (proto_dir != ".") { On 2016/08/15 13:00:05, petrcermak wrote: > "positive" if statements are generally easier to read: > > if (proto_dir == ".") > basic_path = proto_name > } else { > ... > } > > This of course doesn't appear when you don't have an else clause. Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:168: basic_path = proto_name On 2016/08/15 13:00:05, petrcermak wrote: > Why do you need this special case? "./FILE" is the same path as "FILE", isn't > it? Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:203: # may be different to |root_out_dir| of protoc built on host toolchain. On 2016/08/15 13:00:05, petrcermak wrote: > nit: s/different to/different from/ Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:252: # System protoc is not used so need to build a chromium one. On 2016/08/15 13:00:05, petrcermak wrote: > s/need/we need/ (or "it's necessary") Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:256: # Depends on generator plugin but for host toolchain only. On 2016/08/15 13:00:05, petrcermak wrote: > What "depends on generator plugin"? "This target"? This action https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:294: # If using built-in cc generator the resulting headers reference headers On 2016/08/15 13:00:05, petrcermak wrote: > There should be a comma after "generator". Otherwise, this sentence is ambiguous > (as it's unclear where the condition ends). Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:295: # within protobuf_lite, hence dependencies require those headers too. On 2016/08/15 13:00:05, petrcermak wrote: > "hence" should start a separate sentence and there should be a comma after it: > > within protobuf_lite. Hence, dependencies [...] Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:296: # In case of generator plugin such issues should be resolved by invoker. On 2016/08/15 13:00:05, petrcermak wrote: > nit: s/invoker/the invoker/ Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:2: # Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/08/15 13:00:06, petrcermak wrote: > The date should NOT be changed. See > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.... Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:10: - Automatic include insertion. On 2016/08/15 13:00:06, petrcermak wrote: > Maybe add a hash ("#include") so that it's easier to understand? Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:11: - Preventing bad proto names. On 2016/08/15 13:00:06, petrcermak wrote: > s/Preventing/Prevents/ Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:24: def ProtocGenOptions(options): On 2016/08/15 13:00:06, petrcermak wrote: > The name is a noun, so it suggests that it's a class (rather than a method). > Something like "ConvertProtocGenOptionsToString" would be better. Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:24: def ProtocGenOptions(options): On 2016/08/15 13:00:06, petrcermak wrote: > If I understand correctly, all methods in this file are not meant to be used > outside it. If that is indeed the case, please prefix their names with a single > underscore (e.g. _ConvertProtocGenOptionsToString). It's standalone utility. I've followed the previous style. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:25: if options is not None: On 2016/08/15 13:00:06, petrcermak wrote: > Don't use nested if statements unless necessary: > > if options is None: > return '' > if options.endswith(':'): > return options > return options + ':' Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:34: def StripProtos(protos): On 2016/08/15 13:00:06, petrcermak wrote: > (Feel free to ignore) I would personally use a comprehension: > > > def StripProtos(protos): > return [StripProto(p) for p in protos) > > def StripProto(proto): > if not proto.endswith(".proto"): > ... > ... > return filename.rsplit('.', 1)[0] Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:40: raise RuntimeError("Proto files must not have hyphens in their names " On 2016/08/15 13:00:06, petrcermak wrote: > suggestion "Proto file names must not contain hyphens" Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:41: "(see http://crbug.com/386125 for more information).") On 2016/08/15 13:00:06, petrcermak wrote: > supernit: Your error ends with a period here, but not on lines 38 and 61 Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:55: if stripped_line == PROTOC_INCLUDE_POINT: On 2016/08/15 13:00:06, petrcermak wrote: > you should add an if statement asserting that include_point_found is false here. > if not, you will insert the include twice. Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:64: for line in contents: On 2016/08/15 13:00:06, petrcermak wrote: > f.writelines(contents) Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:69: parser = optparse.OptionParser() On 2016/08/15 13:00:06, petrcermak wrote: > Could you use the argparse module instead? Optparse has been deprecated > (https://docs.python.org/2/library/optparse.html). Okay. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:80: help="Output directory for custom generator.") On 2016/08/15 13:00:06, petrcermak wrote: > I think that the help string should contain the word "plugin" since it's in the > flag name Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:85: help="Custom generator options.") On 2016/08/15 13:00:07, petrcermak wrote: > I think that the help string should contain the word "plugin" since it's in the > flag name Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:89: help="Name of include to put into generated headers.") On 2016/08/15 13:00:06, petrcermak wrote: > supernit: s/put/insert/ Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:90: (options, args) = parser.parse_args(argv) On 2016/08/15 13:00:06, petrcermak wrote: > no need for parentheses (feel free to ignore) Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:92: protoc = os.path.realpath(options.protoc) On 2016/08/15 13:00:06, petrcermak wrote: > In my opinion, there's no need to define this here when you don't need it until > line 119. (Feel free to ignore) Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:94: protoc_args = ["--proto_path", proto_dir] On 2016/08/15 13:00:06, petrcermak wrote: > To avoid pre-pending the protoc binary at the end on line 119, why not just call > this protoc_cmd: > > proto_cmd = [os.path.realpath(options.protoc), "--proto_path", proto_dir] Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:100: if options.cc_out_dir is not None: On 2016/08/15 13:00:06, petrcermak wrote: > you could do the following: > > if options.cc_out_dir is None: > headers = [] > else: > ... > headers = [os.path.join(...) for name in stripped_protos] Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:117: protoc_args.append(os.path.join(proto_dir, name)) On 2016/08/15 13:00:06, petrcermak wrote: > I personally prefer comprehensions over explicit loops > > protoc_args += [os.path.join(proto_dir, name) for name in protos] Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:123: if options.include is not None: On 2016/08/15 13:00:06, petrcermak wrote: > I think you can drop "is not None" here Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:127: if __name__ == '__main__': On 2016/08/15 13:00:06, petrcermak wrote: > Before this patch, this file used single quotes everywhere. Now, double quotes > are used everywhere but here. Either switch back to single quotes everywhere, or > change this to double quotes as well, please. Acknowledged.
The CQ bit was checked by kraynov@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 kraynov@chromium.org
Is it better?
LGTM with a few more comments ;-) Thanks, Petr https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:18: # This path will be appended to the root_gen_dir for cc or plugin output On 2016/08/15 13:37:49, kraynov wrote: > On 2016/08/15 13:00:05, petrcermak wrote: > > So it won't be appended in other use cases (apart from cc and plugin)? Are > there > > any other use cases? > > In case of python it appends to $root_build_dir/pyproto/ OK. Thanks for the explanation. I think that the current wording is still confusing for someone who doesn't know this code well (such as me). Would the following work: For cc and plugin output, the path will be appended to ... For Python stubs, the path will be appended to ... BTW you removed "will be set as an include path as well". Was that intentional? https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:19: # and will be set as an include path as well. On 2016/08/15 13:37:49, kraynov wrote: > On 2016/08/15 13:00:05, petrcermak wrote: > > s/set/appended/? I don't know gn very well, but it seems strange to use a > folder > > as both as output and include (maybe explain more?). > > It's "two-in-one", at first we convert .proto to .cc and .h files, then we > compile these generated files as a static library. Acknowledged. https://codereview.chromium.org/2239383002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:44: # Don't specify a toolchain, host toolchain is assumed. On 2016/08/15 13:37:48, kraynov wrote: > On 2016/08/15 13:00:05, petrcermak wrote: > > s/Don't specify a toolchain/If a toolchain is not specified/ > > There is no condition whether toolchain is specified or not. Using any other > toolchain for generator plugin is non-sense. OK, now I understand :-) To make it less ambiguous, I suggest you replace the comma with a period (to make it clear that the first sentence is not a condition). https://codereview.chromium.org/2239383002/diff/40001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2239383002/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:20: # it will be appended to the |root_build_dir|/pyproto. nit: remove "the" here. In general, it should be either "the NAME THING" (e.g. "the /root/ folder") or just "NAME" (e.g. "/root/"). https://codereview.chromium.org/2239383002/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:20: # it will be appended to the |root_build_dir|/pyproto. Didn't you forget |proto_out_dir| at the end of the folder (it was there in the previous patch)? https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:35: "{0} .".format(filename)) nit: Please remove space before period. https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:51: continue Wouldn't it be better to add this if check inside the next if statement and raise an error: if stripped_line == PROTOC_INCLUDE_POINT: if include_point_found: raise RuntimeError('Multiple include points found') ... https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:102: stripped_protos = [StripProtoExtension(filename) for filename in protos] You only need this inside one of the if statements (namely on line 112). Please move it there. https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:115: if options.py_out_dir is not None: (also applies to line 121) Unless empty strings are valid values here, consider using `if X:` rather than `if X is not None:`. https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:134: "{0} .".format(str(ret))) I don't think you need to call str(...) explicitly. format(...) should take care of it automatically.
The CQ bit was checked by kraynov@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: This issue passed the CQ dry run.
The CQ bit was unchecked by kraynov@chromium.org
https://codereview.chromium.org/2239383002/diff/40001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2239383002/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:20: # it will be appended to the |root_build_dir|/pyproto. On 2016/08/16 14:10:31, petrcermak wrote: > Didn't you forget |proto_out_dir| at the end of the folder (it was there in the > previous patch)? It's correct here. |root_gen_dir| and |root_build_dir|/pyproto are base paths. https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:35: "{0} .".format(filename)) On 2016/08/16 14:10:31, petrcermak wrote: > nit: Please remove space before period. It's just to separate from an identifier. https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:51: continue On 2016/08/16 14:10:31, petrcermak wrote: > Wouldn't it be better to add this if check inside the next if statement and > raise an error: > > if stripped_line == PROTOC_INCLUDE_POINT: > if include_point_found: > raise RuntimeError('Multiple include points found') > ... Done. https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:102: stripped_protos = [StripProtoExtension(filename) for filename in protos] On 2016/08/16 14:10:31, petrcermak wrote: > You only need this inside one of the if statements (namely on line 112). Please > move it there. Done. https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:115: if options.py_out_dir is not None: On 2016/08/16 14:10:31, petrcermak wrote: > (also applies to line 121) Unless empty strings are valid values here, consider > using `if X:` rather than `if X is not None:`. Done. https://codereview.chromium.org/2239383002/diff/40001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:134: "{0} .".format(str(ret))) On 2016/08/16 14:10:31, petrcermak wrote: > I don't think you need to call str(...) explicitly. format(...) should take care > of it automatically. Done.
kraynov@chromium.org changed reviewers: + brettw@chromium.org, scottmg@chromium.org, xyzzyz@chromium.org
+xyzzyz +brettw +scottmg PTAL
LGTM https://codereview.chromium.org/2239383002/diff/60001/tools/protoc_wrapper/pr... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2239383002/diff/60001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:35: raise RuntimeError("Proto file names must not contents hyphens " contain? https://codereview.chromium.org/2239383002/diff/60001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:113: "--cpp_out", Can this protoc_cmd be a single line? Here and below too?
https://codereview.chromium.org/2239383002/diff/60001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2239383002/diff/60001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:289: include_dirs += [ cc_out_dir ] Does this include directory need to be applied to targets that depend on this also (that include these headers). I don't see where this happens if you do indeed need this.
The CQ bit was checked by kraynov@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...
Include dirs made public :) https://codereview.chromium.org/2239383002/diff/60001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2239383002/diff/60001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:289: include_dirs += [ cc_out_dir ] On 2016/08/16 22:08:10, brettw (ping on IM after 24h) wrote: > Does this include directory need to be applied to targets that depend on this > also (that include these headers). I don't see where this happens if you do > indeed need this. Thank you! This case now fortified by unit test. https://codereview.chromium.org/2239383002/diff/60001/tools/protoc_wrapper/pr... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2239383002/diff/60001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:35: raise RuntimeError("Proto file names must not contents hyphens " On 2016/08/16 21:04:37, xyzzyz wrote: > contain? Done. https://codereview.chromium.org/2239383002/diff/60001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:113: "--cpp_out", On 2016/08/16 21:04:37, xyzzyz wrote: > Can this protoc_cmd be a single line? Here and below too? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kraynov@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2239383002/diff/100001/third_party/protobuf/p... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2239383002/diff/100001/third_party/protobuf/p... third_party/protobuf/proto_library.gni:274: config(config_name) { Do we always need to define configs? There are a lot of proto target sos this will introduce a lot of configs which each have some overhead. Given that we've lived so long without this, I wonder if there might only be a few cases where these are needed (like when you specify an in dir?)
Seems like just yesterday someone submitted a conflicting CL that rolls own its own generator plugin support, completely ignoring existing one, just FYI: https://codereview.chromium.org/2034373002/ I asked them to revert their change.
Yes, I already realized conflicting change. Their plugin is written in Python so will think how to support this in a consistent way. https://codereview.chromium.org/2239383002/diff/100001/third_party/protobuf/p... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2239383002/diff/100001/third_party/protobuf/p... third_party/protobuf/proto_library.gni:274: config(config_name) { On 2016/08/17 18:23:50, brettw (ping on IM after 24h) wrote: > Do we always need to define configs? There are a lot of proto target sos this > will introduce a lot of configs which each have some overhead. Given that we've > lived so long without this, I wonder if there might only be a few cases where > these are needed (like when you specify an in dir?) Okay, will consider an alternative.
The CQ bit was checked by kraynov@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: This issue passed the CQ dry run.
The CQ bit was checked by kraynov@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: This issue passed the CQ dry run.
PTAL There is temporary workaround to satisfy //third_party/dom_distiller_js/BUILD.gn Will be removed very shortly in the next CL.
lgtm
lgtm
kraynov@chromium.org changed reviewers: + dpranke@chromium.org, oysteine@chromium.org
+oysteine +dpranke Hi, could you please take a look into changes made at //tools and //components/tracing? Thank you!
scottmg@chromium.org changed reviewers: + phajdan.jr@chromium.org
//tools lgtm +phajdan fyi for system-level protoc, which doesn't seem to be supported already in the GN build.
rs- lgtm.
components/tracing lgtm
The CQ bit was checked by kraynov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2239383002/#ps140001 (title: "optimisation")
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.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== GN proto_libary refactoring. Added support for sub-directories. Builds multi-file libraries in a single pass to avoid repeated proto parsing and to follow dependencies strictier. BUG=637292 ========== to ========== GN proto_libary refactoring. Added support for sub-directories. Builds multi-file libraries in a single pass to avoid repeated proto parsing and to follow dependencies strictier. BUG=637292 Committed: https://crrev.com/b72583a75181ba099159ce89b3f751054181fa62 Cr-Commit-Position: refs/heads/master@{#412970} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b72583a75181ba099159ce89b3f751054181fa62 Cr-Commit-Position: refs/heads/master@{#412970} |
