diff --git a/src/main/starlark/builtins_bzl/common/proto/proto_common.bzl b/src/main/starlark/builtins_bzl/common/proto/proto_common.bzl index 06ae1712efc302..b08014083a8ccb 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_common.bzl +++ b/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, diff --git a/src/main/starlark/builtins_bzl/common/proto/proto_info.bzl b/src/main/starlark/builtins_bzl/common/proto/proto_info.bzl index 864233ca6a44c4..56d8917eb95c19 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_info.bzl +++ b/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. diff --git a/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl b/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl index 47b4a243f0ef72..200d33ed78a73b 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl +++ b/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.""" diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommonTest.java index e4e1beb7414c16..ac53d3d26b2d1d 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommonTest.java +++ b/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,7 +453,7 @@ 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']}", @@ -459,14 +461,14 @@ public void generateCode_inDirectGeneratedProtos() throws Exception { + " ['--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 expectedFlags) diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java index 45b88e62e34fcd..ed3453e1f53666 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java +++ b/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); diff --git a/src/test/shell/bazel/bazel_proto_library_test.sh b/src/test/shell/bazel/bazel_proto_library_test.sh index 094c12d3b8dc2a..468c95f9193a45 100755 --- a/src/test/shell/bazel/bazel_proto_library_test.sh +++ b/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 <& $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 < e/f/BUILD < e/f/good/f.proto < e/f/good/gensrc.txt < 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 < h/BUILD < h/h.proto <& $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"