Fix paths for sibling repository setup and generated .proto files

Implement additional tests for protos across different repos with and without strip prefixes. Verify cc_proto_library and java_proto_library work over this setup without warnings.

Choose cc_proto_library, because it generates files per .proto file and java_proto_library, because it generates a single file (jar) per proto_library.

Address problems with --experimental_sibling_repository_layout: `-I../a.proto=<path>` never worked. Correct to `-Ia.proto=<path>`.

Address problems with generated files in Bazel repositories when generate_protos_in_virtual_imports is False, which were hidden because the flag defaults to true. Fix incorrect and/or missing paths in --proto_path argument.

Make it explicit that ProtoInfo.proto_source_root is in some cases returning a path with bindir/genfilesdir and in other cases without.

PiperOrigin-RevId: 531414364
Change-Id: I1edf734632c58f50ffae7db22ee361dccbfb4917

Author: comius

src/main/starlark/builtins_bzl/common/proto/proto_common.bzl

@@ -38,9 +38,10 @@ def _Iimport_path_equals_fullpath(proto_source):
     return "-I%s=%s" % (_get_import_path(proto_source), proto_source._source_file.path)
 
 def _get_import_path(proto_source):
-    if proto_source._proto_path == "":
-        return proto_source._source_file.path
-    return proto_source._source_file.path[len(proto_source._proto_path) + 1:]
+    proto_path = proto_source._proto_path
+    if proto_path and not proto_source._source_file.path.startswith(proto_path + "/"):
+        fail("Bad proto_path %s for proto %s" % (proto_path, proto_source._source_file.path))
+    return proto_source._source_file.path.removeprefix(proto_path + "/")
 
 def _compile(
         actions,

src/main/starlark/builtins_bzl/common/proto/proto_info.bzl

@@ -33,7 +33,21 @@ ProtoInfo = provider(
         "proto_source_root": """(str) The directory relative to which the `.proto` files defined in
             the `proto_library` are defined. For example, if this is `a/b` and the rule has the
             file `a/b/c/d.proto` as a source, that source file would be imported as
-            `import c/d.proto`""",
+            `import c/d.proto`
+
+            In principle, the `proto_source_root` directory itself should always
+            be relative to the output directory (`ctx.bin_dir` or `ctx.genfiles_dir`).
+
+            This is at the moment not true for `proto_libraries` using (additional and/or strip)
+            import prefixes. `proto_source_root` is in this case prefixed with the output
+            directory. For example, the value is similar to
+            `bazel-out/k8-fastbuild/bin/a/_virtual_includes/b` for an input file in
+            `a/_virtual_includes/b/c.proto` that should be imported as `c.proto`.
+
+            When using the value please account for both cases in a general way.
+            That is assume the value is either prefixed with the output directory or not.
+            This will make it possible to fix `proto_library` in the future.
+            """,
         "transitive_proto_path": """(depset(str) A set of `proto_source_root`s collected from the
             transitive closure of this rule.""",
         "check_deps_sources": """(depset[File]) The `.proto` sources from the 'srcs' attribute.

src/main/starlark/builtins_bzl/common/proto/proto_library.bzl

@@ -71,7 +71,7 @@ def _proto_library_impl(ctx):
 
     proto_path, direct_sources = _create_proto_sources(ctx, srcs, import_prefix, strip_import_prefix)
     descriptor_set = ctx.actions.declare_file(ctx.label.name + "-descriptor-set.proto.bin")
-    proto_info = _create_proto_info(direct_sources, deps, proto_path, descriptor_set)
+    proto_info = _create_proto_info(direct_sources, deps, proto_path, descriptor_set, bin_dir = ctx.bin_dir.path)
 
     _write_descriptor_set(ctx, direct_sources, deps, exports, proto_info, descriptor_set)
 
@@ -90,6 +90,35 @@ def _proto_library_impl(ctx):
         ),
     ]
 
+def _remove_sibling_repo(relpath):
+    # Addresses sibling repository layout: ../repo/package/path -> package/path
+    if relpath.startswith("../"):
+        split = relpath.split("/", 2)
+        relpath = split[2] if len(split) >= 3 else ""
+    return relpath
+
+def _from_root(root, relpath):
+    """Constructs an exec path from root to relpath"""
+    if not root:
+        # `relpath` is a directory with an input source file, the exec path is one of:
+        # - when in main repo: `package/path`
+        # - when in a external repository: `external/repo/package/path`
+        #   - with sibling layout: `../repo/package/path`
+        return relpath
+    else:
+        # `relpath` is a directory with a generated file or an output directory:
+        # - when in main repo: `{root}/package/path`
+        # - when in an external repository: `{root}/external/repo/package/path`
+        #   - with sibling layout: `{root}/package/path`
+        return _join(root, _remove_sibling_repo(relpath))
+
+def _empty_to_dot(path):
+    return path if path else "."
+
+def _uniq(iterable):
+    unique_elements = {element: None for element in iterable}
+    return list(unique_elements.keys())
+
 def _create_proto_sources(ctx, srcs, import_prefix, strip_import_prefix):
     """Transforms Files in srcs to ProtoSourceInfos, optionally symlinking them to _virtual_imports.
 
@@ -105,17 +134,13 @@ def _create_proto_sources(ctx, srcs, import_prefix, strip_import_prefix):
         return _symlink_to_virtual_imports(ctx, srcs, import_prefix, strip_import_prefix)
     else:
         # No virtual source roots
-        direct_sources = []
-        for src in srcs:
-            if ctx.label.workspace_name == "" or ctx.label.workspace_root.startswith(".."):
-                # source_root == ''|'bazel-out/foo/k8-fastbuild/bin'
-                source_root = src.root.path
-            else:
-                # source_root == ''|'bazel-out/foo/k8-fastbuild/bin' / 'external/repo'
-                source_root = _join(src.root.path, ctx.label.workspace_root)
-            direct_sources.append(ProtoSourceInfo(_source_file = src, _proto_path = source_root))
-
-        return ctx.label.workspace_root if ctx.label.workspace_root else ".", direct_sources
+        proto_path = ctx.label.workspace_root  # ''|'../repo'|'external/repo'
+        direct_sources = [ProtoSourceInfo(
+            _source_file = src,
+            _proto_path = _from_root(src.root.path, proto_path),
+        ) for src in srcs]
+
+        return proto_path, direct_sources
 
 def _join(*path):
     return "/".join([p for p in path if p != ""])
@@ -127,12 +152,8 @@ def _symlink_to_virtual_imports(ctx, srcs, import_prefix, strip_import_prefix):
           A pair proto_path, directs_sources.
     """
     virtual_imports = _join("_virtual_imports", ctx.label.name)
-    if ctx.label.workspace_name == "" or ctx.label.workspace_root.startswith(".."):  # siblingRepositoryLayout
-        # Example: `bazel-out/[repo/]target/bin / pkg / _virtual_imports/name`
-        proto_path = _join(ctx.genfiles_dir.path, ctx.label.package, virtual_imports)
-    else:
-        # Example: `bazel-out/target/bin / repo / pkg / _virtual_imports/name`
-        proto_path = _join(ctx.genfiles_dir.path, ctx.label.workspace_root, ctx.label.package, virtual_imports)
+    proto_path = _join(ctx.label.workspace_root, ctx.label.package, virtual_imports)
+    root_proto_path = _from_root(ctx.genfiles_dir.path, proto_path)
 
     if ctx.label.workspace_name == "":
         full_strip_import_prefix = strip_import_prefix
@@ -156,10 +177,10 @@ def _symlink_to_virtual_imports(ctx, srcs, import_prefix, strip_import_prefix):
             target_file = src,
             progress_message = "Symlinking virtual .proto sources for %{label}",
         )
