pw_protobuf_compiler: Symlink options to proto root

This fixes nanopb generation for proto_library targets with the import_prefix attribute. Bug: 396230548 Change-Id: I4c869e52e172027ea3ea057e5178e2ff72fbf1a9 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/267936 Docs-Not-Needed: Ted Pudlik <[email protected]> Lint: Lint 🤖 <[email protected]> Reviewed-by: Austin Foxley <[email protected]> Commit-Queue: Ted Pudlik <[email protected]>
google · Feb 14, 2025 · bfc7d9e · bfc7d9e
1 parent f27dd15
commit bfc7d9e
Show file tree

Hide file tree

Showing 6 changed files with 267 additions and 26 deletions.
diff --git a/pw_protobuf_compiler/BUILD.bazel b/pw_protobuf_compiler/BUILD.bazel
@@ -16,6 +16,7 @@ load("@rules_python//python:proto.bzl", "py_proto_library")
 load("@rules_python//sphinxdocs:sphinx_docs_library.bzl", "sphinx_docs_library")
 load("//pw_build:compatibility.bzl", "incompatible_with_mcu")
 load("//pw_build:copy_to_bin.bzl", "copy_to_bin")
+load("//pw_protobuf_compiler:pw_proto_filegroup.bzl", "pw_proto_filegroup")
 load(
     "//pw_protobuf_compiler:pw_proto_library.bzl",
     "nanopb_proto_library",
@@ -136,3 +137,75 @@ copy_to_bin(
         "pw_protobuf_compiler_protos/test.proto",
     ],
 )
+
+pw_proto_filegroup(
+    name = "prefix_tests_proto_with_options",
+    srcs = ["nanopb_test_protos/prefix_tests.proto"],
+    options_files = ["nanopb_test_protos/prefix_tests.options"],
+)
+
+proto_library(
+    name = "prefix_tests_default_proto",
+    srcs = [":prefix_tests_proto_with_options"],
+)
+
+nanopb_proto_library(
+    name = "prefix_tests_default_nanopb",
+    deps = [":prefix_tests_default_proto"],
+)
+
+pw_cc_test(
+    name = "prefix_default_test",
+    srcs = ["prefix_test.cc"],
+    local_defines = ["PW_PROTOBUF_COMPILER_IMPORT=DEFAULT"],
+    deps = [
+        ":prefix_tests_default_nanopb",
+        "//pw_status",
+        "//pw_string:util",
+    ],
+)
+
+proto_library(
+    name = "prefix_tests_strip_import_prefix_proto",
+    srcs = [":prefix_tests_proto_with_options"],
+    strip_import_prefix = "/pw_protobuf_compiler/nanopb_test_protos",
+)
+
+nanopb_proto_library(
+    name = "prefix_tests_strip_import_prefix_nanopb",
+    deps = [":prefix_tests_strip_import_prefix_proto"],
+)
+
+pw_cc_test(
+    name = "prefix_strip_import_prefix_test",
+    srcs = ["prefix_test.cc"],
+    local_defines = ["PW_PROTOBUF_COMPILER_IMPORT=STRIP_IMPORT_PREFIX"],
+    deps = [
+        ":prefix_tests_strip_import_prefix_nanopb",
+        "//pw_status",
+        "//pw_string:util",
+    ],
+)
+
+proto_library(
+    name = "prefix_tests_import_prefix_proto",
+    srcs = [":prefix_tests_proto_with_options"],
+    import_prefix = "some_prefix",
+    strip_import_prefix = "/pw_protobuf_compiler/nanopb_test_protos",
+)
+
+nanopb_proto_library(
+    name = "prefix_tests_import_prefix_nanopb",
+    deps = [":prefix_tests_import_prefix_proto"],
+)
+
+pw_cc_test(
+    name = "prefix_import_prefix_test",
+    srcs = ["prefix_test.cc"],
+    local_defines = ["PW_PROTOBUF_COMPILER_IMPORT=IMPORT_PREFIX"],
+    deps = [
+        ":prefix_tests_import_prefix_nanopb",
+        "//pw_status",
+        "//pw_string:util",
+    ],
+)
diff --git a/pw_protobuf_compiler/BUILD.gn b/pw_protobuf_compiler/BUILD.gn
@@ -30,6 +30,7 @@ pw_test_group("tests") {
     ":pwpb_test",
     ":nested_packages_test",
     ":pwpb_no_prefix_test",
+    ":prefix_default_test",
   ]
 }
 
@@ -84,3 +85,55 @@ pw_test("pwpb_no_prefix_test") {
   sources = [ "pwpb_test_no_prefix.cc" ]
   deps = [ ":pwpb_no_prefix_test_protos.pwpb" ]
 }
