GN proto_libary refactoring.

third_party/protobuf/proto_library.gni

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 # Compile a protocol buffer.
 #
 # Protobuf parameters:
 #
+#   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.

+#
 #   proto_out_dir (optional)
-#       Specifies the path suffix that output files are generated under. This
-#       path will be appended to the root_gen_dir.
-#
-#       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).
+#       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.
 #
 #   generate_cc (optional, default true)
 #       Generate C++ protobuf stubs.
 #
 #   cc_generator_options (optional)
 #       List of extra flags passed to the protocol compiler.  If you need to
 #       add an EXPORT macro to a protobuf's C++ header, set the
 #       'cc_generator_options' variable with the value:
 #       'dllexport_decl=FOO_EXPORT:' (note trailing colon).
 #
 #       It is likely you also need to #include a file for the above EXPORT
 #       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.
 #
 # Parameters for compiling the generated code:
 #
 #   component_build_force_source_set (Default=false)
 #       When set true the generated code will be compiled as a source set in
 #       the component build. This does not affect static builds.  If you are
 #       exporting symbols from a component, this is required to prevent those
 #       symbols from being stripped. If you're not using dllexports in
 #       cc_generator_options, it's usually best to leave this false.
 #
 #   defines (optional)
 #       Defines to supply to the source set that compiles the generated source
 #       code.
 #
 #   extra_configs (optional)
 #       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",
 #    ]
 #  }
 
 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
   # when this template was invoked. If they weren't, it was on purpose and
   # this template shouldn't re-apply the filter.
   set_sources_assignment_filter([])
 
+  if (defined(invoker.generate_cc)) {
+    generate_cc = invoker.generate_cc
+  } else {
+    generate_cc = true
+  }
+
+  if (defined(invoker.generate_python)) {
+    generate_python = invoker.generate_python
+  } else {
+    generate_python = true
+  }
+
+  if (defined(invoker.generator_plugin_label)) {
+    generator_plugin_label = invoker.generator_plugin_label
+    generator_plugin_suffix = invoker.generator_plugin_suffix
+    generate_with_plugin = true
+
+    # 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"
+    }
+    plugin_path = rebase_path(plugin_path, root_build_dir)
+  } else {
+    generate_with_plugin = false
+  }
+
+  if (defined(invoker.proto_in_dir)) {
+    proto_in_dir = invoker.proto_in_dir
+  } else {
+    proto_in_dir = get_path_info(proto_sources[0], "dir")
+
+    # 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",
+      ]
+    }
+  }
+
   action_name = "${target_name}_gen"
   source_set_name = target_name
-  action_foreach(action_name) {
+
+  # Generate protobuf stubs.
+  action(action_name) {
     visibility = [ ":$source_set_name" ]
+    script = "//tools/protoc_wrapper/protoc_wrapper.py"
+    sources = proto_sources
+    outputs = protogens
+    args = protos
 
-    script = "//tools/protoc_wrapper/protoc_wrapper.py"
+    protoc_label = "//third_party/protobuf:protoc($host_toolchain)"
+    protoc_out_dir = get_label_info(protoc_label, "root_out_dir")
+    args += [
+      # 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),
+    ]
 
-    sources = invoker.sources
-
-    # 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}}"
-    }
-    out_dir = "$root_gen_dir/" + proto_out_dir
-    rel_out_dir = rebase_path(out_dir, root_build_dir)
-
-    outputs = []
-
-    args = []
-    if (defined(invoker.cc_include)) {
+    if (generate_cc) {
       args += [
-        "--include",
-        invoker.cc_include,
-        "--protobuf",
-        "$rel_out_dir/{{source_name_part}}.pb.h",
+        "--cc-out-dir",
+        rel_cc_out_dir,
       ]
+      if (defined(invoker.cc_generator_options)) {
+        args += [
+          "--cc-options",
+          invoker.cc_generator_options,
+        ]
+      }
+      if (defined(invoker.cc_include)) {
+        args += [
+          "--include",
+          invoker.cc_include,
+        ]
+      }
     }
 
-    args += [
-      "--proto-in-dir",
-      "{{source_dir}}",
-      "--proto-in-file",
-      "{{source_file_part}}",
-
-      # TODO(brettw) support system protobuf compiler.
-      "--use-system-protobuf=0",
-    ]
-
-    protoc_label = "//third_party/protobuf:protoc($host_toolchain)"
-    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),
-    ]
-
-    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_python) {
       args += [
-        "--python_out",
+        "--py-out-dir",
         rel_py_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 = ""
-      }
-      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",
-      ]
-
-      # 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) {
     link_target_type = "source_set"
   } 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,
                            [
                              "visibility",
                              "defines",
                            ])
 
     sources = get_target_outputs(":$action_name")
 
     if (defined(invoker.extra_configs)) {
       configs += invoker.extra_configs
     }
 
     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",
     ]
 
     # This will link any libraries in the deps (the use of invoker.deps in the
     # action won't link it).
     if (defined(invoker.deps)) {
       deps += invoker.deps
     }
   }
 }