-        direct_sources.append(ProtoSourceInfo(_source_file = virtual_src, _proto_path = proto_path))
+        direct_sources.append(ProtoSourceInfo(_source_file = virtual_src, _proto_path = root_proto_path))
     return proto_path, direct_sources
 
-def _create_proto_info(direct_sources, deps, proto_path, descriptor_set):
+def _create_proto_info(direct_sources, deps, proto_path, descriptor_set, bin_dir):
     """Constructs ProtoInfo."""
 
     # Construct ProtoInfo
@@ -173,10 +194,15 @@ def _create_proto_info(direct_sources, deps, proto_path, descriptor_set):
         transitive = [dep.transitive_sources for dep in deps],
         order = "preorder",
     )
+
+    # There can be up more than 1 direct proto_paths, for example when there's
+    # a generated and non-generated .proto file in srcs
+    root_paths = _uniq([src._source_file.root.path for src in direct_sources])
     transitive_proto_path = depset(
-        direct = [proto_path],
+        direct = [_empty_to_dot(_from_root(root, proto_path)) for root in root_paths],
         transitive = [dep.transitive_proto_path for dep in deps],
     )
+
     if direct_sources:
         check_deps_sources = depset(direct = [src._source_file for src in direct_sources])
     else:
@@ -198,7 +224,8 @@ def _create_proto_info(direct_sources, deps, proto_path, descriptor_set):
         transitive_sources = transitive_sources,
         direct_descriptor_set = descriptor_set,
         transitive_descriptor_sets = transitive_descriptor_sets,