+
+pw_proto_library("prefix_tests_default_proto") {
+  sources = [ "nanopb_test_protos/prefix_tests.proto" ]
+  inputs = [ "nanopb_test_protos/prefix_tests.options" ]
+  prefix = "pw_protobuf_compiler"
+}
+
+pw_test("prefix_default_test") {
+  sources = [ "prefix_test.cc" ]
+  defines = [ "PW_PROTOBUF_COMPILER_IMPORT=DEFAULT" ]
+  deps = [
+    ":prefix_tests_default_proto.nanopb",
+    "$dir_pw_status",
+    "$dir_pw_string",
+  ]
+  enable_if = dir_pw_third_party_nanopb != ""
+}
+
+pw_proto_library("prefix_tests_strip_import_prefix_proto") {
+  sources = [ "nanopb_test_protos/prefix_tests.proto" ]
+  inputs = [ "nanopb_test_protos/prefix_tests.options" ]
+  strip_prefix = "nanopb_test_protos"
+}
+
+pw_test("prefix_strip_import_prefix_test") {
+  sources = [ "prefix_test.cc" ]
+  defines = [ "PW_PROTOBUF_COMPILER_IMPORT=STRIP_IMPORT_PREFIX" ]
+  deps = [
+    ":prefix_tests_default_proto.nanopb",
+    "$dir_pw_status",
+    "$dir_pw_string",
+  ]
+  enable_if = dir_pw_third_party_nanopb != ""
+}
+
+pw_proto_library("prefix_tests_import_prefix_proto") {
+  sources = [ "nanopb_test_protos/prefix_tests.proto" ]
+  inputs = [ "nanopb_test_protos/prefix_tests.options" ]
+  strip_prefix = "nanopb_test_protos"
+  prefix = "some_prefix"
+}
+
+pw_test("prefix_import_prefix_test") {
+  sources = [ "prefix_test.cc" ]
+  defines = [ "PW_PROTOBUF_COMPILER_IMPORT=IMPORT_PREFIX" ]
+  deps = [
+    ":prefix_tests_default_proto.nanopb",
+    "$dir_pw_status",
+    "$dir_pw_string",
+  ]
+  enable_if = dir_pw_third_party_nanopb != ""
+}
diff --git a/pw_protobuf_compiler/nanopb_test_protos/prefix_tests.options b/pw_protobuf_compiler/nanopb_test_protos/prefix_tests.options
@@ -0,0 +1,14 @@
+// Copyright 2025 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.
+prefixtest.Message.name max_size:64
diff --git a/pw_protobuf_compiler/nanopb_test_protos/prefix_tests.proto b/pw_protobuf_compiler/nanopb_test_protos/prefix_tests.proto
@@ -0,0 +1,20 @@
+// Copyright 2025 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.
+syntax = "proto3";
+
+package prefixtest;
+
+message Message {
+  string name = 1;
+}
diff --git a/pw_protobuf_compiler/prefix_test.cc b/pw_protobuf_compiler/prefix_test.cc
@@ -0,0 +1,53 @@
+// Copyright 2025 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.
+
+#include <string>
+
+#if !defined(PW_PROTOBUF_COMPILER_IMPORT)
+#error "This test requires specifying a value for PW_PROTOBUF_COMPILER_IMPORT!"
+#endif
+
+#define DEFAULT 1
+#define STRIP_IMPORT_PREFIX 2
+#define IMPORT_PREFIX 3
+
+#if PW_PROTOBUF_COMPILER_IMPORT == DEFAULT
+#include "pw_protobuf_compiler/nanopb_test_protos/prefix_tests.pb.h"
+#endif
+
+#if PW_PROTOBUF_COMPILER_IMPORT == STRIP_IMPORT_PREFIX
+#include "prefix_tests.pb.h"
+#endif
+
+#if PW_PROTOBUF_COMPILER_IMPORT == IMPORT_PREFIX
+#include "some_prefix/prefix_tests.pb.h"
+#endif
+
+#include "pw_status/status.h"
+#include "pw_string/util.h"
+#include "pw_unit_test/framework.h"
+
+namespace pw::protobuf_compiler {
+namespace {
+
+TEST(PrefixTest, Compiles) {
+  std::string str = "abcd";
+  prefixtest_Message proto = prefixtest_Message_init_default;
+  EXPECT_EQ(
+      pw::string::Copy(str.c_str(), proto.name, sizeof(proto.name)).status(),
+      pw::OkStatus());
+}
+
+}  // namespace
+}  // namespace pw::protobuf_compiler
diff --git a/pw_protobuf_compiler/private/proto.bzl b/pw_protobuf_compiler/private/proto.bzl
@@ -98,6 +98,21 @@ def compile_proto(ctx):
         defines = [],
     )
 
