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/"