-        proto_source_root = proto_path,
+        #TODO(b/281812523): remove bin_dir from proto_source_root (when users assuming it's there are migrated)
+        proto_source_root = _empty_to_dot(_from_root(bin_dir, proto_path) if "_virtual_imports/" in proto_path else _remove_sibling_repo(proto_path)),
         transitive_proto_path = transitive_proto_path,
         check_deps_sources = check_deps_sources,
         transitive_imports = transitive_sources,
@@ -208,7 +235,10 @@ def _create_proto_info(direct_sources, deps, proto_path, descriptor_set):
     )
 
 def _get_import_path(proto_source):
-    return paths.relativize(proto_source._source_file.path, proto_source._proto_path)
+    proto_path = proto_source._proto_path
+    if proto_path and not proto_source._source_file.path.startswith(proto_path + "/"):
+        fail("Bad proto_path %s for proto %s" % (proto_path, proto_source._source_file.path))
+    return proto_source._source_file.path.removeprefix(proto_path + "/")
 
 def _write_descriptor_set(ctx, direct_sources, deps, exports, proto_info, descriptor_set):
     """Writes descriptor set."""

src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommonTest.java

@@ -405,6 +405,7 @@ public void generateCode_directGeneratedProtos() throws Exception {
         .comparingElementsUsing(MATCHES_REGEX)
         .containsExactly(
             "--plugin=bl?azel?-out/[^/]*-exec-[^/]*/bin/third_party/x/plugin",
+            "--proto_path=bl?azel?-out/k8-fastbuild/bin",
             "-Ibar/A.proto=bar/A.proto",
             "-Ibar/G.proto=bl?azel?-out/k8-fastbuild/bin/bar/G.proto",
             "bar/A.proto",
@@ -436,6 +437,7 @@ public void generateCode_inDirectGeneratedProtos() throws Exception {
         .comparingElementsUsing(MATCHES_REGEX)
         .containsExactly(
             "--plugin=bl?azel?-out/[^/]*-exec-[^/]*/bin/third_party/x/plugin",
+            "--proto_path=bl?azel?-out/k8-fastbuild/bin",
             "-Ibar/A.proto=bar/A.proto",
             "-Ibar/G.proto=bl?azel?-out/k8-fastbuild/bin/bar/G.proto",
             "bar/A.proto")
@@ -451,22 +453,22 @@ public void generateCode_inDirectGeneratedProtos() throws Exception {
     "{virtual: false, sibling: false, generated: false, expectedFlags:"
         + " ['--proto_path=external/foo','-Ie/E.proto=external/foo/e/E.proto']}",
     "{virtual: false, sibling: false, generated: true, expectedFlags:"
-        + " ['--proto_path=external/foo',"
+        + " ['--proto_path=bl?azel?-out/k8-fastbuild/bin/external/foo',"
         + " '-Ie/E.proto=bl?azel?-out/k8-fastbuild/bin/external/foo/e/E.proto']}",
     "{virtual: true, sibling: false, generated: false,expectedFlags:"
         + " ['--proto_path=external/foo','-Ie/E.proto=external/foo/e/E.proto']}",
     "{virtual: true, sibling: false, generated: true, expectedFlags:"
         + " ['--proto_path=bl?azel?-out/k8-fastbuild/bin/external/foo/e/_virtual_imports/e',"
         + " '-Ie/E.proto=bl?azel?-out/k8-fastbuild/bin/external/foo/e/_virtual_imports/e/e/E.proto']}",
     "{virtual: true, sibling: true, generated: false,expectedFlags:"
-        + " ['--proto_path=../foo','-I../foo/e/E.proto=../foo/e/E.proto']}",
+        + " ['--proto_path=../foo','-Ie/E.proto=../foo/e/E.proto']}",
     "{virtual: true, sibling: true, generated: true, expectedFlags:"
         + " ['--proto_path=bl?azel?-out/foo/k8-fastbuild/bin/e/_virtual_imports/e',"
         + " '-Ie/E.proto=bl?azel?-out/foo/k8-fastbuild/bin/e/_virtual_imports/e/e/E.proto']}",
     "{virtual: false, sibling: true, generated: false,expectedFlags:"
-        + " ['--proto_path=../foo','-I../foo/e/E.proto=../foo/e/E.proto']}",
+        + " ['--proto_path=../foo','-Ie/E.proto=../foo/e/E.proto']}",
     "{virtual: false, sibling: true, generated: true, expectedFlags:"
-        + " ['--proto_path=../foo','-Ie/E.proto=bl?azel?-out/foo/k8-fastbuild/bin/e/E.proto']}",
+        + " ['--proto_path=bl?azel?-out/foo/k8-fastbuild/bin','-Ie/E.proto=bl?azel?-out/foo/k8-fastbuild/bin/e/E.proto']}",
   })
   public void generateCode_externalProtoLibrary(
       boolean virtual, boolean sibling, boolean generated, List<String> expectedFlags)

src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java

@@ -449,7 +449,7 @@ private void testExternalRepoWithGeneratedProto(
       fooProtoRoot =
           genfiles + (siblingRepoLayout ? "" : "/external/foo") + "/x/_virtual_imports/x";
     } else {
-      fooProtoRoot = (siblingRepoLayout ? "../foo" : "external/foo");
+      fooProtoRoot = (siblingRepoLayout ? genfiles : genfiles + "/external/foo");
     }
     ConfiguredTarget a = getConfiguredTarget("//a:a");
     ProtoInfo aInfo = a.get(ProtoInfo.PROVIDER);

src/test/shell/bazel/bazel_proto_library_test.sh

@@ -49,6 +49,7 @@ function write_workspace() {
   fi
 
   cat $(rlocation io_bazel/src/tests/shell/bazel/rules_proto_stanza.txt) >> "$workspace"WORKSPACE
+
   cat >> "$workspace"WORKSPACE << EOF
 load("@rules_proto//proto:repositories.bzl", "rules_proto_dependencies", "rules_proto_toolchains")
 rules_proto_dependencies()
@@ -672,6 +673,17 @@ proto_library(
   srcs = ["h.proto"],
   deps = ["//g", "@repo//f"],
 )
+
+cc_proto_library(
+  name = "h_cc_proto",
+  deps = ["//h"],
+)
+
+java_proto_library(
+  name = "h_java_proto",
+  deps = ["//h"],
+)
+
 EOF
 
   cat > h/h.proto <<EOF
@@ -687,7 +699,130 @@ message H {
 }
 EOF
 
-  bazel build //h || fail "build failed"
+  bazel build -s --noexperimental_sibling_repository_layout //h >& $TEST_log || fail "failed"
+  bazel build -s --noexperimental_sibling_repository_layout //h:h_cc_proto >& $TEST_log || fail "failed"
+  bazel build -s --noexperimental_sibling_repository_layout //h:h_java_proto >& $TEST_log || fail "failed"
+
+  bazel build -s --experimental_sibling_repository_layout //h >& $TEST_log || fail "failed"
+  bazel build -s --experimental_sibling_repository_layout //h:h_cc_proto >& $TEST_log || fail "failed"
+  bazel build -s --experimental_sibling_repository_layout //h:h_java_proto >& $TEST_log || fail "failed"
+
+  expect_not_log "warning: directory does not exist." # --proto_path is wrong
+}
+
+function test_cross_repo_protos() {
+  mkdir -p e
+  touch e/WORKSPACE
+  write_workspace ""
+
+  cat >> WORKSPACE <<EOF
+local_repository(
+  name = "repo",
+  path = "e"
+)
+EOF
+
+  mkdir -p e/f/good
+  cat > e/f/BUILD <<EOF
+load("@rules_proto//proto:defs.bzl", "proto_library")
+proto_library(
+  name = "f",
+  srcs = ["good/f.proto"],
+  visibility = ["//visibility:public"],
+)
+
+proto_library(
+  name = "gen",
+  srcs = ["good/gen.proto"],
+  visibility = ["//visibility:public"],
+)
+
+genrule(name = 'generate', srcs = ['good/gensrc.txt'], cmd = 'cat \$(SRCS) > \$@', outs = ['good/gen.proto'])
+
+EOF
+
+  cat > e/f/good/f.proto <<EOF
+syntax = "proto2";
+package f;
+
+message F {
+  optional int32 f = 1;
+}
+EOF
+
+  cat > e/f/good/gensrc.txt <<EOF
+syntax = "proto2";
+package gen;
+
+message Gen {
+  optional int32 gen = 1;
+}
+EOF
+
+  mkdir -p g/good
+  cat > g/BUILD << EOF
+load("@rules_proto//proto:defs.bzl", "proto_library")
+proto_library(
+  name = 'g',
+  srcs = ['good/g.proto'],
+  visibility = ["//visibility:public"],
+)
+EOF
+
+  cat > g/good/g.proto <<EOF
+syntax = "proto2";
+package g;
+
+message G {
+  optional int32 g = 1;
+}
+EOF
+
+  mkdir -p h
+  cat > h/BUILD <<EOF
+load("@rules_proto//proto:defs.bzl", "proto_library")
+proto_library(
+  name = "h",
+  srcs = ["h.proto"],
+  deps = ["//g", "@repo//f", "@repo//f:gen"],
+)
+
+cc_proto_library(
+  name = "h_cc_proto",
+  deps = ["//h"],
+)
+
+java_proto_library(
+  name = "h_java_proto",
+  deps = ["//h"],
+)
+EOF
+
+  cat > h/h.proto <<EOF
+syntax = "proto2";
+package h;
+
+import "f/good/f.proto";
+import "g/good/g.proto";
+import "f/good/gen.proto";
+
+message H {
+  optional f.F f = 1;
+  optional g.G g = 2;
+  optional gen.Gen h = 3;
+}
+EOF
+
+  bazel build -s --noexperimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h >& $TEST_log || fail "failed"
+  bazel build -s --noexperimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h:h_cc_proto >& $TEST_log || fail "failed"
+  bazel build -s --noexperimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h:h_java_proto >& $TEST_log || fail "failed"
+
+  bazel build -s --experimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h -s >& $TEST_log || fail "failed"
+  bazel build -s --experimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports //h:h_cc_proto -s >& $TEST_log || fail "failed"
+  bazel build -s --experimental_sibling_repository_layout --noincompatible_generated_protos_in_virtual_imports  //h:h_java_proto  -s >& $TEST_log || fail "failed"
+
+  expect_not_log "warning: directory does not exist." # --proto_path is wrong
+
 }
 
 run_suite "Integration tests for proto_library"