import paths from external Bazel modules are broken with Bazel 8.0.0

[armandomontanez]

Summary

imports runfiles handling of external projects appears to have broken with the switch to --legacy_external_runfiles=False in Bazel 8.0.0.

Minimal reproducer

https://github.com/armandomontanez/bazel_reproducers/tree/main/external_runfiles_broken_rules_python

Reproduces on both macOS and linux.

Additional notes

I haven't unwound yet whether or not all import handling of external py_* rules are completely broken, I noticed this specifically with py_proto_library rules that use strip_import_prefix and live in an external repository.

I noticed this when working with nanopb where sys.path reported the import path as:

/private/var/tmp/_bazel_amontanez/e724b21efc8bc19866072fbc72ee5907/sandbox/darwin-sandbox/7/execroot/_main/bazel-out/darwin_x86_64-opt-exec-ST-e8d06b237d06/bin/external/nanopb+/protoc-gen-nanopb.runfiles/_main/external/nanopb+/_virtual_imports/nanopb_proto

when the files actually existed at:

/private/var/tmp/_bazel_amontanez/e724b21efc8bc19866072fbc72ee5907/sandbox/darwin-sandbox/7/execroot/_main/bazel-out/darwin_x86_64-opt-exec-ST-e8d06b237d06/bin/external/nanopb+/protoc-gen-nanopb.runfiles/_main/../nanopb+/_virtual_imports/nanopb_proto

[tpudlik]

I wonder if this is an issue specifically with how py_proto_library constructs import paths? This logic basically lives here. (Note that this is now in protobuf, not in rules_python.)

I touched that code in the past, let me take a closer look.

[tpudlik]

I came up with a fix for py_proto_library in the protobuf repo. This fixes the reproducer (but I've not yet done any testing to verify that nothing else breaks). However, to my surprise, there now appear to be copies of this implementation in both protobuf and rules_python, so I guess they both need to be fixed :/

This fix is very hacky, though. I need to think about how to address this more cleanly.

diff --git a/bazel/py_proto_library.bzl b/bazel/py_proto_library.bzl
index 1c4059842..46b22d1a3 100644
--- a/bazel/py_proto_library.bzl
+++ b/bazel/py_proto_library.bzl
@@ -76,7 +76,10 @@ def _py_proto_aspect_impl(target, ctx):
             proto_root = proto_root[len(ctx.bin_dir.path) + 1:]
 
         plugin_output = ctx.bin_dir.path + "/" + proto_root
-        proto_root = ctx.workspace_name + "/" + proto_root
+        if proto_root.startswith('external/'):
+            proto_root = proto_root[len('external') + 1:]
+        else:
+            proto_root = ctx.workspace_name + "/" + proto_root
 
         proto_common.compile(
             actions = ctx.actions,

[tpudlik]

I have a draft PR, but I want to try adding a test for it. Won't get to it today, but hopefully early tomorrow!

[aignas]

I think we could just point to the protobuf implementation from rules_python instead of duplicating the fix. rules_python already depends on protobuf anyway.

[tpudlik]

That sounds like a good idea, but it looks it was tried a few months ago and had to be reverted (8b0eaed) :(

I'm afraid this is way out of scope of this issue.

[aignas]

It was reverted because the landscape of proto libraries was different. I think right now it would be a different story. I am happy to also accept the contribution in the PR and we can leave the shimming for the future.