From 66c60121b980b2e1e0baa330eca76840f09f243c Mon Sep 17 00:00:00 2001 From: Yannic Date: Mon, 13 Jan 2020 03:45:41 -0800 Subject: [PATCH 1/2] proto_lang_toolchain: Add original sources of ProtoInfo to blacklistedProtos Fixes #10484 Closes #10493. PiperOrigin-RevId: 289411825 --- .../lib/rules/cpp/proto/CcProtoAspect.java | 2 +- .../build/lib/rules/proto/ProtoCommon.java | 17 +++- .../build/lib/rules/proto/ProtoInfo.java | 17 ++-- .../lib/rules/proto/ProtoLangToolchain.java | 4 +- .../proto/ProtoCompileActionBuilderTest.java | 4 +- .../rules/proto/ProtoLangToolchainTest.java | 98 ++++++++++++++++--- 6 files changed, 115 insertions(+), 27 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java index 1014c3977619f4..417a766dd83411 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java @@ -276,7 +276,7 @@ private static class Impl { private boolean areSrcsBlacklisted() { return !new ProtoSourceFileBlacklist( ruleContext, getProtoToolchainProvider().blacklistedProtos()) - .checkSrcs(protoInfo.getOriginalDirectProtoSources(), "cc_proto_library"); + .checkSrcs(protoInfo.getOriginalTransitiveProtoSources(), "cc_proto_library"); } private FeatureConfiguration getFeatureConfiguration() { diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java index b75ec62ccb2243..a7e8dcd22a21f6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java @@ -127,6 +127,19 @@ private static NestedSet computeTransitiveProtoSources( return result.build(); } + private static NestedSet computeTransitiveOriginalProtoSources( + RuleContext ruleContext, ImmutableList originalProtoSources) { + NestedSetBuilder result = NestedSetBuilder.naiveLinkOrder(); + + result.addAll(originalProtoSources); + + for (ProtoInfo dep : ruleContext.getPrerequisites("deps", Mode.TARGET, ProtoInfo.PROVIDER)) { + result.addTransitive(dep.getOriginalTransitiveProtoSources()); + } + + return result.build(); + } + static NestedSet computeDependenciesDescriptorSets(RuleContext ruleContext) { NestedSetBuilder result = NestedSetBuilder.stableOrder(); @@ -462,6 +475,8 @@ public static ProtoInfo createProtoInfo( NestedSet transitiveProtoSources = computeTransitiveProtoSources(ruleContext, library.getSources()); + NestedSet transitiveOriginalProtoSources = + computeTransitiveOriginalProtoSources(ruleContext, directProtoSources); NestedSet transitiveProtoSourceRoots = computeTransitiveProtoSourceRoots(ruleContext, library.getSourceRoot()); @@ -491,8 +506,8 @@ public static ProtoInfo createProtoInfo( ProtoInfo protoInfo = new ProtoInfo( library.getSources(), - directProtoSources, library.getSourceRoot(), + transitiveOriginalProtoSources, transitiveProtoSources, transitiveProtoSourceRoots, strictImportableProtosForDependents, diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java index f9a9280e539b39..7621e22c74602a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java @@ -54,7 +54,7 @@ public Provider() { public static final String LEGACY_SKYLARK_NAME = "proto"; private final ImmutableList directProtoSources; - private final ImmutableList originalDirectProtoSources; + private final NestedSet originalTransitiveProtoSources; private final String directProtoSourceRoot; private final NestedSet transitiveProtoSources; private final NestedSet transitiveProtoSourceRoots; @@ -71,8 +71,8 @@ public Provider() { @AutoCodec.Instantiator public ProtoInfo( ImmutableList directProtoSources, - ImmutableList originalDirectProtoSources, String directProtoSourceRoot, + NestedSet originalTransitiveProtoSources, NestedSet transitiveProtoSources, NestedSet transitiveProtoSourceRoots, NestedSet strictImportableProtoSourcesForDependents, @@ -86,7 +86,7 @@ public ProtoInfo( Location location) { super(PROVIDER, location); this.directProtoSources = directProtoSources; - this.originalDirectProtoSources = originalDirectProtoSources; + this.originalTransitiveProtoSources = originalTransitiveProtoSources; this.directProtoSourceRoot = directProtoSourceRoot; this.transitiveProtoSources = transitiveProtoSources; this.transitiveProtoSourceRoots = transitiveProtoSourceRoots; @@ -103,17 +103,18 @@ public ProtoInfo( /** * The proto source files that are used in compiling this {@code proto_library}. - * - *

