Put the removal of the legacy repository-relative proto path behind the --incompatible_generated_protos_in_virtual_imports flag.
Fixes #9219.
*now* this sounds like more of an atonement for my sins...
RELNOTES: None.
PiperOrigin-RevId: 264820354
diff --git a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java
index 59ba0c2..1abe88a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java
@@ -264,6 +264,7 @@
// Add include maps
addIncludeMapArguments(
+ !ruleContext.getFragment(ProtoConfiguration.class).generatedProtosInVirtualImports(),
getOutputDirectory(ruleContext),
result,
areDepsStrict ? protoInfo.getStrictImportableProtoSourcesImportPaths() : null,
@@ -296,6 +297,9 @@
.each(protosInExports)
.mapped(
new ExpandToPathFnWithImports(
+ !ruleContext
+ .getFragment(ProtoConfiguration.class)
+ .generatedProtosInVirtualImports(),
getOutputDirectory(ruleContext),
protoInfo.getTransitiveProtoSourceRoots())));
}
@@ -448,6 +452,9 @@
.setExecutable(compilerTarget)
.addCommandLine(
createCommandLineFromToolchains(
+ !ruleContext
+ .getFragment(ProtoConfiguration.class)
+ .generatedProtosInVirtualImports(),
toolchainInvocations,
getOutputDirectory(ruleContext),
protoInfo,
@@ -489,6 +496,7 @@
*/
@VisibleForTesting
static CustomCommandLine createCommandLineFromToolchains(
+ boolean alwaysAddRepositoryPath,
List<ToolchainInvocation> toolchainInvocations,
String outputDirectory,
ProtoInfo protoInfo,
@@ -537,6 +545,7 @@
// Add include maps
addIncludeMapArguments(
+ alwaysAddRepositoryPath,
outputDirectory,
cmdLine,
strictDeps == Deps.STRICT ? protoInfo.getStrictImportableProtoSourcesImportPaths() : null,
@@ -558,7 +567,9 @@
.each(protoInfo.getExportedProtoSourcesImportPaths())
.mapped(
new ExpandToPathFnWithImports(
- outputDirectory, protoInfo.getExportedProtoSourceRoots())));
+ alwaysAddRepositoryPath,
+ outputDirectory,
+ protoInfo.getExportedProtoSourceRoots())));
}
}
@@ -575,6 +586,7 @@
@VisibleForTesting
static void addIncludeMapArguments(
+ boolean alwaysAddRepositoryPath,
String outputDirectory,
CustomCommandLine.Builder commandLine,
@Nullable NestedSet<Pair<Artifact, String>> protosInDirectDependencies,
@@ -585,14 +597,18 @@
// path when including other protos.
commandLine.addAll(
VectorArg.of(transitiveImports)
- .mapped(new ExpandImportArgsFn(outputDirectory, directProtoSourceRoots)));
+ .mapped(
+ new ExpandImportArgsFn(
+ alwaysAddRepositoryPath, outputDirectory, directProtoSourceRoots)));
if (protosInDirectDependencies != null) {
if (!protosInDirectDependencies.isEmpty()) {
commandLine.addAll(
"--direct_dependencies",
VectorArg.join(":")
.each(protosInDirectDependencies)
- .mapped(new ExpandToPathFnWithImports(outputDirectory, directProtoSourceRoots)));
+ .mapped(
+ new ExpandToPathFnWithImports(
+ alwaysAddRepositoryPath, outputDirectory, directProtoSourceRoots)));
} else {
// The proto compiler requires an empty list to turn on strict deps checking
@@ -636,10 +652,15 @@
@AutoCodec
@AutoCodec.VisibleForSerialization
static final class ExpandImportArgsFn implements CapturingMapFn<Artifact> {
+ private final boolean alwaysAddRepositoryPath;
private final String outputDirectory;
private final NestedSet<String> directProtoSourceRoots;
- public ExpandImportArgsFn(String outputDirectory, NestedSet<String> directProtoSourceRoots) {
+ public ExpandImportArgsFn(
+ boolean alwaysAddRepositoryPath,
+ String outputDirectory,
+ NestedSet<String> directProtoSourceRoots) {
+ this.alwaysAddRepositoryPath = alwaysAddRepositoryPath;
this.outputDirectory = outputDirectory;
this.directProtoSourceRoots = directProtoSourceRoots;
}
@@ -651,24 +672,40 @@
*/
@Override
public void expandToCommandLine(Artifact proto, Consumer<String> args) {
+ boolean repositoryPathAdded = !alwaysAddRepositoryPath;
+ String pathIgnoringRepository = getPathIgnoringRepository(proto);
+
for (String directProtoSourceRoot : directProtoSourceRoots) {
PathFragment sourceRootPath = PathFragment.create(directProtoSourceRoot);
String arg = guessProtoPathUnderRoot(outputDirectory, sourceRootPath, proto);
if (arg != null) {
+ if (arg.equals(pathIgnoringRepository)) {
+ repositoryPathAdded = true;
+ }
+
args.accept("-I" + arg + "=" + proto.getExecPathString());
}
}
+
+ // TODO(lberki): This should really be removed. It's only there for backward compatibility.
+ if (!repositoryPathAdded) {
+ args.accept("-I" + getPathIgnoringRepository(proto) + "=" + proto.getExecPathString());
+ }
}
}
@AutoCodec
@AutoCodec.VisibleForSerialization
static final class ExpandToPathFnWithImports implements CapturingMapFn<Pair<Artifact, String>> {
+ private final boolean alwaysAddRepositoryPath;
private final String outputDirectory;
private final NestedSet<String> directProtoSourceRoots;
public ExpandToPathFnWithImports(
- String outputDirectory, NestedSet<String> directProtoSourceRoots) {
+ boolean alwaysAddRepositoryPath,
+ String outputDirectory,
+ NestedSet<String> directProtoSourceRoots) {
+ this.alwaysAddRepositoryPath = alwaysAddRepositoryPath;
this.outputDirectory = outputDirectory;
this.directProtoSourceRoots = directProtoSourceRoots;
}
@@ -678,18 +715,44 @@
if (proto.second != null) {
args.accept(proto.second);
} else {
+ boolean repositoryPathAdded = !alwaysAddRepositoryPath;
+ String pathIgnoringRepository = getPathIgnoringRepository(proto.first);
+
for (String directProtoSourceRoot : directProtoSourceRoots) {
PathFragment sourceRootPath = PathFragment.create(directProtoSourceRoot);
String arg = guessProtoPathUnderRoot(outputDirectory, sourceRootPath, proto.first);
if (arg != null) {
+ if (arg.equals(pathIgnoringRepository)) {
+ repositoryPathAdded = true;
+ }
args.accept(arg);
}
}
+
+ // TODO(lberki): This should really be removed. It's only there for backward compatibility.
+ if (!repositoryPathAdded) {
+ args.accept(pathIgnoringRepository);
+ }
}
}
}
/**
+ * Gets the artifact's path relative to the root, ignoring the external repository the artifact is
+ * at. For example, <code>
+ * //a:b.proto --> a/b.proto
+ * {@literal @}foo//a:b.proto --> a/b.proto
+ * </code>
+ */
+ private static String getPathIgnoringRepository(Artifact artifact) {
+ return artifact
+ .getRootRelativePath()
+ .relativeTo(
+ artifact.getOwnerLabel().getPackageIdentifier().getRepository().getPathUnderExecRoot())
+ .toString();
+ }
+
+ /**
* Describes a toolchain and the value to replace for a $(OUT) that might appear in its
* commandLine() (e.g., "bazel-out/foo.srcjar").
*/
diff --git a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java
index 914d86e..5bc18c4 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java
@@ -104,6 +104,7 @@
CustomCommandLine cmdLine =
createCommandLineFromToolchains(
+ true,
ImmutableList.of(
new ToolchainInvocation(
"dontcare_because_no_plugin", toolchainNoPlugin, "foo.srcjar"),
@@ -116,7 +117,7 @@
artifact("//:dont-care", "import1.proto"),
artifact("//:dont-care", "import2.proto")),
- /* transitiveProtoSourceRoots= */ NestedSetBuilder.create(Order.STABLE_ORDER, "."),
+ /* transitiveProtoSourceRoots= */ NestedSetBuilder.emptySet(STABLE_ORDER),
/* strictImportableProtoSourceRoots= */ NestedSetBuilder.create(
Order.STABLE_ORDER, "."),
/* strictImportableProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER),
@@ -143,6 +144,7 @@
// Verify that the command line contains the correct path to a generated protocol buffers.
CustomCommandLine cmdLine =
createCommandLineFromToolchains(
+ true,
/* toolchainInvocations= */ ImmutableList.of(),
"bazel-out",
protoInfo(
@@ -173,6 +175,7 @@
CustomCommandLine cmdLine =
createCommandLineFromToolchains(
+ true,
ImmutableList.of(new ToolchainInvocation("dontcare", toolchain, "foo.srcjar")),
"bazel-out",
protoInfo(
@@ -217,6 +220,7 @@
CustomCommandLine cmdLine =
createCommandLineFromToolchains(
+ true,
ImmutableList.of(new ToolchainInvocation("dontcare", toolchain, "foo.srcjar")),
"bazel-out",
protoInfo(
@@ -225,9 +229,9 @@
STABLE_ORDER,
artifact("//:dont-care", "import1.proto"),
artifact("//:dont-care", "import2.proto")),
- /* transitiveProtoSourceRoots= */ NestedSetBuilder.create(Order.STABLE_ORDER, "."),
- /* strictImportableProtoSourceRoots= */ NestedSetBuilder.create(
- Order.STABLE_ORDER, "."),
+
+ /* transitiveProtoSourceRoots= */ NestedSetBuilder.emptySet(STABLE_ORDER),
+ /* strictImportableProtoSourceRoots= */ NestedSetBuilder.emptySet(STABLE_ORDER),
/* strictImportableProtos= */ NestedSetBuilder.emptySet(STABLE_ORDER),
/* exportedProtos = */ NestedSetBuilder.create(
STABLE_ORDER,
@@ -253,6 +257,7 @@
public void otherParameters() throws Exception {
CustomCommandLine cmdLine =
createCommandLineFromToolchains(
+ true,
ImmutableList.of(),
"bazel-out",
protoInfo(
@@ -294,6 +299,7 @@
CustomCommandLine cmdLine =
createCommandLineFromToolchains(
+ true,
ImmutableList.of(new ToolchainInvocation("pluginName", toolchain, outReplacement)),
"bazel-out",
protoInfo(
@@ -339,6 +345,7 @@
IllegalStateException.class,
() ->
createCommandLineFromToolchains(
+ true,
ImmutableList.of(
new ToolchainInvocation("pluginName", toolchain1, "outReplacement"),
new ToolchainInvocation("pluginName", toolchain2, "outReplacement")),
@@ -455,6 +462,7 @@
NestedSet<Artifact> transitiveImportsNestedSet =
NestedSetBuilder.wrap(STABLE_ORDER, transitiveImports);
ProtoCompileActionBuilder.addIncludeMapArguments(
+ true,
"blaze-out",
commandLine,
protosInDirectDependenciesBuilder,
diff --git a/src/test/shell/bazel/bazel_proto_library_test.sh b/src/test/shell/bazel/bazel_proto_library_test.sh
index 2669e95..35f1fdd 100755
--- a/src/test/shell/bazel/bazel_proto_library_test.sh
+++ b/src/test/shell/bazel/bazel_proto_library_test.sh
@@ -388,6 +388,53 @@
############# TESTS #############
+function test_legacy_proto_library_include_well_known_protos() {
+ write_workspace ""
+
+ mkdir -p a
+ cat > a/BUILD <<EOF
+proto_library(
+ name="a",
+ srcs=["a.proto"],
+ deps=[":b"],
+)
+
+proto_library(
+ name="b",
+ srcs=["b.proto"],
+ deps=["@com_google_protobuf//:duration_proto"],
+)
+EOF
+
+ cat > a/a.proto <<EOF
+syntax = "proto3";
+
+package a;
+
+import "a/b.proto";
+
+message A {
+ int32 a = 1;
+ b.B b = 2;
+}
+EOF
+
+ cat > a/b.proto <<EOF
+syntax = "proto3";
+
+package b;
+
+import "google/protobuf/duration.proto";
+
+message B {
+ int32 b = 1;
+ google.protobuf.Duration timing = 2;
+}
+EOF
+
+ bazel build //a:a || fail "build failed"
+}
+
function test_javainfo_proto_aspect() {
write_workspace ""
