From 067040d7bcb3b24a88432e210a96adacee3f37b4 Mon Sep 17 00:00:00 2001 From: lberki Date: Thu, 22 Aug 2019 06:23:46 -0700 Subject: [PATCH] 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 --- .../proto/ProtoCompileActionBuilder.java | 73 +++++++++++++++++-- .../proto/ProtoCompileActionBuilderTest.java | 16 +++- .../shell/bazel/bazel_proto_library_test.sh | 47 ++++++++++++ 3 files changed, 127 insertions(+), 9 deletions(-) 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 59ba0c212e5bc3..1abe88ac56f929 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 @@ CustomCommandLine.Builder createProtoCompilerCommandLine() throws MissingPrerequ // Add include maps addIncludeMapArguments( + !ruleContext.getFragment(ProtoConfiguration.class).generatedProtosInVirtualImports(), getOutputDirectory(ruleContext), result, areDepsStrict ? protoInfo.getStrictImportableProtoSourcesImportPaths() : null, @@ -296,6 +297,9 @@ CustomCommandLine.Builder createProtoCompilerCommandLine() throws MissingPrerequ .each(protosInExports) .mapped( new ExpandToPathFnWithImports( + !ruleContext + .getFragment(ProtoConfiguration.class) + .generatedProtosInVirtualImports(), getOutputDirectory(ruleContext), protoInfo.getTransitiveProtoSourceRoots()))); } @@ -448,6 +452,9 @@ private static SpawnAction.Builder createActions( .setExecutable(compilerTarget) .addCommandLine( createCommandLineFromToolchains( + !ruleContext + .getFragment(ProtoConfiguration.class) + .generatedProtosInVirtualImports(), toolchainInvocations, getOutputDirectory(ruleContext), protoInfo, @@ -489,6 +496,7 @@ public static boolean arePublicImportsStrict(RuleContext ruleContext) { */ @VisibleForTesting static CustomCommandLine createCommandLineFromToolchains( + boolean alwaysAddRepositoryPath, List toolchainInvocations, String outputDirectory, ProtoInfo protoInfo, @@ -537,6 +545,7 @@ static CustomCommandLine createCommandLineFromToolchains( // Add include maps addIncludeMapArguments( + alwaysAddRepositoryPath, outputDirectory, cmdLine, strictDeps == Deps.STRICT ? protoInfo.getStrictImportableProtoSourcesImportPaths() : null, @@ -558,7 +567,9 @@ static CustomCommandLine createCommandLineFromToolchains( .each(protoInfo.getExportedProtoSourcesImportPaths()) .mapped( new ExpandToPathFnWithImports( - outputDirectory, protoInfo.getExportedProtoSourceRoots()))); + alwaysAddRepositoryPath, + outputDirectory, + protoInfo.getExportedProtoSourceRoots()))); } } @@ -575,6 +586,7 @@ static CustomCommandLine createCommandLineFromToolchains( @VisibleForTesting static void addIncludeMapArguments( + boolean alwaysAddRepositoryPath, String outputDirectory, CustomCommandLine.Builder commandLine, @Nullable NestedSet> protosInDirectDependencies, @@ -585,14 +597,18 @@ static void addIncludeMapArguments( // 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 @@ private static String guessProtoPathUnderRoot( @AutoCodec @AutoCodec.VisibleForSerialization static final class ExpandImportArgsFn implements CapturingMapFn { + private final boolean alwaysAddRepositoryPath; private final String outputDirectory; private final NestedSet directProtoSourceRoots; - public ExpandImportArgsFn(String outputDirectory, NestedSet directProtoSourceRoots) { + public ExpandImportArgsFn( + boolean alwaysAddRepositoryPath, + String outputDirectory, + NestedSet directProtoSourceRoots) { + this.alwaysAddRepositoryPath = alwaysAddRepositoryPath; this.outputDirectory = outputDirectory; this.directProtoSourceRoots = directProtoSourceRoots; } @@ -651,24 +672,40 @@ public ExpandImportArgsFn(String outputDirectory, NestedSet directProtoS */ @Override public void expandToCommandLine(Artifact proto, Consumer 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> { + private final boolean alwaysAddRepositoryPath; private final String outputDirectory; private final NestedSet directProtoSourceRoots; public ExpandToPathFnWithImports( - String outputDirectory, NestedSet directProtoSourceRoots) { + boolean alwaysAddRepositoryPath, + String outputDirectory, + NestedSet directProtoSourceRoots) { + this.alwaysAddRepositoryPath = alwaysAddRepositoryPath; this.outputDirectory = outputDirectory; this.directProtoSourceRoots = directProtoSourceRoots; } @@ -678,17 +715,43 @@ public void expandToCommandLine(Pair proto, Consumer a 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, + * //a:b.proto --> a/b.proto + * {@literal @}foo//a:b.proto --> a/b.proto + * + */ + 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 914d86e0c10da4..5bc18c4fa80058 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 @@ public void commandLine_basic() throws Exception { CustomCommandLine cmdLine = createCommandLineFromToolchains( + true, ImmutableList.of( new ToolchainInvocation( "dontcare_because_no_plugin", toolchainNoPlugin, "foo.srcjar"), @@ -116,7 +117,7 @@ public void commandLine_basic() throws Exception { 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 @@ public void commandline_derivedArtifact() { // 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 @@ public void commandLine_strictDeps() throws Exception { CustomCommandLine cmdLine = createCommandLineFromToolchains( + true, ImmutableList.of(new ToolchainInvocation("dontcare", toolchain, "foo.srcjar")), "bazel-out", protoInfo( @@ -217,6 +220,7 @@ public void commandLine_exports() throws Exception { CustomCommandLine cmdLine = createCommandLineFromToolchains( + true, ImmutableList.of(new ToolchainInvocation("dontcare", toolchain, "foo.srcjar")), "bazel-out", protoInfo( @@ -225,9 +229,9 @@ public void commandLine_exports() throws Exception { 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 commandLine_exports() throws Exception { public void otherParameters() throws Exception { CustomCommandLine cmdLine = createCommandLineFromToolchains( + true, ImmutableList.of(), "bazel-out", protoInfo( @@ -294,6 +299,7 @@ public String toString() { CustomCommandLine cmdLine = createCommandLineFromToolchains( + true, ImmutableList.of(new ToolchainInvocation("pluginName", toolchain, outReplacement)), "bazel-out", protoInfo( @@ -339,6 +345,7 @@ public void exceptionIfSameName() throws Exception { IllegalStateException.class, () -> createCommandLineFromToolchains( + true, ImmutableList.of( new ToolchainInvocation("pluginName", toolchain1, "outReplacement"), new ToolchainInvocation("pluginName", toolchain2, "outReplacement")), @@ -455,6 +462,7 @@ private static Iterable protoArgv( NestedSet 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 88a6dabe68f386..2ae74816ec3420 100755 --- a/src/test/shell/bazel/bazel_proto_library_test.sh +++ b/src/test/shell/bazel/bazel_proto_library_test.sh @@ -384,6 +384,53 @@ EOF ############# TESTS ############# +function test_legacy_proto_library_include_well_known_protos() { + write_workspace "" + + mkdir -p a + cat > a/BUILD < a/a.proto < a/b.proto <