GN proto_libary refactoring.

third_party/protobuf/proto_library.gni

Index: third_party/protobuf/proto_library.gni
diff --git a/third_party/protobuf/proto_library.gni b/third_party/protobuf/proto_library.gni
index 050154ce8cfbd559e01f24346ca22f7da88af1f5..ccacdae49adae92fc2aeb9d6db8b510312127387 100644
--- a/third_party/protobuf/proto_library.gni
+++ b/third_party/protobuf/proto_library.gni
@@ -6,15 +6,18 @@
 #
 # Protobuf parameters:
 #
-#   proto_out_dir (optional)
-#       Specifies the path suffix that output files are generated under. This
-#       path will be appended to the root_gen_dir.
+#   proto_in_dir (optional)
+#       Specifies the path relative to current BUILD.gn file where .proto files

[petrcermak, 2016/08/15 13:00:05]

nit s/current/the current/

[kraynov, 2016/08/15 13:37:48]

Acknowledged.

+#       are located and directory structure of this proto library starts.

[petrcermak, 2016/08/15 13:00:05]

s/directory structure/the directory structure/

[kraynov, 2016/08/15 13:37:48]

Acknowledged.

+#
+#       This option can be calculated automatically but it will raise an
+#       assertion error if any nested directories found.

[petrcermak, 2016/08/15 13:00:05]

s/found/are found/

[kraynov, 2016/08/15 13:37:48]

Acknowledged.

 #
-#       Targets that depend on the proto target will be able to include the

[petrcermak, 2016/08/15 13:00:05]

Why do you remove this usage comment?

-#       resulting proto headers with an include like:
-#         #include "dir/for/my_proto_lib/foo.pb.h"
-#       If undefined, this defaults to matching the input directory for each
-#       .proto file (you should almost always use the default mode).
+#   proto_out_dir (optional)
+#       Specifies the path suffix that output files are generated under.
+#       This path will be appended to the root_gen_dir for cc or plugin output

[petrcermak, 2016/08/15 13:00:05]

So it won't be appended in other use cases (apart from cc and plugin)? Are there
any other use cases?

[kraynov, 2016/08/15 13:37:49]

In case of python it appends to $root_build_dir/pyproto/

[petrcermak, 2016/08/16 14:10:31]

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?

+#       and will be set as an include path as well.

[petrcermak, 2016/08/15 13:00:05]

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?).

[kraynov, 2016/08/15 13:37:49]

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.

[petrcermak, 2016/08/16 14:10:31]

Acknowledged.

+#       Python stubs will be placed to |$root_build_dir/pyproto/$proto_out_dir|.

[petrcermak, 2016/08/15 13:00:05]

I think that the correct notation is:

|root_build_dir|/pyproto/|proto_out_dir|

[kraynov, 2016/08/15 13:37:48]

Acknowledged.

 #
 #   generate_python (optional, default true)
 #       Generate Python protobuf stubs.
@@ -32,19 +35,20 @@
 #       macro to work (see cc_include) and set
 #       component_build_force_source_set = true.
 #
+#   cc_include (optional)
+#       String listing an extra include that should be passed.
+#       Example: cc_include = "foo/bar.h"
+#
 #   generator_plugin_label (optional)
 #       GN label for plugin executable which generates custom cc stubs.
+#       Don't specify a toolchain, host toolchain is assumed.

[petrcermak, 2016/08/15 13:00:05]

s/Don't specify a toolchain/If a toolchain is not specified/

[kraynov, 2016/08/15 13:37:48]

There is no condition whether toolchain is specified or not. Using any other
toolchain for generator plugin is non-sense.

[petrcermak, 2016/08/16 14:10:31]

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).

 #
-#   generator_plugin_suffix (required if generator_plugin_label set)
+#   generator_plugin_suffix (required if |generator_plugin_label| set)
 #       Suffix (before extension) for generated .cc and .h files.
 #
 #   generator_plugin_options (optional)
 #       Extra flags passed to the plugin. See cc_generator_options.
 #
-#   cc_include (optional)
-#       String listing an extra include that should be passed.
-#       Example: cc_include = "foo/bar.h"
-#
 #   deps (optional)
 #       Additional dependencies.
 #
@@ -65,7 +69,7 @@
 #       A list of config labels that will be appended to the configs applying
 #       to the source set.
 #
-# Example
+# Example:
 #  proto_library("mylib") {
 #    sources = [
 #      "foo.proto",
