Skip to content

Commit 62e5daa

Browse files
lberkilberki
committed
Handle .proto source files in external repositories
1 parent b189157 commit 62e5daa

File tree

6 files changed

+67
-90
lines changed

6 files changed

+67
-90
lines changed

‎src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ private void createProtoCompileAction(SupportData supportData, Collection<Artifa
363363
.getPackageIdentifier()
364364
.getRepository()
365365
.getPathUnderExecRoot())
366-
.getRelative(protoRoot == null ? "" : protoRoot)
366+
.getRelative(protoRoot)
367367
.getPathString();
368368

369369
ImmutableList.Builder<ToolchainInvocation> invocations = ImmutableList.builder();

‎src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -123,19 +123,26 @@ public static NestedSet<String> collectTransitiveProtoPathFlags(
123123
}
124124

125125
/**
126-
* Returns the {@code proto_source_root} of the current library or null if none is specified.
126+
* Returns the source root of the currentl {@code proto_library} or "." if it's the source root.
127127
*
128-
* <p>Build will fail if the {@code proto_source_root} of the current library is different than
129-
* the package name.
128+
* <p>Build will fail if the {@code proto_source_root} of the current library is neither the
129+
* package name nor the source root.
130130
*/
131+
// TODO(lberki): This should really be a PathFragment. Unfortunately, it's on the Starlark API of
132+
// ProtoSourcesProvider so it's not an easy change :(
131133
@Nullable
132134
public static String getProtoSourceRoot(RuleContext ruleContext) {
133135
String protoSourceRoot =
134136
ruleContext.attributes().get("proto_source_root", Type.STRING);
135-
if (protoSourceRoot != null && !protoSourceRoot.isEmpty()) {
137+
if (!protoSourceRoot.isEmpty()) {
136138
checkProtoSourceRootIsTheSameAsPackage(protoSourceRoot, ruleContext);
137139
}
138-
return protoSourceRoot;
140+
141+
// This is the same as getPackageIdentifier().getPathUnderExecRoot() due to the check above for
142+
// protoSourceRoot == package name, but it's a bit more future-proof.
143+
String result = ruleContext.getLabel().getPackageIdentifier().getRepository()
144+
.getPathUnderExecRoot().getRelative(protoSourceRoot).getPathString();
145+
return result.isEmpty() ? "." : result;
139146
}
140147

141148
/**
@@ -147,16 +154,11 @@ public static String getProtoSourceRoot(RuleContext ruleContext) {
147154
private static NestedSet<String> getProtoSourceRootsOfAttribute(
148155
RuleContext ruleContext, String currentProtoSourceRoot, String attributeName) {
149156
NestedSetBuilder<String> protoSourceRoots = NestedSetBuilder.stableOrder();
150-
if (currentProtoSourceRoot != null && !currentProtoSourceRoot.isEmpty()) {
151-
protoSourceRoots.add(currentProtoSourceRoot);
152-
}
157+
protoSourceRoots.add(currentProtoSourceRoot);
153158

154159
for (ProtoSourcesProvider provider :
155160
ruleContext.getPrerequisites(attributeName, Mode.TARGET, ProtoSourcesProvider.class)) {
156-
String protoSourceRoot = provider.getProtoSourceRoot();
157-
if (protoSourceRoot != null && !protoSourceRoot.isEmpty()) {
158-
protoSourceRoots.add(provider.getProtoSourceRoot());
159-
}
161+
protoSourceRoots.add(provider.getProtoSourceRoot());
160162
}
161163

162164
return protoSourceRoots.build();

‎src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java

Lines changed: 13 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -686,12 +686,15 @@ public ExpandImportArgsFn(NestedSet<String> directProtoSourceRoots) {
686686
@Override
687687
public void expandToCommandLine(Artifact proto, Consumer<String> args) {
688688
for (String directProtoSourceRoot : directProtoSourceRoots) {
689-
String path = getPathIgnoringSourceRoot(proto, directProtoSourceRoot);
690-
if (path != null) {
691-
args.accept("-I" + path + "=" + proto.getExecPathString());
689+
// TODO(lberki): Instead of guesswork like this, we should track which proto belongs to
690+
// which source root. Unfortunately, that's a non-trivial migration since
691+
// ProtoSourcesProvider is on the Starlark API.
692+
PathFragment sourceRootPath = PathFragment.create(directProtoSourceRoot);
693+
if (proto.getRootRelativePath().startsWith(sourceRootPath)) {
694+
args.accept("-I" + proto.getRootRelativePath().relativeTo(sourceRootPath) + "="
695+
+ proto.getExecPathString());
692696
}
693697
}
694-
args.accept("-I" + getPathIgnoringRepository(proto) + "=" + proto.getExecPathString());
695698
}
696699
}
697700

@@ -707,51 +710,17 @@ public ExpandToPathFn(NestedSet<String> directProtoSourceRoots) {
707710
@Override
708711
public void expandToCommandLine(Artifact proto, Consumer<String> args) {
709712
for (String directProtoSourceRoot : directProtoSourceRoots) {
710-
String path = getPathIgnoringSourceRoot(proto, directProtoSourceRoot);
711-
if (path != null) {
712-
args.accept(path);
713+
PathFragment sourceRootPath = PathFragment.create(directProtoSourceRoot);
714+
// TODO(lberki): Instead of guesswork like this, we should track which proto belongs to
715+
// which source root. Unfortunately, that's a non-trivial migration since
716+
// ProtoSourcesProvider is on the Starlark API.
717+
if (proto.getRootRelativePath().startsWith(sourceRootPath)) {
718+
args.accept(proto.getRootRelativePath().relativeTo(sourceRootPath).getPathString());
713719
}
714720
}
715-
args.accept(getPathIgnoringRepository(proto));
716721
}
717722
}
718723

719-
/**
720-
* Gets the artifact's path relative to the root, ignoring the external repository the artifact is
721-
* at. For example, <code>
722-
* //a:b.proto --> a/b.proto
723-
* {@literal @}foo//a:b.proto --> a/b.proto
724-
* </code>
725-
*/
726-
private static String getPathIgnoringRepository(Artifact artifact) {
727-
return artifact
728-
.getRootRelativePath()
729-
.relativeTo(
730-
artifact.getOwnerLabel().getPackageIdentifier().getRepository().getPathUnderExecRoot())
731-
.toString();
732-
}
733-
734-
/**
735-
* Gets the artifact's path relative to the proto source root, ignoring the external repository
736-
* the artifact is at. For example, <code>
737-
* //a/b/c:d.proto with proto source root a/b --> c/d.proto
738-
* {@literal @}foo//a/b/c:d.proto with proto source root a/b --> c/d.proto
739-
* </code>
740-
*/
741-
private static String getPathIgnoringSourceRoot(Artifact artifact, String directProtoSourceRoot) {
742-
// TODO(bazel-team): IAE is caught here because every artifact is relativized against every
743-
// directProtoSourceRoot. Instead of catching the exception, a check should be performed
744-
// to see if the artifact has the root as a substring before relativizing.
745-
try {
746-
return PathFragment.createAlreadyNormalized(getPathIgnoringRepository(artifact))
747-
.relativeTo(directProtoSourceRoot)
748-
.toString();
749-
} catch (IllegalArgumentException exception) {
750-
// do nothing
751-
}
752-
return null;
753-
}
754-
755724
/**
756725
* Describes a toolchain and the value to replace for a $(OUT) that might appear in its
757726
* commandLine() (e.g., "bazel-out/foo.srcjar").

‎src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -342,12 +342,12 @@ public void testProtoSourceRootWithDeps() throws Exception {
342342
ConfiguredTarget protoTarget = getConfiguredTarget("//x/foo:withdeps");
343343
ProtoSourcesProvider sourcesProvider = protoTarget.getProvider(ProtoSourcesProvider.class);
344344
assertThat(sourcesProvider.getTransitiveProtoPathFlags())
345-
.containsExactly("x/foo", "x/bar");
345+
.containsExactly("x/foo", "x/bar", ".");
346346

347347
SupportData supportData =
348348
protoTarget.getProvider(ProtoSupportDataProvider.class).getSupportData();
349349
assertThat(supportData.getTransitiveProtoPathFlags())
350-
.containsExactly("x/foo", "x/bar");
350+
.containsExactly("x/foo", "x/bar", ".");
351351

352352
assertThat(getGeneratingSpawnAction(getDescriptorOutput("//x/foo:withdeps"))
353353
.getRemainingArguments())

‎src/test/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilderTest.java

Lines changed: 36 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
import com.google.devtools.build.lib.vfs.Root;
3838
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
3939
import javax.annotation.Nullable;
40-
import org.junit.Ignore;
4140
import org.junit.Test;
4241
import org.junit.runner.RunWith;
4342
import org.junit.runners.JUnit4;
@@ -83,11 +82,11 @@ public void commandLine_basic() throws Exception {
8382
artifact("//:dont-care", "import1.proto"),
8483
artifact("//:dont-care", "import2.proto")),
8584
/* transitiveProtoPathFlags= */ NestedSetBuilder.emptySet(STABLE_ORDER),
86-
/* protoSourceRoot= */ "",
87-
/* directProtoSourceRoots= */ NestedSetBuilder.<String>stableOrder().build(),
85+
/* protoSourceRoot= */ ".",
86+
/* directProtoSourceRoots= */ NestedSetBuilder.create(Order.STABLE_ORDER, "."),
8887
/* hasProtoSources= */ true,
8988
/* protosInExports= */ null,
90-
/* exportedProtoSourceRoots= */ null);
89+
/* exportedProtoSourceRoots= */ NestedSetBuilder.emptySet(STABLE_ORDER));
9190

