Skip to content

Commit

Permalink
Accept proto paths relative to proto_source_root as direct dependencies.
Browse files Browse the repository at this point in the history
This will make protoc see as direct dependencies the .proto files that were included using the proto_source_root flag.

Until now, Bazel passed to protoc the direct dependencies of a target as the path relative to the WORKSPACE, which made it fail when a shorter path, relative to the package was used.

Progress on #4544.

RELNOTES: None.
PiperOrigin-RevId: 186294997
  • Loading branch information
iirina authored and Copybara-Service committed Feb 20, 2018
1 parent 9ac1069 commit 5deca4c
Show file tree
Hide file tree
Showing 11 changed files with 548 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,7 @@ private void createProtoCompileAction(SupportData supportData, Collection<Artifa
supportData.getTransitiveImports(),
supportData.getProtosInDirectDeps(),
supportData.getTransitiveProtoPathFlags(),
supportData.getDirectProtoSourceRoots(),
ruleContext.getLabel(),
outputs,
"C++",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ private void createProtoCompileAction(Artifact sourceJar) {
supportData.getTransitiveImports(),
supportData.getProtosInDirectDeps(),
supportData.getTransitiveProtoPathFlags(),
supportData.getDirectProtoSourceRoots(),
ruleContext.getLabel(),
ImmutableList.of(sourceJar),
"JavaLite",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ private void createProtoCompileAction(Artifact sourceJar) {
supportData.getTransitiveImports(),
supportData.getProtosInDirectDeps(),
supportData.getTransitiveProtoPathFlags(),
supportData.getDirectProtoSourceRoots(),
ruleContext.getLabel(),
ImmutableList.of(sourceJar),
"Java (Immutable)",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public static void createProtoCompileAction(
supportData.getTransitiveImports(),
supportData.getProtosInDirectDeps(),
supportData.getTransitiveProtoPathFlags(),
supportData.getDirectProtoSourceRoots(),
skylarkRuleContext.getLabel(),
ImmutableList.of(sourceJar),
"JavaLite",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,17 +44,22 @@ public ConfiguredTarget create(RuleContext ruleContext)

NestedSet<Artifact> transitiveImports =
ProtoCommon.collectTransitiveImports(ruleContext, protoSources);
NestedSet<String> protoPathFlags = ProtoCommon.collectTransitiveProtoPathFlags(ruleContext);

NestedSet<Artifact> protosInDirectDeps = ProtoCommon.computeProtosInDirectDeps(ruleContext);

String protoSourceRoot = ProtoCommon.getProtoSourceRoot(ruleContext);
NestedSet<String> directProtoSourceRoots =
ProtoCommon.getProtoSourceRootsOfDirectDependencies(ruleContext, protoSourceRoot);
NestedSet<String> protoPathFlags =
ProtoCommon.collectTransitiveProtoPathFlags(ruleContext, protoSourceRoot);

final SupportData supportData =
SupportData.create(
Predicates.<TransitiveInfoCollection>alwaysTrue() /* nonWeakDepsPredicate */,
protoSources,
protosInDirectDeps,
transitiveImports,
protoPathFlags,
directProtoSourceRoots,
!protoSources.isEmpty());

Artifact descriptorSetOutput =
Expand All @@ -74,7 +79,8 @@ public ConfiguredTarget create(RuleContext ruleContext)
descriptorSetOutput,
true /* allowServices */,
dependenciesDescriptorSets,
protoPathFlags);
protoPathFlags,
directProtoSourceRoots);

Runfiles dataRunfiles =
ProtoCommon.createDataRunfilesProvider(transitiveImports, ruleContext)
Expand All @@ -90,7 +96,8 @@ public ConfiguredTarget create(RuleContext ruleContext)
checkDepsProtoSources,
descriptorSetOutput,
transitiveDescriptorSetOutput,
protoPathFlags);
protoPathFlags,
protoSourceRoot);

return new RuleConfiguredTargetBuilder(ruleContext)
.setFilesToBuild(NestedSetBuilder.create(STABLE_ORDER, descriptorSetOutput))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,29 +100,66 @@ public static NestedSet<Artifact> collectDependenciesDescriptorSets(RuleContext
}

