From cbad324d2c8520195d2ddafcabeb2d2ca96da0a1 Mon Sep 17 00:00:00 2001 From: ilist Date: Wed, 24 Nov 2021 00:53:08 -0800 Subject: [PATCH] Clean up proto_library.bzl implementation. PiperOrigin-RevId: 411986102 --- .../common/proto/proto_library.bzl | 187 +++++++++++------- .../rules/proto/BazelProtoLibraryTest.java | 38 +++- 2 files changed, 145 insertions(+), 80 deletions(-) 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 01ea4f69f932a2..03345ac26902c9 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl +++ b/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl @@ -24,73 +24,146 @@ ProtoInfo = _builtins.toplevel.ProtoInfo native_proto_common = _builtins.toplevel.proto_common def _check_srcs_package(target_package, srcs): - """Makes sure the given srcs live in the given package.""" + """Check that .proto files in sources are from the same package. + + This is done to avoid clashes with the generated sources.""" + + #TODO(bazel-team): this does not work with filegroups that contain files that are not in the package for src in srcs: if target_package != src.label.package: fail("Proto source with label '%s' must be in same package as consuming rule." % src.label) -def _join(*path): - return "/".join([p for p in path if p != ""]) - -def _create_proto_info(ctx): - srcs = ctx.files.srcs - deps = [dep[ProtoInfo] for dep in ctx.attr.deps] - exports = [dep[ProtoInfo] for dep in ctx.attr.exports] +def _get_import_prefix(ctx): + """Gets and verifies import_prefix attribute if it is declared.""" import_prefix = ctx.attr.import_prefix if hasattr(ctx.attr, "import_prefix") else "" + if not paths.is_normalized(import_prefix): fail("should be normalized (without uplevel references or '.' path segments)", attr = "import_prefix") + if paths.is_absolute(import_prefix): + fail("should be a relative path", attr = "import_prefix") + + return import_prefix + +def _get_strip_import_prefix(ctx): + """Gets and verifies strip_import_prefix.""" strip_import_prefix = ctx.attr.strip_import_prefix + if not paths.is_normalized(strip_import_prefix): fail("should be normalized (without uplevel references or '.' path segments)", attr = "strip_import_prefix") - if strip_import_prefix.startswith("/"): + + if paths.is_absolute(strip_import_prefix): strip_import_prefix = strip_import_prefix[1:] elif strip_import_prefix != "DO_NOT_STRIP": # Relative to current package strip_import_prefix = _join(ctx.label.package, strip_import_prefix) else: strip_import_prefix = "" - has_generated_sources = False - if ctx.fragments.proto.generated_protos_in_virtual_imports(): - has_generated_sources = any([not src.is_source for src in srcs]) + return strip_import_prefix - direct_sources = [] - if import_prefix != "" or strip_import_prefix != "" or has_generated_sources: - # Use virtual source roots - if paths.is_absolute(import_prefix): - fail("should be a relative path", attr = "import_prefix") +def _proto_library_impl(ctx): + semantics.preprocess(ctx) - virtual_imports = _join("_virtual_imports", ctx.label.name) - if ctx.label.workspace_name == "" or ctx.label.workspace_root.startswith(".."): # siblingRepositoryLayout - proto_path = _join(ctx.genfiles_dir.path, ctx.label.package, virtual_imports) - else: - proto_path = _join(ctx.genfiles_dir.path, ctx.label.workspace_root, ctx.label.package, virtual_imports) + # Verifies attributes. + _check_srcs_package(ctx.label.package, ctx.attr.srcs) + srcs = ctx.files.srcs + deps = [dep[ProtoInfo] for dep in ctx.attr.deps] + exports = [dep[ProtoInfo] for dep in ctx.attr.exports] + import_prefix = _get_import_prefix(ctx) + strip_import_prefix = _get_strip_import_prefix(ctx) - for src in srcs: - if ctx.label.workspace_name == "": - repository_relative_path = src.short_path - else: - repository_relative_path = paths.relativize(src.short_path, "../" + ctx.label.workspace_name) + 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(ctx, direct_sources, deps, exports, proto_path, descriptor_set) + _write_descriptor_set(ctx, deps, proto_info, descriptor_set) + + # We assume that the proto sources will not have conflicting artifacts + # with the same root relative path + data_runfiles = ctx.runfiles( + files = [proto_info.direct_descriptor_set], + transitive_files = depset(transitive = [proto_info.transitive_sources]), + ) + return [ + proto_info, + DefaultInfo( + files = depset([proto_info.direct_descriptor_set]), + default_runfiles = ctx.runfiles(), # empty + data_runfiles = data_runfiles, + ), + ] - if not repository_relative_path.startswith(strip_import_prefix): - fail(".proto file '%s' is not under the specified strip prefix '%s'" % - (src.short_path, strip_import_prefix)) - import_path = repository_relative_path[len(strip_import_prefix):] +def _create_proto_sources(ctx, srcs, import_prefix, strip_import_prefix): + """Transforms Files in srcs to ProtoSources, optionally symlinking them to _virtual_imports. - virtual_src = ctx.actions.declare_file(_join(virtual_imports, import_prefix, import_path)) - ctx.actions.symlink( - output = virtual_src, - target_file = src, - progress_message = "Symlinking virtual .proto sources for %{label}", - ) - direct_sources.append(native_proto_common.ProtoSource(virtual_src, src, proto_path)) + Returns: + A pair proto_path, directs_sources. + """ + generate_protos_in_virtual_imports = False + if ctx.fragments.proto.generated_protos_in_virtual_imports(): + generate_protos_in_virtual_imports = any([not src.is_source for src in srcs]) + if import_prefix != "" or strip_import_prefix != "" or generate_protos_in_virtual_imports: + # Use virtual source roots + return _symlink_to_virtual_imports(ctx, srcs, import_prefix, strip_import_prefix) else: # No virtual source roots - proto_path = "." + direct_sources = [] for src in srcs: - direct_sources.append(native_proto_common.ProtoSource(src, src, ctx.label.workspace_root + src.root.path)) + 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(native_proto_common.ProtoSource(src, src, source_root)) + + return ctx.label.workspace_root if ctx.label.workspace_root else ".", direct_sources + +def _join(*path): + return "/".join([p for p in path if p != ""]) + +def _symlink_to_virtual_imports(ctx, srcs, import_prefix, strip_import_prefix): + """Symlinks srcs to _virtual_imports. + + Returns: + 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) + + direct_sources = [] + for src in srcs: + if ctx.label.workspace_name == "": + repository_relative_path = src.short_path + else: + # src.short_path = ../repo/pkg/a.proto + repository_relative_path = paths.relativize(src.short_path, "../" + ctx.label.workspace_name) + + # Remove strip_import_prefix + if not repository_relative_path.startswith(strip_import_prefix): + fail(".proto file '%s' is not under the specified strip prefix '%s'" % + (src.short_path, strip_import_prefix)) + import_path = repository_relative_path[len(strip_import_prefix):] + + # Add import_prefix + virtual_src = ctx.actions.declare_file(_join(virtual_imports, import_prefix, import_path)) + + ctx.actions.symlink( + output = virtual_src, + target_file = src, + progress_message = "Symlinking virtual .proto sources for %{label}", + ) + direct_sources.append(native_proto_common.ProtoSource(virtual_src, src, proto_path)) + return proto_path, direct_sources + +def _create_proto_info(ctx, direct_sources, deps, exports, proto_path, descriptor_set): + """Constructs ProtoInfo.""" # Construct ProtoInfo transitive_proto_sources = depset( @@ -112,9 +185,8 @@ def _create_proto_info(ctx): else: check_deps_sources = depset(transitive = [dep.check_deps_sources for dep in deps]) - direct_descriptor_set = ctx.actions.declare_file(ctx.label.name + "-descriptor-set.proto.bin") transitive_descriptor_sets = depset( - direct = [direct_descriptor_set], + direct = [descriptor_set], transitive = [dep.transitive_descriptor_sets for dep in deps], ) @@ -137,21 +209,19 @@ def _create_proto_info(ctx): transitive_proto_sources, transitive_proto_path, check_deps_sources, - direct_descriptor_set, + descriptor_set, transitive_descriptor_sets, exported_sources, strict_importable_sources, public_import_protos, ) -def _write_descriptor_set(ctx, proto_info): - descriptor_set = proto_info.direct_descriptor_set - +def _write_descriptor_set(ctx, deps, proto_info, descriptor_set): + """Writes descriptor set.""" if proto_info.direct_sources == []: ctx.actions.write(descriptor_set, "") return - deps = [dep[ProtoInfo] for dep in ctx.attr.deps] dependencies_descriptor_sets = depset(transitive = [dep.transitive_descriptor_sets for dep in deps]) args = [] @@ -170,29 +240,6 @@ def _write_descriptor_set(ctx, proto_info): additional_args = args, ) -def _proto_library_impl(ctx): - semantics.preprocess(ctx) - - _check_srcs_package(ctx.label.package, ctx.attr.srcs) - - proto_info = _create_proto_info(ctx) - - _write_descriptor_set(ctx, proto_info) - - data_runfiles = ctx.runfiles( - files = [proto_info.direct_descriptor_set], - transitive_files = depset(transitive = [proto_info.transitive_sources]), - ) - - return [ - proto_info, - DefaultInfo( - files = depset([proto_info.direct_descriptor_set]), - default_runfiles = ctx.runfiles(), # empty - data_runfiles = data_runfiles, - ), - ] - proto_library = rule( _proto_library_impl, attrs = dict({ 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 a293f0be0b0596..a55418ae8ae4ee 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 @@ -351,7 +351,8 @@ public void testStripImportPrefixWithDeps() throws Exception { "."); } - private void testExternalRepoWithGeneratedProto(boolean siblingRepoLayout) throws Exception { + private void testExternalRepoWithGeneratedProto( + boolean siblingRepoLayout, boolean useVirtualImports) throws Exception { if (!isThisBazel()) { return; } @@ -361,6 +362,9 @@ private void testExternalRepoWithGeneratedProto(boolean siblingRepoLayout) throw if (siblingRepoLayout) { setBuildLanguageOptions("--experimental_sibling_repository_layout"); } + if (!useVirtualImports) { + useConfiguration("--noincompatible_generated_protos_in_virtual_imports"); + } invalidatePackages(); scratch.file("/foo/WORKSPACE"); @@ -369,7 +373,6 @@ private void testExternalRepoWithGeneratedProto(boolean siblingRepoLayout) throw TestConstants.LOAD_PROTO_LIBRARY, "proto_library(name='x', srcs=['generated.proto'])", "genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')"); - scratch.file( "a/BUILD", TestConstants.LOAD_PROTO_LIBRARY, @@ -380,27 +383,42 @@ private void testExternalRepoWithGeneratedProto(boolean siblingRepoLayout) throw .getGenfilesFragment( siblingRepoLayout ? RepositoryName.create("@foo") : RepositoryName.MAIN) .toString(); + String fooProtoRoot; + if (useVirtualImports) { + fooProtoRoot = + genfiles + (siblingRepoLayout ? "" : "/external/foo") + "/x/_virtual_imports/x"; + } else { + fooProtoRoot = (siblingRepoLayout ? "../foo" : "external/foo"); + } ConfiguredTarget a = getConfiguredTarget("//a:a"); ProtoInfo aInfo = a.get(ProtoInfo.PROVIDER); - assertThat(aInfo.getTransitiveProtoSourceRoots().toList()) - .containsExactly( - ".", genfiles + (siblingRepoLayout ? "" : "/external/foo") + "/x/_virtual_imports/x"); + assertThat(aInfo.getTransitiveProtoSourceRoots().toList()).containsExactly(".", fooProtoRoot); ConfiguredTarget x = getConfiguredTarget("@foo//x:x"); ProtoInfo xInfo = x.get(ProtoInfo.PROVIDER); - assertThat(xInfo.getTransitiveProtoSourceRoots().toList()) - .containsExactly( - genfiles + (siblingRepoLayout ? "" : "/external/foo") + "/x/_virtual_imports/x"); + assertThat(xInfo.getTransitiveProtoSourceRoots().toList()).containsExactly(fooProtoRoot); } @Test public void testExternalRepoWithGeneratedProto_withSubdirRepoLayout() throws Exception { - testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ false); + testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ false, true); } @Test public void test_siblingRepoLayout_externalRepoWithGeneratedProto() throws Exception { - testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ true); + testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ true, true); + } + + @Test + public void testExternalRepoWithGeneratedProto_withSubdirRepoLayoutAndNoVritualImports() + throws Exception { + testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ false, false); + } + + @Test + public void test_siblingRepoLayout_externalRepoWithGeneratedProtoAndNoVritualImports() + throws Exception { + testExternalRepoWithGeneratedProto(/*siblingRepoLayout=*/ true, false); } @Test