From 2a44dbd2b780e6f8b4b1e17881127247fc9b1ab3 Mon Sep 17 00:00:00 2001 From: ilist Date: Wed, 17 Nov 2021 05:34:04 -0800 Subject: [PATCH] Implement proto_common.write_descriptor_set in Starlark. The semantics of empty descriptor set is changed, as it doesn't depend on other descriptor sets any longer. This kind of dependencies would be hard to observe by integrations and testing showed there are no such integrations. PiperOrigin-RevId: 410501409 --- .../devtools/build/lib/rules/proto/BUILD | 1 + .../lib/rules/proto/BazelProtoCommon.java | 23 ----- .../build/lib/rules/proto/ProtoCommon.java | 15 ++++ .../lib/rules/proto/ProtoConfiguration.java | 25 ++++++ .../build/lib/rules/proto/ProtoInfo.java | 14 +++ .../build/lib/rules/proto/ProtoSource.java | 24 ++++- .../lib/starlarkbuildapi/ProtoInfoApi.java | 8 ++ .../common/proto/proto_common.bzl | 87 +++++++++++++++++++ .../common/proto/proto_library.bzl | 5 +- .../common/proto/proto_semantics.bzl | 10 +++ .../rules/proto/BazelProtoLibraryTest.java | 2 + 11 files changed, 188 insertions(+), 26 deletions(-) create mode 100644 src/main/starlark/builtins_bzl/common/proto/proto_common.bzl diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/BUILD b/src/main/java/com/google/devtools/build/lib/rules/proto/BUILD index 510edafb3786a2..2b162382c54ade 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/BUILD +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/BUILD @@ -22,6 +22,7 @@ java_library( ["*.java"], ), deps = [ + "//src/main/java/com/google/devtools/build/docgen/annot", "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommon.java b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommon.java index 62114d7cdc2ce9..4eb0511f70e59e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommon.java @@ -17,7 +17,6 @@ import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.BazelModuleContext; -import com.google.devtools.build.lib.rules.proto.ProtoCompileActionBuilder.Services; import com.google.devtools.build.lib.starlarkbuildapi.proto.ProtoCommonApi; import javax.annotation.Nullable; import net.starlark.java.annot.Param; @@ -56,26 +55,4 @@ public ProtoInfo createProtoInfo(StarlarkRuleContext ruleContext, StarlarkThread .getFragment(ProtoConfiguration.class) .generatedProtosInVirtualImports()); } - - @StarlarkMethod( - name = "write_descriptor_set", - documented = false, - parameters = { - @Param(name = "ctx", doc = "The rule context"), - @Param(name = "proto_info", doc = "The ProtoInfo") - }, - useStarlarkThread = true) - public void writeDescriptorSet( - StarlarkRuleContext ruleContext, ProtoInfo protoInfo, StarlarkThread thread) - throws EvalException { - Label label = - ((BazelModuleContext) Module.ofInnermostEnclosingStarlarkFunction(thread).getClientData()) - .label(); - if (!label.getPackageIdentifier().getRepository().toString().equals("@_builtins")) { - throw Starlark.errorf("Rule in '%s' cannot use private API", label.getPackageName()); - } - - ProtoCompileActionBuilder.writeDescriptorSet( - ruleContext.getRuleContext(), protoInfo, Services.ALLOW); - } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java index 8be9fe656a9a98..6b991ebc07b2be 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java @@ -31,11 +31,16 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.concurrent.BlazeInterners; +import com.google.devtools.build.lib.packages.BazelModuleContext; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; import javax.annotation.Nullable; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Module; +import net.starlark.java.eval.Starlark; +import net.starlark.java.eval.StarlarkThread; /** * Utility functions for proto_library and proto aspect implementations. @@ -519,4 +524,14 @@ public static boolean areDepsStrict(RuleContext ruleContext) { StrictDepsMode getBool = ruleContext.getFragment(ProtoConfiguration.class).strictProtoDeps(); return getBool != StrictDepsMode.OFF && getBool != StrictDepsMode.DEFAULT; } + + public static void checkPrivateStarlarkificationAllowlist(StarlarkThread thread) + throws EvalException { + Label label = + ((BazelModuleContext) Module.ofInnermostEnclosingStarlarkFunction(thread).getClientData()) + .label(); + if (!label.getPackageIdentifier().getRepository().toString().equals("@_builtins")) { + throw Starlark.errorf("Rule in '%s' cannot use private API", label.getPackageName()); + } + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java index 7b1e2c018bb67a..8b508f737cccb1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java @@ -31,6 +31,9 @@ import com.google.devtools.common.options.OptionEffectTag; import com.google.devtools.common.options.OptionMetadataTag; import java.util.List; +import net.starlark.java.annot.StarlarkMethod; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.StarlarkThread; /** Configuration for Protocol Buffer Libraries. */ @Immutable @@ -203,10 +206,26 @@ public ProtoConfiguration(BuildOptions buildOptions) { this.options = options; } + @StarlarkMethod(name = "protoc_opts", useStarlarkThread = true, documented = false) + public ImmutableList protocOptsForStarlark(StarlarkThread thread) throws EvalException { + ProtoCommon.checkPrivateStarlarkificationAllowlist(thread); + return protocOpts(); + } + public ImmutableList protocOpts() { return protocOpts; } + @StarlarkMethod( + name = "experimental_proto_descriptorsets_include_source_info", + useStarlarkThread = true, + documented = false) + public boolean experimentalProtoDescriptorSetsIncludeSourceInfoForStarlark(StarlarkThread thread) + throws EvalException { + ProtoCommon.checkPrivateStarlarkificationAllowlist(thread); + return experimentalProtoDescriptorSetsIncludeSourceInfo(); + } + public boolean experimentalProtoDescriptorSetsIncludeSourceInfo() { return options.experimentalProtoDescriptorSetsIncludeSourceInfo; } @@ -244,6 +263,12 @@ public Label protoToolchainForCc() { return options.protoToolchainForCc; } + @StarlarkMethod(name = "strict_proto_deps", useStarlarkThread = true, documented = false) + public String strictProtoDepsForStarlark(StarlarkThread thread) throws EvalException { + ProtoCommon.checkPrivateStarlarkificationAllowlist(thread); + return strictProtoDeps().toString(); + } + public StrictDepsMode strictProtoDeps() { return options.strictProtoDeps; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java index 22dfef525b535b..742c8447288e66 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java @@ -24,6 +24,8 @@ import com.google.devtools.build.lib.starlarkbuildapi.ProtoInfoApi; import com.google.devtools.build.lib.starlarkbuildapi.proto.ProtoBootstrap; import com.google.devtools.build.lib.vfs.PathFragment; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.StarlarkThread; /** * Configured target classes that implement this class can contribute .proto files to the @@ -128,6 +130,12 @@ public String getDirectProtoSourceRoot() { return Depset.of(Artifact.TYPE, getTransitiveProtoSources()); } + @Override + public Depset getTransitiveSourcesForStarlark(StarlarkThread thread) throws EvalException { + ProtoCommon.checkPrivateStarlarkificationAllowlist(thread); + return Depset.of(ProtoSource.TYPE, transitiveSources); + } + /** * The {@code .proto} source files in this {@code proto_library}'s {@code srcs} and all of its * transitive dependencies. @@ -217,6 +225,12 @@ public NestedSet getStrictImportableSources() { return strictImportableSources; } + @Override + public Depset getStrictImportableSourcesForStarlark(StarlarkThread thread) throws EvalException { + ProtoCommon.checkPrivateStarlarkificationAllowlist(thread); + return Depset.of(ProtoSource.TYPE, strictImportableSources); + } + /** * Returns a set of {@code .proto} sources that may be re-exported by this {@code proto_library}'s * direct sources. diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoSource.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoSource.java index 754c37521a6fc7..c48de52fa4eddb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoSource.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoSource.java @@ -14,15 +14,25 @@ package com.google.devtools.build.lib.rules.proto; +import com.google.devtools.build.docgen.annot.DocCategory; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.collect.nestedset.Depset; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.vfs.PathFragment; +import net.starlark.java.annot.StarlarkBuiltin; +import net.starlark.java.annot.StarlarkMethod; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.StarlarkThread; +import net.starlark.java.eval.StarlarkValue; /** Represents a single {@code .proto} source file. */ @Immutable @AutoCodec -class ProtoSource { +@StarlarkBuiltin(name = "ProtoSource", category = DocCategory.BUILTIN, documented = false) +class ProtoSource implements StarlarkValue { + public static final Depset.ElementType TYPE = Depset.ElementType.of(ProtoSource.class); + private final Artifact sourceFile; private final Artifact originalSourceFile; private final PathFragment sourceRoot; @@ -42,6 +52,12 @@ public Artifact getSourceFile() { return sourceFile; } + @StarlarkMethod(name = "source_file", documented = false, useStarlarkThread = true) + public Artifact getSourceFileForStarlark(StarlarkThread thread) throws EvalException { + ProtoCommon.checkPrivateStarlarkificationAllowlist(thread); + return sourceFile; + } + /** Returns the original source file. Only for forbidding protos! */ @Deprecated Artifact getOriginalSourceFile() { @@ -52,6 +68,12 @@ public PathFragment getSourceRoot() { return sourceRoot; } + @StarlarkMethod(name = "import_path", documented = false, useStarlarkThread = true) + public String getImportPathForStarlark(StarlarkThread thread) throws EvalException { + ProtoCommon.checkPrivateStarlarkificationAllowlist(thread); + return getImportPath().getPathString(); + } + public PathFragment getImportPath() { return sourceFile.getExecPath().relativeTo(sourceRoot); } diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/ProtoInfoApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/ProtoInfoApi.java index ce1fad68e96a5d..ea7214f3251db3 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/ProtoInfoApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/ProtoInfoApi.java @@ -21,6 +21,8 @@ import com.google.devtools.build.lib.starlarkbuildapi.core.StructApi; import net.starlark.java.annot.StarlarkBuiltin; import net.starlark.java.annot.StarlarkMethod; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.StarlarkThread; /** Info object propagating information about protocol buffer sources. */ @StarlarkBuiltin( @@ -105,4 +107,10 @@ interface ProtoInfoProviderApi extends ProviderApi { + " as a source, that source file would be imported as 'import c/d.proto'", structField = true) String getDirectProtoSourceRoot(); + + @StarlarkMethod(name = "strict_importable_sources", documented = false, useStarlarkThread = true) + Depset getStrictImportableSourcesForStarlark(StarlarkThread thread) throws EvalException; + + @StarlarkMethod(name = "transitive_proto_sources", documented = false, useStarlarkThread = true) + Depset getTransitiveSourcesForStarlark(StarlarkThread thread) throws EvalException; } diff --git a/src/main/starlark/builtins_bzl/common/proto/proto_common.bzl b/src/main/starlark/builtins_bzl/common/proto/proto_common.bzl new file mode 100644 index 00000000000000..be24c4c463be16 --- /dev/null +++ b/src/main/starlark/builtins_bzl/common/proto/proto_common.bzl @@ -0,0 +1,87 @@ +# Copyright 2021 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +""" +Definition of proto_common module. +""" + +load(":common/proto/proto_semantics.bzl", "semantics") + +ProtoInfo = _builtins.toplevel.ProtoInfo + +def _write_descriptor_set(ctx, proto_info): + output = proto_info.direct_descriptor_set + proto_deps = [dep[ProtoInfo] for dep in ctx.attr.deps] + dependencies_descriptor_sets = depset(transitive = [dep.transitive_descriptor_sets for dep in proto_deps]) + + if proto_info.direct_sources == []: + ctx.actions.write(output, "") + return + + args = ctx.actions.args() + args.use_param_file(param_file_arg = "@%s") + args.set_param_file_format("multiline") + args.add_all(proto_info.transitive_proto_path, map_each = _EXPAND_TRANSITIVE_PROTO_PATH_FLAGS) + args.add(output, format = "--descriptor_set_out=%s") + + if ctx.fragments.proto.experimental_proto_descriptorsets_include_source_info(): + args.add("--include_source_info") + args.add_all(ctx.fragments.proto.protoc_opts()) + + strict_deps_mode = ctx.fragments.proto.strict_proto_deps() + are_deps_strict = strict_deps_mode != "OFF" and strict_deps_mode != "DEFAULT" + + # Include maps + + # For each import, include both the import as well as the import relativized against its + # protoSourceRoot. This ensures that protos can reference either the full path or the short + # path when including other protos. + args.add_all(proto_info.transitive_proto_sources(), map_each = _ExpandImportArgsFn) + if are_deps_strict: + strict_importable_sources = proto_info.strict_importable_sources() + if strict_importable_sources: + args.add_joined("--direct_dependencies", strict_importable_sources, map_each = _EXPAND_TO_IMPORT_PATHS, join_with = ":") + else: + # The proto compiler requires an empty list to turn on strict deps checking + args.add("--direct_dependencies=") + + args.add(ctx.label, format = semantics.STRICT_DEPS_FLAG_TEMPLATE) + + # use exports + + args.add_all(proto_info.direct_sources) + + ctx.actions.run( + mnemonic = "GenProtoDescriptorSet", + progress_message = "Generating Descriptor Set proto_library %{label}", + executable = ctx.executable._proto_compiler, + arguments = [args], + inputs = depset(transitive = [dependencies_descriptor_sets, proto_info.transitive_sources]), + outputs = [output], + ) + +def _EXPAND_TRANSITIVE_PROTO_PATH_FLAGS(flag): + if flag == ".": + return None + return "--proto_path=" + flag + +def _EXPAND_TO_IMPORT_PATHS(proto_source): + return proto_source.import_path() + +def _ExpandImportArgsFn(proto_source): + return "-I%s=%s" % (proto_source.import_path(), proto_source.source_file().path) + +proto_common = struct( + write_descriptor_set = _write_descriptor_set, +) 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 4fb17a5a5af2e4..731a7a61fcf04d 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl +++ b/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl @@ -17,9 +17,10 @@ Definition of proto_library rule. """ load(":common/proto/proto_semantics.bzl", "semantics") +load(":common/proto/proto_common.bzl", "proto_common") ProtoInfo = _builtins.toplevel.ProtoInfo -proto_common = _builtins.toplevel.proto_common +native_proto_common = _builtins.toplevel.proto_common def _check_srcs_package(target_package, srcs): """Makes sure the given srcs live in the given package.""" @@ -32,7 +33,7 @@ def _proto_library_impl(ctx): _check_srcs_package(ctx.label.package, ctx.attr.srcs) - proto_info = proto_common.create_proto_info(ctx) + proto_info = native_proto_common.create_proto_info(ctx) proto_common.write_descriptor_set(ctx, proto_info) diff --git a/src/main/starlark/builtins_bzl/common/proto/proto_semantics.bzl b/src/main/starlark/builtins_bzl/common/proto/proto_semantics.bzl index 0cdddcb9b907a6..ea1566106a5d8b 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_semantics.bzl +++ b/src/main/starlark/builtins_bzl/common/proto/proto_semantics.bzl @@ -26,4 +26,14 @@ semantics = struct( }, EXTRA_FRAGMENTS = [], preprocess = _preprocess, + # This constant is used in ProtoCompileActionBuilder to generate an error message that's + # displayed when a strict proto deps violation occurs. + # + # %s is replaced with the label of the proto_library rule that's currently being built. + # %%s is replaced with the literal "%s", which is passed to the proto-compiler, which replaces + # it with the .proto file that violates strict proto deps. + STRICT_DEPS_FLAG_TEMPLATE = ( + "--direct_dependencies_violation_msg=" + + "%%s is imported, but %s doesn't directly depend on a proto_library that 'srcs' it." + ), ) \ No newline at end of file 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 8fa997e625747e..a293f0be0b0596 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 @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.testutil.TestConstants; import com.google.devtools.build.lib.vfs.FileSystemUtils; import org.junit.Before; +import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -102,6 +103,7 @@ public void descriptorSets_ruleWithoutSrcsWritesEmptyFile() throws Exception { * deps, would break. */ @Test + @Ignore("b/204266604 Remove if the testing shows it's not needed.") public void descriptorSetsDependOnChildren() throws Exception { scratch.file( "x/BUILD",