Different from {@link #getOriginalDirectProtoSources()} when a virtual import root is used. */ @Override public ImmutableList getDirectProtoSources() { return directProtoSources; } - /** The proto sources of the {@code proto_library} declaring this provider. */ - public ImmutableList getOriginalDirectProtoSources() { - return originalDirectProtoSources; + /** + * The non-virtual transitive proto source files. Different from {@link + * #getTransitiveProtoSources()} if a transitive dependency has {@code import_prefix} or the like. + */ + public NestedSet getOriginalTransitiveProtoSources() { + return originalTransitiveProtoSources; } /** The source root of the current library. */ diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java index ea124b56ee52cb..7c16d8d3989abd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchain.java @@ -44,8 +44,10 @@ 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()); + blacklistedProtos.addTransitive(protoInfo.getOriginalTransitiveProtoSources()); } 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()); } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java index 72c0dc66b6ccc1..6d4fd31cc601c2 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java @@ -65,8 +65,8 @@ private ProtoInfo protoInfo( NestedSet> exportedProtos) { return new ProtoInfo( directProtos, - directProtos, - "", + /* directProtoSourceRoot= */ "", + transitiveProtos, transitiveProtos, transitiveProtoSourceRoots, /* strictImportableProtosForDependents */ NestedSetBuilder.emptySet(Order.STABLE_ORDER), diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainTest.java index 2ff655a022ed51..21ef5ef269cd28 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainTest.java @@ -39,10 +39,27 @@ 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("third_party/x/plugin"); + + TransitiveInfoCollection runtimes = toolchain.runtime(); + assertThat(runtimes.getLabel()) + .isEqualTo(Label.parseAbsolute("//third_party/x:runtime", ImmutableMap.of())); + + assertThat(prettyArtifactNames(toolchain.blacklistedProtos())) + .containsExactly( + "third_party/x/metadata.proto", + "third_party/x/descriptor.proto", + "third_party/x/any.proto"); + } + @Test public void protoToolchain() throws Exception { scratch.file( - "x/BUILD", + "third_party/x/BUILD", + "licenses(['unencumbered'])", "cc_binary(name = 'plugin', srcs = ['plugin.cc'])", "cc_library(name = 'runtime', srcs = ['runtime.cc'])", "filegroup(name = 'descriptors', srcs = ['metadata.proto', 'descriptor.proto'])", @@ -52,29 +69,82 @@ public void protoToolchain() throws Exception { scratch.file( "foo/BUILD", TestConstants.LOAD_PROTO_LANG_TOOLCHAIN, + "licenses(['unencumbered'])", "proto_lang_toolchain(", " name = 'toolchain',", " command_line = 'cmd-line',", - " plugin = '//x:plugin',", - " runtime = '//x:runtime',", - " blacklisted_protos = ['//x:blacklist']", + " plugin = '//third_party/x:plugin',", + " runtime = '//third_party/x:runtime',", + " blacklisted_protos = ['//third_party/x:blacklist']", ")"); 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( + "third_party/x/BUILD", + TestConstants.LOAD_PROTO_LIBRARY, + "licenses(['unencumbered'])", + "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'], strip_import_prefix = '/third_party')"); - 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 = '//third_party/x:plugin',", + " runtime = '//third_party/x:runtime',", + " blacklisted_protos = ['//third_party/x:descriptors', '//third_party/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( + "third_party/x/BUILD", + TestConstants.LOAD_PROTO_LIBRARY, + "licenses(['unencumbered'])", + "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'],", + " strip_import_prefix = '/third_party')", + "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 = '//third_party/x:plugin',", + " runtime = '//third_party/x:runtime',", + " blacklisted_protos = [", + " '//third_party/x:metadata',", + " '//third_party/x:descriptor',", + " '//third_party/x:any']", + ")"); + + update(ImmutableList.of("//foo:toolchain"), false, 1, true, new EventBus()); + + validateProtoLangToolchain( + getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class)); } @Test From 8c92cee633902e95f8b1b2cedf98e9934dc3c5fb Mon Sep 17 00:00:00 2001 From: Yannic Bonenberger Date: Wed, 15 Jan 2020 15:13:52 +0100 Subject: [PATCH 2/2] Fix problem reported in 10588 --- .../lib/rules/cpp/proto/CcProtoAspect.java | 2 +- .../build/lib/rules/proto/ProtoCommon.java | 3 +- .../build/lib/rules/proto/ProtoInfo.java | 30 +++++++++++---- .../rules/cpp/proto/CcProtoLibraryTest.java | 37 ++++++++++++++++++- .../proto/ProtoCompileActionBuilderTest.java | 1 + 5 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java index 417a766dd83411..1014c3977619f4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java @@ -276,7 +276,7 @@ private static class Impl { private boolean areSrcsBlacklisted() { return !new ProtoSourceFileBlacklist( ruleContext, getProtoToolchainProvider().blacklistedProtos()) - .checkSrcs(protoInfo.getOriginalTransitiveProtoSources(), "cc_proto_library"); + .checkSrcs(protoInfo.getOriginalDirectProtoSources(), "cc_proto_library"); } private FeatureConfiguration getFeatureConfiguration() { diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java index a7e8dcd22a21f6..a0eb92a1996b45 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java @@ -506,9 +506,10 @@ public static ProtoInfo createProtoInfo( ProtoInfo protoInfo = new ProtoInfo( library.getSources(), + directProtoSources, library.getSourceRoot(), - transitiveOriginalProtoSources, transitiveProtoSources, + transitiveOriginalProtoSources, transitiveProtoSourceRoots, strictImportableProtosForDependents, strictImportableProtos, diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java index 7621e22c74602a..9a5c17bd249b07 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoInfo.java @@ -54,9 +54,10 @@ public Provider() { public static final String LEGACY_SKYLARK_NAME = "proto"; private final ImmutableList directProtoSources; - private final NestedSet originalTransitiveProtoSources; + private final ImmutableList originalDirectProtoSources; private final String directProtoSourceRoot; private final NestedSet transitiveProtoSources; + private final NestedSet originalTransitiveProtoSources; private final NestedSet transitiveProtoSourceRoots; private final NestedSet strictImportableProtoSourcesForDependents; private final NestedSet> strictImportableProtoSourcesImportPaths; @@ -71,9 +72,10 @@ public Provider() { @AutoCodec.Instantiator public ProtoInfo( ImmutableList directProtoSources, + ImmutableList originalDirectProtoSources, String directProtoSourceRoot, - NestedSet originalTransitiveProtoSources, NestedSet transitiveProtoSources, + NestedSet originalTransitiveProtoSources, NestedSet transitiveProtoSourceRoots, NestedSet strictImportableProtoSourcesForDependents, NestedSet> strictImportableProtoSourcesImportPaths, @@ -86,9 +88,10 @@ public ProtoInfo( Location location) { super(PROVIDER, location); this.directProtoSources = directProtoSources; - this.originalTransitiveProtoSources = originalTransitiveProtoSources; + this.originalDirectProtoSources = originalDirectProtoSources; this.directProtoSourceRoot = directProtoSourceRoot; this.transitiveProtoSources = transitiveProtoSources; + this.originalTransitiveProtoSources = originalTransitiveProtoSources; this.transitiveProtoSourceRoots = transitiveProtoSourceRoots; this.strictImportableProtoSourcesForDependents = strictImportableProtoSourcesForDependents; this.strictImportableProtoSourcesImportPaths = strictImportableProtoSourcesImportPaths; @@ -110,11 +113,13 @@ public ImmutableList getDirectProtoSources() { } /** - * The non-virtual transitive proto source files. Different from {@link - * #getTransitiveProtoSources()} if a transitive dependency has {@code import_prefix} or the like. + * The non-virtual proto sources of the {@code proto_library} declaring this provider. + * + *

Different from {@link #getDirectProtoSources()} if a transitive dependency has + * {@code import_prefix} or the like. */ - public NestedSet getOriginalTransitiveProtoSources() { - return originalTransitiveProtoSources; + public ImmutableList getOriginalDirectProtoSources() { + return originalDirectProtoSources; } /** The source root of the current library. */ @@ -133,6 +138,17 @@ public NestedSet getTransitiveProtoSources() { return transitiveProtoSources; } + /** + * The non-virtual transitive proto source files. + * + *

Different from {@link #getTransitiveProtoSources()} if a transitive dependency has + * {@code import_prefix} or the like. + */ + public NestedSet getOriginalTransitiveProtoSources() { + return originalTransitiveProtoSources; + } + + /** * The proto source roots of the transitive closure of this rule. These flags will be passed to * {@code protoc} in the specified order, via the {@code --proto_path} flag. diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoLibraryTest.java index 521ba1a4420950..025de7d8428d77 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoLibraryTest.java @@ -52,12 +52,17 @@ public void setUp() throws Exception { scratch.overwriteFile( "protobuf/BUILD", TestConstants.LOAD_PROTO_LANG_TOOLCHAIN, + TestConstants.LOAD_PROTO_LIBRARY, "package(default_visibility=['//visibility:public'])", "exports_files(['protoc'])", + "proto_library(", + " name = 'any_proto',", + " srcs = ['any.proto'],", + ")", "proto_lang_toolchain(", " name = 'cc_toolchain',", " command_line = '--cpp_out=$(OUT)',", - " blacklisted_protos = [],", + " blacklisted_protos = [':any_proto'],", ")"); scratch.appendFile( "WORKSPACE", @@ -140,6 +145,36 @@ public void aliasProtos() throws Exception { .containsExactly("x/foo.pb.h"); } + @Test + public void blacklistedProtos() throws Exception { + scratch.file( + "x/BUILD", + TestConstants.LOAD_PROTO_LIBRARY, + "cc_proto_library(name = 'any_cc_proto', deps = ['@com_google_protobuf//:any_proto'])"); + + CcCompilationContext ccCompilationContext = + getConfiguredTarget("//x:any_cc_proto").get(CcInfo.PROVIDER).getCcCompilationContext(); + assertThat(prettyArtifactNames(ccCompilationContext.getDeclaredIncludeSrcs())).isEmpty(); + } + + @Test + public void blacklistedProtosInTransitiveDeps() throws Exception { + scratch.file( + "x/BUILD", + TestConstants.LOAD_PROTO_LIBRARY, + "cc_proto_library(name = 'foo_cc_proto', deps = ['foo_proto'])", + "proto_library(", + " name = 'foo_proto',", + " srcs = ['foo.proto'],", + " deps = ['@com_google_protobuf//:any_proto'],", + ")"); + + CcCompilationContext ccCompilationContext = + getConfiguredTarget("//x:foo_cc_proto").get(CcInfo.PROVIDER).getCcCompilationContext(); + assertThat(prettyArtifactNames(ccCompilationContext.getDeclaredIncludeSrcs())) + .containsExactly("x/foo.pb.h"); + } + @Test public void ccCompilationContext() throws Exception { scratch.file( diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java index 6d4fd31cc601c2..c37f408e076df4 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java @@ -64,6 +64,7 @@ private ProtoInfo protoInfo( NestedSet> strictImportableProtos, NestedSet> exportedProtos) { return new ProtoInfo( + directProtos, directProtos, /* directProtoSourceRoot= */ "", transitiveProtos,