pw_protobuf_compiler: Support standalone external protos

Projects may want to use externally defined proto files that are not
organized for Python packaging as required by pw_proto_library. This
change makes it possible to support standalone, externally defined
protobufs, such as nanopb.proto.

Change-Id: I4ae053a950c664878150d911cea9e9de031bb20d
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/31800
Reviewed-by: Alexei Frolov <frolv@google.com>

pw_protobuf_compiler/BUILD.gn

@@ -17,6 +17,7 @@ import("//build_overrides/pigweed.gni")
 import("$dir_pw_build/python.gni")
 import("$dir_pw_docgen/docs.gni")
 import("$dir_pw_protobuf_compiler/proto.gni")
+import("$dir_pw_third_party/nanopb/nanopb.gni")
 import("$dir_pw_unit_test/test.gni")
 
 pw_doc_group("docs") {
@@ -35,6 +36,10 @@ pw_test("nanopb_test") {
 
 pw_proto_library("nanopb_test_protos") {
   sources = [ "pw_protobuf_compiler_nanopb_protos/nanopb_test.proto" ]
+
+  if (dir_pw_third_party_nanopb != "") {
+    deps = [ "$dir_pw_third_party/nanopb:proto" ]
+  }
 }
 
 pw_proto_library("test_protos") {

pw_protobuf_compiler/docs.rst

@@ -46,7 +46,7 @@ For example, given the following target:
 .. code-block::
 
   pw_proto_library("test_protos") {
-    sources = [ "test.proto" ]
+    sources = [ "my_test_protos/test.proto" ]
   }
 
 ``test_protos.pwpb`` compiles code for pw_protobuf, and ``test_protos.nanopb``
@@ -68,15 +68,13 @@ dependency of another GN target.
 
   pw_proto_library("my_protos") {
     sources = [
-      "foo.proto",
+      "my_protos/foo.proto",
-      "bar.proto",
+      "my_protos/bar.proto",
     ]
   }
 
   pw_proto_library("my_other_protos") {
-    sources = [
+    sources = [ "my_other_protos/baz.proto" ]  # imports foo.proto
-      "baz.proto",  # imports foo.proto
-    ]
 
     # Proto libraries depend on other proto libraries directly.
     deps = [ ":my_protos" ]
@@ -92,3 +90,22 @@ dependency of another GN target.
     # When depending on protos in a source_set, specify the generator suffix.
     deps = [ ":my_other_protos.pwpb" ]
   }
+
+Proto file structure
+--------------------
+Protobuf source files must be nested under another directory when they are
+listed in sources. This ensures that they can be packaged properly in Python.
+The first directory is used as the Python package name.
+
+The requirements for proto file structure in the source tree will be relaxed in
+future updates.
+
+Working with externally defined protos
+--------------------------------------
+``pw_proto_library`` targets may be used to build ``.proto`` sources from
+existing projects. In these cases, it may be necessary to supply the
+``include_path`` argument, which specifies the protobuf include path to use for
+``protoc``. If only a single external protobuf is being compiled, the
+requirement that the protobuf be nested under a directory is waived. This
+exception should only be used when absolutely necessary -- for example, to
+support proto files that includes ``import "nanopb.proto"`` in them.

pw_protobuf_compiler/proto.cmake

@@ -90,7 +90,7 @@ function(_pw_generate_protos
       "${script}"
       --language "${LANGUAGE}"
       --plugin-path "${PLUGIN}"
-      --module-path "${CMAKE_CURRENT_SOURCE_DIR}"
+      --include-path "${CMAKE_CURRENT_SOURCE_DIR}"
       --include-file "${INCLUDE_FILE}"
       --out-dir "${OUT_DIR}"
       ${ARGN}
@@ -168,7 +168,6 @@ function(_pw_nanopb_library NAME SOURCES DEPS INCLUDE_FILE OUT_DIR)
       "${OUT_DIR}"
       "${SOURCES}"
       "${DEPS}"
-      --include-paths "${nanopb_dir}/generator/proto"
   )
 
   # Create the library with the generated source files.

pw_protobuf_compiler/proto.gni

@@ -54,8 +54,8 @@ template("_pw_invoke_protoc") {
     args = [
              "--language",
              invoker.language,
-             "--module-path",
+             "--include-path",
-             rebase_path("."),
+             rebase_path(invoker.include_path),
              "--include-file",
              _output[0],
              "--out-dir",
@@ -69,17 +69,11 @@ template("_pw_invoke_protoc") {
       args += [ "--plugin-path=" + rebase_path(invoker.plugin) ]
     }
 
-    if (defined(invoker.include_paths)) {
-      args += [
-        "--include-paths",
-        string_join(";", rebase_path(invoker.include_paths)),
-      ]
-    }
-
     outputs = []
     foreach(extension, invoker.output_extensions) {
       foreach(proto,
-              rebase_path(invoker.sources, get_path_info(".", "abspath"))) {
+              rebase_path(invoker.sources,
+                          get_path_info(invoker.include_path, "abspath"))) {
         _output = string_replace(proto, ".proto", extension)
         outputs += [ "${invoker.gen_dir}/$_output" ]
       }
@@ -127,7 +121,6 @@ template("_pw_nanopb_rpc_proto_library") {
     language = "nanopb_rpc"
     plugin = "$dir_pw_rpc/py/pw_rpc/plugin_nanopb.py"
     python_deps = [ "$dir_pw_rpc/py" ]
-    include_paths = [ "$dir_pw_third_party_nanopb/generator/proto" ]
     output_extensions = [ ".rpc.pb.h" ]
   }
 
@@ -156,7 +149,6 @@ template("_pw_nanopb_proto_library") {
     forward_variables_from(invoker, "*", _forwarded_vars)
     language = "nanopb"
     plugin = "$dir_pw_third_party_nanopb/generator/protoc-gen-nanopb"
-    include_paths = [ "$dir_pw_third_party_nanopb/generator/proto" ]
     output_extensions = [
       ".pb.h",
       ".pb.c",
@@ -230,12 +222,24 @@ template("_pw_go_proto_library") {
 # file. Use pw_proto_library instead.
 template("_pw_python_proto_library") {
   _target = target_name
-  _package_dir = invoker.package_dir
+
+  # For standalone protos (e.g. `import "nanopb.proto"`), nest the proto file in
+  # a directory with the same name for Python packaging purposes.
+  if (invoker.standalone_proto) {
+    _source_name = get_path_info(invoker.sources, "name")
+    _proto_gen_dir = "${invoker.gen_dir}/${_source_name[0]}_pb2"
+  } else {
+    _proto_gen_dir = invoker.gen_dir
+  }
 
   _pw_invoke_protoc(target_name) {
     forward_variables_from(invoker, "*", _forwarded_vars)
+    gen_dir = _proto_gen_dir
     language = "python"
-    output_extensions = [ "_pb2.py" ]
+    output_extensions = [
+      "_pb2.py",
+      "_pb2.pyi",
+    ]
     deps += [ "$dir_pw_protobuf_compiler:protobuf_requirements.install" ]
   }
 
@@ -249,9 +253,13 @@ template("_pw_python_proto_library") {
              "--setup",
              rebase_path(_setup_py),
              "--package",
-             _package_dir,
+             invoker._package_dir,
            ] + rebase_path(_generated_files, invoker.gen_dir)
 
+    if (invoker.standalone_proto) {
+      args += [ "--standalone" ]
+    }
+
     public_deps = [ ":$_target._gen" ]
     outputs = [ _setup_py ]
   }
@@ -276,18 +284,52 @@ template("_pw_python_proto_library") {
 #  sources: List of input .proto files.
 #  deps: List of other pw_proto_library dependencies.
 #  inputs: Other files on which the protos depend (e.g. nanopb .options files).
+#  include_path: Sets the proto include path. The default is ".". It is not
+#      recommended to set this, unless pulling in an externally defined proto.
 #
 template("pw_proto_library") {
   assert(defined(invoker.sources) && invoker.sources != [],
          "pw_proto_library requires .proto source files")
 
+  _common = {
+    base_target = target_name
+    gen_dir = "$target_gen_dir/$target_name"
+    sources = invoker.sources
+
+    if (defined(invoker.include_path)) {
+      include_path = invoker.include_path
+    } else {
+      include_path = "."
+    }
+  }
+
+  _rebased_sources = rebase_path(invoker.sources, _common.include_path)
+
+  # The pw_proto_library GN target requires protos to be nested under the
+  # include directory unless three conditions are met:
+  #
+  #   1. There is only one .proto file.
+  #   2. The file is in a different directory (an externally defined proto).
+  #   3. The include path is the .proto's include directory. Since there are no
+  #      nested directories, the proto cannot be packaged properly in Python.
+  #
+  # When these conditions are met, the proto library is allowed, even though the
+  # proto file is not nested. The Python package for it uses the Python module's
+  # name. This is a special exception to the typical pattern to allow for
+  # working with single, external, standalone protobuf not set up for Python
+  # packaging (such as nanopb.proto).
+  _standalone_proto =
+      _rebased_sources == [ _rebased_sources[0] ] &&
+      _common.include_path != "." &&
+      string_split(_rebased_sources[0], "/") == [ _rebased_sources[0] ]
+
   _package_dir = ""
 
-  foreach(_rebased_proto_path, rebase_path(invoker.sources, ".")) {
+  foreach(_rebased_source, _rebased_sources) {
     _path_components = []
-    _path_components = string_split(_rebased_proto_path, "/")
+    _path_components = string_split(_rebased_source, "/")
 
-    assert(_path_components != [ _rebased_proto_path ] &&
+    assert((_standalone_proto || _path_components != [ _rebased_source ]) &&
                _path_components[0] != "..",
            "Sources in a pw_proto_library must live in subdirectories " +
                "of where it is defined")
@@ -308,12 +350,6 @@ template("pw_proto_library") {
     visibility = []
   }
 
-  _common = {
-    base_target = target_name
-    gen_dir = "$target_gen_dir/$target_name"
-    sources = invoker.sources
-  }
-
   if (defined(invoker.deps)) {
     _deps = invoker.deps
   } else {
@@ -331,7 +367,7 @@ template("pw_proto_library") {
 
     # Indicate this library's base directory for its dependents.
     metadata = {
-      protoc_includes = [ rebase_path(".") ]
+      protoc_includes = [ rebase_path(_common.include_path) ]
     }
   }
 
@@ -368,10 +404,19 @@ template("pw_proto_library") {
       deps = process_file_template(_deps, "{{source}}.nanopb_rpc")
     }
 
-    _pw_nanopb_proto_library("$target_name.nanopb") {
+    # When compiling with the Nanopb plugin, the nanopb.proto file is already
-      forward_variables_from(invoker, _forwarded_vars)
+    # compiled internally, so skip recompiling it here.
-      forward_variables_from(_common, "*")
+    if (invoker.sources ==
-      deps = process_file_template(_deps, "{{source}}.nanopb")
+        [ "$dir_pw_third_party_nanopb/generator/proto/nanopb.proto" ]) {
+      pw_input_group("$target_name.nanopb") {
+        sources = invoker.sources
+      }
+    } else {
+      _pw_nanopb_proto_library("$target_name.nanopb") {
+        forward_variables_from(invoker, _forwarded_vars)
+        forward_variables_from(_common, "*")
+        deps = process_file_template(_deps, "{{source}}.nanopb")
+      }
     }
   } else {
     pw_error("$target_name.nanopb_rpc") {
@@ -395,6 +440,7 @@ template("pw_proto_library") {
     sources = invoker.sources
     deps = process_file_template(_deps, "{{source}}.go")
     base_target = _common.base_target
+    include_path = _common.include_path
   }
 
   _pw_python_proto_library("$target_name.python") {
@@ -402,7 +448,7 @@ template("pw_proto_library") {
     forward_variables_from(_common, "*")
     deps = process_file_template(_deps, "{{source}}.python")
     base_target = _common.base_target
-    package_dir = _package_dir
+    standalone_proto = _standalone_proto
   }
 
   # All supported pw_protobuf generators.

pw_protobuf_compiler/py/BUILD.gn

@@ -15,6 +15,7 @@
 import("//build_overrides/pigweed.gni")
 
 import("$dir_pw_build/python.gni")
+import("$dir_pw_third_party/nanopb/nanopb.gni")
 
 pw_python_package("py") {
   setup = [ "setup.py" ]
@@ -32,4 +33,10 @@ pw_python_package("py") {
   python_deps = [ "$dir_pw_cli/py" ]
   python_test_deps = [ "..:test_protos.python" ]
   pylintrc = "$dir_pigweed/.pylintrc"
+
+  # If Nanopb is available, test protos that import nanopb.proto.
+  if (dir_pw_third_party_nanopb != "") {
+    python_test_deps += [ "..:nanopb_test_protos.python" ]
+    tests += [ "compiled_nanopb_protos_test.py" ]
+  }
 }

pw_protobuf_compiler/py/compiled_nanopb_protos_test.py

@@ -0,0 +1,29 @@
+#!/usr/bin/env python3
+# Copyright 2020 The Pigweed Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may not
+# use this file except in compliance with the License. You may obtain a copy of
+# the License at
+#
+#     https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations under
+# the License.
+"""Tests compiling and importing Python protos that import Nanopb."""
+
+import unittest
+
+from pw_protobuf_compiler_nanopb_protos import nanopb_test_pb2
+
+
+class TestCompileAndImport(unittest.TestCase):
+    def test_access_compiled_protobufs(self):
+        message = nanopb_test_pb2.Point(name='hello world')
+        self.assertEqual(message.name, 'hello world')
+
+
+if __name__ == '__main__':
+    unittest.main()

pw_protobuf_compiler/py/pw_protobuf_compiler/generate_protos.py

@@ -50,13 +50,9 @@ def argument_parser(
     parser.add_argument('--plugin-path',
                         type=Path,
                         help='Path to the protoc plugin')
-    parser.add_argument('--module-path',
+    parser.add_argument('--include-path',
                         required=True,
-                        help='Path to the module containing the .proto files')
+                        help='Include path for proto compilation')
-    parser.add_argument('--include-paths',
-                        default=[],
-                        type=lambda arg: arg.split(';'),
-                        help='protoc include paths')
     parser.add_argument('--include-file',
                         type=argparse.FileType('r'),
                         help='File containing additional protoc include paths')
@@ -94,7 +90,7 @@ def protoc_nanopb_args(args: argparse.Namespace) -> Tuple[str, ...]:
         f'protoc-gen-nanopb={args.plugin_path}',
         # nanopb_opt provides the flags to use for nanopb_out. Windows doesn't
         # like when you merge the two using the `flag,...:out` syntax.
-        f'--nanopb_opt=-I{args.module_path}',
+        f'--nanopb_opt=-I{args.include_path}',
         f'--nanopb_out={args.out_dir}',
     )
 
@@ -155,8 +151,7 @@ def main() -> int:
 
     os.makedirs(args.out_dir, exist_ok=True)
 
-    include_paths = [f'-I{path}' for path in args.include_paths]
+    include_paths = [f'-I{line.strip()}' for line in args.include_file]
-    include_paths += [f'-I{line.strip()}' for line in args.include_file]
 
     wrapper_script: Optional[Path] = None
 
@@ -178,7 +173,7 @@ def main() -> int:
         process = subprocess.run(
             [
                 'protoc',
-                f'-I{args.module_path}',
+                f'-I{args.include_path}',
                 *include_paths,
                 *DEFAULT_PROTOC_ARGS[args.language](args),
                 *args.protos,

pw_protobuf_compiler/py/pw_protobuf_compiler/generate_python_package.py

@@ -54,15 +54,21 @@ def _parse_args():
                         required=True,
                         type=Path,
                         help='Path to setup.py file')
+    parser.add_argument('--standalone',
+                        action='store_true',
+                        help='The package is a standalone external proto')
     parser.add_argument('sources',
                         type=Path,
                         nargs='+',
-                        help='Relative paths to sources in the package')
+                        help='Relative paths to the .py and .pyi files')
     return parser.parse_args()
 
 
-def main(package: str, setup: Path, sources: List[Path]) -> int:
+def main(package: str, setup: Path, standalone: bool,
+         sources: List[Path]) -> int:
     """Generates __init__.py and py.typed files and a setup.py."""
+    assert not standalone or len(sources) == 2
+
     base = setup.parent.resolve()
     base.mkdir(exist_ok=True)
 
@@ -77,24 +83,27 @@ def main(package: str, setup: Path, sources: List[Path]) -> int:
     # Create __init__.py and py.typed files for each subdirectory.
     for pkg in subpackages:
         pkg.mkdir(exist_ok=True, parents=True)
-        pkg.joinpath('__init__.py').touch()
+        pkg.joinpath('__init__.py').write_text('')
 
-        package_name = '.'.join(pkg.relative_to(base).as_posix().split('/'))
+        package_name = pkg.relative_to(base).as_posix().replace('/', '.')
         pkg.joinpath('py.typed').touch()
         pkg_data[package_name].append('py.typed')
 
-    # Add the .pyi for each source file.
+    # Add the Mypy stub (.pyi) for each source file.
-    for source in sources:
+    for mypy_stub in (s for s in sources if s.suffix == '.pyi'):
-        pkg = base / source.parent
+        pkg = base / mypy_stub.parent
-        package_name = '.'.join(pkg.relative_to(base).as_posix().split('/'))
+        package_name = pkg.relative_to(base).as_posix().replace('/', '.')
+        pkg_data[package_name].append(mypy_stub.name)
 
-        path = base.joinpath(source).relative_to(pkg).with_suffix('.pyi')
+        if standalone:
-        pkg_data[package_name].append(str(path))
+            pkg.joinpath('__init__.py').write_text(
+                f'from {mypy_stub.stem}.{mypy_stub.stem} import *\n')
 
     setup.write_text(
         _SETUP_TEMPLATE.format(name=package,
                                packages=list(pkg_data),
                                package_data=dict(pkg_data)))
+
     return 0
 
 

third_party/nanopb/BUILD.gn

@@ -15,6 +15,7 @@
 import("//build_overrides/pigweed.gni")
 
 import("$dir_pw_build/target_types.gni")
+import("$dir_pw_protobuf_compiler/proto.gni")
 import("nanopb.gni")
 
 # This file defines a GN source_set for an external installation of nanopb.
@@ -40,6 +41,11 @@ if (dir_pw_third_party_nanopb != "") {
       "$dir_pw_third_party_nanopb/pb_encode.c",
     ]
   }
+
+  pw_proto_library("proto") {
+    include_path = "$dir_pw_third_party_nanopb/generator/proto"
+    sources = [ "$dir_pw_third_party_nanopb/generator/proto/nanopb.proto" ]
+  }
 } else {
   group("nanopb") {
   }