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 5b99bf2e0d1c2f..e1aa1ca856cf07 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 @@ -22,6 +22,7 @@ import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory; +import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.Type; @@ -55,6 +56,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 48f0c8e4086f04..f88ab5c6b9b245 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 @@ -47,25 +47,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.bin_dir.path): - path = proto_root - else: - path = ctx.bin_dir.path + "/" + proto_root - - if proto_root == ".": - path = ctx.bin_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 = path, - ) - def _get_output_files(ctx, target, suffixes): result = [] for suffix in suffixes: @@ -170,7 +151,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 a99764dbb9ca85..4d3ec6ae9250ce 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 c04ddf30567dfd..a39b6643d09a82 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 d9fc869d5153b1..a3266e64b260d3 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.", @@ -87,6 +91,17 @@ def get_import_path(proto_source): repo_path = repo_path[index + 1:] return repo_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, @@ -98,7 +113,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: @@ -109,8 +125,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, however the implementation corrects it for directories. 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. @@ -123,12 +141,41 @@ 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) + else: + # legacy behaviour + if plugin_output: + # Process it in backwards compatible fashion: + # In case this is not a single file output, fix it to the correct directory: + if plugin_output != generated_files[0].path and plugin_output != generated_files[0]: + 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 0981e72642636d..0cc2c0e50a43b4 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 @@ -40,6 +40,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, @@ -59,6 +60,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 5d384f982f0146..a6cc3ced5b5018 100644 --- a/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl +++ b/src/main/starlark/builtins_bzl/common/proto/proto_library.bzl @@ -193,6 +193,7 @@ def _write_descriptor_set(ctx, proto_info, deps, exports, descriptor_set): 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, @@ -204,7 +205,6 @@ def _write_descriptor_set(ctx, proto_info, deps, exports, descriptor_set): 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 31f4f0794f344b..b06daf4a6f3a32 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)", @@ -209,7 +213,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 { @@ -218,7 +222,61 @@ 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", + "-I.", + "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"); + + List cmdLine = + getGeneratingSpawnAction(getBinArtifact("out", target)).getRemainingArguments(); + assertThat(cmdLine) + .comparingElementsUsing(MATCHES_REGEX) + .containsExactly( + "--java_out=param1,param2:bl?azel?-out/k8-fastbuild/bin", + "--plugin=bl?azel?-out/[^/]*-exec-[^/]*/bin/third_party/x/plugin", + "-I.", + "bar/A.proto") + .inOrder(); + } + + /** + * Verifies usage of proto_common.generate_code with plugin_output + * parameter set to incorrect directory. + */ + @Test + public void generateCode_withPluginOutputWrong() 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 = 'wrong')"); ConfiguredTarget target = getConfiguredTarget("//bar:simple"); @@ -227,7 +285,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", "-I.", "bar/A.proto")