9291
CustomCommandLine cmdLine =
9392
createCommandLineFromToolchains(
@@ -97,11 +96,11 @@ public void commandLine_basic() throws Exception {
9796
new ToolchainInvocation("pluginName", toolchainWithPlugin, "bar.srcjar")),
9897
supportData.getDirectProtoSources(),
9998
supportData.getTransitiveImports(),
100-
/* transitiveProtoPathFlags= */ NestedSetBuilder.<String>stableOrder().build(),
101-
/* directProtoSourceRoots= */ NestedSetBuilder.<String>stableOrder().build(),
102-
/* exportedProtoSourceRoots= */ null,
103-
/* protosInDirectDeps= */ null,
104-
/* protosInExports= */ null,
99+
supportData.getTransitiveProtoPathFlags(),
100+
supportData.getDirectProtoSourceRoots(),
101+
supportData.getExportedProtoSourceRoots(),
102+
/* protosInDirectDeps */ null,
103+
supportData.getProtosInExports(),
105104
Label.parseAbsoluteUnchecked("//foo:bar"),
106105
/* allowServices= */ true,
107106
/* protocOpts= */ ImmutableList.of());
@@ -169,22 +168,22 @@ public void commandLine_strictDeps() throws Exception {
169168
artifact("//:dont-care", "import1.proto"),
170169
artifact("//:dont-care", "import2.proto")),
171170
/* transitiveProtoPathFlags= */ NestedSetBuilder.emptySet(STABLE_ORDER),
172-
/* protoSourceRoot= */ "",
173-
/* directProtoSourceRoots= */ NestedSetBuilder.<String>stableOrder().build(),
171+
/* protoSourceRoot= */ ".",
172+
NestedSetBuilder.create(Order.STABLE_ORDER, "."),
174173
/* hasProtoSources= */ true,
175174
/* protosInExports= */ null,
176-
/* exportedProtoSourceRoots= */ null);
175+
NestedSetBuilder.create(Order.STABLE_ORDER, "."));
177176

