Skip to content

Commit 4162cc5

Browse files
Yanniccopybara-github
Yannic
authored and
copybara-github
committed
proto_lang_toolchain: Add original sources of ProtoInfo to blacklistedProtos
Fixes #10484 Closes #10493. PiperOrigin-RevId: 289411825
1 parent 63de242 commit 4162cc5

File tree

6 files changed

+115
-27
lines changed

6 files changed

+115
-27
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ private static class Impl {
276276
private boolean areSrcsBlacklisted() {
277277
return !new ProtoSourceFileBlacklist(
278278
ruleContext, getProtoToolchainProvider().blacklistedProtos())
279-
.checkSrcs(protoInfo.getOriginalDirectProtoSources(), "cc_proto_library");
279+
.checkSrcs(protoInfo.getOriginalTransitiveProtoSources(), "cc_proto_library");
280280
}
281281

282282
private FeatureConfiguration getFeatureConfiguration() {

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

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,19 @@ private static NestedSet<Artifact> computeTransitiveProtoSources(
127127
return result.build();
128128
}
129129

130+
private static NestedSet<Artifact> computeTransitiveOriginalProtoSources(
131+
RuleContext ruleContext, ImmutableList<Artifact> originalProtoSources) {
132+
NestedSetBuilder<Artifact> result = NestedSetBuilder.naiveLinkOrder();
133+
134+
result.addAll(originalProtoSources);
135+
136+
for (ProtoInfo dep : ruleContext.getPrerequisites("deps", Mode.TARGET, ProtoInfo.PROVIDER)) {
137+
result.addTransitive(dep.getOriginalTransitiveProtoSources());
138+
}
139+
140+
return result.build();
141+
}
142+
130143
static NestedSet<Artifact> computeDependenciesDescriptorSets(RuleContext ruleContext) {
131144
NestedSetBuilder<Artifact> result = NestedSetBuilder.stableOrder();
132145

@@ -462,6 +475,8 @@ public static ProtoInfo createProtoInfo(
462475

463476
NestedSet<Artifact> transitiveProtoSources =
464477
computeTransitiveProtoSources(ruleContext, library.getSources());
478+
NestedSet<Artifact> transitiveOriginalProtoSources =
479+
computeTransitiveOriginalProtoSources(ruleContext, directProtoSources);
465480
NestedSet<String> transitiveProtoSourceRoots =
466481
computeTransitiveProtoSourceRoots(ruleContext, library.getSourceRoot());
467482

@@ -491,8 +506,8 @@ public static ProtoInfo createProtoInfo(
491506
ProtoInfo protoInfo =
492507
new ProtoInfo(
493508
library.getSources(),
494-
directProtoSources,
495509
library.getSourceRoot(),
510+
transitiveOriginalProtoSources,
496511
transitiveProtoSources,
497512
transitiveProtoSourceRoots,
498513
strictImportableProtosForDependents,

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

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public Provider() {
5454
public static final String LEGACY_SKYLARK_NAME = "proto";
5555

5656
private final ImmutableList<Artifact> directProtoSources;
57-
private final ImmutableList<Artifact> originalDirectProtoSources;
57+
private final NestedSet<Artifact> originalTransitiveProtoSources;
5858
private final String directProtoSourceRoot;
5959
private final NestedSet<Artifact> transitiveProtoSources;
6060
private final NestedSet<String> transitiveProtoSourceRoots;
@@ -71,8 +71,8 @@ public Provider() {
7171
@AutoCodec.Instantiator
7272
public ProtoInfo(
7373
ImmutableList<Artifact> directProtoSources,
74-
ImmutableList<Artifact> originalDirectProtoSources,
7574
String directProtoSourceRoot,
75+
NestedSet<Artifact> originalTransitiveProtoSources,
7676
NestedSet<Artifact> transitiveProtoSources,
7777
NestedSet<String> transitiveProtoSourceRoots,
7878
NestedSet<Artifact> strictImportableProtoSourcesForDependents,
@@ -86,7 +86,7 @@ public ProtoInfo(
8686
Location location) {
8787
super(PROVIDER, location);
8888
this.directProtoSources = directProtoSources;
89-
this.originalDirectProtoSources = originalDirectProtoSources;
89+
this.originalTransitiveProtoSources = originalTransitiveProtoSources;
9090
this.directProtoSourceRoot = directProtoSourceRoot;
9191
this.transitiveProtoSources = transitiveProtoSources;
9292
this.transitiveProtoSourceRoots = transitiveProtoSourceRoots;
@@ -103,17 +103,18 @@ public ProtoInfo(
103103

104104
/**
105105
* The proto source files that are used in compiling this {@code proto_library}.
106-
*
107-
* <p>Different from {@link #getOriginalDirectProtoSources()} when a virtual import root is used.
108106
*/
109107
@Override
110108
public ImmutableList<Artifact> getDirectProtoSources() {
111109
return directProtoSources;
112110
}
113111

114-
/** The proto sources of the {@code proto_library} declaring this provider. */
115-
public ImmutableList<Artifact> getOriginalDirectProtoSources() {
116-
return originalDirectProtoSources;
112+
/**
113+
* The non-virtual transitive proto source files. Different from {@link
114+
* #getTransitiveProtoSources()} if a transitive dependency has {@code import_prefix} or the like.
115+
*/
116+
public NestedSet<Artifact> getOriginalTransitiveProtoSources() {
117+
return originalTransitiveProtoSources;
117118
}
118119

119120
/** The source root of the current library. */

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,10 @@ public ConfiguredTarget create(RuleContext ruleContext)
4444
ProtoInfo protoInfo = protos.get(ProtoInfo.PROVIDER);
4545
// TODO(cushon): it would be nice to make this mandatory and stop adding files to build too
4646
if (protoInfo != null) {
47-
blacklistedProtos.addTransitive(protoInfo.getTransitiveProtoSources());
47+
blacklistedProtos.addTransitive(protoInfo.getOriginalTransitiveProtoSources());
4848
} else {
49+
// Only add files from FileProvider if |protos| is not a proto_library to avoid adding
50+
// the descriptor_set of proto_library to the list of blacklisted files.
4951
blacklistedProtos.addTransitive(protos.getProvider(FileProvider.class).getFilesToBuild());
5052
}
5153
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,8 @@ private ProtoInfo protoInfo(
6565
NestedSet<Pair<Artifact, String>> exportedProtos) {
6666
return new ProtoInfo(
6767
directProtos,
68-
directProtos,
69-
"",
68+
/* directProtoSourceRoot= */ "",
69+
transitiveProtos,
7070
transitiveProtos,
7171
transitiveProtoSourceRoots,
7272
/* strictImportableProtosForDependents */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),

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

Lines changed: 84 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,27 @@ public void setUp() throws Exception {
3939
invalidatePackages();
4040
}
4141

42+
private void validateProtoLangToolchain(ProtoLangToolchainProvider toolchain) throws Exception {
43+
assertThat(toolchain.commandLine()).isEqualTo("cmd-line");
44+
assertThat(toolchain.pluginExecutable().getExecutable().getRootRelativePathString())
45+
.isEqualTo("third_party/x/plugin");
46+
47+
TransitiveInfoCollection runtimes = toolchain.runtime();
48+
assertThat(runtimes.getLabel())
49+
.isEqualTo(Label.parseAbsolute("//third_party/x:runtime", ImmutableMap.of()));
50+
51+
assertThat(prettyArtifactNames(toolchain.blacklistedProtos()))
52+
.containsExactly(
53+
"third_party/x/metadata.proto",
54+
"third_party/x/descriptor.proto",
55+
"third_party/x/any.proto");
56+
}
57+
4258
@Test
4359
public void protoToolchain() throws Exception {
4460
scratch.file(
45-
"x/BUILD",
61+
"third_party/x/BUILD",
62+
"licenses(['unencumbered'])",
4663
"cc_binary(name = 'plugin', srcs = ['plugin.cc'])",
4764
"cc_library(name = 'runtime', srcs = ['runtime.cc'])",
4865
"filegroup(name = 'descriptors', srcs = ['metadata.proto', 'descriptor.proto'])",
@@ -52,29 +69,82 @@ public void protoToolchain() throws Exception {
5269
scratch.file(
5370
"foo/BUILD",
5471
TestConstants.LOAD_PROTO_LANG_TOOLCHAIN,
72+
"licenses(['unencumbered'])",
5573
"proto_lang_toolchain(",
5674
" name = 'toolchain',",
5775
" command_line = 'cmd-line',",
58-
" plugin = '//x:plugin',",
59-
" runtime = '//x:runtime',",
60-
" blacklisted_protos = ['//x:blacklist']",
76+
" plugin = '//third_party/x:plugin',",
77+
" runtime = '//third_party/x:runtime',",
78+
" blacklisted_protos = ['//third_party/x:blacklist']",
6179
")");
6280

6381
update(ImmutableList.of("//foo:toolchain"), false, 1, true, new EventBus());
6482

65-
ProtoLangToolchainProvider toolchain =
66-
getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class);
83+
validateProtoLangToolchain(
84+
getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class));
85+
}
6786

68-
assertThat(toolchain.commandLine()).isEqualTo("cmd-line");
69-
assertThat(toolchain.pluginExecutable().getExecutable().getRootRelativePathString())
70-
.isEqualTo("x/plugin");
87+
@Test
88+
public void protoToolchainBlacklistProtoLibraries() throws Exception {
89+
scratch.file(
90+
"third_party/x/BUILD",
91+
TestConstants.LOAD_PROTO_LIBRARY,
92+
"licenses(['unencumbered'])",
93+
"cc_binary(name = 'plugin', srcs = ['plugin.cc'])",
94+
"cc_library(name = 'runtime', srcs = ['runtime.cc'])",
95+
"proto_library(name = 'descriptors', srcs = ['metadata.proto', 'descriptor.proto'])",
96+
"proto_library(name = 'any', srcs = ['any.proto'], strip_import_prefix = '/third_party')");
7197

72-
TransitiveInfoCollection runtimes = toolchain.runtime();
73-
assertThat(runtimes.getLabel())
74-
.isEqualTo(Label.parseAbsolute("//x:runtime", ImmutableMap.of()));
98+
scratch.file(
99+
"foo/BUILD",
100+
TestConstants.LOAD_PROTO_LANG_TOOLCHAIN,
101+
"proto_lang_toolchain(",
102+
" name = 'toolchain',",
103+
" command_line = 'cmd-line',",
104+
" plugin = '//third_party/x:plugin',",
105+
" runtime = '//third_party/x:runtime',",
106+
" blacklisted_protos = ['//third_party/x:descriptors', '//third_party/x:any']",
107+
")");
75108

76-
assertThat(prettyArtifactNames(toolchain.blacklistedProtos()))
77-
.containsExactly("x/metadata.proto", "x/descriptor.proto", "x/any.proto");
109+
update(ImmutableList.of("//foo:toolchain"), false, 1, true, new EventBus());
110+
111+
validateProtoLangToolchain(
112+
getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class));
113+
}
114+
115+
@Test
116+
public void protoToolchainMixedBlacklist() throws Exception {
117+
scratch.file(
118+
"third_party/x/BUILD",
119+
TestConstants.LOAD_PROTO_LIBRARY,
120+
"licenses(['unencumbered'])",
121+
"cc_binary(name = 'plugin', srcs = ['plugin.cc'])",
122+
"cc_library(name = 'runtime', srcs = ['runtime.cc'])",
123+
"proto_library(name = 'metadata', srcs = ['metadata.proto'])",
124+
"proto_library(",
125+
" name = 'descriptor',",
126+
" srcs = ['descriptor.proto'],",
127+
" strip_import_prefix = '/third_party')",
128+
"filegroup(name = 'any', srcs = ['any.proto'])");
129+
130+
scratch.file(
131+
"foo/BUILD",
132+
TestConstants.LOAD_PROTO_LANG_TOOLCHAIN,
133+
"proto_lang_toolchain(",
134+
" name = 'toolchain',",
135+
" command_line = 'cmd-line',",
136+
" plugin = '//third_party/x:plugin',",
137+
" runtime = '//third_party/x:runtime',",
138+
" blacklisted_protos = [",
139+
" '//third_party/x:metadata',",
140+
" '//third_party/x:descriptor',",
141+
" '//third_party/x:any']",
142+
")");
143+
144+
update(ImmutableList.of("//foo:toolchain"), false, 1, true, new EventBus());
145+
146+
validateProtoLangToolchain(
147+
getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class));
78148
}
79149

80150
@Test

0 commit comments

Comments
 (0)