+def _options_symlink_path(options_file, workspace_root, proto_source_root, import_prefix, strip_import_prefix):
+    path_in_module = paths.relativize(options_file.path, workspace_root)
+
+    if strip_import_prefix:
+        stripped_path = paths.relativize(path_in_module, strip_import_prefix.lstrip("/"))
+    else:
+        stripped_path = path_in_module
+
+    if import_prefix:
+        extended_path = paths.join(import_prefix, stripped_path)
+    else:
+        extended_path = stripped_path
+
+    return paths.join(proto_source_root, extended_path)
+
 def _proto_compiler_aspect_impl(target, ctx):
     # List the files we will generate for this proto_library target.
     proto_info = target[ProtoInfo]
@@ -146,6 +161,43 @@ def _proto_compiler_aspect_impl(target, ctx):
             else:
                 srcs.append(out_file)
 
+    # The proto_source_root may be prefixed with the output directory. But it
+    # may not. Ensure that there is no such prefix, which is intended to become
+    # the only case one day. See
+    # https://github.com/protocolbuffers/protobuf/blob/069a66850d1d8bb83c1ca1eb5bdee87525290584/bazel/private/proto_info.bzl#L154-L165
+    relative_proto_source_root = proto_info.proto_source_root
+    if relative_proto_source_root.startswith(out_path):
+        relative_proto_source_root = paths.relativize(relative_proto_source_root, out_path)
+
+    # Symlink the .options files into the proto_source_root, so that they can be
+    # found by protoc plugins regardless of [strip_]import_prefix attribute
+    # values.
+    #
+    # For example, say we have a proto_library in //a/b/BUILD.bazel with
+    # strip_import_prefix = b and import_prefix = xyz. Then, the `.proto` files
+    # will live in the directory,
+    #
+    # bazel-bin/a/b/_virtual_imports/a/xyz/
+    #
+    # What we do here is move the `.options` files to the same directory. Later
+    # on, we'll provide `bazel-bin/a/b/_virtual_imports` to the protoc plugin's
+    # search path via `--custom_opt=-I`. This way, the proto and options files
+    # will be alongside each other, as the plugins expect.
+    symlinks = []
+    for src in ctx.rule.attr.srcs:
+        if PwProtoOptionsInfo in src:
+            for options_file in src[PwProtoOptionsInfo].options_files.to_list():
+                path_to_options_file = _options_symlink_path(
+                    options_file,
+                    target.label.workspace_root,
+                    relative_proto_source_root,
+                    ctx.rule.attr.import_prefix,
+                    ctx.rule.attr.strip_import_prefix,
+                )
+                options_symlink_out = ctx.actions.declare_file(path_to_options_file)
+                ctx.actions.symlink(output = options_symlink_out, target_file = options_file)
+                symlinks.append(options_symlink_out)
+
     # List the `.options` files from any `pw_proto_filegroup` targets listed
     # under this target's `srcs`.
     options_files = [
@@ -155,36 +207,12 @@ def _proto_compiler_aspect_impl(target, ctx):
         for options_file in src[PwProtoOptionsInfo].options_files.to_list()
     ]
 
-    # Local repository options files.
-    options_file_include_paths = [paths.join(".", ctx.rule.attr.strip_import_prefix.lstrip("/"))]
-    for options_file in options_files:
-        # Handle .options files residing in external repositories.
-        if options_file.owner.workspace_root:
-            options_file_include_paths.append(
-                paths.join(
-                    options_file.owner.workspace_root,
-                    ctx.rule.attr.strip_import_prefix.lstrip("/"),
-                ),
-            )
-
-        # Handle generated .options files.
-        if options_file.root.path:
-            options_file_include_paths.append(
-                paths.join(
-                    options_file.root.path,
-                    ctx.rule.attr.strip_import_prefix.lstrip("/"),
-                ),
-            )
-
     args = ctx.actions.args()
     for path in proto_info.transitive_proto_path.to_list():
         args.add("-I{}".format(path))
 
     args.add("--plugin=protoc-gen-custom={}".format(ctx.executable._protoc_plugin.path))
-
-    # Convert include paths to a depset and back to deduplicate entries.
-    for options_file_include_path in depset(options_file_include_paths).to_list():
-        args.add("--custom_opt=-I{}".format(options_file_include_path))
+    args.add("--custom_opt=-I{}".format(paths.join(out_path, relative_proto_source_root)))
 
     for plugin_option in ctx.attr._plugin_options:
         # If the plugin supports directly specifying the location of the options files, pass them here.
@@ -207,7 +235,7 @@ def _proto_compiler_aspect_impl(target, ctx):
         inputs = depset(
             direct = proto_info.direct_sources +
                      proto_info.transitive_sources.to_list() +
-                     options_files,
+                     options_files + symlinks,
             transitive = [proto_info.transitive_descriptor_sets],
         ),
         progress_message = "Generating %s C++ files for %s" % (ctx.attr._extensions, ctx.label.name),