Automated rollback of commit bcc9812e27d172d964dd728b70651fe0b972df0e.
Fixes #8832.
*** Reason for rollback ***
Breaks internal tests
*** Original change description ***
Create a virtual proto import directory for proto_library rules with generated sources and ones with proto_source_root= set.
Fixes #7157.
RELNOTES: None.
PiperOrigin-RevId: 257179608
diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java
index 8f44434..5933d06 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java
@@ -31,7 +31,7 @@
@Override
public ConfiguredTarget create(RuleContext ruleContext) throws ActionConflictException {
- ProtoInfo protoInfo = ProtoCommon.createProtoInfo(ruleContext, true);
+ ProtoInfo protoInfo = ProtoCommon.createProtoInfo(ruleContext);
if (ruleContext.hasErrors()) {
return null;
}
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 195ca3d..1463bbf 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
@@ -194,22 +194,29 @@
// TODO(lberki): This should really be a PathFragment. Unfortunately, it's on the Starlark API of
// ProtoInfo so it's not an easy change :(
@Nullable
- private static Library createLibraryWithoutVirtualSourceRoot(
+ private static Library getProtoSourceRoot(
RuleContext ruleContext, ImmutableList<Artifact> directSources) {
- String protoSourceRoot =
+ String protoSourceRoot = computeEffectiveProtoSourceRoot(ruleContext);
+ if (ruleContext.hasErrors()) {
+ return null;
+ }
+
+ // This is the same as getPackageIdentifier().getPathUnderExecRoot() due to the check above for
+ // protoSourceRoot == package name, but it's a bit more future-proof.
+ String result =
ruleContext
.getLabel()
.getPackageIdentifier()
.getRepository()
.getPathUnderExecRoot()
+ .getRelative(protoSourceRoot)
.getPathString();
ImmutableList.Builder<Pair<Artifact, String>> builder = ImmutableList.builder();
for (Artifact protoSource : directSources) {
builder.add(new Pair<Artifact, String>(protoSource, null));
}
- return new Library(
- directSources, protoSourceRoot.isEmpty() ? "." : protoSourceRoot, builder.build());
+ return new Library(directSources, result.isEmpty() ? "." : result, builder.build());
}
private static PathFragment getPathFragmentAttribute(
@@ -236,36 +243,18 @@
* Returns the {@link Library} representing this <code>proto_library</code> rule if import prefix
* munging is done. Otherwise, returns null.
*/
- private static Library createLibraryWithVirtualSourceRootMaybe(
- RuleContext ruleContext,
- ImmutableList<Artifact> protoSources,
- boolean virtualImportsWithGeneratedProtoFiles) {
+ private static Library createVirtualImportDirectoryMaybe(
+ RuleContext ruleContext, ImmutableList<Artifact> protoSources) {
PathFragment importPrefixAttribute = getPathFragmentAttribute(ruleContext, "import_prefix");
PathFragment stripImportPrefixAttribute =
getPathFragmentAttribute(ruleContext, "strip_import_prefix");
- PathFragment protoSourceRootAttribute =
- getPathFragmentAttribute(ruleContext, "proto_source_root");
- boolean hasGeneratedSources = false;
- if (virtualImportsWithGeneratedProtoFiles) {
- for (Artifact protoSource : protoSources) {
- if (!protoSource.isSourceArtifact()) {
- hasGeneratedSources = true;
- break;
- }
- }
- }
-
- if (importPrefixAttribute == null
- && stripImportPrefixAttribute == null
- && protoSourceRootAttribute == null
- && !hasGeneratedSources) {
+ if (importPrefixAttribute == null && stripImportPrefixAttribute == null) {
// Simple case, no magic required.
return null;
}
- if (protoSourceRootAttribute != null
- && (importPrefixAttribute != null || stripImportPrefixAttribute != null)) {
+ if (!ruleContext.attributes().get("proto_source_root", STRING).isEmpty()) {
ruleContext.ruleError(
"the 'proto_source_root' attribute is incompatible with "
+ "'strip_import_prefix' and 'import_prefix");
@@ -273,62 +262,30 @@
}
PathFragment stripImportPrefix;
+ if (stripImportPrefixAttribute == null) {
+ stripImportPrefix = PathFragment.create(ruleContext.getLabel().getWorkspaceRoot());
+ } else if (stripImportPrefixAttribute.isAbsolute()) {
+ stripImportPrefix =
+ ruleContext
+ .getLabel()
+ .getPackageIdentifier()
+ .getRepository()
+ .getSourceRoot()
+ .getRelative(stripImportPrefixAttribute.toRelative());
+ } else {
+ stripImportPrefix = ruleContext.getPackageDirectory().getRelative(stripImportPrefixAttribute);
+ }
+
PathFragment importPrefix;
-
- if (stripImportPrefixAttribute != null || importPrefixAttribute != null) {
- if (stripImportPrefixAttribute == null) {
- stripImportPrefix = PathFragment.create(ruleContext.getLabel().getWorkspaceRoot());
- } else if (stripImportPrefixAttribute.isAbsolute()) {
- stripImportPrefix =
- ruleContext
- .getLabel()
- .getPackageIdentifier()
- .getRepository()
- .getSourceRoot()
- .getRelative(stripImportPrefixAttribute.toRelative());
- } else {
- stripImportPrefix =
- ruleContext.getPackageDirectory().getRelative(stripImportPrefixAttribute);
- }
-
- if (importPrefixAttribute != null) {
- importPrefix = importPrefixAttribute;
- } else {
- importPrefix = PathFragment.EMPTY_FRAGMENT;
- }
-
- if (importPrefix.isAbsolute()) {
- ruleContext.attributeError("import_prefix", "should be a relative path");
- return null;
- }
- } else if (protoSourceRootAttribute != null) {
- if (!ruleContext.getFragment(ProtoConfiguration.class).enableProtoSourceroot()) {
- ruleContext.attributeError("proto_source_root", "this attribute is not supported anymore");
- return null;
- }
-
- if (!ruleContext.getLabel().getPackageFragment().equals(protoSourceRootAttribute)) {
- ruleContext.attributeError(
- "proto_source_root",
- "proto_source_root must be the same as the package name ("
- + ruleContext.getLabel().getPackageName()
- + ")."
- + " not '"
- + protoSourceRootAttribute
- + "'.");
-
- return null;
- }
-
- stripImportPrefix = ruleContext.getPackageDirectory();
+ if (importPrefixAttribute == null) {
importPrefix = PathFragment.EMPTY_FRAGMENT;
} else {
- // Has generated sources, but neither strip_import_prefix nor import_prefix nor
- // proto_source_root.
- stripImportPrefix =
- ruleContext.getLabel().getPackageIdentifier().getRepository().getPathUnderExecRoot();
+ importPrefix = importPrefixAttribute;
+ }
- importPrefix = PathFragment.EMPTY_FRAGMENT;
+ if (importPrefix.isAbsolute()) {
+ ruleContext.attributeError("import_prefix", "should be a relative path");
+ return null;
}
ImmutableList.Builder<Artifact> symlinks = ImmutableList.builder();
@@ -351,9 +308,7 @@
symlinks.add(importsPair.second);
if (!ruleContext.getFragment(ProtoConfiguration.class).doNotUseBuggyImportPath()
- && stripImportPrefixAttribute == null
- && protoSourceRootAttribute == null
- && !hasGeneratedSources) {
+ && stripImportPrefixAttribute == null) {
Pair<PathFragment, Artifact> oldImportsPair =
computeImports(
ruleContext,
@@ -440,6 +395,29 @@
return getProtoSourceRootsOfAttribute(ruleContext, currentProtoSourceRoot, "exports");
}
+ private static String computeEffectiveProtoSourceRoot(RuleContext ruleContext) {
+ if (!ruleContext.attributes().isAttributeValueExplicitlySpecified("proto_source_root")) {
+ return "";
+ }
+
+ if (!ruleContext.getFragment(ProtoConfiguration.class).enableProtoSourceroot()) {
+ ruleContext.attributeError("proto_source_root", "this attribute is not supported anymore");
+ return "";
+ }
+
+ String protoSourceRoot = ruleContext.attributes().get("proto_source_root", STRING);
+ if (!ruleContext.getLabel().getPackageName().equals(protoSourceRoot)) {
+ ruleContext.attributeError(
+ "proto_source_root",
+ "proto_source_root must be the same as the package name ("
+ + ruleContext.getLabel().getPackageName() + ")."
+ + " not '" + protoSourceRoot + "'."
+ );
+ }
+
+ return protoSourceRoot;
+ }
+
/**
* Check that .proto files in sources are from the same package. This is done to avoid clashes
* with the generated sources.
@@ -464,20 +442,21 @@
* Creates the {@link ProtoInfo} for the {@code proto_library} rule associated with {@code
* ruleContext}.
*/
- public static ProtoInfo createProtoInfo(
- RuleContext ruleContext, boolean virtualImportsWithGeneratedProtoFiles) {
+ public static ProtoInfo createProtoInfo(RuleContext ruleContext) {
checkSourceFilesAreInSamePackage(ruleContext);
ImmutableList<Artifact> directProtoSources =
ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET).list();
- Library library =
- createLibraryWithVirtualSourceRootMaybe(
- ruleContext, directProtoSources, virtualImportsWithGeneratedProtoFiles);
+ Library library = createVirtualImportDirectoryMaybe(ruleContext, directProtoSources);
if (ruleContext.hasErrors()) {
return null;
}
if (library == null) {
- library = createLibraryWithoutVirtualSourceRoot(ruleContext, directProtoSources);
+ library = getProtoSourceRoot(ruleContext, directProtoSources);
+ }
+
+ if (ruleContext.hasErrors()) {
+ return null;
}
NestedSet<Artifact> transitiveProtoSources =
diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoInfoStarlarkTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoInfoStarlarkTest.java
index 0815636..2f22ae2 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoInfoStarlarkTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoInfoStarlarkTest.java
@@ -164,7 +164,7 @@
public void testProtoSourceRootExportedInStarlark() throws Exception {
scratch.file(
- "foo/myTestRule.bzl",
+ "foo/myTsetRule.bzl",
"load('//myinfo:myinfo.bzl', 'MyInfo')",
"",
"def _my_test_rule_impl(ctx):",
@@ -180,7 +180,7 @@
scratch.file(
"foo/BUILD",
"",
- "load(':myTestRule.bzl', 'my_test_rule')",
+ "load(':myTsetRule.bzl', 'my_test_rule')",
"my_test_rule(",
" name = 'myRule',",
" protodep = ':myProto',",
@@ -193,8 +193,7 @@
ConfiguredTarget ct = getConfiguredTarget("//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("foo");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java
index 803b514..225134e 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java
@@ -261,13 +261,11 @@
);
ConfiguredTarget protoTarget = getConfiguredTarget("//x/foo:nodeps");
ProtoInfo sourcesProvider = protoTarget.get(ProtoInfo.PROVIDER);
- String genfiles = getTargetConfiguration().getGenfilesFragment().toString();
+ assertThat(sourcesProvider.getTransitiveProtoSourceRoots()).containsExactly("x/foo");
- assertThat(sourcesProvider.getTransitiveProtoSourceRoots())
- .containsExactly(genfiles + "/x/foo/_virtual_imports/nodeps");
- assertThat(
- getGeneratingSpawnAction(getDescriptorOutput("//x/foo:nodeps")).getRemainingArguments())
- .contains("--proto_path=" + genfiles + "/x/foo/_virtual_imports/nodeps");
+ assertThat(getGeneratingSpawnAction(getDescriptorOutput("//x/foo:nodeps"))
+ .getRemainingArguments())
+ .contains("--proto_path=x/foo");
}
@Test
@@ -310,18 +308,11 @@
);
ConfiguredTarget protoTarget = getConfiguredTarget("//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");
+ assertThat(sourcesProvider.getTransitiveProtoSourceRoots()).containsExactly("x/foo");
- assertThat(
- getGeneratingSpawnAction(getDescriptorOutput("//x/foo:withdeps"))
- .getRemainingArguments())
- .containsAtLeast(
- "--proto_path=" + genfiles + "/x/foo/_virtual_imports/withdeps",
- "--proto_path=" + genfiles + "/x/foo/_virtual_imports/dep");
+ assertThat(getGeneratingSpawnAction(getDescriptorOutput("//x/foo:withdeps"))
+ .getRemainingArguments())
+ .contains("--proto_path=x/foo");
}
@Test
@@ -349,42 +340,8 @@
);
ConfiguredTarget protoTarget = getConfiguredTarget("//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",
- ".");
- }
-
- @Test
- public void testExternalRepoWithGeneratedProto() throws Exception {
- if (!isThisBazel()) {
- return;
- }
-
- FileSystemUtils.appendIsoLatin1(
- scratch.resolve("WORKSPACE"), "local_repository(name = 'foo', path = '/foo')");
- invalidatePackages();
-
- scratch.file("/foo/WORKSPACE");
- scratch.file(
- "/foo/x/BUILD",
- "proto_library(name='x', srcs=['generated.proto'])",
- "genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')");
-
- scratch.file("a/BUILD", "proto_library(name='a', srcs=['a.proto'], deps=['@foo//x:x'])");
-
- String genfiles = getTargetConfiguration().getGenfilesFragment().toString();
- ConfiguredTarget a = getConfiguredTarget("//a:a");
- ProtoInfo aInfo = a.get(ProtoInfo.PROVIDER);
- assertThat(aInfo.getTransitiveProtoSourceRoots())
- .containsExactly(".", genfiles + "/external/foo/x/_virtual_imports/x");
-
- ConfiguredTarget x = getConfiguredTarget("@foo//x:x");
- ProtoInfo xInfo = x.get(ProtoInfo.PROVIDER);
- assertThat(xInfo.getTransitiveProtoSourceRoots())
- .containsExactly(genfiles + "/external/foo/x/_virtual_imports/x");
+ .containsExactly("x/foo", "x/bar", ".");
}
@Test
@@ -423,9 +380,7 @@
// exported proto source roots should be the source root of the rule and the direct source roots
// of its exports and nothing else (not the exports of its exports or the deps of its exports
// or the exports of its deps)
- String genfiles = getTargetConfiguration().getGenfilesFragment().toString();
- assertThat(c.get(ProtoInfo.PROVIDER).getExportedProtoSourceRoots())
- .containsExactly(genfiles + "/a/_virtual_imports/a", genfiles + "/c/_virtual_imports/c");
+ assertThat(c.get(ProtoInfo.PROVIDER).getExportedProtoSourceRoots()).containsExactly("a", "c");
}
@Test
@@ -441,10 +396,7 @@
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");
+ assertThat(sourcesProvider.getDirectProtoSourceRoot()).isEqualTo("x/foo");
}
@Test
diff --git a/src/test/shell/bazel/bazel_proto_library_test.sh b/src/test/shell/bazel/bazel_proto_library_test.sh
index 9075736..f211683 100755
--- a/src/test/shell/bazel/bazel_proto_library_test.sh
+++ b/src/test/shell/bazel/bazel_proto_library_test.sh
@@ -121,7 +121,6 @@
$proto_library_name(
name = "phonenumber_proto",
srcs = ["phonenumber/phonenumber.proto"],
- $proto_source_root
)
EOF
@@ -382,7 +381,7 @@
function test_proto_source_root() {
write_workspace ""
write_setup "proto_library" "proto_source_root = 'x/person'" ""
- bazel build --verbose_failures //x/person:person_proto > "$TEST_log" || fail "Expected success"
+ bazel build //x/person:person_proto > "$TEST_log" || fail "Expected success"
}
function test_proto_source_root_fails() {
@@ -416,6 +415,13 @@
bazel build //proto_library/src:all >& "$TEST_log" || fail "Expected success"
}
+function test_proto_source_root_glob() {
+ write_workspace ""
+ write_regression_setup
+ bazel build //proto_library/src:all --strict_proto_deps=off >& "$TEST_log" \
+ || fail "Expected success"
+}
+
function test_proto_source_root_multiple_workspaces() {
write_workspace "a/b/"
write_workspace "c/d/"
