Skip to content

Commit

Permalink
Always strip /external/<repo_name> from proto import path
Browse files Browse the repository at this point in the history 
Before this PR, when strip_import_prefix was not specified, and the proto target
was declared in an external repository, Bazel generated import path that leaked
`external/<repository_name>` path fragment. That path fragment is an
implementation detail of Bazel's execroot layout and should not be used in proto
source files.

This PR adds a correct path fragment to the import paths. It also introduces an incompatible flag `--incompatible_do_not_emit_buggy_external_repo_import` that removes the incorrect import path from Bazel.

Fixes bazelbuild#8030.

Incompatible change: bazelbuild#8711.

Closes bazelbuild#8683.

PiperOrigin-RevId: 255606742
  • Loading branch information
@hlopko @irengrig
hlopko authored and irengrig committed Jul 15, 2019
1 parent 0e1788e commit d790ba5
Show file tree
Hide file tree
Showing 6 changed files with 363 additions and 23 deletions.
5 changes: 5 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,11 @@ private CcCompilationHelper initializeCompilationHelper(
if (protoRootFragment.startsWith(binOrGenfiles)) {
protoRootFragment = protoRootFragment.relativeTo(binOrGenfiles);
}
PathFragment repositoryPath =
ruleContext.getLabel().getPackageIdentifier().getRepository().getPathUnderExecRoot();
if (protoRootFragment.startsWith(repositoryPath)) {
protoRootFragment = protoRootFragment.relativeTo(repositoryPath);
}

String stripIncludePrefix =
PathFragment.create("//").getRelative(protoRootFragment).toString();
Expand Down
74 changes: 51 additions & 23 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 @@ -245,10 +245,11 @@ private static PathFragment getPathFragmentAttribute(
*/
private static Library createVirtualImportDirectoryMaybe(
RuleContext ruleContext, ImmutableList<Artifact> protoSources) {
PathFragment importPrefix = getPathFragmentAttribute(ruleContext, "import_prefix");
PathFragment stripImportPrefix = getPathFragmentAttribute(ruleContext, "strip_import_prefix");
PathFragment importPrefixAttribute = getPathFragmentAttribute(ruleContext, "import_prefix");
PathFragment stripImportPrefixAttribute =
getPathFragmentAttribute(ruleContext, "strip_import_prefix");

if (importPrefix == null && stripImportPrefix == null) {
if (importPrefixAttribute == null && stripImportPrefixAttribute == null) {
// Simple case, no magic required.
return null;
}
Expand All @@ -260,22 +261,26 @@ private static Library createVirtualImportDirectoryMaybe(
return null;
}

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

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

if (importPrefix.isAbsolute()) {
Expand All @@ -296,12 +301,44 @@ private static Library createVirtualImportDirectoryMaybe(
realProtoSource.getExecPathString(), stripImportPrefix.getPathString()));
continue;
}
Pair<PathFragment, Artifact> importsPair =
computeImports(
ruleContext, realProtoSource, sourceRootPath, importPrefix, stripImportPrefix);
protoSourceImportPair.add(new Pair<>(realProtoSource, importsPair.first.toString()));
symlinks.add(importsPair.second);

if (!ruleContext.getFragment(ProtoConfiguration.class).doNotUseBuggyImportPath()
&& stripImportPrefixAttribute == null) {
Pair<PathFragment, Artifact> oldImportsPair =
computeImports(
ruleContext,
realProtoSource,
sourceRootPath,
importPrefix,
PathFragment.EMPTY_FRAGMENT);
protoSourceImportPair.add(new Pair<>(realProtoSource, oldImportsPair.first.toString()));
symlinks.add(oldImportsPair.second);
}
}

PathFragment importPath =
importPrefix.getRelative(
realProtoSource.getRootRelativePath().relativeTo(stripImportPrefix));
String sourceRoot =
ruleContext
.getBinOrGenfilesDirectory()
.getExecPath()
.getRelative(sourceRootPath)
.getPathString();
return new Library(symlinks.build(), sourceRoot, protoSourceImportPair.build());
}

protoSourceImportPair.add(new Pair<>(realProtoSource, importPath.toString()));
private static Pair<PathFragment, Artifact> computeImports(
RuleContext ruleContext,
Artifact realProtoSource,
PathFragment sourceRootPath,
PathFragment importPrefix,
PathFragment stripImportPrefix) {
PathFragment importPath =
importPrefix.getRelative(
realProtoSource.getRootRelativePath().relativeTo(stripImportPrefix));

Artifact virtualProtoSource =
ruleContext.getDerivedArtifact(
Expand All @@ -314,16 +351,7 @@ private static Library createVirtualImportDirectoryMaybe(
virtualProtoSource,
"Symlinking virtual .proto sources for " + ruleContext.getLabel()));

symlinks.add(virtualProtoSource);
}

String sourceRoot =
ruleContext
.getBinOrGenfilesDirectory()
.getExecPath()
.getRelative(sourceRootPath)
.getPathString();
return new Library(symlinks.build(), sourceRoot, protoSourceImportPair.build());
return Pair.of(importPath, virtualProtoSource);
}