/**
* Returns all proto source roots in this lib and in its transitive dependencies, each prefixed
* by {@code --proto_path}.
* Returns all proto source roots in this lib ({@code currentProtoSourceRoot}) and in its
* transitive dependencies, each prefixed by {@code --proto_path}.
*
* Build will fail if the {@code proto_source_root} of the current lib is different than the
* package name.
* Assumes {@code currentProtoSourceRoot} is the same as the package name.
*/
public static NestedSet<String> collectTransitiveProtoPathFlags(RuleContext ruleContext) {
public static NestedSet<String> collectTransitiveProtoPathFlags(
RuleContext ruleContext, String currentProtoSourceRoot) {
NestedSetBuilder<String> protoPathFlags = NestedSetBuilder.stableOrder();

// first add the protoSourceRoot of the current target, if any
if (currentProtoSourceRoot != null && !currentProtoSourceRoot.isEmpty()) {
protoPathFlags.add("--proto_path=" + currentProtoSourceRoot);
}

for (ProtoSourcesProvider provider : ruleContext.getPrerequisites(
"deps", Mode.TARGET, ProtoSourcesProvider.class)) {
protoPathFlags.addTransitive(provider.getTransitiveProtoPathFlags());
}

return protoPathFlags.build();
}

/**
* Returns the {@code proto_source_root} of the current library or null if none is specified.
*
* Build will fail if the {@code proto_source_root} of the current library is different than the
* package name.
*/
@Nullable
public static String getProtoSourceRoot(RuleContext ruleContext) {
String protoSourceRoot =
ruleContext.attributes().get("proto_source_root", Type.STRING);
if (protoSourceRoot != null && !protoSourceRoot.isEmpty()) {
checkProtoSourceRootIsTheSameAsPackage(protoSourceRoot, ruleContext);
protoPathFlags.add("--proto_path=" + protoSourceRoot);
}
return protoSourceRoot;
}

/**
* Returns a set of the {@code proto_source_root} collected from the current library and the
* direct dependencies.
*
* Assumes {@code currentProtoSourceRoot} is the same as the package name.
*/
public static NestedSet<String> getProtoSourceRootsOfDirectDependencies(
RuleContext ruleContext, String currentProtoSourceRoot) {
NestedSetBuilder<String> protoSourceRoots = NestedSetBuilder.stableOrder();
if (currentProtoSourceRoot != null && !currentProtoSourceRoot.isEmpty()) {
protoSourceRoots.add(currentProtoSourceRoot);
}

for (ProtoSourcesProvider provider : ruleContext.getPrerequisites(
"deps", Mode.TARGET, ProtoSourcesProvider.class)) {
protoPathFlags.addTransitive(provider.getTransitiveProtoPathFlags());
"deps", Mode.TARGET, ProtoSourcesProvider.class)) {
String protoSourceRoot = provider.getProtoSourceRoot();
if (protoSourceRoot != null && !protoSourceRoot.isEmpty()) {
protoSourceRoots.add(provider.getProtoSourceRoot());
}
}

return protoPathFlags.build();
return protoSourceRoots.build();
}

