Support strip_import_prefix

[alexeagle]

Related to https://github.com/bazelbuild/bazel-gazelle/issues/1173

This fails

# gazelle:proto_strip_import_prefix /rt/proto/libs/libbdican
# gazelle:proto file
# gazelle:go_generate_proto false
# gazelle:proto_plugin py implementation builtin:python
# gazelle:proto_rule proto_compile implementation stackb:rules_proto:proto_compile
# gazelle:proto_rule proto_py_library implementation stackb:rules_proto:proto_py_library
# gazelle:proto_language py plugin py
# gazelle:proto_language py rule proto_compile
# gazelle:proto_language py rule proto_py_library

proto_library(
    name = "GenericParamEnumeration_proto",
    srcs = ["GenericParamEnumeration.proto"],
    strip_import_prefix = "/rt/proto/libs/libbdican",
    visibility = ["//visibility:public"],
)

proto_compile(
    name = "GenericParamEnumeration_py_compile",
    outputs = ["GenericParamEnumeration_pb2.py"],
    plugins = ["@build_stack_rules_proto//plugin/builtin:python"],
    proto = "GenericParamEnumeration_proto",
)

Because the protoc command line is

bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/protoc $@' '' '--python_out=bazel-out/k8-opt/bin' '--descriptor_set_in=bazel-out/k8-opt/bin/rt/proto/libs/libbdican/GenericParamEnumeration_proto-descriptor-set.proto.bin' GenericParamEnumeration.proto

that means that the file was written to bazel-out/k8-opt/bin/GenericParamEnumeration_pb2.py but Bazel expected it in the output directory for the package:

ERROR: BUILD.bazel:14: output 'rt/proto/libs/libbdican/GenericParamEnumeration_pb2.py' was not created

If I comment out the strip_import_prefix line then it works (the pb2.py file is written to the path Bazel expects)

[alexeagle]

I traced it to here: https://github.com/stackb/rules_proto/blob/4f9e9a917a16bddeaf4ecb97e437f50b3bff09d1/rules/proto_compile.bzl#L255

When there is no strip_import_prefix on the proto_library rule, the proto reference here looks like <source file rt/proto/libs/libbdican/GenericParamEnumeration.proto> so the output location is calculated correctly.

When strip_import_prefix appears, proto is now <generated file rt/proto/libs/libbdican/_virtual_imports/GenericParamEnumeration_proto/GenericParamEnumeration.proto>

I think this means the ProtoInfo provider is giving us an object that's not quite suitable for predicting where protoc actually writes its output? For reference, in that second case the full proto_info looks like this:

proto_info struct(
 check_deps_sources = depset([<generated file rt/proto/libs/libbdican/_virtual_imports/GenericParamEnumeration_proto/GenericParamEnumeration.proto>]),
direct_descriptor_set = <generated file rt/proto/libs/libbdican/GenericParamEnumeration_proto-descriptor-set.proto.bin>,
direct_sources = [<generated file rt/proto/libs/libbdican/_virtual_imports/GenericParamEnumeration_proto/GenericParamEnumeration.proto>], 
proto_source_root = "bazel-out/k8-opt/bin/rt/proto/libs/libbdican/_virtual_imports/GenericParamEnumeration_proto", 
transitive_descriptor_sets = depset([<generated file rt/proto/libs/libbdican/GenericParamEnumeration_proto-descriptor-set.proto.bin>]), 
transitive_imports = depset([<generated file rt/proto/libs/libbdican/_virtual_imports/GenericParamEnumeration_proto/GenericParamEnumeration.proto>], order = "preorder"), 
transitive_proto_path = depset(["bazel-out/k8-opt/bin/rt/proto/libs/libbdican/_virtual_imports/GenericParamEnumeration_proto"]), 
transitive_sources = depset([<generated file rt/proto/libs/libbdican/_virtual_imports/GenericParamEnumeration_proto/GenericParamEnumeration.proto>], order = "preorder"))

[alexeagle]

I see that protoc wants that last argument to be something that appears in the descriptor file. So maybe it's actually the calculation of the python_out argument that needs to be adjusted.

[alexeagle]

I was able to work around this issue for the protoc emit, by listing my possible values of strip_import_prefix, and use the current package to sense which should be used, then fixup the python_out argument: https://github.com/alexeagle/repro_stackb_py_proto_deps/blob/main/rt/fourth_party/stackb_rules_proto.hack_for_issue_221.patch

However, there's a follow-on issue as shown in the repro https://github.com/alexeagle/repro_stackb_py_proto_deps - the proto_py_library targets don't have their 1p dependencies added. So I think my hacky workaround must be in the wrong layer, where it made the proto_library targets work but Gazelle still doesn't understand how to resolve the deps.

[alexeagle]

