Skip to content

Commit

Permalink
Put the removal of the legacy repository-relative proto path behind t…
Browse files Browse the repository at this point in the history 
…he --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
  • Loading branch information
@lberki @copybara-github
lberki authored and copybara-github committed Aug 22, 2019
1 parent 644060b commit 067040d
Show file tree
Hide file tree
Showing 3 changed files with 127 additions and 9 deletions.
73 changes: 68 additions & 5 deletions src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -296,6 +297,9 @@ CustomCommandLine.Builder createProtoCompilerCommandLine() throws MissingPrerequ
.each(protosInExports)
.mapped(
new ExpandToPathFnWithImports(
!ruleContext
.getFragment(ProtoConfiguration.class)
.generatedProtosInVirtualImports(),
getOutputDirectory(ruleContext),
protoInfo.getTransitiveProtoSourceRoots())));
}
Expand Down Expand Up @@ -448,6 +452,9 @@ private static SpawnAction.Builder createActions(
.setExecutable(compilerTarget)
.addCommandLine(
createCommandLineFromToolchains(
!ruleContext
.getFragment(ProtoConfiguration.class)
.generatedProtosInVirtualImports(),
toolchainInvocations,
getOutputDirectory(ruleContext),
protoInfo,
Expand Down Expand Up @@ -489,6 +496,7 @@ public static boolean arePublicImportsStrict(RuleContext ruleContext) {
*/
@VisibleForTesting
static CustomCommandLine createCommandLineFromToolchains(
boolean alwaysAddRepositoryPath,
List<ToolchainInvocation> toolchainInvocations,
String outputDirectory,
ProtoInfo protoInfo,
Expand Down Expand Up @@ -537,6 +545,7 @@ static CustomCommandLine createCommandLineFromToolchains(

// Add include maps
addIncludeMapArguments(
alwaysAddRepositoryPath,
outputDirectory,
cmdLine,
strictDeps == Deps.STRICT ? protoInfo.getStrictImportableProtoSourcesImportPaths() : null,
Expand All @@ -558,7 +567,9 @@ static CustomCommandLine createCommandLineFromToolchains(
.each(protoInfo.getExportedProtoSourcesImportPaths())
.mapped(
new ExpandToPathFnWithImports(
outputDirectory, protoInfo.getExportedProtoSourceRoots())));
alwaysAddRepositoryPath,
outputDirectory,
protoInfo.getExportedProtoSourceRoots())));
}
}

Expand All @@ -575,6 +586,7 @@ static CustomCommandLine createCommandLineFromToolchains(

@VisibleForTesting
static void addIncludeMapArguments(
boolean alwaysAddRepositoryPath,
String outputDirectory,
CustomCommandLine.Builder commandLine,
@Nullable NestedSet<Pair<Artifact, String>> protosInDirectDependencies,
Expand All @@ -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
Expand Down Expand Up @@ -636,10 +652,15 @@ private static String guessProtoPathUnderRoot(
@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;
}
Expand All @@ -651,24 +672,40 @@ public ExpandImportArgsFn(String outputDirectory, NestedSet<String> directProtoS
*/
@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;
}
Expand All @@ -678,17 +715,43 @@ public void expandToCommandLine(Pair<Artifact, String> proto, Consumer<String> 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, <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").
Expand Down
16 changes: 12 additions & 4 deletions src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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),
Expand All @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand All @@ -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,
Expand All @@ -253,6 +257,7 @@ public void commandLine_exports() throws Exception {
public void otherParameters() throws Exception {
CustomCommandLine cmdLine =
createCommandLineFromToolchains(
true,
ImmutableList.of(),
"bazel-out",
protoInfo(
Expand Down Expand Up @@ -294,6 +299,7 @@ public String toString() {

CustomCommandLine cmdLine =
createCommandLineFromToolchains(
true,
ImmutableList.of(new ToolchainInvocation("pluginName", toolchain, outReplacement)),
"bazel-out",
protoInfo(
Expand Down Expand Up @@ -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")),
Expand Down Expand Up @@ -455,6 +462,7 @@ private static Iterable<String> protoArgv(
NestedSet<Artifact> transitiveImportsNestedSet =
NestedSetBuilder.wrap(STABLE_ORDER, transitiveImports);
ProtoCompileActionBuilder.addIncludeMapArguments(
true,
"blaze-out",
commandLine,
protosInDirectDependenciesBuilder,
Expand Down
47 changes: 47 additions & 0 deletions src/test/shell/bazel/bazel_proto_library_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,53 @@ EOF

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

Expand Down

0 comments on commit 067040d

Please sign in to comment.