@@ -74,6 +78,7 @@
 
 template("proto_library") {
   assert(defined(invoker.sources), "Need sources for proto_library")
+  proto_sources = invoker.sources
 
   # Don't apply OS-specific sources filtering to the assignments later on.
   # Platform files should have gotten filtered out in the sources assignment
@@ -81,136 +86,183 @@ template("proto_library") {
   # this template shouldn't re-apply the filter.
   set_sources_assignment_filter([])
 
-  action_name = "${target_name}_gen"
-  source_set_name = target_name
-  action_foreach(action_name) {
-    visibility = [ ":$source_set_name" ]
+  if (defined(invoker.generate_cc)) {
+    generate_cc = invoker.generate_cc
+  } else {
+    generate_cc = true
+  }
 
-    script = "//tools/protoc_wrapper/protoc_wrapper.py"
+  if (defined(invoker.generate_python)) {
+    generate_python = invoker.generate_python
+  } else {
+    generate_python = true
+  }
 
-    sources = invoker.sources
+  if (defined(invoker.generator_plugin_label)) {
+    generator_plugin_label = invoker.generator_plugin_label
+    generator_plugin_suffix = invoker.generator_plugin_suffix
+    generate_with_plugin = true
 
-    # Compute the output directory, both relative to the source root (for
-    # declaring "outputs") and relative to the build dir (for passing to the
-    # script).
-    if (defined(invoker.proto_out_dir)) {
-      proto_out_dir = invoker.proto_out_dir
-    } else {
-      proto_out_dir = "{{source_root_relative_dir}}"
+    # Straightforward way to get the name of executable doesn't work because
+    # |root_out_dir| and |root_build_dir| may differ in cross-compilation and
+    # also Windows executables have .exe at the end.
+
+    plugin_host_label = generator_plugin_label + "($host_toolchain)"
+    plugin_path = get_label_info(plugin_host_label, "root_out_dir") + "/" +
+                  get_label_info(plugin_host_label, "name")
+    if (host_os == "win") {
+      plugin_path += ".exe"
     }
-    out_dir = "$root_gen_dir/" + proto_out_dir
-    rel_out_dir = rebase_path(out_dir, root_build_dir)
+    plugin_path = rebase_path(plugin_path, root_build_dir)
+  } else {
+    generate_with_plugin = false
+  }
 
-    outputs = []
+  if (defined(invoker.proto_in_dir)) {
+    proto_in_dir = invoker.proto_in_dir
+  } else {
+    proto_in_dir = get_path_info(proto_sources[0], "dir")
 
-    args = []
-    if (defined(invoker.cc_include)) {
-      args += [
-        "--include",
-        invoker.cc_include,
-        "--protobuf",
-        "$rel_out_dir/{{source_name_part}}.pb.h",
+    # Sanity check, |proto_in_dir| should be defined to allow sub-directories.
+    foreach(proto_source, proto_sources) {
+      assert(get_path_info(proto_source, "dir") == proto_in_dir,
+             "Please define |proto_in_dir| to allow nested directories.")
+    }
+  }
+
+  # Avoid absolute path because of assumption that |proto_in_dir| is relative

[petrcermak, 2016/08/15 13:00:05]

nit: s/assumption/the assumption/

[kraynov, 2016/08/15 13:37:48]

Acknowledged.

+  # to the directory of current BUILD.gn file.
+  proto_in_dir = rebase_path(proto_in_dir, ".")
+
+  if (defined(invoker.proto_out_dir)) {
+    proto_out_dir = invoker.proto_out_dir
+  } else {
+    # Absolute path to the directory of current BUILD.gn file excluding "//".
+    proto_out_dir = rebase_path(".", "//")
+    if (proto_in_dir != ".") {
+      proto_out_dir += "/$proto_in_dir"
+    }
+  }
+
+  # We need both absolute path to use in GN statements and a relative one
+  # to pass to external script.
+  if (generate_cc || generate_with_plugin) {
+    cc_out_dir = "$root_gen_dir/" + proto_out_dir
+    rel_cc_out_dir = rebase_path(cc_out_dir, root_build_dir)
+  }
+  if (generate_python) {
+    py_out_dir = "$root_out_dir/pyproto/" + proto_out_dir
+    rel_py_out_dir = rebase_path(py_out_dir, root_build_dir)
+  }
+
+  protos = rebase_path(invoker.sources, proto_in_dir)
+  protogens = []
+
+  # List output files.
+  foreach(proto, protos) {
+    proto_dir = get_path_info(proto, "dir")
+    proto_name = get_path_info(proto, "name")
+    if (proto_dir != ".") {

[petrcermak, 2016/08/15 13:00:05]

"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.

[kraynov, 2016/08/15 13:37:48]

Acknowledged.

+      basic_path = proto_dir + "/" + proto_name
+    } else {
+      basic_path = proto_name

[petrcermak, 2016/08/15 13:00:05]

Why do you need this special case? "./FILE" is the same path as "FILE", isn't
it?

[kraynov, 2016/08/15 13:37:48]

Acknowledged.

+    }
+    if (generate_cc) {
+      protogens += [
+        "$cc_out_dir/$basic_path.pb.h",
+        "$cc_out_dir/$basic_path.pb.cc",
+      ]
+    }
+    if (generate_python) {
+      protogens += [ "$py_out_dir/${basic_path}_pb2.py" ]
+    }
+    if (generate_with_plugin) {
+      protogens += [
+        "$cc_out_dir/$basic_path$generator_plugin_suffix.h",
+        "$cc_out_dir/$basic_path$generator_plugin_suffix.cc",
       ]
     }
+  }
 
-    args += [
-      "--proto-in-dir",
-      "{{source_dir}}",
-      "--proto-in-file",
-      "{{source_file_part}}",
+  action_name = "${target_name}_gen"
+  source_set_name = target_name
 
-      # TODO(brettw) support system protobuf compiler.
-      "--use-system-protobuf=0",
-    ]
+  # Generate protobuf stubs.
+  action(action_name) {
+    visibility = [ ":$source_set_name" ]
+    script = "//tools/protoc_wrapper/protoc_wrapper.py"
+    sources = proto_sources
+    outputs = protogens
+    args = protos
 
     protoc_label = "//third_party/protobuf:protoc($host_toolchain)"
+    protoc_out_dir = get_label_info(protoc_label, "root_out_dir")
     args += [
-      "--",
-
-      # Prepend with "./" so this will never pick up the system one (normally
-      # when not cross-compiling, protoc's output directory will be the same
-      # as the build dir, so the relative location will be empty).
-      "./" +
-          rebase_path(get_label_info(protoc_label, "root_out_dir") + "/protoc",
-                      root_build_dir),
+      # Wrapper should never pick a system protoc.
+      # Path should be rebased because |root_build_dir| for current toolchain
+      # may be different to |root_out_dir| of protoc built on host toolchain.

[petrcermak, 2016/08/15 13:00:05]

nit: s/different to/different from/

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+      "--protoc",
+      "./" + rebase_path(protoc_out_dir, root_build_dir) + "/protoc",
+      "--proto-in-dir",
+      rebase_path(proto_in_dir, root_build_dir),
     ]
 
-    if (!defined(invoker.generate_python) || invoker.generate_python) {
-      py_out_dir = "$root_out_dir/pyproto/" + proto_out_dir
-      rel_py_out_dir = rebase_path(py_out_dir, root_build_dir)
-
-      outputs += [ "$py_out_dir/{{source_name_part}}_pb2.py" ]
+    if (generate_cc) {
       args += [
-        "--python_out",
-        rel_py_out_dir,
+        "--cc-out-dir",
+        rel_cc_out_dir,
       ]
-    }
-
-    if (!defined(invoker.generate_cc) || invoker.generate_cc) {
-      # If passed cc_generator_options should end in a colon, which will
-      # separate it from the directory when we concatenate them. The proto
-      # compiler understands this syntax.
       if (defined(invoker.cc_generator_options)) {
-        cc_generator_options = invoker.cc_generator_options
-      } else {
-        cc_generator_options = ""
+        args += [
+          "--cc-options",
+          invoker.cc_generator_options,
+        ]
+      }
+      if (defined(invoker.cc_include)) {
+        args += [
+          "--include",
+          invoker.cc_include,
+        ]
       }
-      outputs += [
-        "$out_dir/{{source_name_part}}.pb.cc",
-        "$out_dir/{{source_name_part}}.pb.h",
-      ]
-      args += [
-        "--cpp_out",
-        "$cc_generator_options$rel_out_dir",  # Separated by colon.
-      ]
     }
 
-    if (defined(invoker.generator_plugin_label)) {
-      generator_plugin_label = invoker.generator_plugin_label
-      generator_plugin_suffix = invoker.generator_plugin_suffix
-      if (defined(invoker.generator_plugin_options)) {
-        generator_plugin_options = invoker.generator_plugin_options
-      } else {
-        generator_plugin_options = ""
-      }
-      outputs += [
-        "$out_dir/{{source_name_part}}$generator_plugin_suffix.cc",
-        "$out_dir/{{source_name_part}}$generator_plugin_suffix.h",
+    if (generate_python) {
+      args += [
+        "--py-out-dir",
+        rel_py_out_dir,
       ]
+    }
 
-      # Straightforward way to get the name of executable doesn't work because
-      # root_out_dir and root_build_dir may differ in cross-compilation and
-      # also Windows executables have .exe at the end.
-
-      plugin_host_label = generator_plugin_label + "($host_toolchain)"
-      plugin_path = get_label_info(plugin_host_label, "root_out_dir") + "/" +
-                    get_label_info(plugin_host_label, "name")
-      if (host_os == "win") {
-        plugin_path += ".exe"
-      }
-
-      # Need "./" for script to find plugin binary (working dir is not on PATH).
+    if (generate_with_plugin) {
       args += [
         "--plugin",
-        "protoc-gen-plugin=./" + rebase_path(plugin_path, root_build_dir),
-        "--plugin_out",
-        "$generator_plugin_options$rel_out_dir",  # Separated by colon.
+        plugin_path,
+        "--plugin-out-dir",
+        rel_cc_out_dir,
       ]
+      if (defined(invoker.generator_plugin_options)) {
+        args += [
+          "--plugin-options",
+          invoker.generator_plugin_options,
+        ]
+      }
     }
 
     deps = [
+      # System protoc is not used so need to build a chromium one.

[petrcermak, 2016/08/15 13:00:05]

s/need/we need/ (or "it's necessary")

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

       protoc_label,
     ]
     if (defined(plugin_host_label)) {
+      # Depends on generator plugin but for host toolchain only.

[petrcermak, 2016/08/15 13:00:05]

What "depends on generator plugin"? "This target"?

[kraynov, 2016/08/15 13:37:49]

This action

       deps += [ plugin_host_label ]
     }
-
-    # The deps may have steps that have to run before running protobuf.
     if (defined(invoker.deps)) {
+      # The deps may have steps that have to run before running protoc.
       deps += invoker.deps
     }
   }
 
+  # Option to disable building a library in component build.
   if (defined(invoker.component_build_force_source_set) &&
       invoker.component_build_force_source_set &&
       is_component_build) {
@@ -218,6 +270,8 @@ template("proto_library") {
   } else {
     link_target_type = "static_library"
   }
+
+  # Build generated protobuf stubs as libary or source set.
   target(link_target_type, target_name) {
     forward_variables_from(invoker,
                            [
@@ -233,14 +287,21 @@ template("proto_library") {
 
     public_configs = [ "//third_party/protobuf:using_proto" ]
 
-    # If using built-in cc generator the resulting headers reference headers
-    # within protobuf_lite, hence dependencies require those headers too.
-    # In case of generator plugin such issues should be resolved by invoker.
-    if (!defined(invoker.generate_cc) || invoker.generate_cc) {
+    include_dirs = []
+    if (generate_cc) {
+      include_dirs += [ cc_out_dir ]
+
+      # If using built-in cc generator the resulting headers reference headers

[petrcermak, 2016/08/15 13:00:05]

There should be a comma after "generator". Otherwise, this sentence is ambiguous
(as it's unclear where the condition ends).

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+      # within protobuf_lite, hence dependencies require those headers too.

[petrcermak, 2016/08/15 13:00:05]

"hence" should start a separate sentence and there should be a comma after it:

within protobuf_lite. Hence, dependencies [...]

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

+      # In case of generator plugin such issues should be resolved by invoker.

[petrcermak, 2016/08/15 13:00:05]

nit: s/invoker/the invoker/

[petrcermak, 2016/08/15 13:00:05]

Again, there should be a comma after "plugin" to make it clear where the
condition ends.

[kraynov, 2016/08/15 13:37:49]

Acknowledged.

       public_deps = [
         "//third_party/protobuf:protobuf_lite",
       ]
     }
+    if (generate_with_plugin) {
+      include_dirs += [ cc_out_dir ]
+    }
+
     deps = [
       ":$action_name",
     ]