Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2646123002:
Option added to protobuf build for import directories (Closed)
|
Created:
3 years, 11 months ago by Ramin Halavati Modified:
3 years, 10 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOption added to protobuf build for import directories
Protobuf library GNI does not let BUILD.gn define directories outside the base directory of the source files to be used for importing proto files in source proto files. This is resolved by adding an extra optional parameter that specifies the directories from which proto files are imported.
BUG=683143
Review-Url: https://codereview.chromium.org/2646123002
Cr-Commit-Position: refs/heads/master@{#450309}
Committed: https://chromium.googlesource.com/chromium/src/+/0c4b1788e3e0010560d6a064433f4f99afae5fda
Patch Set 1 #
Total comments: 10
Patch Set 2 : Comments addressed. #
Total comments: 10
Patch Set 3 : Comments addressed. #
Total comments: 2
Patch Set 4 : Comment addressed. #
Total comments: 2
Patch Set 5 : Warning added. #
Dependent Patchsets: Messages
Total messages: 32 (10 generated)
rhalavati@chromium.org changed reviewers: + primiano@chromium.org, xyzzyz@chromium.org
Hi, I created the CL following yesterday's email on importing from other directories. Regarding DEPS check, I think importing a protobuf in another one in this task is like including a C++ header file in another header or source file. This is not necessarily reflected in DEPS now and only if something needs to be linked to current project, it should be specified in DEPS. Similarly here as this import would not result in compilation of the imported protobuf into required cc and h files, if one needs those files, has to explicitly add them to project dependencies and therefore without that the final link break. So do you think it still needs more checks?
rhalavati@chromium.org changed reviewers: + scottmg@chromium.org
scottmg@chromium.org: Please review changes in protoc_wrapper.py
On 2017/01/20 14:36:13, Ramin Halavati wrote: > Hi, > > I created the CL following yesterday's email on importing from other > directories. > > Regarding DEPS check, I think importing a protobuf in another one in this task > is like including a C++ header file in another header or source file. This is > not necessarily reflected in DEPS now and only if something needs to be linked > to current project, it should be specified in DEPS. Similarly here as this > import would not result in compilation of the imported protobuf into required cc > and h files, if one needs those files, has to explicitly add them to project > dependencies and therefore without that the final link break. > So do you think it still needs more checks? I don't understand the first part of the argument about the headers including other headers. However, the second part makes more sense to me -- what you claim is basically that the checkdeps will be enforced by checking if the generated files include other generated files, and if DEPS do not allow this dependency, it will break at that point. Is it correct? Can you create a CL, or at least test it manually to confirm it is indeed true?
https://codereview.chromium.org/2646123002/diff/1/third_party/protobuf/proto_... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2646123002/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:68: # default case is just the source directory. The "source directory" here is pretty ambiguous. I think what you mean (and what you should put here instead) is proto_in_dir. https://codereview.chromium.org/2646123002/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:284: if (defined(invoker.import_dirs)) { The comment and the name of the variable say it's a list of directories, but it looks like it only supports a single directory. https://codereview.chromium.org/2646123002/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:286: args += rebase_path(invoker.import_dirs, proto_in_dir) Are you sure you want to rebase it on proto_in_dir? I think importing protos that are in a subdirectory of proto_in_dir works already. https://codereview.chromium.org/2646123002/diff/1/tools/protoc_wrapper/protoc... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2646123002/diff/1/tools/protoc_wrapper/protoc... tools/protoc_wrapper/protoc_wrapper.py:93: help="Name of include to insert into generated headers.") Please update the help text. https://codereview.chromium.org/2646123002/diff/1/tools/protoc_wrapper/protoc... tools/protoc_wrapper/protoc_wrapper.py:129: protoc_cmd += options.import_dir Isn't options.import_dir a list? You should probably loop through it.
On 2017/01/20 18:27:33, xyzzyz wrote: > On 2017/01/20 14:36:13, Ramin Halavati wrote: > > Hi, > > > > I created the CL following yesterday's email on importing from other > > directories. > > > > Regarding DEPS check, I think importing a protobuf in another one in this task > > is like including a C++ header file in another header or source file. This is > > not necessarily reflected in DEPS now and only if something needs to be linked > > to current project, it should be specified in DEPS. Similarly here as this > > import would not result in compilation of the imported protobuf into required > cc > > and h files, if one needs those files, has to explicitly add them to project > > dependencies and therefore without that the final link break. > > So do you think it still needs more checks? > > I don't understand the first part of the argument about the headers including > other headers. However, the second part makes more sense to me -- what you claim > is basically that the checkdeps will be enforced by checking if the generated > files include other generated files, and if DEPS do not allow this dependency, > it will break at that point. Is it correct? Can you create a CL, or at least > test it manually to confirm it is indeed true? I was partly wrong in my previous discussion and only focused on the deps part of BUILD.gn, so I will start again: Currently, src/buildtools/checkdeps/cpp_checker.py checks if all include files of CC files and H files are covered by rules in DEPS file, there is a similar script for java, and there is nothing for protos. The previous limitation that protos could only be loaded from one directory did not help controlling them from DEPS check perspective. I have create a CL here (https://codereview.chromium.org/2649883002) where a CC file in directory src/tools/test_deps uses a protobuf from src/foreign_proto without declaring it in DEPS and it is built and uploaded successful. Therefore, I think that this new addition to proto_library.gni would not cause a "new" DEPS problem (because it already exists) and it's good if we just file DEPS check for protos as a bug and go for it later.
All comments addressed, review please. https://codereview.chromium.org/2646123002/diff/1/third_party/protobuf/proto_... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2646123002/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:68: # default case is just the source directory. On 2017/01/20 18:27:40, xyzzyz wrote: > The "source directory" here is pretty ambiguous. I think what you mean (and what > you should put here instead) is proto_in_dir. Done. https://codereview.chromium.org/2646123002/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:284: if (defined(invoker.import_dirs)) { On 2017/01/20 18:27:40, xyzzyz wrote: > The comment and the name of the variable say it's a list of directories, but it > looks like it only supports a single directory. If one specifies several directories in import_dirs, rebase_path creates a list of rebased paths and they are added to arguments with one single --import-dir switch and protoc_wrapper accepts it this way. https://codereview.chromium.org/2646123002/diff/1/third_party/protobuf/proto_... third_party/protobuf/proto_library.gni:286: args += rebase_path(invoker.import_dirs, proto_in_dir) On 2017/01/20 18:27:40, xyzzyz wrote: > Are you sure you want to rebase it on proto_in_dir? I think importing protos > that are in a subdirectory of proto_in_dir works already. I didn't find any documentations on this but it seems that all paths should be specified relative to the directory of current BUILD.gn file. So I had to rebase it to root_build_dir. I should note that this new change should also work for directories which are not subdirectory of protos_in_dir. https://codereview.chromium.org/2646123002/diff/1/tools/protoc_wrapper/protoc... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2646123002/diff/1/tools/protoc_wrapper/protoc... tools/protoc_wrapper/protoc_wrapper.py:93: help="Name of include to insert into generated headers.") On 2017/01/20 18:27:40, xyzzyz wrote: > Please update the help text. Done. https://codereview.chromium.org/2646123002/diff/1/tools/protoc_wrapper/protoc... tools/protoc_wrapper/protoc_wrapper.py:129: protoc_cmd += options.import_dir On 2017/01/20 18:27:40, xyzzyz wrote: > Isn't options.import_dir a list? You should probably loop through it. Yes, it had to be changed into looping through the list with each item having its own --proto_path switch.
Description was changed from ========== Option added to protobuf build for import directories Protobuf library GNI does not let BUILD.gn define directories outside the base directory of the source files to be used for importing proto files in source proto files. This is resolved by adding an extra optional parameter that specifies the directories from which proto files are imported. BUG=683143 ========== to ========== Option added to protobuf build for import directories Protobuf library GNI does not let BUILD.gn define directories outside the base directory of the source files to be used for importing proto files in source proto files. This is resolved by adding an extra optional parameter that specifies the directories from which proto files are imported. BUG=683143 ==========
scottmg@chromium.org changed reviewers: + brettw@chromium.org, dpranke@chromium.org
scottmg@chromium.org changed reviewers: - brettw@chromium.org, dpranke@chromium.org
scottmg@chromium.org changed reviewers: + brettw@chromium.org, dpranke@chromium.org
On 2017/01/20 14:36:13, Ramin Halavati wrote: > Hi, > > I created the CL following yesterday's email on importing from other > directories. > > Regarding DEPS check, I think importing a protobuf in another one in this task > is like including a C++ header file in another header or source file. This is > not necessarily reflected in DEPS now and only if something needs to be linked > to current project, it should be specified in DEPS. Similarly here as this > import would not result in compilation of the imported protobuf into required cc > and h files, if one needs those files, has to explicitly add them to project > dependencies and therefore without that the final link break. > So do you think it still needs more checks? I'm not sure that this is how DEPS works. We routinely use it to restrict includes in include_rules blocks. There's also `gn check` that tries to make sure that nothing is including from targets that it shouldn't be. I'm not sure how this will be used, but will that be a problem?
https://codereview.chromium.org/2646123002/diff/20001/tools/protoc_wrapper/pr... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2646123002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:92: parser.add_argument("--import-dir", nargs='*', default=[], I have no idea why this script uses " everywhere, but please make nargs use " too, unless you want to fix the whole script to use ' instead. https://codereview.chromium.org/2646123002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:93: help="Name of extra import directories for protos.") nit; "Extra import directories for protos." would sound more natural.
https://codereview.chromium.org/2646123002/diff/20001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2646123002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:285: args += ["--import-dir"] I was under impression before that the way this flag is supposed to work is that we pass multiple occurences of it to the script, as in "wrapper.py --import-dir dir1/ --import-dir dir2/". Seems like Python's argparse actually expects that we do "--import-dir dir1/ dir2/", and your code is written this way too. It means that this flag should rather be named "--import-dirs", to reflect that we want to pass a list of directories to it, not a single directory. https://codereview.chromium.org/2646123002/diff/20001/tools/protoc_wrapper/pr... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2646123002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:93: help="Name of extra import directories for protos.") Just like my comment in the other file, please rename the flag to --import-dirs. https://codereview.chromium.org/2646123002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:127: for path in options.import_dir: This loop should probably be between lines 124 and 125 -- line 124 adds extra paths, just like this loop does, and 125 finally adds proto files to generate bindings from.
On 2017/01/23 14:20:13, Ramin Halavati wrote: > I was partly wrong in my previous discussion and only focused on the deps part > of BUILD.gn, so I will start again: > > Currently, src/buildtools/checkdeps/cpp_checker.py checks if all include files > of CC files and H files are covered by rules in DEPS file, there is a similar > script for java, and there is nothing for protos. > > The previous limitation that protos could only be loaded from one directory did > not help controlling them from DEPS check perspective. I have create a CL here > (https://codereview.chromium.org/2649883002) where a CC file in directory > src/tools/test_deps uses a protobuf from src/foreign_proto without declaring it > in DEPS and it is built and uploaded successful. > > Therefore, I think that this new addition to proto_library.gni would not cause a > "new" DEPS problem (because it already exists) and it's good if we just file > DEPS check for protos as a bug and go for it later. I looked at your example CL, and I'm not sure it is a proper example -- see my comment there. In any case, I think I'm fine with working on checkdeps in another CL, as long as bug is filed and assigned to you, and it is implemented before code in this CL is used. Actually, the checkdeps code doesn't depend on this CL at all, so you can actually finish that one first before submitting this one.
https://codereview.chromium.org/2646123002/diff/20001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2646123002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:285: args += ["--import-dir"] On 2017/01/23 17:53:56, xyzzyz wrote: > I was under impression before that the way this flag is supposed to work is that > we pass multiple occurences of it to the script, as in "wrapper.py --import-dir > dir1/ --import-dir dir2/". Seems like Python's argparse actually expects that we > do "--import-dir dir1/ dir2/", and your code is written this way too. It means > that this flag should rather be named "--import-dirs", to reflect that we want > to pass a list of directories to it, not a single directory. argparse will actually accept either, but the latter usage is weird and can lead to strange bugs, particularly when you're also expecting positional arguments. In particular, in this case, if you have `protoc_wrapper.py --import-dir foo bar.proto`, this will get parsed as import_dir=["foo", "bar.proto"] and then error out because it didn't find a proto file. To avoid this you need to specify '--' before any positional args that might follow --import-dir (which doesn't actually happen in this usage). For this reason I really don't like using --foo f1 f2; I'd rather you did --foo=f1 --foo=f2, even if the positional arguments come first.
All comments addressed. Please review. Filed a bug for protos checkdeps and started working on it in a separate CL. https://bugs.chromium.org/p/chromium/issues/detail?id=684383 https://codereview.chromium.org/2646123002/diff/20001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2646123002/diff/20001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:285: args += ["--import-dir"] On 2017/01/24 02:31:20, Dirk Pranke wrote: > On 2017/01/23 17:53:56, xyzzyz wrote: > > I was under impression before that the way this flag is supposed to work is > that > > we pass multiple occurences of it to the script, as in "wrapper.py > --import-dir > > dir1/ --import-dir dir2/". Seems like Python's argparse actually expects that > we > > do "--import-dir dir1/ dir2/", and your code is written this way too. It means > > that this flag should rather be named "--import-dirs", to reflect that we want > > to pass a list of directories to it, not a single directory. > > argparse will actually accept either, but the latter usage is weird and can lead > to strange bugs, > particularly when you're also expecting positional arguments. > > In particular, in this case, if you have `protoc_wrapper.py --import-dir foo > bar.proto`, this will get parsed as import_dir=["foo", "bar.proto"] and then > error out because it didn't find a proto file. > > To avoid this you need to specify '--' before any positional args that might > follow --import-dir (which doesn't actually happen in this usage). > > For this reason I really don't like using --foo f1 f2; I'd rather you did > --foo=f1 --foo=f2, even if the positional arguments come first. Done, changed it to one --import-dir switch for each directory. https://codereview.chromium.org/2646123002/diff/20001/tools/protoc_wrapper/pr... File tools/protoc_wrapper/protoc_wrapper.py (right): https://codereview.chromium.org/2646123002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:92: parser.add_argument("--import-dir", nargs='*', default=[], On 2017/01/23 16:53:11, scottmg wrote: > I have no idea why this script uses " everywhere, but please make nargs use " > too, unless you want to fix the whole script to use ' instead. Done. https://codereview.chromium.org/2646123002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:93: help="Name of extra import directories for protos.") On 2017/01/23 16:53:11, scottmg wrote: > nit; "Extra import directories for protos." would sound more natural. Done. Help corrected, name did not change as it now accepts just one directory for each --import-dir switch. https://codereview.chromium.org/2646123002/diff/20001/tools/protoc_wrapper/pr... tools/protoc_wrapper/protoc_wrapper.py:127: for path in options.import_dir: On 2017/01/23 17:53:56, xyzzyz wrote: > This loop should probably be between lines 124 and 125 -- line 124 adds extra > paths, just like this loop does, and 125 finally adds proto files to generate > bindings from. Done.
lgtm https://codereview.chromium.org/2646123002/diff/40001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2646123002/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:286: args += ["--import-dir", rebase_path(path, root_build_dir)] This might actually need to be --import-dir=" + rebase_path(...) to avoid the problem I was warning about.
On 2017/01/25 03:32:41, Dirk Pranke wrote: > lgtm > > https://codereview.chromium.org/2646123002/diff/40001/third_party/protobuf/pr... > File third_party/protobuf/proto_library.gni (right): > > https://codereview.chromium.org/2646123002/diff/40001/third_party/protobuf/pr... > third_party/protobuf/proto_library.gni:286: args += ["--import-dir", > rebase_path(path, root_build_dir)] > This might actually need to be --import-dir=" + rebase_path(...) to avoid the > problem I was warning about. I am not clear about your concern. I changed the other script so that it requires one --import-dir switch per each directory, and the code here is changed to pass directories one by one to rebase_path() and therefore get individual rebased paths and not a list. I kept it without equal sign to comply with the rest of the args throughout the script. Do you recommend changing it? P.S., 2 CLs are created to add proto dependency checks: https://codereview.chromium.org/2653023004/ https://codereview.chromium.org/2651553006/
On 2017/01/25 12:39:21, Ramin Halavati wrote: > On 2017/01/25 03:32:41, Dirk Pranke wrote: > > lgtm > > > > > https://codereview.chromium.org/2646123002/diff/40001/third_party/protobuf/pr... > > File third_party/protobuf/proto_library.gni (right): > > > > > https://codereview.chromium.org/2646123002/diff/40001/third_party/protobuf/pr... > > third_party/protobuf/proto_library.gni:286: args += ["--import-dir", > > rebase_path(path, root_build_dir)] > > This might actually need to be --import-dir=" + rebase_path(...) to avoid the > > problem I was warning about. > > I am not clear about your concern. I changed the other script so that it > requires one --import-dir switch per each directory, and the code here is > changed to pass directories one by one to rebase_path() and therefore get > individual rebased paths and not a list. I don't think that part of the python script actually needed to be changed. > I kept it without equal sign to comply with the rest of the args throughout the > script. Do you recommend changing it? Yes.
All comments addressed. xyzzyz@: I've created CrBug.com/684383 for protobuf dependency check and am working in it. Please approve this one to unblock my project. https://codereview.chromium.org/2646123002/diff/40001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2646123002/diff/40001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:286: args += ["--import-dir", rebase_path(path, root_build_dir)] On 2017/01/25 03:32:40, Dirk Pranke wrote: > This might actually need to be --import-dir=" + rebase_path(...) to avoid the > problem I was warning about. Done.
https://codereview.chromium.org/2646123002/diff/60001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2646123002/diff/60001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:67: # A list of extra import directories to be passed to protoc compiler. The As I mentioned in the email, you should add a comment here that explains that this option is not to be used in Chromium until we figure out the duplicate symbols issue, along with the link to bug tracking that effort.
xyzzyz@: http://crbug.com/691451 is created for tracking the effort on updating partial imports, and warning added to proto_library.gni. Please review. https://codereview.chromium.org/2646123002/diff/60001/third_party/protobuf/pr... File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2646123002/diff/60001/third_party/protobuf/pr... third_party/protobuf/proto_library.gni:67: # A list of extra import directories to be passed to protoc compiler. The On 2017/02/10 19:36:03, xyzzyz wrote: > As I mentioned in the email, you should add a comment here that explains that > this option is not to be used in Chromium until we figure out the duplicate > symbols issue, along with the link to bug tracking that effort. Done.
lgtm
lgtm
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2646123002/#ps80001 (title: "Warning added.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487061222285250,
"parent_rev": "d80d672c08a421739a5ecd80c6e9bc2cfc745946", "commit_rev":
"0c4b1788e3e0010560d6a064433f4f99afae5fda"}
Message was sent while issue was closed.
Description was changed from ========== Option added to protobuf build for import directories Protobuf library GNI does not let BUILD.gn define directories outside the base directory of the source files to be used for importing proto files in source proto files. This is resolved by adding an extra optional parameter that specifies the directories from which proto files are imported. BUG=683143 ========== to ========== Option added to protobuf build for import directories Protobuf library GNI does not let BUILD.gn define directories outside the base directory of the source files to be used for importing proto files in source proto files. This is resolved by adding an extra optional parameter that specifies the directories from which proto files are imported. BUG=683143 Review-Url: https://codereview.chromium.org/2646123002 Cr-Commit-Position: refs/heads/master@{#450309} Committed: https://chromium.googlesource.com/chromium/src/+/0c4b1788e3e0010560d6a064433f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0c4b1788e3e0010560d6a064433f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
