Skip to content

Commit d69a55c

Browse files
comiuscopybara-github
comius
authored and
copybara-github
committed
Remove proto_lang_toolchain rule's $(PLUGIN_OUT) placeholder and simplify ProtoCompileActionBuilder.
`$(OUT)` placeholder is replaced with `%s` so it can be directly used in addFormatted. This simplifies construction of proto compile action and makes it possible for Starlark version to have the same performance. github shows no uses of the placeholder: https://github.com/search?q=%22%24%28PLUGIN_OUT%29%22&type=Repositories PiperOrigin-RevId: 412286743
1 parent ed819b2 commit d69a55c

7 files changed

Lines changed: 40 additions & 75 deletions

File tree

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,7 @@
4242
import net.starlark.java.eval.Starlark;
4343
import net.starlark.java.eval.StarlarkThread;
4444

45-
/**
46-
* Utility functions for proto_library and proto aspect implementations.
47-
*/
45+
/** Utility functions for proto_library and proto aspect implementations. */
4846
public class ProtoCommon {
4947
private ProtoCommon() {
5048
throw new UnsupportedOperationException();

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

Lines changed: 9 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import com.google.common.base.Preconditions;
2222
import com.google.common.base.Supplier;
2323
import com.google.common.collect.ImmutableList;
24-
import com.google.common.collect.ImmutableMap;
2524
import com.google.devtools.build.lib.actions.AbstractAction;
2625
import com.google.devtools.build.lib.actions.Artifact;
2726
import com.google.devtools.build.lib.actions.CommandLineItem;
@@ -36,17 +35,13 @@
3635
import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg;
3736
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
3837
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
39-
import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
40-
import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
41-
import com.google.devtools.build.lib.analysis.stringtemplate.TemplateExpander;
4238
import com.google.devtools.build.lib.cmdline.Label;
4339
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
4440
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
4541
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
4642
import com.google.devtools.build.lib.util.OnDemandString;
4743
import java.util.HashSet;
4844
import java.util.List;
49-
import java.util.Map;
5045
import java.util.function.Consumer;
5146
import javax.annotation.Nullable;
5247

@@ -150,48 +145,6 @@ public String toString() {
150145
}
151146
}
152147

153-
private static class OnDemandCommandLineExpansion extends OnDemandString {
154-
// E.g., --java_out=%s
155-
private final String template;
156-
private final Map<String, ? extends CharSequence> variableValues;
157-
158-
OnDemandCommandLineExpansion(
159-
String template, Map<String, ? extends CharSequence> variableValues) {
160-
this.template = template;
161-
this.variableValues = variableValues;
162-
}
163-
164-
@Override
165-
public String toString() {
166-
try {
167-
return TemplateExpander.expand(
168-
template,
169-
new TemplateContext() {
170-
@Override
171-
public String lookupVariable(String name) throws ExpansionException {
172-
CharSequence value = variableValues.get(name);
173-
if (value == null) {
174-
throw new ExpansionException(String.format("$(%s) not defined", name));
175-
}
176-
return value.toString();
177-
}
178-
179-
@Override
180-
public String lookupFunction(String name, String param)
181-
throws ExpansionException {
182-
throw new ExpansionException(String.format("$(%s) not defined", name));
183-
}
184-
})
185-
.expansion();
186-
} catch (ExpansionException e) {
187-
// Squeelch. We don't throw this exception in the lookupMakeVariable implementation above,
188-
// and we can't report it here anyway, because this code will typically execute in the
189-
// Execution phase.
190-
}
191-
return template;
192-
}
193-
}
194-
195148
/** Builds a ResourceSet based on the number of inputs. */
196149
public static class ProtoCompileResourceSetBuilder implements ResourceSetOrBuilder {
197150
@Override
@@ -367,7 +320,7 @@ private static ToolchainInvocation createDescriptorSetToolchain(
367320
// output size to become quadratic, so don't.
368321
// A rule that concatenates the artifacts from ctx.deps.proto.transitive_descriptor_sets
369322
// provides similar results.
370-
"--descriptor_set_out=$(OUT)",
323+
"--descriptor_set_out=%s",
371324
/* pluginExecutable= */ null,
372325
/* runtime= */ null,
373326
/* providedProtoSources= */ ImmutableList.of()),
@@ -492,8 +445,6 @@ public static boolean arePublicImportsStrict(RuleContext ruleContext) {
492445
* <ul>
493446
* <li>Each toolchain contributes a command-line, formatted from its commandLine() method.
494447
* <li>$(OUT) is replaced with the outReplacement field of ToolchainInvocation.
495-
* <li>$(PLUGIN_out) is replaced with PLUGIN_<key>_out where 'key' is the key of
496-
* toolchainInvocations. The key thus allows multiple plugins in one command-line.
497448
* <li>If a toolchain's {@code plugin()} is non-null, we point at it by emitting
498449
* --plugin=protoc-gen-PLUGIN_<key>=<location of plugin>.
499450
* </ul>
@@ -537,14 +488,15 @@ static CustomCommandLine createCommandLineFromToolchains(
537488

538489
ProtoLangToolchainProvider toolchain = invocation.toolchain;
539490

491+
final String formatString = toolchain.outReplacementFormatFlag();
492+
final CharSequence outReplacement = invocation.outReplacement;
540493
cmdLine.addLazyString(
541-
new OnDemandCommandLineExpansion(
542-
toolchain.commandLine(),
543-
ImmutableMap.of(
544-
"OUT",
545-
invocation.outReplacement,
546-
"PLUGIN_OUT",
547-
String.format("PLUGIN_%s_out", invocation.name))));
494+
new OnDemandString() {
495+
@Override
496+
public String toString() {
497+
return String.format(formatString, outReplacement);
498+
}
499+
});
548500

549501
if (toolchain.pluginExecutable() != null) {
550502
cmdLine.addFormatted(

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,19 @@ public ConfiguredTarget create(RuleContext ruleContext)
3939
providedProtoSources.addTransitive(protoInfo.getTransitiveSources());
4040
}
4141

42+
String flag = ruleContext.attributes().get("command_line", Type.STRING);
43+
44+
if (flag.contains("$(PLUGIN_OUT)")) {
45+
ruleContext.attributeError("command_line", "Placeholder '$(PLUGIN_OUT)' is not supported.");
46+
return null;
47+
}
48+
49+
flag = flag.replace("$(OUT)", "%s");
50+
4251
return new RuleConfiguredTargetBuilder(ruleContext)
4352
.addProvider(
4453
ProtoLangToolchainProvider.create(
45-
ruleContext.attributes().get("command_line", Type.STRING),
54+
flag,
4655
ruleContext.getPrerequisite("plugin", FilesToRunProvider.class),
4756
ruleContext.getPrerequisite("runtime"),
4857
// We intentionally flatten the NestedSet here.

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

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434
@AutoValue
3535
@AutoCodec
3636
public abstract class ProtoLangToolchainProvider implements TransitiveInfoProvider {
37-
public abstract String commandLine();
37+
public abstract String outReplacementFormatFlag();
3838

3939
@Nullable
4040
public abstract FilesToRunProvider pluginExecutable();
@@ -63,17 +63,21 @@ public NestedSet<Artifact> blacklistedProtos() {
6363

6464
@AutoCodec.Instantiator
6565
public static ProtoLangToolchainProvider createForDeserialization(
66-
String commandLine,
66+
String outReplacementFormatFlag,
6767
FilesToRunProvider pluginExecutable,
6868
TransitiveInfoCollection runtime,
6969
ImmutableList<ProtoSource> providedProtoSources,
7070
NestedSet<Artifact> blacklistedProtos) {
7171
return new AutoValue_ProtoLangToolchainProvider(
72-
commandLine, pluginExecutable, runtime, providedProtoSources, blacklistedProtos);
72+
outReplacementFormatFlag,
73+
pluginExecutable,
74+
runtime,
75+
providedProtoSources,
76+
blacklistedProtos);
7377
}
7478

7579
public static ProtoLangToolchainProvider create(
76-
String commandLine,
80+
String outReplacementFormatFlag,
7781
FilesToRunProvider pluginExecutable,
7882
TransitiveInfoCollection runtime,
7983
ImmutableList<ProtoSource> providedProtoSources) {
@@ -82,6 +86,10 @@ public static ProtoLangToolchainProvider create(
8286
blacklistedProtos.add(protoSource.getOriginalSourceFile());
8387
}
8488
return new AutoValue_ProtoLangToolchainProvider(
85-
commandLine, pluginExecutable, runtime, providedProtoSources, blacklistedProtos.build());
89+
outReplacementFormatFlag,
90+
pluginExecutable,
91+
runtime,
92+
providedProtoSources,
93+
blacklistedProtos.build());
8694
}
8795
}

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,6 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
3939
<li><code>$(OUT)</code> is LANG_proto_library-specific. The rules are expected to define
4040
how they interpret this variable. For Java, for example, $(OUT) will be replaced with
4141
the src-jar filename to create.</li>
42-
<li><code>$(PLUGIN_out)</code> will be substituted to work with a
43-
`--plugin=protoc-gen-PLUGIN` command line.</li>
4442
</ul>
4543
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
4644
.add(attr("command_line", Type.STRING).mandatory())
@@ -122,7 +120,7 @@ Specifies how a LANG_proto_library rule (e.g., <code>java_proto_library</code>)
122120
<pre class="code">
123121
proto_lang_toolchain(
124122
name = "javalite_toolchain",
125-
command_line = "--$(PLUGIN_OUT)=shared,immutable:$(OUT)",
123+
command_line = "--javalite_out=shared,immutable:$(OUT)",
126124
plugin = ":javalite_plugin",
127125
runtime = ":protobuf_lite",
128126
)

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -104,14 +104,14 @@ public void commandLine_basic() throws Exception {
104104

105105
ProtoLangToolchainProvider toolchainNoPlugin =
106106
ProtoLangToolchainProvider.create(
107-
"--java_out=param1,param2:$(OUT)",
107+
"--java_out=param1,param2:%s",
108108
/* pluginExecutable= */ null,
109109
/* runtime= */ mock(TransitiveInfoCollection.class),
110110
/* providedProtoSources= */ ImmutableList.of());
111111

112112
ProtoLangToolchainProvider toolchainWithPlugin =
113113
ProtoLangToolchainProvider.create(
114-
"--$(PLUGIN_OUT)=param3,param4:$(OUT)",
114+
"--PLUGIN_pluginName_out=param3,param4:%s",
115115
plugin,
116116
/* runtime= */ mock(TransitiveInfoCollection.class),
117117
/* providedProtoSources= */ ImmutableList.of());
@@ -174,7 +174,7 @@ public void commandline_derivedArtifact() throws Exception {
174174
public void commandLine_strictDeps() throws Exception {
175175
ProtoLangToolchainProvider toolchain =
176176
ProtoLangToolchainProvider.create(
177-
"--java_out=param1,param2:$(OUT)",
177+
"--java_out=param1,param2:%s",
178178
/* pluginExecutable= */ null,
179179
/* runtime= */ mock(TransitiveInfoCollection.class),
180180
/* providedProtoSources= */ ImmutableList.of());
@@ -212,7 +212,7 @@ public void commandLine_strictDeps() throws Exception {
212212
public void commandLine_exports() throws Exception {
213213
ProtoLangToolchainProvider toolchain =
214214
ProtoLangToolchainProvider.create(
215-
"--java_out=param1,param2:$(OUT)",
215+
"--java_out=param1,param2:%s",
216216
/* pluginExecutable= */ null,
217217
/* runtime= */ mock(TransitiveInfoCollection.class),
218218
/* providedProtoSources= */ ImmutableList.of());
@@ -282,7 +282,7 @@ public String toString() {
282282

283283
ProtoLangToolchainProvider toolchain =
284284
ProtoLangToolchainProvider.create(
285-
"--java_out=param1,param2:$(OUT)",
285+
"--java_out=param1,param2:%s",
286286
/* pluginExecutable= */ null,
287287
/* runtime= */ mock(TransitiveInfoCollection.class),
288288
/* providedProtoSources= */ ImmutableList.of());

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ public void setUp() throws Exception {
4040
}
4141

4242
private void validateProtoLangToolchain(ProtoLangToolchainProvider toolchain) throws Exception {
43-
assertThat(toolchain.commandLine()).isEqualTo("cmd-line");
43+
assertThat(toolchain.outReplacementFormatFlag()).isEqualTo("cmd-line");
4444
assertThat(toolchain.pluginExecutable().getExecutable().getRootRelativePathString())
4545
.isEqualTo("third_party/x/plugin");
4646

0 commit comments

Comments
 (0)