Skip to content

Commit

Permalink
Flip the flag --incompatible_disable_proto_source_root.
Browse files Browse the repository at this point in the history 
Progress towards #7153.

Technically, it's a *fix* to the above bug but I'd like to keep it open until the code supporting the unflipped version of the flag is removed and the flag is moved to the graveyard.

RELNOTES: None.
PiperOrigin-RevId: 262309528
  • Loading branch information
@lberki @copybara-github
lberki authored and copybara-github committed Aug 8, 2019
1 parent 455f594 commit e4b5841
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 142 deletions.
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ public static class Options extends FragmentOptions {

@Option(
name = "incompatible_disable_proto_source_root",
defaultValue = "false",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {
Expand Down
13 changes: 6 additions & 7 deletions src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoInfoStarlarkTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,8 @@ public void testProvider() throws Exception {

@Test
public void testProtoSourceRootExportedInStarlark() throws Exception {

scratch.file(
"foo/myTestRule.bzl",
"third_party/foo/myTestRule.bzl",
"load('//myinfo:myinfo.bzl', 'MyInfo')",
"",
"def _my_test_rule_impl(ctx):",
Expand All @@ -178,8 +177,8 @@ public void testProtoSourceRootExportedInStarlark() throws Exception {
")");

scratch.file(
"foo/BUILD",
"",
"third_party/foo/BUILD",
"licenses(['unencumbered'])",
"load(':myTestRule.bzl', 'my_test_rule')",
"my_test_rule(",
" name = 'myRule',",
Expand All @@ -188,13 +187,13 @@ public void testProtoSourceRootExportedInStarlark() throws Exception {
"proto_library(",
" name = 'myProto',",
" srcs = ['myProto.proto'],",
" proto_source_root = 'foo',",
" strip_import_prefix = '/third_party/foo',",
")");

ConfiguredTarget ct = getConfiguredTarget("//foo:myRule");
ConfiguredTarget ct = getConfiguredTarget("//third_party/foo:myRule");
String protoSourceRoot = (String) getMyInfoFromTarget(ct).getValue("fetched_proto_source_root");
String genfiles = getTargetConfiguration().getGenfilesFragment().toString();

assertThat(protoSourceRoot).isEqualTo(genfiles + "/foo/_virtual_imports/myProto");
assertThat(protoSourceRoot).isEqualTo(genfiles + "/third_party/foo/_virtual_imports/myProto");
}
}
150 changes: 38 additions & 112 deletions src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,110 +267,93 @@ public void testDisableProtoSourceRoot() throws Exception {
}

@Test
public void testProtoSourceRootWithoutDeps() throws Exception {
public void testStripImportPrefixWithoutDeps() throws Exception {
scratch.file(
"x/foo/BUILD",
"third_party/x/foo/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"licenses(['unencumbered'])",
"proto_library(",
" name = 'nodeps',",
" srcs = ['foo/nodeps.proto'],",
" proto_source_root = 'x/foo',",
" strip_import_prefix = '/third_party/x/foo',",
")");
ConfiguredTarget protoTarget = getConfiguredTarget("//x/foo:nodeps");
ConfiguredTarget protoTarget = getConfiguredTarget("//third_party/x/foo:nodeps");
ProtoInfo sourcesProvider = protoTarget.get(ProtoInfo.PROVIDER);
String genfiles = getTargetConfiguration().getGenfilesFragment().toString();

assertThat(sourcesProvider.getTransitiveProtoSourceRoots())
.containsExactly(genfiles + "/x/foo/_virtual_imports/nodeps");
.containsExactly(genfiles + "/third_party/x/foo/_virtual_imports/nodeps");
assertThat(
getGeneratingSpawnAction(getDescriptorOutput("//x/foo:nodeps")).getRemainingArguments())
.contains("--proto_path=" + genfiles + "/x/foo/_virtual_imports/nodeps");
}

@Test
public void testProtoSourceRootWithoutDeps_notPackageName() throws Exception {
scratch.file(
"x/foo/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"proto_library(",
" name = 'nodeps',",
" srcs = ['foo/nodeps.proto'],",
" proto_source_root = 'something/else',",
")");

try {
getConfiguredTarget("//x/foo:nodeps");
} catch (AssertionError error) {
assertThat(error)
.hasMessageThat()
.contains("proto_source_root must be the same as the package name (x/foo)");
return;
}
throw new Exception("Target should have failed building.");
getGeneratingSpawnAction(getDescriptorOutput("//third_party/x/foo:nodeps"))
.getRemainingArguments())
.contains("--proto_path=" + genfiles + "/third_party/x/foo/_virtual_imports/nodeps");
}

@Test
public void testProtoSourceRootWithDepsDuplicate() throws Exception {
scratch.file(
"x/foo/BUILD",
"third_party/x/foo/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"licenses(['unencumbered'])",
"proto_library(",
" name = 'withdeps',",
" srcs = ['foo/withdeps.proto'],",
" proto_source_root = 'x/foo',",
" strip_import_prefix = '/third_party/x/foo',",
" deps = [':dep'],",
")",
"proto_library(",
" name = 'dep',",
" srcs = ['foo/dep.proto'],",
" proto_source_root = 'x/foo',",
" strip_import_prefix = '/third_party/x/foo',",
")");
ConfiguredTarget protoTarget = getConfiguredTarget("//x/foo:withdeps");
ConfiguredTarget protoTarget = getConfiguredTarget("//third_party/x/foo:withdeps");
ProtoInfo sourcesProvider = protoTarget.get(ProtoInfo.PROVIDER);
String genfiles = getTargetConfiguration().getGenfilesFragment().toString();
assertThat(sourcesProvider.getTransitiveProtoSourceRoots())
.containsExactly(
genfiles + "/x/foo/_virtual_imports/dep",
genfiles + "/x/foo/_virtual_imports/withdeps");
genfiles + "/third_party/x/foo/_virtual_imports/dep",
genfiles + "/third_party/x/foo/_virtual_imports/withdeps");

assertThat(
getGeneratingSpawnAction(getDescriptorOutput("//x/foo:withdeps"))
getGeneratingSpawnAction(getDescriptorOutput("//third_party/x/foo:withdeps"))
.getRemainingArguments())
.containsAtLeast(
"--proto_path=" + genfiles + "/x/foo/_virtual_imports/withdeps",
"--proto_path=" + genfiles + "/x/foo/_virtual_imports/dep");
"--proto_path=" + genfiles + "/third_party/x/foo/_virtual_imports/withdeps",
"--proto_path=" + genfiles + "/third_party/x/foo/_virtual_imports/dep");
}

@Test
public void testProtoSourceRootWithDeps() throws Exception {
public void testStripImportPrefixWithDeps() throws Exception {
scratch.file(
"x/foo/BUILD",
"third_party/x/foo/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"licenses(['unencumbered'])",
"proto_library(",
" name = 'withdeps',",
" srcs = ['foo/withdeps.proto'],",
" proto_source_root = 'x/foo',",
" deps = ['//x/bar:dep', ':dep'],",
" strip_import_prefix = '/third_party/x/foo',",
" deps = ['//third_party/x/bar:dep', ':dep'],",
")",
"proto_library(",
" name = 'dep',",
" srcs = ['foo/dep.proto'],",
")");
scratch.file(
"x/bar/BUILD",
"third_party/x/bar/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"licenses(['unencumbered'])",
"proto_library(",
" name = 'dep',",
" srcs = ['foo/dep.proto'],",
" proto_source_root = 'x/bar',",
" strip_import_prefix = '/third_party/x/bar',",
")");
ConfiguredTarget protoTarget = getConfiguredTarget("//x/foo:withdeps");
ConfiguredTarget protoTarget = getConfiguredTarget("//third_party/x/foo:withdeps");
ProtoInfo sourcesProvider = protoTarget.get(ProtoInfo.PROVIDER);
String genfiles = getTargetConfiguration().getGenfilesFragment().toString();
assertThat(sourcesProvider.getTransitiveProtoSourceRoots())
.containsExactly(
genfiles + "/x/foo/_virtual_imports/withdeps",
genfiles + "/x/bar/_virtual_imports/dep",
genfiles + "/third_party/x/foo/_virtual_imports/withdeps",
genfiles + "/third_party/x/bar/_virtual_imports/dep",
".");
}

Expand Down Expand Up @@ -409,33 +392,33 @@ public void testExternalRepoWithGeneratedProto() throws Exception {
}

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

scratch.file(
"ad/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"proto_library(name='ad', proto_source_root='ad', srcs=['ad.proto'])");
"proto_library(name='ad', strip_import_prefix='/ad', srcs=['ad.proto'])");
scratch.file(
"ae/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"proto_library(name='ae', proto_source_root='ae', srcs=['ae.proto'])");
"proto_library(name='ae', strip_import_prefix='/ae', srcs=['ae.proto'])");
scratch.file(
"bd/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"proto_library(name='bd', proto_source_root='bd', srcs=['bd.proto'])");
"proto_library(name='bd', strip_import_prefix='/bd', srcs=['bd.proto'])");
scratch.file(
"be/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"proto_library(name='be', proto_source_root='be', srcs=['be.proto'])");
"proto_library(name='be', strip_import_prefix='/be', srcs=['be.proto'])");
scratch.file(
"a/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"proto_library(",
" name='a',",
" proto_source_root='a',",
" strip_import_prefix='/a',",
" srcs=['a.proto'],",
" exports=['//ae:ae'],",
" deps=['//ad:ad'])");
Expand All @@ -444,7 +427,7 @@ public void testExportedProtoSourceRoots() throws Exception {
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"proto_library(",
" name='b',",
" proto_source_root='b',",
" strip_import_prefix='/b',",
" srcs=['b.proto'],",
" exports=['//be:be'],",
" deps=['//bd:bd'])");
Expand All @@ -453,7 +436,7 @@ public void testExportedProtoSourceRoots() throws Exception {
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"proto_library(",
" name='c',",
" proto_source_root='c',",
" strip_import_prefix='/c',",
" srcs=['c.proto'],",
" exports=['//a:a'],",
" deps=['//b:b'])");
Expand All @@ -467,63 +450,6 @@ public void testExportedProtoSourceRoots() throws Exception {
.containsExactly(genfiles + "/a/_virtual_imports/a", genfiles + "/c/_virtual_imports/c");
}

@Test
public void testProtoSourceRoot() throws Exception {
scratch.file(
"x/foo/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"proto_library(",
" name = 'banana',",
" srcs = ['foo.proto'],",
" proto_source_root = 'x/foo',",
")");

ConfiguredTarget protoTarget = getConfiguredTarget("//x/foo:banana");
ProtoInfo sourcesProvider = protoTarget.get(ProtoInfo.PROVIDER);

String genfiles = getTargetConfiguration().getGenfilesFragment().toString();

assertThat(sourcesProvider.getDirectProtoSourceRoot())
.isEqualTo(genfiles + "/x/foo/_virtual_imports/banana");
}

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

scratch.file(
"a/BUILD",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"proto_library(",
" name = 'a',",
" srcs = ['a.proto'],",
" proto_source_root = 'a',",
" import_prefix = 'foo')");

reporter.removeHandler(failFastHandler);
getConfiguredTarget("//a");
assertContainsEvent("the 'proto_source_root' attribute is incompatible");
}

@Test
public void testProtoSourceRootWithStripImportPrefix() throws Exception {
scratch.file(
"third_party/a/BUILD",
"licenses(['unencumbered'])",
ProtoTestHelper.LOAD_PROTO_LIBRARY,
"proto_library(",
" name = 'a',",
" srcs = ['a.proto'],",
" proto_source_root = 'third_party/a',",
" strip_import_prefix = 'third_party/a')");

reporter.removeHandler(failFastHandler);
getConfiguredTarget("//third_party/a");
assertContainsEvent("the 'proto_source_root' attribute is incompatible");
}

@Test
public void testImportPrefixInExternalRepoLegacyBehavior() throws Exception {
if (!isThisBazel()) {
Expand Down
Loading

0 comments on commit e4b5841

Please sign in to comment.