Skip to content

Commit 482d2be

Browse files
comiuscopybara-github
comius
authored and
copybara-github
committed
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
1 parent acaef96 commit 482d2be

File tree

8 files changed

+138
-33
lines changed

8 files changed

+138
-33
lines changed

‎src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.google.devtools.build.lib.analysis.RuleDefinition;
2323
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
2424
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
25+
import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet;
2526
import com.google.devtools.build.lib.packages.RuleClass;
2627
import com.google.devtools.build.lib.packages.Type;
2728

@@ -55,6 +56,16 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
5556
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
5657
.add(attr("command_line", Type.STRING).mandatory())
5758

59+
/* <!-- #BLAZE_RULE(proto_lang_toolchain).ATTRIBUTE(output_files) -->
60+
Controls how <code>$(OUT)</code> in <code>command_line</code> is formatted, either by
61+
a path to a single file or output directory in case of multiple files.
62+
Possible values are: "single", "multiple".
63+
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
64+
.add(
65+
attr("output_files", Type.STRING)
66+
.allowedValues(new AllowedValueSet("single", "multiple", "legacy"))
67+
.value("legacy"))
68+
5869
/* <!-- #BLAZE_RULE(proto_lang_toolchain).ATTRIBUTE(plugin_format_flag) -->
5970
If provided, this value will be passed to proto-compiler to use the plugin. The value must
6071
contain a single %s which is replaced with plugin executable.

‎src/main/starlark/builtins_bzl/common/cc/cc_proto_library.bzl‎

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -47,25 +47,6 @@ def _check_proto_libraries_in_deps(deps):
4747
if ProtoInfo in dep and CcInfo not in dep:
4848
fail("proto_library '{}' does not produce output for C++".format(dep.label), "deps")
4949

50-
def _create_proto_compile_action(ctx, outputs, proto_info):
51-
proto_root = proto_info.proto_source_root
52-
if proto_root.startswith(ctx.bin_dir.path):
53-
path = proto_root
54-
else:
55-
path = ctx.bin_dir.path + "/" + proto_root
56-
57-
if proto_root == ".":
58-
path = ctx.bin_dir.path
59-
60-
if len(outputs) != 0:
61-
proto_common.compile(
62-
actions = ctx.actions,
63-
proto_info = proto_info,
64-
proto_lang_toolchain_info = ctx.attr._aspect_cc_proto_toolchain[ProtoLangToolchainInfo],
65-
generated_files = outputs,
66-
plugin_output = path,
67-
)
68-
6950
def _get_output_files(ctx, target, suffixes):
7051
result = []
7152
for suffix in suffixes:
@@ -170,7 +151,13 @@ def _aspect_impl(target, ctx):
170151
header_provider = ProtoCcHeaderInfo(headers = depset(transitive = transitive_headers))
171152

172153
files_to_build = list(outputs)
173-
_create_proto_compile_action(ctx, outputs, proto_info)
154+
proto_common.compile(
155+
actions = ctx.actions,
156+
proto_info = proto_info,
157+
proto_lang_toolchain_info = ctx.attr._aspect_cc_proto_toolchain[ProtoLangToolchainInfo],
158+
generated_files = outputs,
159+
experimental_output_files = "multiple",
160+
)
174161

175162
(cc_compilation_context, cc_compilation_outputs) = cc_common.compile(
176163
name = ctx.label.name,

‎src/main/starlark/builtins_bzl/common/java/proto/java_lite_proto_library.bzl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ def _aspect_impl(target, ctx):
5050
target[ProtoInfo],
5151
proto_toolchain_info,
5252
[source_jar],
53-
source_jar,
53+
experimental_output_files = "single",
5454
)
5555
runtime = proto_toolchain_info.runtime
5656
if runtime:

‎src/main/starlark/builtins_bzl/common/java/proto/java_proto_library.bzl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ def _bazel_java_proto_aspect_impl(target, ctx):
5757
target[ProtoInfo],
5858
proto_toolchain_info,
5959
[source_jar],
60-
source_jar,
60+
experimental_output_files = "single",
6161
)
6262

6363
# Compile Java sources (or just merge if there aren't any)

‎src/main/starlark/builtins_bzl/common/proto/proto_common.bzl‎

Lines changed: 52 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,13 @@
1515
"""Definition of proto_common module, together with bazel providers for proto rules."""
1616

1717
ProtoLangToolchainInfo = provider(
18-
doc = "Specifies how to generate language-specific code from .proto files. Used by LANG_proto_library rules.",
18+
doc = """Specifies how to generate language-specific code from .proto files.
19+
Used by LANG_proto_library rules.""",
1920
fields = dict(
20-
out_replacement_format_flag = "(str) Format string used when passing output to the plugin used by proto compiler.",
21+
out_replacement_format_flag = """(str) Format string used when passing output to the plugin
22+
used by proto compiler.""",
23+
output_files = """("single","multiple","legacy") Format out_replacement_format_flag with
24+
a path to single file or a directory in case of multiple files.""",
2125
plugin_format_flag = "(str) Format string used when passing plugin to proto compiler.",
2226
plugin = "(FilesToRunProvider) Proto compiler plugin.",
2327
runtime = "(Target) Runtime.",
@@ -87,6 +91,17 @@ def get_import_path(proto_source):
8791
repo_path = repo_path[index + 1:]
8892
return repo_path
8993

94+
def _output_directory(proto_info, root):
95+
proto_source_root = proto_info.proto_source_root
96+
if proto_source_root.startswith(root.path):
97+
#TODO(b/281812523): remove this branch when bin_dir is removed from proto_source_root
98+
proto_source_root = proto_source_root.removeprefix(root.path).removeprefix("/")
99+
100+
if proto_source_root == "" or proto_source_root == ".":
101+
return root.path
102+
103+
return root.path + "/" + proto_source_root
104+
90105
def _compile(
91106
actions,
92107
proto_info,
@@ -98,7 +113,8 @@ def _compile(
98113
additional_inputs = depset(),
99114
resource_set = None,
100115
experimental_exec_group = None,
101-
experimental_progress_message = None):
116+
experimental_progress_message = None,
117+
experimental_output_files = "legacy"):
102118
"""Creates proto compile action for compiling *.proto files to language specific sources.
103119
104120
Args:
@@ -109,8 +125,10 @@ def _compile(
109125
generated_files: (list[File]) The output files generated by the proto compiler.
110126
Callee needs to declare files using `ctx.actions.declare_file`.
111127
See also: `proto_common.declare_generated_files`.
112-
plugin_output: (File|str) The file or directory passed to the plugin.
113-
Formatted with `proto_lang_toolchain.out_replacement_format_flag`
128+
plugin_output: (File|str) Deprecated: Set `proto_lang_toolchain.output_files`
129+
and remove the parameter.
130+
For backwards compatibility, when the proto_lang_toolchain isn't updated
131+
the value is used, however the implementation corrects it for directories.
114132
additional_args: (Args) Additional arguments to add to the action.
115133
Accepts an ctx.actions.args() object that is added at the beginning
116134
of the command line.
@@ -123,12 +141,41 @@ def _compile(
123141
Avoid using this parameter.
124142
experimental_progress_message: Overrides progress_message from the toolchain.
125143
Don't use this parameter. It's only intended for the transition.
144+
experimental_output_files: (str) Overwrites output_files from the toolchain.
145+
Don't use this parameter. It's only intended for the transition.
126146
"""
147+
if type(generated_files) != type([]):
148+
fail("generated_files is expected to be a list of Files")
149+
if not generated_files:
150+
return # nothing to do
151+
if experimental_output_files not in ["single", "multiple", "legacy"]:
152+
fail('experimental_output_files expected to be one of ["single", "multiple", "legacy"]')
153+
127154
args = actions.args()
128155
args.use_param_file(param_file_arg = "@%s")
129156
args.set_param_file_format("multiline")
130157
tools = list(additional_tools)
131158

159+
if experimental_output_files != "legacy":
160+
output_files = experimental_output_files
161+
else:
162+
output_files = getattr(proto_lang_toolchain_info, "output_files", "legacy")
163+
if output_files != "legacy":
164+
if proto_lang_toolchain_info.out_replacement_format_flag:
165+
if output_files == "single":
166+
if len(generated_files) > 1:
167+
fail("generated_files only expected a single file")
168+
plugin_output = generated_files[0]
169+
else:
170+
plugin_output = _output_directory(proto_info, generated_files[0].root)
171+
else:
172+
# legacy behaviour
173+
if plugin_output:
174+
# Process it in backwards compatible fashion:
175+
# In case this is not a single file output, fix it to the correct directory:
176+
if plugin_output != generated_files[0].path and plugin_output != generated_files[0]:
177+
plugin_output = _output_directory(proto_info, generated_files[0].root)
178+
132179
if plugin_output:
133180
args.add(plugin_output, format = proto_lang_toolchain_info.out_replacement_format_flag)
134181
if proto_lang_toolchain_info.plugin:

‎src/main/starlark/builtins_bzl/common/proto/proto_lang_toolchain.bzl‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ def _rule_impl(ctx):
4040
),
4141
ProtoLangToolchainInfo(
4242
out_replacement_format_flag = flag,
43+
output_files = ctx.attr.output_files,
4344
plugin_format_flag = ctx.attr.plugin_format_flag,
4445
plugin = plugin,
4546
runtime = ctx.attr.runtime,
@@ -59,6 +60,7 @@ def make_proto_lang_toolchain(custom_proto_compiler):
5960
"progress_message": attr.string(default = "Generating proto_library %{label}"),
6061
"mnemonic": attr.string(default = "GenProto"),
6162
"command_line": attr.string(mandatory = True),
63+
"output_files": attr.string(values = ["single", "multiple", "legacy"], default = "legacy"),
6264
"plugin_format_flag": attr.string(),
6365
"plugin": attr.label(
6466
executable = True,

‎src/main/starlark/builtins_bzl/common/proto/proto_library.bzl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,7 @@ def _write_descriptor_set(ctx, proto_info, deps, exports, descriptor_set):
193193
args.add_joined("--allowed_public_imports", public_import_protos, map_each = get_import_path, join_with = ":")
194194
proto_lang_toolchain_info = proto_common.ProtoLangToolchainInfo(
195195
out_replacement_format_flag = "--descriptor_set_out=%s",
196+
output_files = "single",
196197
mnemonic = "GenProtoDescriptorSet",
197198
progress_message = "Generating Descriptor Set proto_library %{label}",
198199
proto_compiler = ctx.executable._proto_compiler,
@@ -204,7 +205,6 @@ def _write_descriptor_set(ctx, proto_info, deps, exports, descriptor_set):
204205
proto_info,
205206
proto_lang_toolchain_info,
206207
generated_files = [descriptor_set],
207-
plugin_output = descriptor_set,
208208
additional_inputs = dependencies_descriptor_sets,
209209
additional_args = args,
210210
)

‎src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoCommonTest.java‎

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -95,8 +95,12 @@ public final void setup() throws Exception {
9595
"def _impl(ctx):",
9696
" outfile = ctx.actions.declare_file('out')",
9797
" kwargs = {}",
98-
" if ctx.attr.plugin_output:",
99-
" kwargs['plugin_output'] = ctx.attr.plugin_output",
98+
" if ctx.attr.plugin_output == 'single':",
99+
" kwargs['plugin_output'] = outfile.path",
100+
" elif ctx.attr.plugin_output == 'multiple':",
101+
" kwargs['plugin_output'] = ctx.genfiles_dir.path",
102+
" elif ctx.attr.plugin_output == 'wrong':",
103+
" kwargs['plugin_output'] = ctx.genfiles_dir.path + '///'",
100104
" if ctx.attr.additional_args:",
101105
" additional_args = ctx.actions.args()",
102106
" additional_args.add_all(ctx.attr.additional_args)",
@@ -209,7 +213,7 @@ public void generateCode_noPlugin() throws Exception {
209213

210214
/**
211215
* Verifies usage of <code>proto_common.generate_code</code> with <code>plugin_output</code>
212-
* parameter.
216+
* parameter set to file.
213217
*/
214218
@Test
215219
public void generateCode_withPluginOutput() throws Exception {
@@ -218,7 +222,61 @@ public void generateCode_withPluginOutput() throws Exception {
218222
TestConstants.LOAD_PROTO_LIBRARY,
219223
"load('//foo:generate.bzl', 'generate_rule')",
220224
"proto_library(name = 'proto', srcs = ['A.proto'])",
221-
"generate_rule(name = 'simple', proto_dep = ':proto', plugin_output = 'foo.srcjar')");
225+
"generate_rule(name = 'simple', proto_dep = ':proto', plugin_output = 'single')");
226+
227+
ConfiguredTarget target = getConfiguredTarget("//bar:simple");
228+
229+
List<String> cmdLine =
230+
getGeneratingSpawnAction(getBinArtifact("out", target)).getRemainingArguments();
231+
assertThat(cmdLine)
232+
.comparingElementsUsing(MATCHES_REGEX)
233+
.containsExactly(
234+
"--java_out=param1,param2:bl?azel?-out/k8-fastbuild/bin/bar/out",
235+
"--plugin=bl?azel?-out/[^/]*-exec-[^/]*/bin/third_party/x/plugin",
236+
"-I.",
237+
"bar/A.proto")
238+
.inOrder();
239+
}
240+
241+
/**
242+
* Verifies usage of <code>proto_common.generate_code</code> with <code>plugin_output</code>
243+
* parameter set to directory.
244+
*/
245+
@Test
246+
public void generateCode_withDirectoryPluginOutput() throws Exception {
247+
scratch.file(
248+
"bar/BUILD",
249+
TestConstants.LOAD_PROTO_LIBRARY,
250+
"load('//foo:generate.bzl', 'generate_rule')",
251+
"proto_library(name = 'proto', srcs = ['A.proto'])",
252+
"generate_rule(name = 'simple', proto_dep = ':proto', plugin_output = 'multiple')");
253+
254+
ConfiguredTarget target = getConfiguredTarget("//bar:simple");
255+
256+
List<String> cmdLine =
257+
getGeneratingSpawnAction(getBinArtifact("out", target)).getRemainingArguments();
258+
assertThat(cmdLine)
259+
.comparingElementsUsing(MATCHES_REGEX)
260+
.containsExactly(
261+
"--java_out=param1,param2:bl?azel?-out/k8-fastbuild/bin",
262+
"--plugin=bl?azel?-out/[^/]*-exec-[^/]*/bin/third_party/x/plugin",
263+
"-I.",
264+
"bar/A.proto")
265+
.inOrder();
266+
}
267+
268+
/**
269+
* Verifies usage of <code>proto_common.generate_code</code> with <code>plugin_output</code>
270+
* parameter set to incorrect directory.
271+
*/
272+
@Test
273+
public void generateCode_withPluginOutputWrong() throws Exception {
274+
scratch.file(
275+
"bar/BUILD",
276+
TestConstants.LOAD_PROTO_LIBRARY,
277+
"load('//foo:generate.bzl', 'generate_rule')",
278+
"proto_library(name = 'proto', srcs = ['A.proto'])",
279+
"generate_rule(name = 'simple', proto_dep = ':proto', plugin_output = 'wrong')");
222280

223281
ConfiguredTarget target = getConfiguredTarget("//bar:simple");
224282

@@ -227,7 +285,7 @@ public void generateCode_withPluginOutput() throws Exception {
227285
assertThat(cmdLine)
228286
.comparingElementsUsing(MATCHES_REGEX)
229287
.containsExactly(
230-
"--java_out=param1,param2:foo.srcjar",
288+
"--java_out=param1,param2:bl?azel?-out/k8-fastbuild/bin",
231289
"--plugin=bl?azel?-out/[^/]*-exec-[^/]*/bin/third_party/x/plugin",
232290
"-I.",
233291
"bar/A.proto")

0 commit comments

Comments
 (0)