178177
CustomCommandLine cmdLine =
179178
createCommandLineFromToolchains(
180179
ImmutableList.of(new ToolchainInvocation("dontcare", toolchain, "foo.srcjar")),
181180
supportData.getDirectProtoSources(),
182181
supportData.getTransitiveImports(),
183-
/* transitiveProtoPathFlags= */ NestedSetBuilder.emptySet(STABLE_ORDER),
184-
/* directProtoSourceRoots= */ NestedSetBuilder.emptySet(STABLE_ORDER),
185-
/* exportedProtoSourceRoots= */ null,
182+
supportData.getTransitiveProtoPathFlags(),
183+
supportData.getDirectProtoSourceRoots(),
184+
supportData.getExportedProtoSourceRoots(),
186185
supportData.getProtosInDirectDeps(),
187-
/* protosInExports= */ null,
186+
supportData.getProtosInExports(),
188187
Label.parseAbsoluteUnchecked("//foo:bar"),
189188
/* allowServices= */ true,
190189
/* protocOpts= */ ImmutableList.of());
@@ -349,28 +348,32 @@ public void testProtoCommandLineArgv() throws Exception {
349348
assertThat(
350349
protoArgv(
351350
null /* directDependencies */,
352-
ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto"))))
351+
ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto")),
352+
ImmutableList.of(".")))
353353
.containsExactly("-Ifoo.proto=out/foo.proto");
354354

355355
assertThat(
356356
protoArgv(
357357
ImmutableList.of() /* directDependencies */,
358-
ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto"))))
358+
ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto")),
359+
ImmutableList.of(".")))
359360
.containsExactly("-Ifoo.proto=out/foo.proto", "--direct_dependencies=");
360361

361362
assertThat(
362-
protoArgv(
363-
ImmutableList.of(
364-
derivedArtifact("//:dont-care", "foo.proto")) /* directDependencies */,
365-
ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto"))))
363+
protoArgv(
364+
ImmutableList.of(
365+
derivedArtifact("//:dont-care", "foo.proto")) /* directDependencies */,
366+
ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto")),
367+
ImmutableList.of(".")))
366368
.containsExactly("-Ifoo.proto=out/foo.proto", "--direct_dependencies", "foo.proto");
367369

368370
assertThat(
369371
protoArgv(
370372
ImmutableList.of(
371373
derivedArtifact("//:dont-care", "foo.proto"),
372374
derivedArtifact("//:dont-care", "bar.proto")) /* directDependencies */,
373-
ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto"))))
375+
ImmutableList.of(derivedArtifact("//:dont-care", "foo.proto")),
376+
ImmutableList.of(".")))
374377
.containsExactly(
375378
"-Ifoo.proto=out/foo.proto", "--direct_dependencies", "foo.proto:bar.proto");
376379
}
@@ -386,17 +389,20 @@ public void testIncludeMapsOfExternalFiles() throws Exception {
386389
assertThat(
387390
protoArgv(
388391
null /* protosInDirectoDependencies */,
389-
ImmutableList.of(artifact("@bla//foo:bar", "external/bla/foo/bar.proto"))))
392+
ImmutableList.of(artifact("@bla//foo:bar", "external/bla/foo/bar.proto")),
393+
ImmutableList.of("external/bla")))
390394
.containsExactly("-Ifoo/bar.proto=external/bla/foo/bar.proto");
391395
}
392396

