Skip to content

Commit

Permalink
Create a virtual proto import directory for proto_library rules with …
Browse files Browse the repository at this point in the history 
…generated sources and ones with proto_source_root= set.

Fixes #7157. Third time is the charm!

This time, I made a flag for the fix so that it can be disabled if needed.

RELNOTES: None.
PiperOrigin-RevId: 257360059
  • Loading branch information
@lberki @copybara-github
lberki authored and copybara-github committed Jul 10, 2019
1 parent 2cc650e commit ce65de4
Show file tree
Hide file tree
Showing 6 changed files with 168 additions and 89 deletions.
5 changes: 4 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibrary.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ public class BazelProtoLibrary implements RuleConfiguredTargetFactory {

@Override
public ConfiguredTarget create(RuleContext ruleContext) throws ActionConflictException {
ProtoInfo protoInfo = ProtoCommon.createProtoInfo(ruleContext);
ProtoInfo protoInfo =
ProtoCommon.createProtoInfo(
ruleContext,
ruleContext.getFragment(ProtoConfiguration.class).generatedProtosInVirtualImports());
if (ruleContext.hasErrors()) {
return null;
}
Expand Down
153 changes: 87 additions & 66 deletions src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,29 +194,22 @@ public String getSourceRoot() {
// TODO(lberki): This should really be a PathFragment. Unfortunately, it's on the Starlark API of
// ProtoInfo so it's not an easy change :(
@Nullable
private static Library getProtoSourceRoot(
private static Library createLibraryWithoutVirtualSourceRoot(
RuleContext ruleContext, ImmutableList<Artifact> directSources) {
String protoSourceRoot = computeEffectiveProtoSourceRoot(ruleContext);
if (ruleContext.hasErrors()) {
return null;
}

// This is the same as getPackageIdentifier().getPathUnderExecRoot() due to the check above for
// protoSourceRoot == package name, but it's a bit more future-proof.
String result =
String protoSourceRoot =
ruleContext
.getLabel()
.getPackageIdentifier()
.getRepository()
.getPathUnderExecRoot()
.getRelative(protoSourceRoot)
.getPathString();

ImmutableList.Builder<Pair<Artifact, String>> builder = ImmutableList.builder();
for (Artifact protoSource : directSources) {
builder.add(new Pair<Artifact, String>(protoSource, null));
}
return new Library(directSources, result.isEmpty() ? "." : result, builder.build());
return new Library(
directSources, protoSourceRoot.isEmpty() ? "." : protoSourceRoot, builder.build());
}

private static PathFragment getPathFragmentAttribute(
Expand All @@ -243,49 +236,99 @@ private static PathFragment getPathFragmentAttribute(
* Returns the {@link Library} representing this <code>proto_library</code> rule if import prefix
* munging is done. Otherwise, returns null.
*/
private static Library createVirtualImportDirectoryMaybe(
RuleContext ruleContext, ImmutableList<Artifact> protoSources) {
private static Library createLibraryWithVirtualSourceRootMaybe(
RuleContext ruleContext,
ImmutableList<Artifact> protoSources,
boolean generatedProtosInVirtualImports) {
PathFragment importPrefixAttribute = getPathFragmentAttribute(ruleContext, "import_prefix");
PathFragment stripImportPrefixAttribute =
getPathFragmentAttribute(ruleContext, "strip_import_prefix");
PathFragment protoSourceRootAttribute =
getPathFragmentAttribute(ruleContext, "proto_source_root");
boolean hasGeneratedSources = false;

if (generatedProtosInVirtualImports) {
for (Artifact protoSource : protoSources) {
if (!protoSource.isSourceArtifact()) {
hasGeneratedSources = true;
break;
}
}
}

if (importPrefixAttribute == null && stripImportPrefixAttribute == null) {
if (importPrefixAttribute == null
&& stripImportPrefixAttribute == null
&& protoSourceRootAttribute == null
&& !hasGeneratedSources) {
// Simple case, no magic required.
return null;
}

if (!ruleContext.attributes().get("proto_source_root", STRING).isEmpty()) {
if (protoSourceRootAttribute != null
&& (importPrefixAttribute != null || stripImportPrefixAttribute != null)) {
ruleContext.ruleError(
"the 'proto_source_root' attribute is incompatible with "
+ "'strip_import_prefix' and 'import_prefix");
return null;
}

PathFragment stripImportPrefix;
if (stripImportPrefixAttribute == null) {
stripImportPrefix = PathFragment.create(ruleContext.getLabel().getWorkspaceRoot());
} else if (stripImportPrefixAttribute.isAbsolute()) {
stripImportPrefix =
ruleContext
.getLabel()
.getPackageIdentifier()
.getRepository()
.getSourceRoot()
.getRelative(stripImportPrefixAttribute.toRelative());
} else {
stripImportPrefix = ruleContext.getPackageDirectory().getRelative(stripImportPrefixAttribute);
}

PathFragment importPrefix;
if (importPrefixAttribute == null) {

if (stripImportPrefixAttribute != null || importPrefixAttribute != null) {
if (stripImportPrefixAttribute == null) {
stripImportPrefix = PathFragment.create(ruleContext.getLabel().getWorkspaceRoot());
} else if (stripImportPrefixAttribute.isAbsolute()) {
stripImportPrefix =
ruleContext
.getLabel()
.getPackageIdentifier()
.getRepository()
.getSourceRoot()
.getRelative(stripImportPrefixAttribute.toRelative());
} else {
stripImportPrefix =
ruleContext.getPackageDirectory().getRelative(stripImportPrefixAttribute);
}

if (importPrefixAttribute != null) {
importPrefix = importPrefixAttribute;
} else {
importPrefix = PathFragment.EMPTY_FRAGMENT;
}

if (importPrefix.isAbsolute()) {
ruleContext.attributeError("import_prefix", "should be a relative path");
return null;
}
} else if (protoSourceRootAttribute != null) {
if (!ruleContext.getFragment(ProtoConfiguration.class).enableProtoSourceroot()) {
ruleContext.attributeError("proto_source_root", "this attribute is not supported anymore");
return null;
}

if (!ruleContext.getLabel().getPackageFragment().equals(protoSourceRootAttribute)) {
ruleContext.attributeError(
"proto_source_root",
"proto_source_root must be the same as the package name ("
+ ruleContext.getLabel().getPackageName()
+ ")."
+ " not '"
+ protoSourceRootAttribute
+ "'.");

return null;
}

stripImportPrefix = ruleContext.getPackageDirectory();
importPrefix = PathFragment.EMPTY_FRAGMENT;
} else {
importPrefix = importPrefixAttribute;
}
// Has generated sources, but neither strip_import_prefix nor import_prefix nor
// proto_source_root.
stripImportPrefix =
ruleContext.getLabel().getPackageIdentifier().getRepository().getPathUnderExecRoot();

if (importPrefix.isAbsolute()) {
ruleContext.attributeError("import_prefix", "should be a relative path");
return null;
importPrefix = PathFragment.EMPTY_FRAGMENT;
}

ImmutableList.Builder<Artifact> symlinks = ImmutableList.builder();
Expand All @@ -308,7 +351,9 @@ private static Library createVirtualImportDirectoryMaybe(
symlinks.add(importsPair.second);

if (!ruleContext.getFragment(ProtoConfiguration.class).doNotUseBuggyImportPath()
&& stripImportPrefixAttribute == null) {
&& stripImportPrefixAttribute == null
&& protoSourceRootAttribute == null
&& !hasGeneratedSources) {
Pair<PathFragment, Artifact> oldImportsPair =
computeImports(
ruleContext,
Expand Down Expand Up @@ -395,29 +440,6 @@ private static NestedSet<String> computeExportedProtoSourceRoots(
return getProtoSourceRootsOfAttribute(ruleContext, currentProtoSourceRoot, "exports");
}

private static String computeEffectiveProtoSourceRoot(RuleContext ruleContext) {
if (!ruleContext.attributes().isAttributeValueExplicitlySpecified("proto_source_root")) {
return "";
}

if (!ruleContext.getFragment(ProtoConfiguration.class).enableProtoSourceroot()) {
ruleContext.attributeError("proto_source_root", "this attribute is not supported anymore");
return "";
}

String protoSourceRoot = ruleContext.attributes().get("proto_source_root", STRING);
if (!ruleContext.getLabel().getPackageName().equals(protoSourceRoot)) {
ruleContext.attributeError(
"proto_source_root",
"proto_source_root must be the same as the package name ("
+ ruleContext.getLabel().getPackageName() + ")."
+ " not '" + protoSourceRoot + "'."
);
}

return protoSourceRoot;
}

/**
* Check that .proto files in sources are from the same package. This is done to avoid clashes
* with the generated sources.
Expand All @@ -442,21 +464,20 @@ private static boolean isConfiguredTargetInSamePackage(RuleContext ruleContext,
* Creates the {@link ProtoInfo} for the {@code proto_library} rule associated with {@code
* ruleContext}.
*/
public static ProtoInfo createProtoInfo(RuleContext ruleContext) {
public static ProtoInfo createProtoInfo(
RuleContext ruleContext, boolean generatedProtosInVirtualImports) {
checkSourceFilesAreInSamePackage(ruleContext);
ImmutableList<Artifact> directProtoSources =
ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET).list();
Library library = createVirtualImportDirectoryMaybe(ruleContext, directProtoSources);
Library library =
createLibraryWithVirtualSourceRootMaybe(
ruleContext, directProtoSources, generatedProtosInVirtualImports);
if (ruleContext.hasErrors()) {
return null;
}

if (library == null) {
library = getProtoSourceRoot(ruleContext, directProtoSources);
}

if (ruleContext.hasErrors()) {
return null;
library = createLibraryWithoutVirtualSourceRoot(ruleContext, directProtoSources);
}

NestedSet<Artifact> transitiveProtoSources =
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ public static class Options extends FragmentOptions {
+ " https://github.com/bazelbuild/bazel/issues/8711 for details.")
public boolean doNotUseBuggyImportPath;

@Option(
name = "experimental_generated_protos_in_virtual_imports",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
help = "If set, generated .proto files are put into a virtual import directory.")
public boolean generatedProtosInVirtualImports;

@Option(
name = "protocopt",
allowMultiple = true,
Expand Down Expand Up @@ -307,4 +315,8 @@ public boolean strictPublicImports() {
public boolean doNotUseBuggyImportPath() {
return options.doNotUseBuggyImportPath;
}

public boolean generatedProtosInVirtualImports() {
return options.generatedProtosInVirtualImports;
}
}
7 changes: 4 additions & 3 deletions src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoInfoStarlarkTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public void testProvider() throws Exception {
public void testProtoSourceRootExportedInStarlark() throws Exception {

scratch.file(
"foo/myTsetRule.bzl",
"foo/myTestRule.bzl",
"load('//myinfo:myinfo.bzl', 'MyInfo')",
"",
"def _my_test_rule_impl(ctx):",
Expand All @@ -180,7 +180,7 @@ public void testProtoSourceRootExportedInStarlark() throws Exception {
scratch.file(
"foo/BUILD",
"",
"load(':myTsetRule.bzl', 'my_test_rule')",
"load(':myTestRule.bzl', 'my_test_rule')",
"my_test_rule(",
" name = 'myRule',",
" protodep = ':myProto',",
Expand All @@ -193,7 +193,8 @@ public void testProtoSourceRootExportedInStarlark() throws Exception {

ConfiguredTarget ct = getConfiguredTarget("//foo:myRule");
String protoSourceRoot = (String) getMyInfoFromTarget(ct).getValue("fetched_proto_source_root");
String genfiles = getTargetConfiguration().getGenfilesFragment().toString();

assertThat(protoSourceRoot).isEqualTo("foo");
assertThat(protoSourceRoot).isEqualTo(genfiles + "/foo/_virtual_imports/myProto");
}
}
70 changes: 59 additions & 11 deletions src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,11 +261,13 @@ public void testProtoSourceRootWithoutDeps() throws Exception {
);
ConfiguredTarget protoTarget = getConfiguredTarget("//x/foo:nodeps");
ProtoInfo sourcesProvider = protoTarget.get(ProtoInfo.PROVIDER);
assertThat(sourcesProvider.getTransitiveProtoSourceRoots()).containsExactly("x/foo");
String genfiles = getTargetConfiguration().getGenfilesFragment().toString();

assertThat(getGeneratingSpawnAction(getDescriptorOutput("//x/foo:nodeps"))
.getRemainingArguments())
.contains("--proto_path=x/foo");
assertThat(sourcesProvider.getTransitiveProtoSourceRoots())
.containsExactly(genfiles + "/x/foo/_virtual_imports/nodeps");
assertThat(
getGeneratingSpawnAction(getDescriptorOutput("//x/foo:nodeps")).getRemainingArguments())
.contains("--proto_path=" + genfiles + "/x/foo/_virtual_imports/nodeps");
}

@Test
Expand Down Expand Up @@ -308,11 +310,18 @@ public void testProtoSourceRootWithDepsDuplicate() throws Exception {
);
ConfiguredTarget protoTarget = getConfiguredTarget("//x/foo:withdeps");
ProtoInfo sourcesProvider = protoTarget.get(ProtoInfo.PROVIDER);
assertThat(sourcesProvider.getTransitiveProtoSourceRoots()).containsExactly("x/foo");
String genfiles = getTargetConfiguration().getGenfilesFragment().toString();
assertThat(sourcesProvider.getTransitiveProtoSourceRoots())
.containsExactly(
genfiles + "/x/foo/_virtual_imports/dep",
genfiles + "/x/foo/_virtual_imports/withdeps");

assertThat(getGeneratingSpawnAction(getDescriptorOutput("//x/foo:withdeps"))
.getRemainingArguments())
.contains("--proto_path=x/foo");
assertThat(
getGeneratingSpawnAction(getDescriptorOutput("//x/foo:withdeps"))
.getRemainingArguments())
.containsAtLeast(
"--proto_path=" + genfiles + "/x/foo/_virtual_imports/withdeps",
"--proto_path=" + genfiles + "/x/foo/_virtual_imports/dep");
}

@Test
Expand Down Expand Up @@ -340,8 +349,42 @@ public void testProtoSourceRootWithDeps() throws Exception {
);
ConfiguredTarget protoTarget = getConfiguredTarget("//x/foo:withdeps");
ProtoInfo sourcesProvider = protoTarget.get(ProtoInfo.PROVIDER);
String genfiles = getTargetConfiguration().getGenfilesFragment().toString();
assertThat(sourcesProvider.getTransitiveProtoSourceRoots())
.containsExactly("x/foo", "x/bar", ".");
.containsExactly(
genfiles + "/x/foo/_virtual_imports/withdeps",
genfiles + "/x/bar/_virtual_imports/dep",
".");
}

@Test
public void testExternalRepoWithGeneratedProto() throws Exception {
if (!isThisBazel()) {
return;
}

FileSystemUtils.appendIsoLatin1(
scratch.resolve("WORKSPACE"), "local_repository(name = 'foo', path = '/foo')");
invalidatePackages();

scratch.file("/foo/WORKSPACE");
scratch.file(
"/foo/x/BUILD",
"proto_library(name='x', srcs=['generated.proto'])",
"genrule(name='g', srcs=[], outs=['generated.proto'], cmd='')");

scratch.file("a/BUILD", "proto_library(name='a', srcs=['a.proto'], deps=['@foo//x:x'])");

String genfiles = getTargetConfiguration().getGenfilesFragment().toString();
ConfiguredTarget a = getConfiguredTarget("//a:a");
ProtoInfo aInfo = a.get(ProtoInfo.PROVIDER);
assertThat(aInfo.getTransitiveProtoSourceRoots())
.containsExactly(".", genfiles + "/external/foo/x/_virtual_imports/x");

ConfiguredTarget x = getConfiguredTarget("@foo//x:x");
ProtoInfo xInfo = x.get(ProtoInfo.PROVIDER);
assertThat(xInfo.getTransitiveProtoSourceRoots())
.containsExactly(genfiles + "/external/foo/x/_virtual_imports/x");
}

@Test
Expand Down Expand Up @@ -380,7 +423,9 @@ public void testExportedProtoSourceRoots() throws Exception {
// exported proto source roots should be the source root of the rule and the direct source roots
// of its exports and nothing else (not the exports of its exports or the deps of its exports
// or the exports of its deps)
assertThat(c.get(ProtoInfo.PROVIDER).getExportedProtoSourceRoots()).containsExactly("a", "c");
String genfiles = getTargetConfiguration().getGenfilesFragment().toString();
assertThat(c.get(ProtoInfo.PROVIDER).getExportedProtoSourceRoots())
.containsExactly(genfiles + "/a/_virtual_imports/a", genfiles + "/c/_virtual_imports/c");
}

@Test
Expand All @@ -396,7 +441,10 @@ public void testProtoSourceRoot() throws Exception {
ConfiguredTarget protoTarget = getConfiguredTarget("//x/foo:banana");
ProtoInfo sourcesProvider = protoTarget.get(ProtoInfo.PROVIDER);

assertThat(sourcesProvider.getDirectProtoSourceRoot()).isEqualTo("x/foo");
String genfiles = getTargetConfiguration().getGenfilesFragment().toString();

assertThat(sourcesProvider.getDirectProtoSourceRoot())
.isEqualTo(genfiles + "/x/foo/_virtual_imports/banana");
}

@Test
Expand Down
Loading

0 comments on commit ce65de4

Please sign in to comment.