From d517afc161a08b8f971119e9ec05867ee45a1edc Mon Sep 17 00:00:00 2001 From: Wyatt Hepler Date: Wed, 3 Feb 2021 19:40:08 -0800 Subject: [PATCH] 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 --- pw_protobuf_compiler/BUILD.gn | 5 + pw_protobuf_compiler/docs.rst | 29 ++++- pw_protobuf_compiler/proto.cmake | 3 +- pw_protobuf_compiler/proto.gni | 106 +++++++++++++----- pw_protobuf_compiler/py/BUILD.gn | 7 ++ .../py/compiled_nanopb_protos_test.py | 29 +++++ .../pw_protobuf_compiler/generate_protos.py | 15 +-- .../generate_python_package.py | 29 +++-- third_party/nanopb/BUILD.gn | 6 + 9 files changed, 171 insertions(+), 58 deletions(-) create mode 100755 pw_protobuf_compiler/py/compiled_nanopb_protos_test.py diff --git a/pw_protobuf_compiler/BUILD.gn b/pw_protobuf_compiler/BUILD.gn index e97442293..c611c6b67 100644 --- a/pw_protobuf_compiler/BUILD.gn +++ b/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") { diff --git a/pw_protobuf_compiler/docs.rst b/pw_protobuf_compiler/docs.rst index 1c4c1d734..30bb37a52 100644 --- a/pw_protobuf_compiler/docs.rst +++ b/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", - "bar.proto", + "my_protos/foo.proto", + "my_protos/bar.proto", ] } pw_proto_library("my_other_protos") { - sources = [ - "baz.proto", # imports foo.proto - ] + sources = [ "my_other_protos/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. diff --git a/pw_protobuf_compiler/proto.cmake b/pw_protobuf_compiler/proto.cmake index bd8584e4b..73b06b6c9 100644 --- a/pw_protobuf_compiler/proto.cmake +++ b/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. diff --git a/pw_protobuf_compiler/proto.gni b/pw_protobuf_compiler/proto.gni index 1478a6432..ce297c603 100644 --- a/pw_protobuf_compiler/proto.gni +++ b/pw_protobuf_compiler/proto.gni @@ -54,8 +54,8 @@ template("_pw_invoke_protoc") { args = [ "--language", invoker.language, - "--module-path", - rebase_path("."), + "--include-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") { - forward_variables_from(invoker, _forwarded_vars) - forward_variables_from(_common, "*") - deps = process_file_template(_deps, "{{source}}.nanopb") + # When compiling with the Nanopb plugin, the nanopb.proto file is already + # compiled internally, so skip recompiling it here. + if (invoker.sources == + [ "$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. diff --git a/pw_protobuf_compiler/py/BUILD.gn b/pw_protobuf_compiler/py/BUILD.gn index f2231e085..1d5a10b1d 100644 --- a/pw_protobuf_compiler/py/BUILD.gn +++ b/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" ] + } } diff --git a/pw_protobuf_compiler/py/compiled_nanopb_protos_test.py b/pw_protobuf_compiler/py/compiled_nanopb_protos_test.py new file mode 100755 index 000000000..d3840634e --- /dev/null +++ b/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() diff --git a/pw_protobuf_compiler/py/pw_protobuf_compiler/generate_protos.py b/pw_protobuf_compiler/py/pw_protobuf_compiler/generate_protos.py index 3d2932bbb..5db2f2c96 100644 --- a/pw_protobuf_compiler/py/pw_protobuf_compiler/generate_protos.py +++ b/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') - parser.add_argument('--include-paths', - default=[], - type=lambda arg: arg.split(';'), - help='protoc include paths') + help='Include path for proto compilation') 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, diff --git a/pw_protobuf_compiler/py/pw_protobuf_compiler/generate_python_package.py b/pw_protobuf_compiler/py/pw_protobuf_compiler/generate_python_package.py index 4c3e8658c..b79d77047 100644 --- a/pw_protobuf_compiler/py/pw_protobuf_compiler/generate_python_package.py +++ b/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. - for source in sources: - pkg = base / source.parent - package_name = '.'.join(pkg.relative_to(base).as_posix().split('/')) + # Add the Mypy stub (.pyi) for each source file. + for mypy_stub in (s for s in sources if s.suffix == '.pyi'): + pkg = base / mypy_stub.parent + 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') - pkg_data[package_name].append(str(path)) + if standalone: + 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 diff --git a/third_party/nanopb/BUILD.gn b/third_party/nanopb/BUILD.gn index 1927ab1d3..695ce18fe 100644 --- a/third_party/nanopb/BUILD.gn +++ b/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") { }