From 138a1cb449794801eb88b2697dcfd2b48ec54078 Mon Sep 17 00:00:00 2001 From: ilist Date: Mon, 7 Feb 2022 23:34:09 -0800 Subject: [PATCH] Accept args object in create_proto_compile_action. This is a better design than type-checking list elements. Only minor reordering of arguments is happening - additional parameters are added before `--proto_path`. Additional parameters are usually outputs related (--descriptor_set_out, --java_out). Callee can decide if additional parameters are put into a params file or not. Native calles don't at the moment. Alternative implementation would reuse additional_parameters, appending to it and creating a single params file. Not using params file for additional parameters and using it for the standard ones seems like the best choice for the sake of caching (params file matches for proto_library and lang_proto_libraries). PiperOrigin-RevId: 427112289 --- .../devtools/build/lib/rules/proto/BUILD | 7 +- .../proto/ProtoCompileActionBuilder.java | 142 ++++++++++++------ .../common/proto/proto_common.bzl | 22 +-- .../common/proto/proto_library.bzl | 6 +- .../lib/analysis/util/BuildViewTestCase.java | 10 ++ .../rules/proto/BazelProtoLibraryTest.java | 4 +- .../proto/ProtoCompileActionBuilderTest.java | 50 +++--- 7 files changed, 148 insertions(+), 93 deletions(-) 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 bb755028624e86..338e62e15f1b19 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 @@ -25,10 +25,7 @@ java_library( "//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", "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", - "//src/main/java/com/google/devtools/build/lib/analysis:actions/custom_command_line", - "//src/main/java/com/google/devtools/build/lib/analysis:actions/symlink_action", "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster", "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options", "//src/main/java/com/google/devtools/build/lib/analysis:config/core_option_converters", @@ -37,6 +34,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment", + "//src/main/java/com/google/devtools/build/lib/analysis:starlark/args", "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection", "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider", "//src/main/java/com/google/devtools/build/lib/analysis/starlark/annotations", @@ -44,11 +42,8 @@ java_library( "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/packages", - "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", - "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi", "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/proto", - "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:filetype", "//src/main/java/com/google/devtools/build/lib/util:os", "//src/main/java/com/google/devtools/build/lib/vfs", diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java index 41e22eba5ba3c1..6436d63110ca75 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.actions.ResourceSetOrBuilder; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.RuleContext; +import com.google.devtools.build.lib.analysis.starlark.Args; import com.google.devtools.build.lib.collect.nestedset.Depset; import com.google.devtools.build.lib.collect.nestedset.Depset.ElementType; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; @@ -34,6 +35,8 @@ import java.util.HashSet; import java.util.List; import net.starlark.java.eval.Dict; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkCallable; import net.starlark.java.eval.StarlarkFloat; import net.starlark.java.eval.StarlarkFunction; @@ -138,24 +141,57 @@ public void maybeRegister(RuleContext ruleContext) return; } - ImmutableList.Builder additionalArgs = ImmutableList.builder(); - - if (langPlugin != null && langPlugin.getExecutable() != null) { - // We pass a separate langPlugin as there are plugins that cannot be overridden - // and thus we have to deal with "$xx_plugin" and "xx_plugin". - additionalArgs.add(Tuple.of(langPlugin.getExecutable(), langPluginFormat)); - } + ruleContext.initStarlarkRuleContext(); + StarlarkThread thread = ruleContext.getStarlarkThread(); + Args additionalArgs = Args.newArgs(thread.mutability(), thread.getSemantics()); + + try { + if (langPlugin != null && langPlugin.getExecutable() != null) { + // We pass a separate langPlugin as there are plugins that cannot be overridden + // and thus we have to deal with "$xx_plugin" and "xx_plugin". + additionalArgs.addArgument( + langPlugin.getExecutable(), /*value=*/ Starlark.UNBOUND, langPluginFormat, thread); + } - if (langPluginParameter != null) { - additionalArgs.add(Tuple.of(langPluginParameter, langPluginParameterFormat)); - } + if (langPluginParameter != null) { + additionalArgs.addJoined( + StarlarkList.immutableCopyOf(langPluginParameter), + /*values=*/ Starlark.UNBOUND, + /*joinWith=*/ "", + /*mapEach=*/ Starlark.NONE, + /*formatEach=*/ Starlark.NONE, + /*formatJoined=*/ langPluginParameterFormat, + /*omitIfEmpty=*/ true, + /*uniquify=*/ false, + /*expandDirectories=*/ true, + /*allowClosure=*/ false, + thread); + } - if (!hasServices) { - additionalArgs.add("--disallow_services"); - } + if (!hasServices) { + additionalArgs.addArgument( + "--disallow_services", + /* value = */ Starlark.UNBOUND, + /* format = */ Starlark.NONE, + thread); + } - if (additionalCommandLineArguments != null) { - additionalArgs.addAll(additionalCommandLineArguments); + if (additionalCommandLineArguments != null) { + additionalArgs.addAll( + StarlarkList.immutableCopyOf(additionalCommandLineArguments), + /*values=*/ Starlark.UNBOUND, + /*mapEach=*/ Starlark.NONE, + /*formatEach=*/ Starlark.NONE, + /*beforeEach=*/ Starlark.NONE, + /*omitIfEmpty=*/ true, + /*uniquify=*/ false, + /*expandDirectories=*/ true, + /*terminateWith=*/ Starlark.NONE, + /*allowClosure=*/ false, + thread); + } + } catch (EvalException e) { + throw ruleContext.throwWithRuleError(e); } ImmutableList.Builder plugins = new ImmutableList.Builder<>(); @@ -168,7 +204,7 @@ public void maybeRegister(RuleContext ruleContext) StarlarkFunction createProtoCompileAction = (StarlarkFunction) ruleContext.getStarlarkDefinedBuiltin("create_proto_compile_action"); - ruleContext.initStarlarkRuleContext(); + ruleContext.callStarlarkOrThrowRuleError( createProtoCompileAction, ImmutableList.of( @@ -177,7 +213,7 @@ public void maybeRegister(RuleContext ruleContext) /* proto_compiler */ protoCompiler, /* progress_message */ progressMessage, /* outputs */ StarlarkList.immutableCopyOf(outputs), - /* additional_args */ StarlarkList.immutableCopyOf(additionalArgs.build()), + /* additional_args */ additionalArgs, /* plugins */ StarlarkList.immutableCopyOf(plugins.build()), /* mnemonic */ mnemonic, /* strict_imports */ checkStrictImportPublic, @@ -246,41 +282,63 @@ public static void registerActions( return; } + ruleContext.initStarlarkRuleContext(); + StarlarkThread thread = ruleContext.getStarlarkThread(); + Args additionalArgs = Args.newArgs(thread.mutability(), thread.getSemantics()); + // A set to check if there are multiple invocations with the same name. HashSet invocationNames = new HashSet<>(); - ImmutableList.Builder additionalArgs = ImmutableList.builder(); ImmutableList.Builder plugins = ImmutableList.builder(); - for (ToolchainInvocation invocation : toolchainInvocations) { - if (!invocationNames.add(invocation.name)) { - throw new IllegalStateException( - "Invocation name " - + invocation.name - + " appears more than once. " - + "This could lead to incorrect proto-compiler behavior"); + try { + for (ToolchainInvocation invocation : toolchainInvocations) { + if (!invocationNames.add(invocation.name)) { + throw new IllegalStateException( + "Invocation name " + + invocation.name + + " appears more than once. " + + "This could lead to incorrect proto-compiler behavior"); + } + + ProtoLangToolchainProvider toolchain = invocation.toolchain; + + String format = toolchain.outReplacementFormatFlag(); + additionalArgs.addArgument( + invocation.outReplacement, /*value=*/ Starlark.UNBOUND, format, thread); + + if (toolchain.pluginExecutable() != null) { + additionalArgs.addArgument( + toolchain.pluginExecutable().getExecutable(), + /*value=*/ Starlark.UNBOUND, + toolchain.pluginFormatFlag(), + thread); + plugins.add(toolchain.pluginExecutable()); + } + + additionalArgs.addJoined( + StarlarkList.immutableCopyOf(invocation.protocOpts), + /*values=*/ Starlark.UNBOUND, + /*joinWith=*/ "", + /*mapEach=*/ Starlark.NONE, + /*formatEach=*/ Starlark.NONE, + /*formatJoined=*/ Starlark.NONE, + /*omitIfEmpty=*/ true, + /*uniquify=*/ false, + /*expandDirectories=*/ true, + /*allowClosure=*/ false, + thread); } - ProtoLangToolchainProvider toolchain = invocation.toolchain; - - String format = toolchain.outReplacementFormatFlag(); - additionalArgs.add(Tuple.of(invocation.outReplacement, format)); - - if (toolchain.pluginExecutable() != null) { - additionalArgs.add( - Tuple.of(toolchain.pluginExecutable().getExecutable(), toolchain.pluginFormatFlag())); - plugins.add(toolchain.pluginExecutable()); + if (allowServices == Services.DISALLOW) { + additionalArgs.addArgument( + "--disallow_services", /*value=*/ Starlark.UNBOUND, /*format=*/ Starlark.NONE, thread); } - - additionalArgs.addAll(invocation.protocOpts); - } - - if (allowServices == Services.DISALLOW) { - additionalArgs.add("--disallow_services"); + } catch (EvalException e) { + throw ruleContext.throwWithRuleError(e.getMessageWithStack()); } StarlarkFunction createProtoCompileAction = (StarlarkFunction) ruleContext.getStarlarkDefinedBuiltin("create_proto_compile_action"); - ruleContext.initStarlarkRuleContext(); ruleContext.callStarlarkOrThrowRuleError( createProtoCompileAction, ImmutableList.of( @@ -289,7 +347,7 @@ public static void registerActions( /* proto_compiler */ protoToolchain.getCompiler(), /* progress_message */ progressMessage, /* outputs */ StarlarkList.immutableCopyOf(outputs), - /* additional_args */ StarlarkList.immutableCopyOf(additionalArgs.build()), + /* additional_args */ additionalArgs, /* plugins */ StarlarkList.immutableCopyOf(plugins.build())), ImmutableMap.of( "strict_imports", 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 fcb885e9a9c2d4..1a1e6f30ef57fb 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_common.bzl +++ b/src/main/starlark/builtins_bzl/common/proto/proto_common.bzl @@ -41,8 +41,9 @@ def _create_proto_compile_action( progress_message: The progress message to set on the action. outputs: The output files generated by the proto compiler. additional_args: Additional arguments to add to the action. - Accepts a list containing: a string, a pair of value and format string, - or a pair of list of strings and a format string. + Accepts an ctx.actions.args() object that is added at the beginning of command line. + Deprecated: Accepts a list containing: a string, a pair of value and + format string, or a pair of list of strings and a format string. plugins: Additional plugin executables used by proto compiler. mnemonic: The mnemonic to set on the action. strict_imports: Whether to check for strict imports. @@ -58,14 +59,15 @@ def _create_proto_compile_action( args.add_all(proto_info.transitive_proto_path, map_each = _proto_path_flag) # Example: `--proto_path=--proto_path=bazel-bin/target/third_party/pkg/_virtual_imports/subpkg` - for arg in additional_args: - if type(arg) == type((None, None)): - if type(arg[0]) == type([]): - args.add_joined(arg[0], join_with = "", format_joined = arg[1]) + if type(additional_args) == type([]): + for arg in additional_args: + if type(arg) == type((None, None)): + if type(arg[0]) == type([]): + args.add_joined(arg[0], join_with = "", format_joined = arg[1]) + else: + args.add(arg[0], format = arg[1]) else: - args.add(arg[0], format = arg[1]) - else: - args.add(arg) + args.add(arg) args.add_all(ctx.fragments.proto.protoc_opts()) @@ -104,7 +106,7 @@ def _create_proto_compile_action( mnemonic = mnemonic, progress_message = progress_message, executable = proto_compiler, - arguments = [args], + arguments = [additional_args, args] if type(additional_args) == type(ctx.actions.args()) else [args], inputs = depset(transitive = [proto_info.transitive_sources, additional_inputs]), outputs = outputs, tools = plugins, 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 03345ac26902c9..c1e9bbabd4c1ab 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl +++ b/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl @@ -224,10 +224,10 @@ def _write_descriptor_set(ctx, deps, proto_info, descriptor_set): dependencies_descriptor_sets = depset(transitive = [dep.transitive_descriptor_sets for dep in deps]) - args = [] + args = ctx.actions.args() if ctx.fragments.proto.experimental_proto_descriptorsets_include_source_info(): - args.append("--include_source_info") - args.append((descriptor_set, "--descriptor_set_out=%s")) + args.add("--include_source_info") + args.add(descriptor_set, format = "--descriptor_set_out=%s") proto_common.create_proto_compile_action( ctx, diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index 4f7519ba259ae3..28bf9edf1b3800 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -833,6 +833,16 @@ protected ActionGraph getActionGraph() { return skyframeExecutor.getActionGraph(reporter); } + /** Returns all arguments used by the action. */ + protected final ImmutableList allArgsForAction(SpawnAction action) throws Exception { + ImmutableList.Builder args = new ImmutableList.Builder<>(); + List commandLines = action.getCommandLines().getCommandLines(); + for (CommandLineAndParamFileInfo pair : commandLines.subList(1, commandLines.size())) { + args.addAll(pair.commandLine.arguments()); + } + return args.build(); + } + /** Locates the first parameter file used by the action and returns its command line. */ @Nullable protected final CommandLine paramFileCommandLineForAction(Action action) { 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 a55418ae8ae4ee..5addbf84fb410f 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 @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.actions.FileWriteAction; +import com.google.devtools.build.lib.analysis.actions.SpawnAction; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.util.MockProtoSupport; @@ -1025,7 +1026,8 @@ public void testExperimentalProtoDescriptorSetsIncludeSourceInfo() throws Except " srcs = ['a.proto'],", ")"); - Iterable commandLine = paramFileArgsForAction(getDescriptorWriteAction("//x:a_proto")); + Iterable commandLine = + allArgsForAction((SpawnAction) getDescriptorWriteAction("//x:a_proto")); assertThat(commandLine).contains("--include_source_info"); } diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java index e3bfe2d2fd355e..e137cb998e4342 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java @@ -24,7 +24,6 @@ import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; -import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.actions.util.LabelArtifactOwner; @@ -157,10 +156,9 @@ public void commandLine_basic() throws Exception { Exports.DO_NOT_USE, Services.ALLOW); - CommandLine cmdLine = - paramFileCommandLineForAction( - (SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); - assertThat(cmdLine.arguments()) + ImmutableList cmdLine = + allArgsForAction((SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); + assertThat(cmdLine) .containsExactly( "--java_out=param1,param2:foo.srcjar", "--PLUGIN_pluginName_out=param3,param4:bar.srcjar", @@ -192,10 +190,9 @@ public void commandline_derivedArtifact() throws Exception { Exports.DO_NOT_USE, Services.ALLOW); - CommandLine cmdLine = - paramFileCommandLineForAction( - (SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); - assertThat(cmdLine.arguments()).containsExactly("out/source_file.proto"); + ImmutableList cmdLine = + allArgsForAction((SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); + assertThat(cmdLine).containsExactly("out/source_file.proto"); } @Test @@ -224,10 +221,9 @@ public void commandLine_strictDeps() throws Exception { Exports.DO_NOT_USE, Services.ALLOW); - CommandLine cmdLine = - paramFileCommandLineForAction( - (SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); - assertThat(cmdLine.arguments()) + ImmutableList cmdLine = + allArgsForAction((SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); + assertThat(cmdLine) .containsExactly( "--java_out=param1,param2:foo.srcjar", "-Iimport1.proto=import1.proto", @@ -267,10 +263,9 @@ public void commandLine_exports() throws Exception { Exports.USE, Services.ALLOW); - CommandLine cmdLine = - paramFileCommandLineForAction( - (SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); - assertThat(cmdLine.arguments()) + ImmutableList cmdLine = + allArgsForAction((SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); + assertThat(cmdLine) .containsExactly( "--java_out=param1,param2:foo.srcjar", "-Iimport1.proto=import1.proto", @@ -300,10 +295,9 @@ public void otherParameters() throws Exception { Exports.DO_NOT_USE, Services.DISALLOW); - CommandLine cmdLine = - paramFileCommandLineForAction( - (SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); - assertThat(cmdLine.arguments()).containsAtLeast("--disallow_services", "--foo", "--bar"); + ImmutableList cmdLine = + allArgsForAction((SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); + assertThat(cmdLine).containsAtLeast("--disallow_services", "--foo", "--bar"); } private static class InterceptOnDemandString extends OnDemandString implements StarlarkValue { @@ -342,12 +336,9 @@ public void outReplacementAreLazilyEvaluated() throws Exception { Exports.DO_NOT_USE, Services.ALLOW); - CommandLine cmdLine = - paramFileCommandLineForAction( - (SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); assertThat(outReplacement.hasBeenCalled).isFalse(); - cmdLine.arguments(); + allArgsForAction((SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); assertThat(outReplacement.hasBeenCalled).isTrue(); } @@ -488,7 +479,7 @@ private Artifact derivedArtifact(String path) { return derivedArtifact; } - private Iterable protoArgv( + private ImmutableList protoArgv( Iterable transitiveSources, @Nullable Iterable importableProtoSources) throws Exception { @@ -509,11 +500,8 @@ private Iterable protoArgv( Exports.DO_NOT_USE, Services.DISALLOW); - CommandLine cmdLine = - paramFileCommandLineForAction( - (SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); - - return cmdLine.arguments(); + return allArgsForAction( + (SpawnAction) collectingAnalysisEnvironment.getRegisteredActions().get(0)); } @Test