Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove import-path guessing from ProtoCompileActionBuilder #10939

Closed
wants to merge 19 commits into from
Closed

Remove import-path guessing from ProtoCompileActionBuilder #10939

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7ab5bd3
Remove import-path guessing from ProtoCompileActionBuilder
Yannic Mar 18, 2020
5b175d8
Revert "Remove import-path guessing from ProtoCompileActionBuilder"
Yannic Apr 3, 2020
3815f44
Merge remote-tracking branch 'origin/master' into proto_deterministic…
Yannic Apr 3, 2020
6b645cf
Force creating generated protos in virtual source root
Yannic Apr 3, 2020
c5ab34b
Merge branch 'master' into proto_deterministic_import_path
Yannic Apr 8, 2020
fa18400
Revert "Force creating generated protos in virtual source root"
Yannic Apr 8, 2020
b457385
ProtoSource-based implementation
Yannic Apr 11, 2020
55d949d
Merge remote-tracking branch 'origin/master' into proto_deterministic…
Yannic Apr 11, 2020
2c15f77
Format
Yannic Apr 11, 2020
b102c2e
Minor fixes
Yannic Apr 11, 2020
2545e97
Fix exetrnal deps
Yannic Apr 12, 2020
13f0d78
Reduce RAM usage -> intern proto_source_root
Yannic Apr 13, 2020
8cf24c3
Reduce RAM usage -> Always derive import path from artifact and (real…
Yannic Apr 13, 2020
c47521c
Merge branch 'master' into proto_deterministic_import_path
Yannic May 27, 2020
df72c3c
Merge branch 'master' into proto_deterministic_import_path
Yannic Oct 25, 2020
80825ef
Fix build
Yannic Oct 25, 2020
7c50be0
Reorder fields and resolve conflicts
Yannic Nov 7, 2020
b70d41c
Extract mapping func into static field
Yannic Nov 7, 2020
4a2415c
Merge SharedProtoLibraryTest into BazelProtoLibraryTest
Yannic Nov 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
353 changes: 150 additions & 203 deletions src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java
View file
Open in desktop

Large diffs are not rendered by default.

161 changes: 22 additions & 139 deletions src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,10 @@
import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
import com.google.devtools.build.lib.analysis.stringtemplate.TemplateExpander;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.util.LazyString;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -263,16 +260,11 @@ CustomCommandLine.Builder createProtoCompilerCommandLine() {

boolean areDepsStrict = areDepsStrict(ruleContext);

boolean siblingRepositoryLayout = ruleContext.getConfiguration().isSiblingRepositoryLayout();

// Add include maps
addIncludeMapArguments(
getOutputDirectory(ruleContext),
result,
areDepsStrict ? protoInfo.getStrictImportableProtoSourcesImportPaths() : null,
protoInfo.getStrictImportableProtoSourceRoots(),
protoInfo.getTransitiveProtoSources(),
siblingRepositoryLayout);
areDepsStrict ? protoInfo.getStrictImportableSources() : null,
protoInfo.getTransitiveSources());

if (areDepsStrict) {
// Note: the %s in the line below is used by proto-compiler. That is, the string we create
Expand All @@ -288,21 +280,16 @@ CustomCommandLine.Builder createProtoCompilerCommandLine() {
result.add("--disallow_services");
}
if (checkStrictImportPublic) {
NestedSet<Pair<Artifact, String>> protosInExports =
protoInfo.getExportedProtoSourcesImportPaths();
if (protosInExports.isEmpty()) {
NestedSet<ProtoSource> publicImportSources = protoInfo.getPublicImportSources();
if (publicImportSources.isEmpty()) {
// This line is necessary to trigger the check.
result.add("--allowed_public_imports=");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test case in our internal test battery fails. I can't export it at the moment, but the BUILD file it uses is as follows:

proto_library(
  name = 'myProto',
  srcs = ['myProto.proto'],
)

then verifies that its proto compile action has --allowed_public_imports=. This fails, and instead, it has --allowed_public_imports test/myProto.proto. I think I know why (see the other comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think you're right about the culprit of the failure which would allow public imports of srcs while they aren't allowed with the current behavior.

I had to update BazelProtoLibraryTest#testExportedStrippedImportPrefixes though because it checked that the direct proto source root was part of the exported source roots (but we don't rely on a set of proto source roots anymore for computing public imports, so it's not a behavioral change). You may have to do the same for internal tests :(.

} else {
result.addAll(
"--allowed_public_imports",
VectorArg.join(":")
.each(protosInExports)
.mapped(
new ExpandToPathFnWithImports(
getOutputDirectory(ruleContext),
protoInfo.getTransitiveProtoSourceRoots(),
siblingRepositoryLayout)));
.each(publicImportSources)
.mapped((s, args) -> s.getImportPath().getPathString()));
Yannic marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -558,31 +545,24 @@ static CustomCommandLine createCommandLineFromToolchains(

// Add include maps
addIncludeMapArguments(
outputDirectory,
cmdLine,
strictDeps == Deps.STRICT ? protoInfo.getStrictImportableProtoSourcesImportPaths() : null,
protoInfo.getStrictImportableProtoSourceRoots(),
protoInfo.getTransitiveProtoSources(),
siblingRepositoryLayout);
strictDeps == Deps.STRICT ? protoInfo.getStrictImportableSources() : null,
protoInfo.getTransitiveSources());

if (strictDeps == Deps.STRICT) {
cmdLine.addFormatted(STRICT_DEPS_FLAG_TEMPLATE, ruleLabel);
}

if (useExports == Exports.USE) {
if (protoInfo.getExportedProtoSourcesImportPaths().isEmpty()) {
if (protoInfo.getPublicImportSources().isEmpty()) {
// This line is necessary to trigger the check.
cmdLine.add("--allowed_public_imports=");
} else {
cmdLine.addAll(
"--allowed_public_imports",
VectorArg.join(":")
.each(protoInfo.getExportedProtoSourcesImportPaths())
.mapped(
new ExpandToPathFnWithImports(
outputDirectory,
protoInfo.getExportedProtoSourceRoots(),
siblingRepositoryLayout)));
.each(protoInfo.getPublicImportSources())
.mapped((s, args) -> args.accept(s.getImportPath().getPathString())));
}
}

Expand All @@ -599,29 +579,20 @@ static CustomCommandLine createCommandLineFromToolchains(

@VisibleForTesting
static void addIncludeMapArguments(
String outputDirectory,
CustomCommandLine.Builder commandLine,
@Nullable NestedSet<Pair<Artifact, String>> protosInDirectDependencies,
NestedSet<String> directProtoSourceRoots,
NestedSet<Artifact> transitiveImports,
boolean siblingRepositoryLayout) {
@Nullable NestedSet<ProtoSource> strictImportableProtoSources,
NestedSet<ProtoSource> transitiveSources) {
// For each import, include both the import as well as the import relativized against its
// protoSourceRoot. This ensures that protos can reference either the full path or the short
// path when including other protos.
commandLine.addAll(
VectorArg.of(transitiveImports)
.mapped(
new ExpandImportArgsFn(
outputDirectory, directProtoSourceRoots, siblingRepositoryLayout)));
if (protosInDirectDependencies != null) {
if (!protosInDirectDependencies.isEmpty()) {
commandLine.addAll(VectorArg.of(transitiveSources).mapped(new ExpandImportArgsFn()));
if (strictImportableProtoSources != null) {
if (!strictImportableProtoSources.isEmpty()) {
commandLine.addAll(
"--direct_dependencies",
VectorArg.join(":")
.each(protosInDirectDependencies)
.mapped(
new ExpandToPathFnWithImports(
outputDirectory, directProtoSourceRoots, siblingRepositoryLayout)));
.each(strictImportableProtoSources)
.mapped((s, args) -> args.accept(s.getImportPath().getPathString())));

} else {
// The proto compiler requires an empty list to turn on strict deps checking
Expand All @@ -630,39 +601,6 @@ static void addIncludeMapArguments(
}
}

private static String guessProtoPathUnderRoot(
String outputDirectory,
PathFragment sourceRootPath,
Artifact proto,
boolean siblingRepositoryLayout) {
// TODO(lberki): Instead of guesswork like this, we should track which proto belongs to
// which source root. Unfortunately, that's a non-trivial migration since
// ProtoInfo is on the Starlark API. Therefore, we hack:
// - If the source root is under the output directory (itself determined in a hacky way and
// relying on the fact that the output roots of all repositories are under the same directory
// under the exec root), we check whether the .proto file is under it. If so, we have a match.
// - Otherwise, we check whether the .proto file is either under that source directory or under
// bin or genfiles by prefix-matching its root-relative path.
if (sourceRootPath.segmentCount() > 0 && sourceRootPath.getSegment(0).equals(outputDirectory)) {
if (proto.getExecPath().startsWith(sourceRootPath)) {
return proto.getExecPath().relativeTo(sourceRootPath).getPathString();
}
} else {
PathFragment prefix =
siblingRepositoryLayout
? LabelConstants.EXPERIMENTAL_EXTERNAL_PATH_PREFIX
: LabelConstants.EXTERNAL_PATH_PREFIX;
if (proto.getRootRelativePath().startsWith(sourceRootPath)) {
return proto.getRootRelativePath().relativeTo(sourceRootPath).getPathString();
} else if (proto.getExecPath().startsWith(prefix)
&& proto.getExecPath().startsWith(sourceRootPath)) {
return proto.getExecPath().relativeTo(sourceRootPath).getPathString();
}
}

return null;
}

@AutoCodec @AutoCodec.VisibleForSerialization
static final CommandLineItem.MapFn<String> EXPAND_TRANSITIVE_PROTO_PATH_FLAGS =
(flag, args) -> {
Expand All @@ -671,73 +609,18 @@ private static String guessProtoPathUnderRoot(
}
};


@AutoCodec
@AutoCodec.VisibleForSerialization
static final class ExpandImportArgsFn implements CapturingMapFn<Artifact> {
private final String outputDirectory;
private final NestedSet<String> directProtoSourceRoots;
private final boolean siblingRepositoryLayout;

public ExpandImportArgsFn(
String outputDirectory,
NestedSet<String> directProtoSourceRoots,
boolean siblingRepositoryLayout) {
this.outputDirectory = outputDirectory;
this.directProtoSourceRoots = directProtoSourceRoots;
this.siblingRepositoryLayout = siblingRepositoryLayout;
}

static final class ExpandImportArgsFn implements CapturingMapFn<ProtoSource> {
/**
* Generates up to two import flags for each artifact: one for full path (only relative to the
* repository root) and one for the path relative to the proto source root (if one exists
* corresponding to the artifact).
*/
@Override
public void expandToCommandLine(Artifact proto, Consumer<String> args) {
for (String directProtoSourceRoot : directProtoSourceRoots.toList()) {
PathFragment sourceRootPath = PathFragment.create(directProtoSourceRoot);
String arg =
guessProtoPathUnderRoot(
outputDirectory, sourceRootPath, proto, siblingRepositoryLayout);
if (arg != null) {
args.accept("-I" + arg + "=" + proto.getExecPathString());
Yannic marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}

@AutoCodec
@AutoCodec.VisibleForSerialization
static final class ExpandToPathFnWithImports implements CapturingMapFn<Pair<Artifact, String>> {
private final String outputDirectory;
private final NestedSet<String> directProtoSourceRoots;
private final boolean siblingRepositoryLayout;

public ExpandToPathFnWithImports(
String outputDirectory,
NestedSet<String> directProtoSourceRoots,
boolean siblingRepositoryLayout) {
this.outputDirectory = outputDirectory;
this.directProtoSourceRoots = directProtoSourceRoots;
this.siblingRepositoryLayout = siblingRepositoryLayout;
}

@Override
public void expandToCommandLine(Pair<Artifact, String> proto, Consumer<String> args) {
if (proto.second != null) {
args.accept(proto.second);
} else {
for (String directProtoSourceRoot : directProtoSourceRoots.toList()) {
PathFragment sourceRootPath = PathFragment.create(directProtoSourceRoot);
String arg =
guessProtoPathUnderRoot(
outputDirectory, sourceRootPath, proto.first, siblingRepositoryLayout);
if (arg != null) {
args.accept(arg);
}
}
}
public void expandToCommandLine(ProtoSource proto, Consumer<String> args) {
String importPath = proto.getImportPath().getPathString();
args.accept("-I" + importPath + "=" + proto.getSourceFile().getExecPathString());
}
}

Expand Down
Loading