Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2388113003:
Allow proto-path in the proto_library target (Closed)
DescriptionAllow proto-path in the proto_library target
Also forward include_dirs as it is needed to compile the resulting .cc.
This is needed to import protos from another directory.
One example of usage is to have a local proto that include a sync proto
to avoid duplicate the sync data.
BUG=652312
Patch Set 1 #
Total comments: 10
Patch Set 2 : feedback #
Total comments: 2
Patch Set 3 : feedback #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 23 (5 generated)
olivierrobin@chromium.org changed reviewers: + sdefresne@chromium.org, thakis@chromium.org
https://codereview.chromium.org/2388113003/diff/1/third_party/protobuf/proto_... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2388113003/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:23: # Specifies the path of an additionnal directory to find imported I would make this an optional list (to allow adding multiple proto_path paramaters to the script). https://codereview.chromium.org/2388113003/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:244: args += [ If using a list as recommended, then you would need to change this to: if (defined(invoker.proto_path)) { foreach(_proto_dir, invoker.proto_path) { args += [ "--proto-path", rebase_path(_proto_dir, root_build_dir), ] } } https://codereview.chromium.org/2388113003/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:246: "./" + rebase_path(invoker.proto_path, root_build_dir), I think you can remove this "./". https://codereview.chromium.org/2388113003/diff/1/tools/protoc_wrapper/protoc... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2388113003/diff/1/tools/protoc_wrapper/protoc... tools/protoc_wrapper/protoc_wrapper.py:77: parser.add_argument("--proto-path", I would make this an argument that can be passed multiple time (ie. action="append"). parser.add_argument("--proto-path", action="append", help="Base directory with imported protos.") https://codereview.chromium.org/2388113003/diff/1/tools/protoc_wrapper/protoc... tools/protoc_wrapper/protoc_wrapper.py:126: if options.proto_path: If you change the argument to one that accept multiple items, then you need to change this to a loop (note that the if before the for is there because the value will be None if the option is not used, it is possible to remove the need for the if by adding default=[] to the add_argument call): if options.proto_path: for extra_proto_dir in options.proto_path: proto_cmd.extend(["--proto_path", os.path.relpath(extra_proto_dir)]
https://codereview.chromium.org/2388113003/diff/1/third_party/protobuf/proto_... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2388113003/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:23: # Specifies the path of an additionnal directory to find imported On 2016/10/04 11:29:55, sdefresne wrote: > I would make this an optional list (to allow adding multiple proto_path > paramaters to the script). Done. https://codereview.chromium.org/2388113003/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:244: args += [ On 2016/10/04 11:29:55, sdefresne wrote: > If using a list as recommended, then you would need to change this to: > > if (defined(invoker.proto_path)) { > foreach(_proto_dir, invoker.proto_path) { > args += [ > "--proto-path", > rebase_path(_proto_dir, root_build_dir), > ] > } > } Done. https://codereview.chromium.org/2388113003/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:246: "./" + rebase_path(invoker.proto_path, root_build_dir), On 2016/10/04 11:29:55, sdefresne wrote: > I think you can remove this "./". Done. https://codereview.chromium.org/2388113003/diff/1/tools/protoc_wrapper/protoc... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2388113003/diff/1/tools/protoc_wrapper/protoc... tools/protoc_wrapper/protoc_wrapper.py:77: parser.add_argument("--proto-path", On 2016/10/04 11:29:55, sdefresne wrote: > I would make this an argument that can be passed multiple time (ie. > action="append"). > > parser.add_argument("--proto-path", action="append", > help="Base directory with imported protos.") Done. https://codereview.chromium.org/2388113003/diff/1/tools/protoc_wrapper/protoc... tools/protoc_wrapper/protoc_wrapper.py:126: if options.proto_path: On 2016/10/04 11:29:55, sdefresne wrote: > If you change the argument to one that accept multiple items, then you need to > change this to a loop (note that the if before the for is there because the > value will be None if the option is not used, it is possible to remove the need > for the if by adding default=[] to the add_argument call): > > if options.proto_path: > for extra_proto_dir in options.proto_path: > proto_cmd.extend(["--proto_path", os.path.relpath(extra_proto_dir)] Done. Kept the + [] instead of extend for file consistency.
lgtm
The CQ bit was checked by olivierrobin@chromium.org
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...)
ping
What's this needed for? Neither the cl description nor the bug say why we need this.
On 2016/10/05 18:57:49, Nico wrote: > What's this needed for? Neither the cl description nor the bug say why we need > this. This is needed to import protos from another directory. One example of usage is to have a local proto that include a sync proto to avoid duplicate the sync data..
Description was changed from ========== Allow proto-path in the proto_library target Also forward include_dirs as it is needed to compile the resulting .cc. BUG=652312 ========== to ========== Allow proto-path in the proto_library target Also forward include_dirs as it is needed to compile the resulting .cc. This is needed to import protos from another directory. One example of usage is to have a local proto that include a sync proto to avoid duplicate the sync data.. BUG=652312 ==========
Description was changed from ========== Allow proto-path in the proto_library target Also forward include_dirs as it is needed to compile the resulting .cc. This is needed to import protos from another directory. One example of usage is to have a local proto that include a sync proto to avoid duplicate the sync data.. BUG=652312 ========== to ========== Allow proto-path in the proto_library target Also forward include_dirs as it is needed to compile the resulting .cc. This is needed to import protos from another directory. One example of usage is to have a local proto that include a sync proto to avoid duplicate the sync data. BUG=652312 ==========
https://codereview.chromium.org/2388113003/diff/20001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2388113003/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:330: "include_dirs", I think this is incorrect as dependent target will also likely need those include_dirs, and instead a config should be used. I'd say you probably need instead an invoker.extra_public_config like there is a invoker.extra_configs (see below).
On 2016/10/06 07:19:08, Olivier Robin wrote: > On 2016/10/05 18:57:49, Nico wrote: > > What's this needed for? Neither the cl description nor the bug say why we need > > this. > > This is needed to import protos from another directory. > One example of usage is to have a local proto that include a sync proto to avoid > duplicate the sync data.. Do DEPS checks work with import lines in proto files?
sdefresne: done thakis: import lines in .proto don't contain folder, so I don't think so. https://codereview.chromium.org/2388113003/diff/20001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2388113003/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:330: "include_dirs", On 2016/10/06 15:08:46, sdefresne wrote: > I think this is incorrect as dependent target will also likely need those > include_dirs, and instead a config should be used. I'd say you probably need > instead an invoker.extra_public_config like there is a invoker.extra_configs > (see below). Done.
On 2016/10/06 16:36:31, Olivier Robin wrote: > sdefresne: done > > thakis: import lines in .proto don't contain folder, so I don't think so. > > https://codereview.chromium.org/2388113003/diff/20001/third_party/protobuf/pr... > File third_party/protobuf/proto_library.gni (right): > > https://codereview.chromium.org/2388113003/diff/20001/third_party/protobuf/pr... > third_party/protobuf/proto_library.gni:330: "include_dirs", > On 2016/10/06 15:08:46, sdefresne wrote: > > I think this is incorrect as dependent target will also likely need those > > include_dirs, and instead a config should be used. I'd say you probably need > > instead an invoker.extra_public_config like there is a invoker.extra_configs > > (see below). > > Done. But the inclusion requires a dependency in the BUILD.gn.
lgtm https://codereview.chromium.org/2388113003/diff/40001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2388113003/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:340: if (!defined(invoker.public_configs)) { The pattern we usually use is the following: forward_variables_from(invoker, [ "defines", "public_configs", "testonly", "visibility", ]) if (!defined(public_configs)) { public_configs = [] } public_configs += [ "//third_party/protobuf:using_proto" ]
On 2016/10/06 16:38:21, Olivier Robin wrote: > On 2016/10/06 16:36:31, Olivier Robin wrote: > > sdefresne: done > > > > thakis: import lines in .proto don't contain folder, so I don't think so. Some do: https://cs.chromium.org/search/?q=import.*%5C/+file:%5C.proto&sq=package:chro... So after this you'd just say `import "foo.proto"` and "foo.proto" could be in a different dir? > > > > > https://codereview.chromium.org/2388113003/diff/20001/third_party/protobuf/pr... > > File third_party/protobuf/proto_library.gni (right): > > > > > https://codereview.chromium.org/2388113003/diff/20001/third_party/protobuf/pr... > > third_party/protobuf/proto_library.gni:330: "include_dirs", > > On 2016/10/06 15:08:46, sdefresne wrote: > > > I think this is incorrect as dependent target will also likely need those > > > include_dirs, and instead a config should be used. I'd say you probably need > > > instead an invoker.extra_public_config like there is a invoker.extra_configs > > > (see below). > > > > Done. > > But the inclusion requires a dependency in the BUILD.gn. Are those checked by DEPS? Like, if you use a sync proto in some other folder, will a sync owner have to approve that?
On 2016/10/06 16:46:46, Nico wrote: > On 2016/10/06 16:38:21, Olivier Robin wrote: > > On 2016/10/06 16:36:31, Olivier Robin wrote: > > > sdefresne: done > > > > > > thakis: import lines in .proto don't contain folder, so I don't think so. > > Some do: > https://cs.chromium.org/search/?q=import.*%5C/+file:%5C.proto&sq=package:chro... > > So after this you'd just say `import "foo.proto"` and "foo.proto" could be in a > different dir? The path you see is relative to the BUILD.gn (or more exactly the folder where the protoc command is executed) and not an absolute path from src. So if I create a src/BUILD.gn and create a protobuf target in it, I will be able to import "components/sync..." But, this will also require that you move the sync protobuf target in src/BUILD, because the symbols generated by protoc include a path part. There is no example in chromium of inclusion of proto by absolute path. https://cs.chromium.org/chromium/src/components/tracing/test/example_proto/li... So this CL is the simplest change. After this, foo.proto could be either in the same dir, or in the dir included by proto-path. Another solution would be to change the way we compile protobufs so we always run protoc in gen. This would require to change all imports in *.proto to have absolute path from src. > > > > > > > > > > https://codereview.chromium.org/2388113003/diff/20001/third_party/protobuf/pr... > > > File third_party/protobuf/proto_library.gni (right): > > > > > > > > > https://codereview.chromium.org/2388113003/diff/20001/third_party/protobuf/pr... > > > third_party/protobuf/proto_library.gni:330: "include_dirs", > > > On 2016/10/06 15:08:46, sdefresne wrote: > > > > I think this is incorrect as dependent target will also likely need those > > > > include_dirs, and instead a config should be used. I'd say you probably > need > > > > instead an invoker.extra_public_config like there is a > invoker.extra_configs > > > > (see below). > > > > > > Done. > > > > But the inclusion requires a dependency in the BUILD.gn. > > Are those checked by DEPS? Like, if you use a sync proto in some other folder, > will a sync owner have to approve that? For my CL a sync owner will have to approve. But I think that it is not enforced by the DEPS system as you don't need to include directly imported pb.h
ping
I understand this can cause security issue by allowing uncontrolled cross-components inclusions. Closing this issue. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