https://github.com/alexeagle/repro_stackb_py_proto_deps/compare/works shows that when the strip_import_prefix is removed, then the deps are generated correctly.

[alexeagle]

one more note, I think if https://github.com/bazelbuild/bazel-gazelle/issues/1173 were resolved, that's probably the principled fix at the right layer, and maybe naturally solves this issue as well. I think @aptenodytes-forsteri is looking into that.

[samschlegel]

Just ran into this as well, but the mentioned https://github.com/bazelbuild/bazel-gazelle/issues/1173 isn't the issue I'm running into. We simply have a top-level /proto directory with # gazelle:proto_strip_import_prefix /proto configured in /proto/BUILD. The proto_library rules are all generated fine and the deps work, but the rules in this repo seem to just not take it into account at all, or do so in a way that also strips the prefix from the generated file path, which leads to

ERROR: /proto/validate/BUILD.bazel:19:14: output 'proto/validate/validate_pb2.py' was not created
ERROR: /proto/validate/BUILD.bazel:19:14: output 'proto/validate/validate_pb2.pyi' was not created
ERROR: /proto/validate/BUILD.bazel:19:14: Compiling protoc outputs for ["validate.proto"] failed: not all outputs were created or valid

Looking at the sandbox afterwards, these were generated, but they were generated at validate/validate_pb2.py, one level up than expected.

[samschlegel]

After manually adding output_mappings things work as expected. Does support for generating this just need to be added to the builtin python plugin, or is there some configuration that's missing?

EDIT: Yeah the cpp plugin seems to generate the output_mappings as expected, so I think it's just the Python plugin

[samschlegel]

Copying the way the cpp_plugin.go works did get the output_mappings working...but it also highlighted an issue that also shows up in the cpp rules. Adjusting the outputs in the Configure() method like so does get the output_mapping generating

diff --git a/pkg/plugin/builtin/python_plugin.go b/pkg/plugin/builtin/python_plugin.go
index 57b32c4..0cbfb47 100644
--- a/pkg/plugin/builtin/python_plugin.go
+++ b/pkg/plugin/builtin/python_plugin.go
@@ -28,7 +28,7 @@ func (p *PythonPlugin) Configure(ctx *protoc.PluginContext) *protoc.PluginConfig
        flags := parsePythonPluginOptions(p.Name(), ctx.PluginConfig.GetFlags())
 
        pyFiles := protoc.FlatMapFiles(
-               pythonGeneratedFileName(ctx.Rel),
+               pythonGeneratedFileName(ctx.ProtoLibrary.StripImportPrefix(), ctx.Rel),
                protoc.Always,
                ctx.ProtoLibrary.Files()...,
        )
@@ -51,12 +51,16 @@ func (p *PythonPlugin) Configure(ctx *protoc.PluginContext) *protoc.PluginConfig
 // pythonGeneratedFileName is a utility function that returns a function that
 // computes the name of a predicted generated file having the given
 // extension(s) relative to the given dir.
-func pythonGeneratedFileName(reldir string) func(f *protoc.File) []string {
+func pythonGeneratedFileName(stripImportPrefix, reldir string) func(f *protoc.File) []string {
+       prefix := strings.TrimPrefix(stripImportPrefix, "/")
        return func(f *protoc.File) []string {
                name := strings.ReplaceAll(f.Name, "-", "_")
                if reldir != "" {
                        name = path.Join(reldir, name)
                }
+               if strings.HasPrefix(name, prefix) {
+                       name = strings.TrimPrefix(name[len(prefix):], "/")
+               }
                return []string{name + "_pb2.py"}
        }
 }

But it also seems to confuse the library srcs and they start generating without the relative directory stripped

diff --git a/proto/validate/BUILD.bazel b/proto/validate/BUILD.bazel
index df13e515410..491824d4bd9 100644
--- a/proto/validate/BUILD.bazel
+++ b/proto/validate/BUILD.bazel
@@ -18,7 +18,10 @@ proto_library(
 
 proto_compile(
     name = "validate_python_compile",
+    output_mappings = [
+        "validate_pb2.pyi=validate/validate_pb2.pyi",
+        "validate_pb2.py=validate/validate_pb2.py",
+    ],
     outputs = [
         "validate_pb2.py",
         "validate_pb2.pyi",
@@ -32,7 +35,7 @@ proto_compile(
 
 proto_py_library(
     name = "validate_py_library",
-    srcs = ["validate_pb2.py"],
+    srcs = ["validate/validate_pb2.py"],
     visibility = ["//visibility:public"],
     deps = ["@com_google_protobuf//:protobuf_python"],
 )

I think strip_import_prefix-y stuff needs to be pass down to the libraries, or adjusted for properly in the plugin output, but haven't read into the code enough to see what that implies