Skip to content

Commit

Permalink
Compute the value of plugin_output from proto_info
Browse files Browse the repository at this point in the history
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: bazelbuild#18263
Tracking issue: bazelbuild#18623
PiperOrigin-RevId: 539047857
Change-Id: Id5c3f48f02ad245bee90eb113d5ba4f681c650ac
  • Loading branch information
comius authored and traversaro committed Jun 24, 2023
1 parent 50f77aa commit e1aed90
Show file tree
Hide file tree
Showing 8 changed files with 138 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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.
Expand Down
27 changes: 7 additions & 20 deletions src/main/starlark/builtins_bzl/common/cc/cc_proto_library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
57 changes: 52 additions & 5 deletions src/main/starlark/builtins_bzl/common/proto/proto_common.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down Expand Up @@ -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,
Expand All @@ -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:
Expand All @@ -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.
Expand All @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)",
Expand Down Expand Up @@ -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 {
Expand All @@ -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");

Expand All @@ -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")
Expand Down

0 comments on commit e1aed90

Please sign in to comment.