393-
// TODO(b/34107586): Fix and enable test.
394-
@Ignore
395397
@Test
396398
public void directDependenciesOnExternalFiles() throws Exception {
397399
ImmutableList<Artifact> protos =
398400
ImmutableList.of(artifact("@bla//foo:bar", "external/bla/foo/bar.proto"));
399-
assertThat(protoArgv(protos, protos)).containsExactly("--direct_dependencies", "foo/bar.proto");
401+
assertThat(protoArgv(protos, protos, ImmutableList.of("external/bla")))
402+
.containsExactly(
403+
"-Ifoo/bar.proto=external/bla/foo/bar.proto",
404+
"--direct_dependencies",
405+
"foo/bar.proto");
400406
}
401407

402408
private Artifact artifact(String ownerLabel, String path) {
@@ -416,7 +422,8 @@ private Artifact derivedArtifact(String ownerLabel, String path) {
416422

417423
private static Iterable<String> protoArgv(
418424
@Nullable Iterable<Artifact> protosInDirectDependencies,
419-
Iterable<Artifact> transitiveImports) {
425+
Iterable<Artifact> transitiveImports,
426+
Iterable<String> protoSourceRoots) {
420427
CustomCommandLine.Builder commandLine = CustomCommandLine.builder();
421428
NestedSet<Artifact> protosInDirectDependenciesBuilder =
422429
protosInDirectDependencies != null
@@ -427,7 +434,7 @@ private static Iterable<String> protoArgv(
427434
ProtoCompileActionBuilder.addIncludeMapArguments(
428435
commandLine,
429436
protosInDirectDependenciesBuilder,
430-
NestedSetBuilder.emptySet(STABLE_ORDER),
437+
NestedSetBuilder.wrap(Order.STABLE_ORDER, protoSourceRoots),
431438
transitiveImportsNestedSet);
432439
return commandLine.build().arguments();
433440
}

‎src/test/shell/bazel/bazel_proto_library_test.sh

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -412,8 +412,7 @@ function test_proto_source_root_glob() {
412412
|| fail "Expected success"
413413
}
414414

415-
# TODO(elenairina): Enable this after #4665 is fixed.
416-
function DISABLED_test_proto_source_root_multiple_workspaces() {
415+
function test_proto_source_root_multiple_workspaces() {
417416
write_workspace "a/b/"
418417
write_workspace "c/d/"
419418
write_workspace ""

0 commit comments

Comments
 (0)