/**
Expand Down
21 changes: 21 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 @@ -40,6 +40,22 @@
public class ProtoConfiguration extends Fragment implements ProtoConfigurationApi {
/** Command line options. */
public static class Options extends FragmentOptions {
@Option(
name = "incompatible_do_not_emit_buggy_external_repo_import",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
help =
"If true, Bazel will not emit import paths for external repos that used to be emitted"
+ " because of https://github.com/bazelbuild/bazel/issues/8030. This is now fixed"
+ " and those imports are only emitted for backwards compatibility. See"
+ " https://github.com/bazelbuild/bazel/issues/8711 for details.")
public boolean doNotUseBuggyImportPath;

@Option(
name = "protocopt",
allowMultiple = true,
Expand Down Expand Up @@ -193,6 +209,7 @@ public FragmentOptions getHost() {
host.ccProtoLibrarySourceSuffixes = ccProtoLibrarySourceSuffixes;
host.experimentalJavaProtoAddAllowedPublicImports =
experimentalJavaProtoAddAllowedPublicImports;
host.doNotUseBuggyImportPath = doNotUseBuggyImportPath;
return host;
}
}
Expand Down Expand Up @@ -286,4 +303,8 @@ public List<String> ccProtoLibrarySourceSuffixes() {
public boolean strictPublicImports() {
return options.experimentalJavaProtoAddAllowedPublicImports;
}

public boolean doNotUseBuggyImportPath() {
return options.doNotUseBuggyImportPath;
}
}
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1497,6 +1497,7 @@ java_test(
":guava_junit_truth",
"//src/main/java/com/google/devtools/build/lib:build-base",
"//src/main/java/com/google/devtools/build/lib:proto-rules",
"//src/main/java/com/google/devtools/build/lib:util",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/vfs",
],
Expand Down
88 changes: 88 additions & 0 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoLibraryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@
import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.prettyArtifactNames;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.eventbus.EventBus;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.bazel.rules.cpp.proto.BazelCcProtoAspect;
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.util.Crosstool.CcToolchainConfig;
import com.google.devtools.build.lib.rules.cpp.CcCompilationContext;
Expand All @@ -41,6 +43,7 @@

@RunWith(JUnit4.class)
public class CcProtoLibraryTest extends BuildViewTestCase {

@Before
public void setUp() throws Exception {
mockToolsConfig.create("/protobuf/WORKSPACE");
Expand Down Expand Up @@ -243,4 +246,89 @@ public void generatedSourcesNotCoverageInstrumented() throws Exception {
assertThat(options).doesNotContain("-fprofile-arcs");
assertThat(options).doesNotContain("-ftest-coverage");
}

@Test
public void importPrefixWorksWithRepositories() throws Exception {
FileSystemUtils.appendIsoLatin1(
scratch.resolve("WORKSPACE"), "local_repository(name = 'yolo_repo', path = '/yolo_repo')");
invalidatePackages();
useConfiguration("--incompatible_do_not_emit_buggy_external_repo_import");

scratch.file("/yolo_repo/WORKSPACE");
scratch.file("/yolo_repo/yolo_pkg/yolo.proto");
scratch.file(
"/yolo_repo/yolo_pkg/BUILD",
"proto_library(",
" name = 'yolo_proto',",
" srcs = ['yolo.proto'],",
" import_prefix = 'bazel.build/yolo',",
")",
"cc_proto_library(",
" name = 'yolo_cc_proto',",
" deps = [':yolo_proto'],",
")");
assertThat(getTarget("@yolo_repo//yolo_pkg:yolo_cc_proto")).isNotNull();
assertThat(getProtoHeaderExecPath())
.endsWith("_virtual_includes/yolo_proto/bazel.build/yolo/yolo_pkg/yolo.pb.h");
}

@Test
public void stripImportPrefixWorksWithRepositories() throws Exception {
FileSystemUtils.appendIsoLatin1(
scratch.resolve("WORKSPACE"), "local_repository(name = 'yolo_repo', path = '/yolo_repo')");
invalidatePackages();
useConfiguration("--incompatible_do_not_emit_buggy_external_repo_import");

scratch.file("/yolo_repo/WORKSPACE");
scratch.file("/yolo_repo/yolo_pkg/yolo.proto");
scratch.file(
"/yolo_repo/yolo_pkg/BUILD",
"proto_library(",
" name = 'yolo_proto',",
" srcs = ['yolo.proto'],",
" strip_import_prefix = '/yolo_pkg',",
")",
"cc_proto_library(",
" name = 'yolo_cc_proto',",
" deps = [':yolo_proto'],",
")");
assertThat(getTarget("@yolo_repo//yolo_pkg:yolo_cc_proto")).isNotNull();
assertThat(getProtoHeaderExecPath()).endsWith("_virtual_includes/yolo_proto/yolo.pb.h");
}

@Test
public void importPrefixAndStripImportPrefixWorksWithRepositories() throws Exception {
FileSystemUtils.appendIsoLatin1(
scratch.resolve("WORKSPACE"), "local_repository(name = 'yolo_repo', path = '/yolo_repo')");
invalidatePackages();
useConfiguration("--incompatible_do_not_emit_buggy_external_repo_import");

scratch.file("/yolo_repo/WORKSPACE");
scratch.file("/yolo_repo/yolo_pkg/yolo.proto");
scratch.file(
"/yolo_repo/yolo_pkg/BUILD",
"proto_library(",
" name = 'yolo_proto',",
" srcs = ['yolo.proto'],",
" import_prefix = 'bazel.build/yolo',",
" strip_import_prefix = '/yolo_pkg'",
")",
"cc_proto_library(",
" name = 'yolo_cc_proto',",
" deps = [':yolo_proto'],",
")");
getTarget("@yolo_repo//yolo_pkg:yolo_cc_proto");

assertThat(getTarget("@yolo_repo//yolo_pkg:yolo_cc_proto")).isNotNull();
assertThat(getProtoHeaderExecPath())
.endsWith("_virtual_includes/yolo_proto/bazel.build/yolo/yolo.pb.h");
}

private String getProtoHeaderExecPath() throws LabelSyntaxException {
ConfiguredTarget configuredTarget = getConfiguredTarget("@yolo_repo//yolo_pkg:yolo_cc_proto");
CcInfo ccInfo = configuredTarget.get(CcInfo.PROVIDER);
ImmutableList<Artifact> headers =
ccInfo.getCcCompilationContext().getDeclaredIncludeSrcs().toList();
return Iterables.getOnlyElement(headers).getExecPathString();
}
}
Loading

0 comments on commit d790ba5

Please sign in to comment.