From e43825d0bef359f645e1cabf2164fd2db6ee4a35 Mon Sep 17 00:00:00 2001 From: Philipp Wollermann Date: Thu, 10 Dec 2020 11:28:51 +0100 Subject: [PATCH] Revert "Remove --incompatible_blacklisted_protos_requires_proto_info" This reverts commit fe4a680b89415ec9e8c2a7ea26466f2f47b474e8. --- .../lib/rules/proto/ProtoConfiguration.java | 18 +++++++++ .../lib/rules/proto/ProtoLangToolchain.java | 21 ++++++++-- .../rules/proto/ProtoLangToolchainRule.java | 7 +++- .../rules/proto/ProtoLangToolchainTest.java | 38 +++++++++++++++++++ 4 files changed, 79 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java index 6965865fe36f2e..b0994fefbfe3d0 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java @@ -168,6 +168,19 @@ public static class Options extends FragmentOptions { help = "If true, add --allowed_public_imports to the java compile actions.") public boolean experimentalJavaProtoAddAllowedPublicImports; + @Option( + name = "incompatible_blacklisted_protos_requires_proto_info", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = + "If enabled, 'proto_lang_toolchain.blacklisted_protos' requires provider 'ProtoInfo'") + public boolean blacklistedProtosRequiresProtoInfo; + @Override public FragmentOptions getHost() { Options host = (Options) super.getHost(); @@ -187,6 +200,7 @@ public FragmentOptions getHost() { host.experimentalJavaProtoAddAllowedPublicImports = experimentalJavaProtoAddAllowedPublicImports; host.generatedProtosInVirtualImports = generatedProtosInVirtualImports; + host.blacklistedProtosRequiresProtoInfo = blacklistedProtosRequiresProtoInfo; return host; } } @@ -260,4 +274,8 @@ public boolean strictPublicImports() { public boolean generatedProtosInVirtualImports() { return options.generatedProtosInVirtualImports; } + + public boolean blacklistedProtosRequiresProtoInfo() { + return options.blacklistedProtosRequiresProtoInfo; + } } 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 e5911c71a6c6c0..db9c3ebdfed7d6 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 @@ -19,12 +19,14 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder; import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.Runfiles; import com.google.devtools.build.lib.analysis.RunfilesProvider; +import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.packages.Type; @@ -34,9 +36,22 @@ public class ProtoLangToolchain implements RuleConfiguredTargetFactory { public ConfiguredTarget create(RuleContext ruleContext) throws InterruptedException, RuleErrorException, ActionConflictException { NestedSetBuilder blacklistedProtos = NestedSetBuilder.stableOrder(); - for (ProtoInfo protoInfo : - ruleContext.getPrerequisites("blacklisted_protos", ProtoInfo.PROVIDER)) { - blacklistedProtos.addTransitive(protoInfo.getOriginalTransitiveProtoSources()); + for (TransitiveInfoCollection protos : ruleContext.getPrerequisites("blacklisted_protos")) { + ProtoInfo protoInfo = protos.get(ProtoInfo.PROVIDER); + if (protoInfo == null + && ruleContext + .getFragment(ProtoConfiguration.class) + .blacklistedProtosRequiresProtoInfo()) { + ruleContext.ruleError( + "'" + ruleContext.getLabel() + "' does not have mandatory provider 'ProtoInfo'."); + } + if (protoInfo != null) { + 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()); + } } return new RuleConfiguredTargetBuilder(ruleContext) diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java index c1a1ddaeab0a28..610a1768b53dc1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoLangToolchainRule.java @@ -18,12 +18,14 @@ import static com.google.devtools.build.lib.packages.BuildType.LABEL; import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.BaseRuleClasses; +import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory; +import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; import com.google.devtools.build.lib.packages.RuleClass; -import com.google.devtools.build.lib.packages.StarlarkProviderIdentifier; import com.google.devtools.build.lib.packages.Type; /** Implements {code proto_lang_toolchain}. */ @@ -72,7 +74,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi .add( attr("blacklisted_protos", LABEL_LIST) .allowedFileTypes() - .mandatoryProviders(StarlarkProviderIdentifier.forKey(ProtoInfo.PROVIDER.getKey()))) + .mandatoryBuiltinProviders( + ImmutableList.>of(FileProvider.class))) .requiresConfigurationFragments(ProtoConfiguration.class) .advertiseProvider(ProtoLangToolchainProvider.class) .removeAttribute("data") 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 c75aace52a95fb..c9e32316d6dc88 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 @@ -140,6 +140,44 @@ public void protoToolchainBlacklistTransitiveProtos() throws Exception { getConfiguredTarget("//foo:toolchain").getProvider(ProtoLangToolchainProvider.class)); } + @Test + public void protoToolchainMixedBlacklist() throws Exception { + // Tests legacy behaviour. + useConfiguration("--incompatible_blacklisted_protos_requires_proto_info=false"); + + 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 public void optionalFieldsAreEmpty() throws Exception { scratch.file(