Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

proto_lang_toolchain: Add original sources of ProtoInfo to blacklistedProtos #10493

Closed
wants to merge 2 commits into from
Closed

proto_lang_toolchain: Add original sources of ProtoInfo to blacklistedProtos #10493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6375e57
proto_lang_toolchain: Add original sources of ProtoInfo to blackliste…
Yannic Dec 27, 2019
5984994
Merge branch 'master' into fix_10484
Yannic Jan 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,12 @@ public ConfiguredTarget create(RuleContext ruleContext)
ProtoInfo protoInfo = protos.get(ProtoInfo.PROVIDER);
// TODO(cushon): it would be nice to make this mandatory and stop adding files to build too
if (protoInfo != null) {
blacklistedProtos.addTransitive(protoInfo.getTransitiveProtoSources());
// TODO(yannic): Switch to getTransitiveProtoSources() and remove references to
// original proto sources when ProtoInfo is mandatory.
blacklistedProtos.addAll(protoInfo.getOriginalDirectProtoSources());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using direct instead of transitive sources un-does a change made in 01df1e5 that I think is going to break some uses of proto_lang_toolchain.blacklisted_protos.

Copy link
Contributor

@lberki lberki Jan 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is exactly what I found out after debugging the aforementioned test failures. Adding both here isn't very nice so I'll try adding the set of transitive original sources (I'd totally ask Yannic to do so, but since the tests that break are internal, he'd be flying blind...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and it blends! I'm sending this out (+my changes) for internal review.

} else {
// Only add files from FileProvider if |protos| is not a proto_library to avoid adding
// the descriptor_set of proto_library to the list of blacklisted files.
blacklistedProtos.addTransitive(protos.getProvider(FileProvider.class).getFilesToBuild());
}
}
Expand Down
77 changes: 67 additions & 10 deletions src/test/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,19 @@ public void setUp() throws Exception {
invalidatePackages();
}

private void validateProtoLangToolchain(ProtoLangToolchainProvider toolchain) throws Exception {
assertThat(toolchain.commandLine()).isEqualTo("cmd-line");
assertThat(toolchain.pluginExecutable().getExecutable().getRootRelativePathString())
.isEqualTo("x/plugin");

TransitiveInfoCollection runtimes = toolchain.runtime();
assertThat(runtimes.getLabel())
.isEqualTo(Label.parseAbsolute("//x:runtime", ImmutableMap.of()));

assertThat(prettyArtifactNames(toolchain.blacklistedProtos()))
.containsExactly("x/metadata.proto", "x/descriptor.proto", "x/any.proto");
}

@Test
public void protoToolchain() throws Exception {
scratch.file(
Expand All @@ -62,19 +75,63 @@ public void protoToolchain() throws Exception {

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

ProtoLangToolchainProvider toolchain =
getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class);
validateProtoLangToolchain(
getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class));
}

assertThat(toolchain.commandLine()).isEqualTo("cmd-line");
assertThat(toolchain.pluginExecutable().getExecutable().getRootRelativePathString())
.isEqualTo("x/plugin");
@Test
public void protoToolchainBlacklistProtoLibraries() throws Exception {
scratch.file(
"x/BUILD",
TestConstants.LOAD_PROTO_LIBRARY,
"cc_binary(name = 'plugin', srcs = ['plugin.cc'])",
"cc_library(name = 'runtime', srcs = ['runtime.cc'])",
"proto_library(name = 'descriptors', srcs = ['metadata.proto', 'descriptor.proto'])",
"proto_library(name = 'any', srcs = ['any.proto'], import_prefix = 'bar')");

TransitiveInfoCollection runtimes = toolchain.runtime();
assertThat(runtimes.getLabel())
.isEqualTo(Label.parseAbsolute("//x:runtime", ImmutableMap.of()));
scratch.file(
"foo/BUILD",
TestConstants.LOAD_PROTO_LANG_TOOLCHAIN,
"proto_lang_toolchain(",
" name = 'toolchain',",
" command_line = 'cmd-line',",
" plugin = '//x:plugin',",
" runtime = '//x:runtime',",
" blacklisted_protos = ['//x:descriptors', '//x:any']",
")");

assertThat(prettyArtifactNames(toolchain.blacklistedProtos()))
.containsExactly("x/metadata.proto", "x/descriptor.proto", "x/any.proto");
update(ImmutableList.of("//foo:toolchain"), false, 1, true, new EventBus());

validateProtoLangToolchain(
getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class));
}

@Test
public void protoToolchainMixedBlacklist() throws Exception {
scratch.file(
"x/BUILD",
TestConstants.LOAD_PROTO_LIBRARY,
"cc_binary(name = 'plugin', srcs = ['plugin.cc'])",
"cc_library(name = 'runtime', srcs = ['runtime.cc'])",
"proto_library(name = 'metadata', srcs = ['metadata.proto'])",
"proto_library(name = 'descriptor', srcs = ['descriptor.proto'], import_prefix = 'bar')",
"filegroup(name = 'any', srcs = ['any.proto'])");

scratch.file(
"foo/BUILD",
TestConstants.LOAD_PROTO_LANG_TOOLCHAIN,
"proto_lang_toolchain(",
" name = 'toolchain',",
" command_line = 'cmd-line',",
" plugin = '//x:plugin',",
" runtime = '//x:runtime',",
" blacklisted_protos = ['//x:metadata', '//x:descriptor', '//x:any']",
")");

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

validateProtoLangToolchain(
getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class));
}

@Test
Expand Down