Skip to content

Commit

Permalink
Add incompatible flag to forbid loading the native Protobuf rules
Browse files Browse the repository at this point in the history
RELNOTES: --incompatible_load_proto_rules_from_bzl was added to forbid loading the native proto rules directly. See more on tracking issue #8922

Closes #8923.

PiperOrigin-RevId: 259294885
  • Loading branch information
Yannic authored and copybara-github committed Jul 22, 2019
1 parent 26266b7 commit 2fad016
Show file tree
Hide file tree
Showing 10 changed files with 176 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@
public class BazelProtoLibrary implements RuleConfiguredTargetFactory {

@Override
public ConfiguredTarget create(RuleContext ruleContext) throws ActionConflictException {
public ConfiguredTarget create(RuleContext ruleContext)
throws ActionConflictException, RuleErrorException {
ProtoCommon.checkRuleHasValidMigrationTag(ruleContext);
ProtoInfo protoInfo =
ProtoCommon.createProtoInfo(
ruleContext,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ public Metadata getMetadata() {

/*<!-- #BLAZE_RULE (NAME = proto_library, TYPE = LIBRARY, FAMILY = Protocol Buffer) -->
<p>Deprecated. Please use <a href="https://github.com/bazelbuild/rules_proto">
https://github.com/bazelbuild/rules_proto</a> instead.
</p>
<p>Use <code>proto_library</code> to define libraries of protocol buffers
which may be used from multiple languages. A <code>proto_library</code> may be listed
in the <code>deps</code> clause of supported rules, such as <code>java_proto_library</code>.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand All @@ -46,6 +48,11 @@ private ProtoCommon() {
throw new UnsupportedOperationException();
}

// Keep in sync with the migration label in
// https://github.com/bazelbuild/rules_proto/blob/master/proto/defs.bzl.
private static final String PROTO_RULES_MIGRATION_LABEL =
"__PROTO_RULES_MIGRATION_DO_NOT_USE_WILL_BREAK__";

/**
* Gets the direct sources of a proto library. If protoSources is not empty, the value is just
* protoSources. Otherwise, it's the combined sources of all direct dependencies of the given
Expand Down Expand Up @@ -460,6 +467,28 @@ private static boolean isConfiguredTargetInSamePackage(RuleContext ruleContext,
return ruleContext.getLabel().getPackageIdentifier().equals(source.getPackageIdentifier());
}

public static void checkRuleHasValidMigrationTag(RuleContext ruleContext)
throws RuleErrorException {
if (!ruleContext.getFragment(ProtoConfiguration.class).loadProtoRulesFromBzl()) {
return;
}

if (!hasValidMigrationTag(ruleContext)) {
ruleContext.ruleError(
"The native Protobuf rules are deprecated. Please load "
+ ruleContext.getRule().getRuleClass()
+ " from the rules_proto repository."
+ " See http://github.com/bazelbuild/rules_proto.");
}
}

private static boolean hasValidMigrationTag(RuleContext ruleContext) {
return ruleContext
.attributes()
.get("tags", Type.STRING_LIST)
.contains(PROTO_RULES_MIGRATION_LABEL);
}

/**
* Creates the {@link ProtoInfo} for the {@code proto_library} rule associated with {@code
* ruleContext}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,26 @@ public static class Options extends FragmentOptions {
+ "been superseded by the strip_import_prefix= and import_prefix= attributes")
public boolean disableProtoSourceRoot;

@Option(
name = "incompatible_load_proto_rules_from_bzl",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
help =
"If enabled, direct usage of the native Protobuf rules is disabled. Please use "
+ "the Starlark rules instead at https://github.com/bazelbuild/rules_proto")
public boolean loadProtoRulesFromBzl;

@Override
public FragmentOptions getHost() {
Options host = (Options) super.getHost();
host.disableLegacyProvider = disableLegacyProvider;
host.disableProtoSourceRoot = disableProtoSourceRoot;
host.loadProtoRulesFromBzl = loadProtoRulesFromBzl;
host.protoCompiler = protoCompiler;
host.protocOpts = protocOpts;
host.experimentalProtoExtraActions = experimentalProtoExtraActions;
Expand Down Expand Up @@ -320,4 +335,8 @@ public boolean doNotUseBuggyImportPath() {
public boolean generatedProtosInVirtualImports() {
return options.generatedProtosInVirtualImports;
}

public boolean loadProtoRulesFromBzl() {
return options.loadProtoRulesFromBzl;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public class ProtoLangToolchain implements RuleConfiguredTargetFactory {
@Override
public ConfiguredTarget create(RuleContext ruleContext)
throws InterruptedException, RuleErrorException, ActionConflictException {
ProtoCommon.checkRuleHasValidMigrationTag(ruleContext);
NestedSetBuilder<Artifact> blacklistedProtos = NestedSetBuilder.stableOrder();
for (TransitiveInfoCollection protos :
ruleContext.getPrerequisites("blacklisted_protos", TARGET)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
.allowedFileTypes()
.mandatoryNativeProviders(
ImmutableList.<Class<? extends TransitiveInfoProvider>>of(FileProvider.class)))
.requiresConfigurationFragments(ProtoConfiguration.class)
.advertiseProvider(ProtoLangToolchainProvider.class)
.removeAttribute("data")
.removeAttribute("deps")
Expand All @@ -90,6 +91,10 @@ public Metadata getMetadata() {

/*<!-- #BLAZE_RULE (NAME = proto_lang_toolchain, TYPE = LIBRARY, FAMILY = Protocol Buffer) -->
<p>Deprecated. Please <a href="https://github.com/bazelbuild/rules_proto">
https://github.com/bazelbuild/rules_proto</a> instead.
</p>
<p>
Specifies how a LANG_proto_library rule (e.g., <code>java_proto_library</code>) should invoke the
proto-compiler.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,16 @@
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;

/** Info object propagating information about protocol buffer sources. */
@SkylarkModule(name = "ProtoInfo", doc = "")
@SkylarkModule(
name = "ProtoInfo",
doc =
"Encapsulates information provided by <a href=\""
+ "../../be/protocol-buffer.html#proto_library\">proto_library.</a>"
+ "<p>"
+ "Please consider using `load(\"@rules_proto//proto:defs.bzl\", \"ProtoInfo\")` "
+ "to load this symbol from <a href=\"https://github.com/bazelbuild/rules_proto.\">"
+ "rules_proto</a>"
+ "</p>")
public interface ProtoInfoApi<FileT extends FileApi> extends StructApi {
/** Provider class for {@link ProtoInfoApi} objects. */
@SkylarkModule(name = "Provider", documented = false, doc = "")
Expand Down Expand Up @@ -62,21 +71,19 @@ interface Provider extends ProviderApi {
name = "direct_descriptor_set",
doc =
"The <a href=\""
+ "https://github.com/google/protobuf/search?q=%22message+FileDescriptorSet%22+path%3A%2Fsrc"
+ "\">FileDescriptorSet</a> of the direct sources. "
+ "If no srcs, contains an empty file.",
+ "https://github.com/google/protobuf/search?q=%22message+FileDescriptorSet%22+path%3A%2Fsrc\">FileDescriptorSet</a>"
+ " of the direct sources. If no srcs, contains an empty file.",
structField = true)
public FileT getDirectDescriptorSet();

@SkylarkCallable(
name = "transitive_descriptor_sets",
doc =
"A set of <a href=\""
+ "https://github.com/google/protobuf/search?q=%22message+FileDescriptorSet%22+path%3A%2Fsrc"
+ "\">FileDescriptorSet</a> files of all dependent proto_library rules, "
+ "and this one's. "
+ "This is not the same as passing --include_imports to proto-compiler. "
+ "Will be empty if no dependencies.",
+ "https://github.com/google/protobuf/search?q=%22message+FileDescriptorSet%22+path%3A%2Fsrc\">FileDescriptorSet</a>"
+ " files of all dependent proto_library rules, and this one's. This is not the same"
+ " as passing --include_imports to proto-compiler. Will be empty if no"
+ " dependencies.",
structField = true)
public NestedSet<FileT> getTransitiveDescriptorSets();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,13 @@
*
* <p>Currently just a placeholder so that we have a {@code proto_common} symbol.
*/
@SkylarkModule(name = "proto_common", doc = "Utilities for protocol buffers")
@SkylarkModule(
name = "proto_common",
doc =
"Utilities for protocol buffers. "
+ "<p>"
+ "Please consider using `load(\"@rules_proto//proto:defs.bzl\", \"proto_common\")` "
+ "to load this symbol from <a href=\"https://github.com/bazelbuild/rules_proto.\">"
+ "rules_proto</a>"
+ "</p>")
public interface ProtoModuleApi {}
Original file line number Diff line number Diff line change
Expand Up @@ -947,4 +947,50 @@ private Artifact getDescriptorOutput(String label) throws Exception {
private Action getDescriptorWriteAction(String label) throws Exception {
return getGeneratingAction(getDescriptorOutput(label));
}

@Test
public void testMigrationLabel() throws Exception {
if (!isThisBazel()) {
return;
}

useConfiguration("--incompatible_load_proto_rules_from_bzl");
scratch.file(
"a/BUILD",
"proto_library(",
" name = 'a',",
" srcs = ['a.proto'],",
// Don't use |ProtoCommon.PROTO_RULES_MIGRATION_LABEL| here
// so we don't accidentally change it without breaking a local test.
" tags = ['__PROTO_RULES_MIGRATION_DO_NOT_USE_WILL_BREAK__'],",
")");

getConfiguredTarget("//a");
}

@Test
public void testMissingMigrationLabel() throws Exception {
if (!isThisBazel()) {
return;
}

useConfiguration("--incompatible_load_proto_rules_from_bzl");
scratch.file("a/BUILD", "proto_library(", " name = 'a',", " srcs = ['a.proto'],", ")");

reporter.removeHandler(failFastHandler);
getConfiguredTarget("//a");
assertContainsEvent("The native Protobuf rules are deprecated.");
}

@Test
public void testMigrationLabelNotRequiredWhenDisabled() throws Exception {
if (!isThisBazel()) {
return;
}

useConfiguration("--noincompatible_load_proto_rules_from_bzl");
scratch.file("a/BUILD", "proto_library(", " name = 'a',", " srcs = ['a.proto'],", ")");

getConfiguredTarget("//a");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,48 @@ public void optionalFieldsAreEmpty() throws Exception {
assertThat(toolchain.runtime()).isNull();
assertThat(toolchain.blacklistedProtos()).isEmpty();
}

@Test
public void testMigrationLabel() throws Exception {
useConfiguration("--incompatible_load_proto_rules_from_bzl");
scratch.file(
"a/BUILD",
"proto_lang_toolchain(",
" name = 'toolchain',",
" command_line = 'cmd-line',",
// Don't use |ProtoCommon.PROTO_RULES_MIGRATION_LABEL| here
// so we don't accidentally change it without breaking a local test.
" tags = ['__PROTO_RULES_MIGRATION_DO_NOT_USE_WILL_BREAK__'],",
")");

getConfiguredTarget("//a:toolchain");
}

@Test
public void testMissingMigrationLabel() throws Exception {
useConfiguration("--incompatible_load_proto_rules_from_bzl");
scratch.file(
"a/BUILD",
"proto_lang_toolchain(",
" name = 'toolchain',",
" command_line = 'cmd-line',",
")");

reporter.removeHandler(failFastHandler);
getConfiguredTarget("//a:toolchain");
assertContainsEvent("The native Protobuf rules are deprecated.");
}

@Test
public void testMigrationLabelNotRequiredWhenDisabled() throws Exception {
useConfiguration("--noincompatible_load_proto_rules_from_bzl");
scratch.file(
"a/BUILD",
"proto_lang_toolchain(",
" name = 'toolchain',",
" command_line = 'cmd-line',",
")");

getConfiguredTarget("//a:toolchain");
}
}

0 comments on commit 2fad016

Please sign in to comment.