pw_protobuf_compiler: Avoid duplicate protobuf Python packages
For compatibility with Python packaging, each pw_protobuf_library target
must have a unique root directory for its protos. If this is violated,
the setup.py for the Python package is generated multiple times,
resulting in flaky builds.
This change ensures that no two pw_proto_library targets generate the
same Python package and fixes existing conflicts in the Pigweed repo.
Change-Id: Icbc13ceeb2fd329bc4fbf1e14ec039e98dc7e483
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/31740
Reviewed-by: Alexei Frolov <frolv@google.com>
diff --git a/pw_protobuf/BUILD.gn b/pw_protobuf/BUILD.gn
index ebe8636..562850f 100644
--- a/pw_protobuf/BUILD.gn
+++ b/pw_protobuf/BUILD.gn
@@ -95,12 +95,12 @@
pw_proto_library("codegen_test_protos") {
sources = [
- "pw_protobuf_protos/test_protos/full_test.proto",
- "pw_protobuf_protos/test_protos/imported.proto",
- "pw_protobuf_protos/test_protos/importer.proto",
- "pw_protobuf_protos/test_protos/non_pw_package.proto",
- "pw_protobuf_protos/test_protos/proto2.proto",
- "pw_protobuf_protos/test_protos/repeated.proto",
+ "pw_protobuf_test_protos/full_test.proto",
+ "pw_protobuf_test_protos/imported.proto",
+ "pw_protobuf_test_protos/importer.proto",
+ "pw_protobuf_test_protos/non_pw_package.proto",
+ "pw_protobuf_test_protos/proto2.proto",
+ "pw_protobuf_test_protos/repeated.proto",
]
deps = [ ":common_protos" ]
}
diff --git a/pw_protobuf/CMakeLists.txt b/pw_protobuf/CMakeLists.txt
index 66fc333..44ed7dc 100644
--- a/pw_protobuf/CMakeLists.txt
+++ b/pw_protobuf/CMakeLists.txt
@@ -27,10 +27,10 @@
pw_proto_library(pw_protobuf.codegen_test_protos
SOURCES
- pw_protobuf_protos/test_protos/full_test.proto
- pw_protobuf_protos/test_protos/imported.proto
- pw_protobuf_protos/test_protos/importer.proto
- pw_protobuf_protos/test_protos/non_pw_package.proto
- pw_protobuf_protos/test_protos/proto2.proto
- pw_protobuf_protos/test_protos/repeated.proto
+ pw_protobuf_test_protos/full_test.proto
+ pw_protobuf_test_protos/imported.proto
+ pw_protobuf_test_protos/importer.proto
+ pw_protobuf_test_protos/non_pw_package.proto
+ pw_protobuf_test_protos/proto2.proto
+ pw_protobuf_test_protos/repeated.proto
)
diff --git a/pw_protobuf/codegen_test.cc b/pw_protobuf/codegen_test.cc
index 385de2f..faceb51 100644
--- a/pw_protobuf/codegen_test.cc
+++ b/pw_protobuf/codegen_test.cc
@@ -23,11 +23,11 @@
// The purpose of the tests in this file is primarily to verify that the
// generated C++ interface is valid rather than the correctness of the
// low-level encoder.
-#include "pw_protobuf_protos/test_protos/full_test.pwpb.h"
-#include "pw_protobuf_protos/test_protos/importer.pwpb.h"
-#include "pw_protobuf_protos/test_protos/non_pw_package.pwpb.h"
-#include "pw_protobuf_protos/test_protos/proto2.pwpb.h"
-#include "pw_protobuf_protos/test_protos/repeated.pwpb.h"
+#include "pw_protobuf_test_protos/full_test.pwpb.h"
+#include "pw_protobuf_test_protos/importer.pwpb.h"
+#include "pw_protobuf_test_protos/non_pw_package.pwpb.h"
+#include "pw_protobuf_test_protos/proto2.pwpb.h"
+#include "pw_protobuf_test_protos/repeated.pwpb.h"
namespace pw::protobuf {
namespace {
diff --git a/pw_protobuf/pw_protobuf_protos/test_protos/full_test.proto b/pw_protobuf/pw_protobuf_test_protos/full_test.proto
similarity index 100%
rename from pw_protobuf/pw_protobuf_protos/test_protos/full_test.proto
rename to pw_protobuf/pw_protobuf_test_protos/full_test.proto
diff --git a/pw_protobuf/pw_protobuf_protos/test_protos/imported.proto b/pw_protobuf/pw_protobuf_test_protos/imported.proto
similarity index 100%
rename from pw_protobuf/pw_protobuf_protos/test_protos/imported.proto
rename to pw_protobuf/pw_protobuf_test_protos/imported.proto
diff --git a/pw_protobuf/pw_protobuf_protos/test_protos/importer.proto b/pw_protobuf/pw_protobuf_test_protos/importer.proto
similarity index 93%
rename from pw_protobuf/pw_protobuf_protos/test_protos/importer.proto
rename to pw_protobuf/pw_protobuf_test_protos/importer.proto
index 3f0e43b..39ad8b9 100644
--- a/pw_protobuf/pw_protobuf_protos/test_protos/importer.proto
+++ b/pw_protobuf/pw_protobuf_test_protos/importer.proto
@@ -13,7 +13,7 @@
// the License.
syntax = "proto3";
-import 'pw_protobuf_protos/test_protos/imported.proto';
+import 'pw_protobuf_test_protos/imported.proto';
import 'pw_protobuf_protos/common.proto';
package pw.protobuf.test;
diff --git a/pw_protobuf/pw_protobuf_protos/test_protos/non_pw_package.proto b/pw_protobuf/pw_protobuf_test_protos/non_pw_package.proto
similarity index 100%
rename from pw_protobuf/pw_protobuf_protos/test_protos/non_pw_package.proto
rename to pw_protobuf/pw_protobuf_test_protos/non_pw_package.proto
diff --git a/pw_protobuf/pw_protobuf_protos/test_protos/proto2.proto b/pw_protobuf/pw_protobuf_test_protos/proto2.proto
similarity index 100%
rename from pw_protobuf/pw_protobuf_protos/test_protos/proto2.proto
rename to pw_protobuf/pw_protobuf_test_protos/proto2.proto
diff --git a/pw_protobuf/pw_protobuf_protos/test_protos/repeated.proto b/pw_protobuf/pw_protobuf_test_protos/repeated.proto
similarity index 100%
rename from pw_protobuf/pw_protobuf_protos/test_protos/repeated.proto
rename to pw_protobuf/pw_protobuf_test_protos/repeated.proto
diff --git a/pw_protobuf_compiler/proto.gni b/pw_protobuf_compiler/proto.gni
index 03ff3c4..1478a64 100644
--- a/pw_protobuf_compiler/proto.gni
+++ b/pw_protobuf_compiler/proto.gni
@@ -230,25 +230,7 @@
# file. Use pw_proto_library instead.
template("_pw_python_proto_library") {
_target = target_name
- _package_dir = ""
-
- foreach(_rebased_proto_path, rebase_path(invoker.sources, ".")) {
- _path_components = []
- _path_components = string_split(_rebased_proto_path, "/")
-
- assert(_path_components != [ _rebased_proto_path ] &&
- _path_components[0] != "..",
- "Sources in a pw_proto_library must live in subdirectories " +
- "of where it is defined")
-
- if (_package_dir == "") {
- _package_dir = _path_components[0]
- } else {
- assert(_path_components[0] == _package_dir,
- "All .proto sources in a pw_proto_library must live " +
- "in the same directory tree")
- }
- }
+ _package_dir = invoker.package_dir
_pw_invoke_protoc(target_name) {
forward_variables_from(invoker, "*", _forwarded_vars)
@@ -261,7 +243,7 @@
_generated_files = get_target_outputs(":$target_name._gen")
# Create the setup and init files for the Python package.
- pw_python_action(target_name + "._package_gen") {
+ action(target_name + "._package_gen") {
script = "$dir_pw_protobuf_compiler/py/pw_protobuf_compiler/generate_python_package.py"
args = [
"--setup",
@@ -271,7 +253,7 @@
] + rebase_path(_generated_files, invoker.gen_dir)
public_deps = [ ":$_target._gen" ]
- stamp = true
+ outputs = [ _setup_py ]
}
# Create a Python package with the generated source files.
@@ -299,9 +281,36 @@
assert(defined(invoker.sources) && invoker.sources != [],
"pw_proto_library requires .proto source files")
+ _package_dir = ""
+
+ foreach(_rebased_proto_path, rebase_path(invoker.sources, ".")) {
+ _path_components = []
+ _path_components = string_split(_rebased_proto_path, "/")
+
+ assert(_path_components != [ _rebased_proto_path ] &&
+ _path_components[0] != "..",
+ "Sources in a pw_proto_library must live in subdirectories " +
+ "of where it is defined")
+
+ if (_package_dir == "") {
+ _package_dir = _path_components[0]
+ } else {
+ assert(_path_components[0] == _package_dir,
+ "All .proto sources in a pw_proto_library must live in the same " +
+ "directory tree")
+ }
+ }
+
+ # Create a group with the package directory in the name. This prevents
+ # multiple pw_proto_libraries from generating the same setup.py file, which
+ # results in awkward ninja errors that require manually re-running gn gen.
+ group("pw_proto_library.$_package_dir") {
+ visibility = []
+ }
+
_common = {
base_target = target_name
- gen_dir = "$target_gen_dir/protos"
+ gen_dir = "$target_gen_dir/$target_name"
sources = invoker.sources
}
@@ -393,6 +402,7 @@
forward_variables_from(_common, "*")
deps = process_file_template(_deps, "{{source}}.python")
base_target = _common.base_target
+ package_dir = _package_dir
}
# All supported pw_protobuf generators.
diff --git a/pw_rpc/BUILD b/pw_rpc/BUILD
index aef8d1c..dfd0910 100644
--- a/pw_rpc/BUILD
+++ b/pw_rpc/BUILD
@@ -188,7 +188,7 @@
proto_library(
name = "packet_proto",
srcs = [
- "pw_rpc_protos/packet.proto",
+ "pw_rpc_protos/internal/packet.proto",
],
)
diff --git a/pw_rpc/BUILD.gn b/pw_rpc/BUILD.gn
index 3e34b38..f25a335 100644
--- a/pw_rpc/BUILD.gn
+++ b/pw_rpc/BUILD.gn
@@ -16,6 +16,7 @@
import("$dir_pw_bloat/bloat.gni")
import("$dir_pw_build/module_config.gni")
+import("$dir_pw_build/python.gni")
import("$dir_pw_build/python_action.gni")
import("$dir_pw_build/target_types.gni")
import("$dir_pw_docgen/docs.gni")
@@ -132,19 +133,23 @@
}
pw_proto_library("protos") {
- sources = [ "pw_rpc_protos/packet.proto" ]
+ sources = [
+ "pw_rpc_protos/echo.proto",
+ "pw_rpc_protos/internal/packet.proto",
+ ]
+ inputs = [ "pw_rpc_protos/echo.options" ]
}
-pw_proto_library("echo_service_proto") {
- sources = [ "pw_rpc_protos/echo.proto" ]
- inputs = [ "pw_rpc_protos/echo.options" ]
+# TODO(hepler): Remove this compatibility wrapper.
+pw_python_group("echo_service_proto.python") {
+ python_deps = [ ":protos.python" ]
}
pw_doc_group("docs") {
sources = [ "docs.rst" ]
inputs = [
"pw_rpc_protos/echo.proto",
- "pw_rpc_protos/packet.proto",
+ "pw_rpc_protos/internal/packet.proto",
]
group_deps = [ "nanopb:docs" ]
report_deps = [ ":server_size" ]
diff --git a/pw_rpc/CMakeLists.txt b/pw_rpc/CMakeLists.txt
index 5a892d1..cba62ca 100644
--- a/pw_rpc/CMakeLists.txt
+++ b/pw_rpc/CMakeLists.txt
@@ -69,11 +69,7 @@
pw_proto_library(pw_rpc.protos
SOURCES
- pw_rpc_protos/packet.proto
-)
-
-pw_proto_library(pw_rpc.echo_proto
- SOURCES
+ pw_rpc_protos/internal/packet.proto
pw_rpc_protos/echo.proto
)
diff --git a/pw_rpc/docs.rst b/pw_rpc/docs.rst
index 6681a87..7235232 100644
--- a/pw_rpc/docs.rst
+++ b/pw_rpc/docs.rst
@@ -238,9 +238,9 @@
-------------
Pigweed RPC packets consist of a type and a set of fields. The packets are
encoded as protocol buffers. The full packet format is described in
-``pw_rpc/pw_rpc_protos/packet.proto``.
+``pw_rpc/pw_rpc_protos/internal/packet.proto``.
-.. literalinclude:: pw_rpc_protos/packet.proto
+.. literalinclude:: pw_rpc_protos/internal/packet.proto
:language: protobuf
:lines: 14-
diff --git a/pw_rpc/nanopb/BUILD.gn b/pw_rpc/nanopb/BUILD.gn
index 3b76625..c2e7788 100644
--- a/pw_rpc/nanopb/BUILD.gn
+++ b/pw_rpc/nanopb/BUILD.gn
@@ -86,7 +86,7 @@
pw_source_set("echo_service") {
public_configs = [ ":public" ]
- public_deps = [ "..:echo_service_proto.nanopb_rpc" ]
+ public_deps = [ "..:protos.nanopb_rpc" ]
sources = [ "public/pw_rpc/echo_service_nanopb.h" ]
}
diff --git a/pw_rpc/nanopb/CMakeLists.txt b/pw_rpc/nanopb/CMakeLists.txt
index 896fefd..87552a0 100644
--- a/pw_rpc/nanopb/CMakeLists.txt
+++ b/pw_rpc/nanopb/CMakeLists.txt
@@ -52,7 +52,7 @@
pw_add_module_library(pw_rpc.nanopb.echo_service
PUBLIC_DEPS
- pw_rpc.echo_proto.nanopb_rpc
+ pw_rpc.protos.nanopb_rpc
)
pw_auto_add_module_tests(pw_rpc.nanopb
@@ -61,7 +61,7 @@
pw_rpc.raw
pw_rpc.server
pw_rpc.nanopb.common
- pw_rpc.echo_proto.nanopb_rpc
+ pw_rpc.protos.nanopb_rpc
pw_rpc.test_protos.nanopb_rpc
pw_rpc.test_utils
)
diff --git a/pw_rpc/public/pw_rpc/internal/packet.h b/pw_rpc/public/pw_rpc/internal/packet.h
index 9eb58a1..9ea3d7c 100644
--- a/pw_rpc/public/pw_rpc/internal/packet.h
+++ b/pw_rpc/public/pw_rpc/internal/packet.h
@@ -18,7 +18,7 @@
#include <span>
#include "pw_bytes/span.h"
-#include "pw_rpc_protos/packet.pwpb.h"
+#include "pw_rpc_protos/internal/packet.pwpb.h"
#include "pw_status/status_with_size.h"
namespace pw::rpc::internal {
diff --git a/pw_rpc/pw_rpc_protos/packet.proto b/pw_rpc/pw_rpc_protos/internal/packet.proto
similarity index 100%
rename from pw_rpc/pw_rpc_protos/packet.proto
rename to pw_rpc/pw_rpc_protos/internal/packet.proto
diff --git a/pw_rpc/py/callback_client_test.py b/pw_rpc/py/callback_client_test.py
index e9a675a..443b820 100755
--- a/pw_rpc/py/callback_client_test.py
+++ b/pw_rpc/py/callback_client_test.py
@@ -19,7 +19,7 @@
from typing import List, Tuple
from pw_protobuf_compiler import python_protos
-from pw_rpc_protos import packet_pb2
+from pw_rpc_protos.internal import packet_pb2
from pw_status import Status
from pw_rpc import callback_client, client, packets
diff --git a/pw_rpc/py/client_test.py b/pw_rpc/py/client_test.py
index 181bd0e..bcf3d06 100755
--- a/pw_rpc/py/client_test.py
+++ b/pw_rpc/py/client_test.py
@@ -16,7 +16,7 @@
import unittest
-from pw_rpc_protos.packet_pb2 import RpcPacket
+from pw_rpc_protos.internal.packet_pb2 import RpcPacket
from pw_protobuf_compiler import python_protos
from pw_status import Status
diff --git a/pw_rpc/py/packets_test.py b/pw_rpc/py/packets_test.py
index acdd40a..ea49f3b 100755
--- a/pw_rpc/py/packets_test.py
+++ b/pw_rpc/py/packets_test.py
@@ -17,7 +17,7 @@
import unittest
from pw_status import Status
-from pw_rpc_protos.packet_pb2 import RpcPacket
+from pw_rpc_protos.internal.packet_pb2 import RpcPacket
from pw_rpc import packets
diff --git a/pw_rpc/py/pw_rpc/client.py b/pw_rpc/py/pw_rpc/client.py
index c5ca88d..6c47ceb 100644
--- a/pw_rpc/py/pw_rpc/client.py
+++ b/pw_rpc/py/pw_rpc/client.py
@@ -19,7 +19,7 @@
from typing import Collection, Dict, Iterable, Iterator, List, NamedTuple
from typing import Optional
-from pw_rpc_protos.packet_pb2 import RpcPacket
+from pw_rpc_protos.internal.packet_pb2 import RpcPacket
from pw_status import Status
from pw_rpc import descriptors, packets
diff --git a/pw_rpc/py/pw_rpc/packets.py b/pw_rpc/py/pw_rpc/packets.py
index 6c53570..3ca6fbd 100644
--- a/pw_rpc/py/pw_rpc/packets.py
+++ b/pw_rpc/py/pw_rpc/packets.py
@@ -14,7 +14,7 @@
"""Functions for working with pw_rpc packets."""
from google.protobuf import message
-from pw_rpc_protos import packet_pb2
+from pw_rpc_protos.internal import packet_pb2
from pw_status import Status
DecodeError = message.DecodeError
diff --git a/pw_target_runner/BUILD.gn b/pw_target_runner/BUILD.gn
index 6e67cbc..9db1016 100644
--- a/pw_target_runner/BUILD.gn
+++ b/pw_target_runner/BUILD.gn
@@ -27,5 +27,5 @@
}
pw_proto_library("exec_server_config_proto") {
- sources = [ "pw_target_runner_protos/exec_server_config.proto" ]
+ sources = [ "pw_target_runner_server_protos/exec_server_config.proto" ]
}
diff --git a/pw_target_runner/docs.rst b/pw_target_runner/docs.rst
index bca7084..28f20e9 100644
--- a/pw_target_runner/docs.rst
+++ b/pw_target_runner/docs.rst
@@ -39,7 +39,7 @@
^^^^^^^^^^^^^
The standalone server is configured from a file written in Protobuf text format
containing a ``pw.target_runner.ServerConfig`` message as defined in
-``//pw_target_runner/pw_target_runner_protos/exec_server_config.proto``.
+``//pw_target_runner/pw_target_runner_server_protos/exec_server_config.proto``.
At least one ``worker`` message must be specified. Each of the workers refers to
a script or program which is invoked with the path to an executable file as a
diff --git a/pw_target_runner/pw_target_runner_protos/exec_server_config.proto b/pw_target_runner/pw_target_runner_server_protos/exec_server_config.proto
similarity index 100%
rename from pw_target_runner/pw_target_runner_protos/exec_server_config.proto
rename to pw_target_runner/pw_target_runner_server_protos/exec_server_config.proto