private static void checkProtoSourceRootIsTheSameAsPackage(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ CustomCommandLine.Builder createProtoCompilerCommandLine() throws MissingPrerequ
addIncludeMapArguments(
result,
areDepsStrict ? supportData.getProtosInDirectDeps() : null,
supportData.getDirectProtoSourceRoots(),
supportData.getTransitiveImports());

if (areDepsStrict) {
Expand Down Expand Up @@ -360,7 +361,8 @@ public static void writeDescriptorSet(
Artifact output,
boolean allowServices,
NestedSet<Artifact> transitiveDescriptorSets,
NestedSet<String> protoSourceRoots) {
NestedSet<String> protoSourceRoots,
NestedSet<String> directProtoSourceRoots) {
if (protosToCompile.isEmpty()) {
ruleContext.registerAction(
FileWriteAction.createEmptyWithInputs(
Expand All @@ -376,6 +378,7 @@ public static void writeDescriptorSet(
transitiveSources,
protosInDirectDeps,
protoSourceRoots,
directProtoSourceRoots,
ruleContext.getLabel(),
ImmutableList.of(output),
"Descriptor Set",
Expand Down Expand Up @@ -423,6 +426,7 @@ public static void registerActions(
NestedSet<Artifact> transitiveSources,
NestedSet<Artifact> protosInDirectDeps,
NestedSet<String> protoSourceRoots,
NestedSet<String> directProtoSourceRoots,
Label ruleLabel,
Iterable<Artifact> outputs,
String flavorName,
Expand All @@ -435,6 +439,7 @@ public static void registerActions(
transitiveSources,
protosInDirectDeps,
protoSourceRoots,
directProtoSourceRoots,
ruleLabel,
outputs,
flavorName,
Expand All @@ -452,6 +457,7 @@ private static SpawnAction.Builder createActions(
NestedSet<Artifact> transitiveSources,
@Nullable NestedSet<Artifact> protosInDirectDeps,
NestedSet<String> protoSourceRoots,
NestedSet<String> directProtoSourceRoots,
Label ruleLabel,
Iterable<Artifact> outputs,
String flavorName,
Expand Down Expand Up @@ -487,6 +493,7 @@ private static SpawnAction.Builder createActions(
protosToCompile,
transitiveSources,
protoSourceRoots,
directProtoSourceRoots,
areDepsStrict(ruleContext) ? protosInDirectDeps : null,
ruleLabel,
allowServices,
Expand Down Expand Up @@ -524,11 +531,13 @@ static CustomCommandLine createCommandLineFromToolchains(
Iterable<Artifact> protosToCompile,
NestedSet<Artifact> transitiveSources,
NestedSet<String> transitiveProtoPathFlags,
NestedSet<String> directProtoSourceRoots,
@Nullable NestedSet<Artifact> protosInDirectDeps,
Label ruleLabel,
boolean allowServices,
ImmutableList<String> protocOpts) {
CustomCommandLine.Builder cmdLine = CustomCommandLine.builder();
cmdLine.addAll(transitiveProtoPathFlags);

cmdLine.addAll(transitiveProtoPathFlags);

Expand Down Expand Up @@ -565,7 +574,7 @@ static CustomCommandLine createCommandLineFromToolchains(
cmdLine.addAll(protocOpts);

// Add include maps
addIncludeMapArguments(cmdLine, protosInDirectDeps, transitiveSources);
addIncludeMapArguments(cmdLine, protosInDirectDeps, directProtoSourceRoots, transitiveSources);

if (protosInDirectDeps != null) {
cmdLine.addFormatted(STRICT_DEPS_FLAG_TEMPLATE, ruleLabel);
Expand All @@ -586,6 +595,7 @@ static CustomCommandLine createCommandLineFromToolchains(
static void addIncludeMapArguments(
CustomCommandLine.Builder commandLine,
@Nullable NestedSet<Artifact> protosInDirectDependencies,
NestedSet<String> directProtoSourceRoots,
NestedSet<Artifact> transitiveImports) {
commandLine.addAll(
VectorArg.of(transitiveImports)
Expand All @@ -596,7 +606,14 @@ static void addIncludeMapArguments(
"--direct_dependencies",
VectorArg.join(":")
.each(protosInDirectDependencies)
.mapped(ProtoCompileActionBuilder::expandToPathIgnoringRepository));
.mapped((Artifact proto, Consumer<String> args) -> {
for (String directProtoSourceRoot : directProtoSourceRoots) {
expandToPathIgnoringSourceRoot(proto, directProtoSourceRoot, args);
}
expandToPathIgnoringRepository(proto, args);
})
);

} else {
// The proto compiler requires an empty list to turn on strict deps checking
commandLine.add("--direct_dependencies=");
Expand Down Expand Up @@ -627,6 +644,20 @@ private static String getPathIgnoringRepository(Artifact artifact) {
.toString();
}

private static void expandToPathIgnoringSourceRoot(
Artifact artifact, String directProtoSourceRoot, Consumer<String> args) {
try {
String relativePath = artifact.getRootRelativePath()
.relativeTo(
artifact.getOwnerLabel().getPackageIdentifier().getRepository().getPathUnderExecRoot())
.relativeTo(directProtoSourceRoot)
.toString();
args.accept(relativePath);
} catch (IllegalArgumentException exception) {
// do nothing
}
}

/**
* 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
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,17 @@ public static ProtoSourcesProvider create(
NestedSet<Artifact> checkDepsProtoSources,
Artifact directDescriptorSet,
NestedSet<Artifact> transitiveDescriptorSets,
NestedSet<String> protoPathFlags) {
NestedSet<String> protoPathFlags,
String protoSourceRoot) {
return new AutoValue_ProtoSourcesProvider(
transitiveImports,
transitiveProtoSources,
protoSources,
checkDepsProtoSources,
directDescriptorSet,
transitiveDescriptorSets,
protoPathFlags);
protoPathFlags,
protoSourceRoot);
}

/**
Expand Down Expand Up @@ -135,5 +137,10 @@ public static ProtoSourcesProvider create(
*/
public abstract NestedSet<String> getTransitiveProtoPathFlags();

/**
* The {@code proto_source_root} of the current library.
*/
public abstract String getProtoSourceRoot();

ProtoSourcesProvider() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,11 @@ public static SupportData create(
NestedSet<Artifact> protosInDirectDeps,
NestedSet<Artifact> transitiveImports,
NestedSet<String> transitiveProtoPathFlags,
NestedSet<String> directProtoSourceRoots,
boolean hasProtoSources) {
return new AutoValue_SupportData(
nonWeakDepsPredicate, protoSources, transitiveImports, protosInDirectDeps,
transitiveProtoPathFlags, hasProtoSources);
transitiveProtoPathFlags, directProtoSourceRoots, hasProtoSources);
}

public abstract Predicate<TransitiveInfoCollection> getNonWeakDepsPredicate();
Expand All @@ -57,6 +58,11 @@ public static SupportData create(
*/
public abstract NestedSet<String> getTransitiveProtoPathFlags();

/**
* The {@code proto_source_root}'s collected from the current library and the direct dependencies.
*/
public abstract NestedSet<String> getDirectProtoSourceRoots();

public abstract boolean hasProtoSources();

SupportData() {}
Expand Down
Loading

0 comments on commit 5deca4c

Please sign in to comment.