Compute the value of plugin_output from proto_info

Additional information is needed whether protoc generates a single file or multiple files.

Add output_files to proto_lang_toolchain (enum "single","multiple") and propagate it through ProtoLangToolchainInfo.

Add experimental_output_files to proto_common.compile, that can override the value, for faster migration path.

When the value is set, automatically compute plugin_output.

When the (legacy) plugin_output is not set to a file, set it automatically to correct directory.

AI: Cherry-pick this change to Bazel minor release and follow up with updates to proto_lang_toolchain.

Fixes: #18263
Tracking issue: #18623
PiperOrigin-RevId: 539047857
Change-Id: Id5c3f48f02ad245bee90eb113d5ba4f681c650ac

Author: comius

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
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("command_line", Type.STRING).mandatory())
 
+        /* <!-- #BLAZE_RULE(proto_lang_toolchain).ATTRIBUTE(output_files) -->
+        Controls how <code>$(OUT)</code> in <code>command_line</code> is formatted, either by
+        a path to a single file or output directory in case of multiple files.
+        Possible values are: "single", "multiple".
+        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+        .add(
+            attr("output_files", Type.STRING)
+                .allowedValues(new AllowedValueSet("single", "multiple", "legacy"))
+                .value("legacy"))
+
         /* <!-- #BLAZE_RULE(proto_lang_toolchain).ATTRIBUTE(plugin_format_flag) -->
         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.

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,

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:

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)

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:

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,

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,
     )

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 <code>proto_common.generate_code</code> with <code>plugin_output</code>
-   * 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<String> 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 <code>proto_common.generate_code</code> with <code>plugin_output</code>
+   * 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<String> 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 <code>proto_common.generate_code</code> with <code>plugin_output</code>
+   * 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")