diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java index 88e1a6fb3bf627..7d9310ba0742fe 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.Attribute; +import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.StarlarkProviderIdentifier; import com.google.devtools.build.lib.packages.Type; @@ -69,6 +70,16 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi */ .add(attr("command_line", Type.STRING).mandatory()) + /* + Controls how $(OUT) in command_line is formatted, either by + a path to a single file or output directory in case of multiple files. + Possible values are: "single", "multiple". + */ + .add( + attr("output_files", Type.STRING) + .allowedValues(new AllowedValueSet("single", "multiple", "legacy")) + .value("legacy")) + /* If provided, this value will be passed to proto-compiler to use the plugin. The value must contain a single %s which is replaced with plugin executable. diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_proto_library.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_proto_library.bzl index 52f43f866abf36..55036600f8ebbf 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_proto_library.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_proto_library.bzl @@ -48,25 +48,6 @@ def _check_proto_libraries_in_deps(deps): if ProtoInfo in dep and CcInfo not in dep: fail("proto_library '{}' does not produce output for C++".format(dep.label), "deps") -def _create_proto_compile_action(ctx, outputs, proto_info): - proto_root = proto_info.proto_source_root - if proto_root.startswith(ctx.genfiles_dir.path): - genfiles_path = proto_root - else: - genfiles_path = ctx.genfiles_dir.path + "/" + proto_root - - if proto_root == ".": - genfiles_path = ctx.genfiles_dir.path - - if len(outputs) != 0: - proto_common.compile( - actions = ctx.actions, - proto_info = proto_info, - proto_lang_toolchain_info = ctx.attr._aspect_cc_proto_toolchain[ProtoLangToolchainInfo], - generated_files = outputs, - plugin_output = genfiles_path, - ) - def _get_output_files(ctx, target, suffixes): result = [] for suffix in suffixes: @@ -171,7 +152,13 @@ def _aspect_impl(target, ctx): header_provider = ProtoCcHeaderInfo(headers = depset(transitive = transitive_headers)) files_to_build = list(outputs) - _create_proto_compile_action(ctx, outputs, proto_info) + proto_common.compile( + actions = ctx.actions, + proto_info = proto_info, + proto_lang_toolchain_info = ctx.attr._aspect_cc_proto_toolchain[ProtoLangToolchainInfo], + generated_files = outputs, + experimental_output_files = "multiple", + ) (cc_compilation_context, cc_compilation_outputs) = cc_common.compile( name = ctx.label.name, diff --git a/src/main/starlark/builtins_bzl/common/java/proto/java_lite_proto_library.bzl b/src/main/starlark/builtins_bzl/common/java/proto/java_lite_proto_library.bzl index 601ed17de69038..56afa3f41c0dca 100644 --- a/src/main/starlark/builtins_bzl/common/java/proto/java_lite_proto_library.bzl +++ b/src/main/starlark/builtins_bzl/common/java/proto/java_lite_proto_library.bzl @@ -50,7 +50,7 @@ def _aspect_impl(target, ctx): target[ProtoInfo], proto_toolchain_info, [source_jar], - source_jar, + experimental_output_files = "single", ) runtime = proto_toolchain_info.runtime if runtime: diff --git a/src/main/starlark/builtins_bzl/common/java/proto/java_proto_library.bzl b/src/main/starlark/builtins_bzl/common/java/proto/java_proto_library.bzl index cd4a8c8c6751e4..09690f027282ce 100644 --- a/src/main/starlark/builtins_bzl/common/java/proto/java_proto_library.bzl +++ b/src/main/starlark/builtins_bzl/common/java/proto/java_proto_library.bzl @@ -57,7 +57,7 @@ def _bazel_java_proto_aspect_impl(target, ctx): target[ProtoInfo], proto_toolchain_info, [source_jar], - source_jar, + experimental_output_files = "single", ) # Compile Java sources (or just merge if there aren't any) 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 7bd0ff5be37e1c..64036dd964b63a 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_common.bzl +++ b/src/main/starlark/builtins_bzl/common/proto/proto_common.bzl @@ -15,9 +15,13 @@ """Definition of proto_common module, together with bazel providers for proto rules.""" ProtoLangToolchainInfo = provider( - doc = "Specifies how to generate language-specific code from .proto files. Used by LANG_proto_library rules.", + doc = """Specifies how to generate language-specific code from .proto files. + Used by LANG_proto_library rules.""", fields = dict( - out_replacement_format_flag = "(str) Format string used when passing output to the plugin used by proto compiler.", + out_replacement_format_flag = """(str) Format string used when passing output to the plugin + used by proto compiler.""", + output_files = """("single","multiple","legacy") Format out_replacement_format_flag with + a path to single file or a directory in case of multiple files.""", plugin_format_flag = "(str) Format string used when passing plugin to proto compiler.", plugin = "(FilesToRunProvider) Proto compiler plugin.", runtime = "(Target) Runtime.", @@ -37,6 +41,17 @@ def _proto_path_flag(path): def _Iimport_path_equals_fullpath(proto_source): return "-I%s=%s" % (proto_source.import_path(), proto_source.source_file().path) +def _output_directory(proto_info, root): + proto_source_root = proto_info.proto_source_root + if proto_source_root.startswith(root.path): + #TODO(b/281812523): remove this branch when bin_dir is removed from proto_source_root + proto_source_root = proto_source_root.removeprefix(root.path).removeprefix("/") + + if proto_source_root == "" or proto_source_root == ".": + return root.path + + return root.path + "/" + proto_source_root + def _compile( actions, proto_info, @@ -48,7 +63,8 @@ def _compile( additional_inputs = depset(), resource_set = None, experimental_exec_group = None, - experimental_progress_message = None): + experimental_progress_message = None, + experimental_output_files = "legacy"): """Creates proto compile action for compiling *.proto files to language specific sources. Args: @@ -59,8 +75,10 @@ def _compile( generated_files: (list[File]) The output files generated by the proto compiler. Callee needs to declare files using `ctx.actions.declare_file`. See also: `proto_common.declare_generated_files`. - plugin_output: (File|str) The file or directory passed to the plugin. - Formatted with `proto_lang_toolchain.out_replacement_format_flag` + plugin_output: (File|str) Deprecated: Set `proto_lang_toolchain.output_files` + and remove the parameter. + For backwards compatibility, when the proto_lang_toolchain isn't updated + the value is used. additional_args: (Args) Additional arguments to add to the action. Accepts an ctx.actions.args() object that is added at the beginning of the command line. @@ -73,12 +91,34 @@ def _compile( Avoid using this parameter. experimental_progress_message: Overrides progress_message from the toolchain. Don't use this parameter. It's only intended for the transition. + experimental_output_files: (str) Overwrites output_files from the toolchain. + Don't use this parameter. It's only intended for the transition. """ + if type(generated_files) != type([]): + fail("generated_files is expected to be a list of Files") + if not generated_files: + return # nothing to do + if experimental_output_files not in ["single", "multiple", "legacy"]: + fail('experimental_output_files expected to be one of ["single", "multiple", "legacy"]') + args = actions.args() args.use_param_file(param_file_arg = "@%s") args.set_param_file_format("multiline") tools = list(additional_tools) + if experimental_output_files != "legacy": + output_files = experimental_output_files + else: + output_files = getattr(proto_lang_toolchain_info, "output_files", "legacy") + if output_files != "legacy": + if proto_lang_toolchain_info.out_replacement_format_flag: + if output_files == "single": + if len(generated_files) > 1: + fail("generated_files only expected a single file") + plugin_output = generated_files[0] + else: + plugin_output = _output_directory(proto_info, generated_files[0].root) + if plugin_output: args.add(plugin_output, format = proto_lang_toolchain_info.out_replacement_format_flag) if proto_lang_toolchain_info.plugin: diff --git a/src/main/starlark/builtins_bzl/common/proto/proto_lang_toolchain.bzl b/src/main/starlark/builtins_bzl/common/proto/proto_lang_toolchain.bzl index 1fd63f7632298e..3d2a9d00cd6bdb 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_lang_toolchain.bzl +++ b/src/main/starlark/builtins_bzl/common/proto/proto_lang_toolchain.bzl @@ -41,6 +41,7 @@ def _rule_impl(ctx): ), ProtoLangToolchainInfo( out_replacement_format_flag = flag, + output_files = ctx.attr.output_files, plugin_format_flag = ctx.attr.plugin_format_flag, plugin = plugin, runtime = ctx.attr.runtime, @@ -60,6 +61,7 @@ def make_proto_lang_toolchain(custom_proto_compiler): "progress_message": attr.string(default = "Generating proto_library %{label}"), "mnemonic": attr.string(default = "GenProto"), "command_line": attr.string(mandatory = True), + "output_files": attr.string(values = ["single", "multiple", "legacy"], default = "legacy"), "plugin_format_flag": attr.string(), "plugin": attr.label( executable = True, 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 ae7527a4cfa324..5ce86c3bfc4ba5 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl +++ b/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl @@ -253,6 +253,7 @@ def _write_descriptor_set(ctx, direct_sources, deps, exports, proto_info, descri args.add_joined("--allowed_public_imports", public_import_protos, map_each = _get_import_path, join_with = ":") proto_lang_toolchain_info = proto_common.ProtoLangToolchainInfo( out_replacement_format_flag = "--descriptor_set_out=%s", + output_files = "single", mnemonic = "GenProtoDescriptorSet", progress_message = "Generating Descriptor Set proto_library %{label}", proto_compiler = ctx.executable._proto_compiler, @@ -264,7 +265,6 @@ def _write_descriptor_set(ctx, direct_sources, deps, exports, proto_info, descri proto_info, proto_lang_toolchain_info, generated_files = [descriptor_set], - plugin_output = descriptor_set, additional_inputs = dependencies_descriptor_sets, additional_args = args, ) 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 386959289bb4ea..39dc3310d2ad06 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 @@ -95,8 +95,12 @@ public final void setup() throws Exception { "def _impl(ctx):", " outfile = ctx.actions.declare_file('out')", " kwargs = {}", - " if ctx.attr.plugin_output:", - " kwargs['plugin_output'] = ctx.attr.plugin_output", + " if ctx.attr.plugin_output == 'single':", + " kwargs['plugin_output'] = outfile.path", + " elif ctx.attr.plugin_output == 'multiple':", + " kwargs['plugin_output'] = ctx.genfiles_dir.path", + " elif ctx.attr.plugin_output == 'wrong':", + " kwargs['plugin_output'] = ctx.genfiles_dir.path + '///'", " if ctx.attr.additional_args:", " additional_args = ctx.actions.args()", " additional_args.add_all(ctx.attr.additional_args)", @@ -211,7 +215,7 @@ public void generateCode_noPlugin() throws Exception { /** * Verifies usage of proto_common.generate_code with plugin_output - * parameter. + * parameter set to file. */ @Test public void generateCode_withPluginOutput() throws Exception { @@ -220,7 +224,34 @@ public void generateCode_withPluginOutput() throws Exception { TestConstants.LOAD_PROTO_LIBRARY, "load('//foo:generate.bzl', 'generate_rule')", "proto_library(name = 'proto', srcs = ['A.proto'])", - "generate_rule(name = 'simple', proto_dep = ':proto', plugin_output = 'foo.srcjar')"); + "generate_rule(name = 'simple', proto_dep = ':proto', plugin_output = 'single')"); + + ConfiguredTarget target = getConfiguredTarget("//bar:simple"); + + List cmdLine = + getGeneratingSpawnAction(getBinArtifact("out", target)).getRemainingArguments(); + assertThat(cmdLine) + .comparingElementsUsing(MATCHES_REGEX) + .containsExactly( + "--java_out=param1,param2:bl?azel?-out/k8-fastbuild/bin/bar/out", + "--plugin=bl?azel?-out/[^/]*-exec-[^/]*/bin/third_party/x/plugin", + "-Ibar/A.proto=bar/A.proto", + "bar/A.proto") + .inOrder(); + } + + /** + * Verifies usage of proto_common.generate_code with plugin_output + * parameter set to directory. + */ + @Test + public void generateCode_withDirectoryPluginOutput() throws Exception { + scratch.file( + "bar/BUILD", + TestConstants.LOAD_PROTO_LIBRARY, + "load('//foo:generate.bzl', 'generate_rule')", + "proto_library(name = 'proto', srcs = ['A.proto'])", + "generate_rule(name = 'simple', proto_dep = ':proto', plugin_output = 'multiple')"); ConfiguredTarget target = getConfiguredTarget("//bar:simple"); @@ -229,7 +260,7 @@ public void generateCode_withPluginOutput() throws Exception { assertThat(cmdLine) .comparingElementsUsing(MATCHES_REGEX) .containsExactly( - "--java_out=param1,param2:foo.srcjar", + "--java_out=param1,param2:bl?azel?-out/k8-fastbuild/bin", "--plugin=bl?azel?-out/[^/]*-exec-[^/]*/bin/third_party/x/plugin", "-Ibar/A.proto=bar/A.proto", "bar/A.proto")