Skip to content

Commit

Permalink
Roll back bazelbuild@4929ad7
Browse files Browse the repository at this point in the history 
And use params files for turbine actions with transitive classpaths

For actions where direct deps cannot be used, turbine spawns should always use
params files. The transitive classpath may exceed the command line length
limit.

PiperOrigin-RevId: 159473987
  • Loading branch information
@cushon @kchodorow
cushon authored and kchodorow committed Jun 20, 2017
1 parent bfd1d33 commit 0aa176a
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 80 deletions.
7 changes: 0 additions & 7 deletions src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@ public boolean alwaysGenerateOutputMapping() {
private final Label javaLauncherLabel;
private final boolean useIjars;
private final boolean useHeaderCompilation;
private final boolean headerCompilationDirectClasspath;
private final boolean headerCompilationDisableJavacFallback;
private final boolean generateJavaDeps;
private final boolean strictDepsJavaProtos;
Expand Down Expand Up @@ -179,7 +178,6 @@ public boolean alwaysGenerateOutputMapping() {
this.javaLauncherLabel = javaOptions.javaLauncher;
this.useIjars = javaOptions.useIjars;
this.useHeaderCompilation = javaOptions.headerCompilation;
this.headerCompilationDirectClasspath = javaOptions.headerCompilationDirectClasspath;
this.headerCompilationDisableJavacFallback = javaOptions.headerCompilationDisableJavacFallback;
this.generateJavaDeps = generateJavaDeps;
this.javaClasspath = javaOptions.javaClasspath;
Expand Down Expand Up @@ -261,11 +259,6 @@ public boolean useHeaderCompilation() {
return useHeaderCompilation;
}

/** Returns true if header compilations should use direct dependencies only. */
public boolean headerCompilationDirectClasspath() {
return headerCompilationDirectClasspath;
}

/**
* If --java_header_compilation is set, report diagnostics from turbine instead of falling back to
* javac. Diagnostics will be produced more quickly, but may be less helpful.
Expand Down
145 changes: 82 additions & 63 deletions src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,24 @@

package com.google.devtools.build.lib.rules.java;

import static com.google.common.base.Preconditions.checkState;
import static com.google.devtools.build.lib.util.Preconditions.checkNotNull;
import static java.nio.charset.StandardCharsets.ISO_8859_1;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.BaseSpawn;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ParameterFile;
import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnActionContext;
Expand Down Expand Up @@ -319,7 +322,7 @@ public void build(JavaToolchainProvider javaToolchain) {
ruleContext.registerAction(buildInternal(javaToolchain));
}

private ActionAnalysisMetadata[] buildInternal(JavaToolchainProvider javaToolchain) {
private Action[] buildInternal(JavaToolchainProvider javaToolchain) {
checkNotNull(outputDepsProto, "outputDepsProto must not be null");
checkNotNull(sourceFiles, "sourceFiles must not be null");
checkNotNull(sourceJars, "sourceJars must not be null");
Expand All @@ -340,47 +343,58 @@ private ActionAnalysisMetadata[] buildInternal(JavaToolchainProvider javaToolcha
directJars = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER);
compileTimeDependencyArtifacts = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
}
boolean useDirectClasspath = useDirectClasspath();
boolean disableJavacFallback =
ruleContext.getFragment(JavaConfiguration.class).headerCompilationDisableJavacFallback();
CommandLine directCommandLine = null;
if (useDirectClasspath) {
CustomCommandLine.Builder builder =
baseCommandLine(getBaseArgs(javaToolchain)).addExecPaths("--classpath", directJars);
if (disableJavacFallback) {
builder.add("--nojavac_fallback");
}
directCommandLine = builder.build();
}

// The compilation uses API-generating annotation processors and has to fall back to
// javac-turbine.
boolean requiresAnnotationProcessing = !processorNames.isEmpty();

Iterable<Artifact> tools = ImmutableList.of(javacJar, javaToolchain.getHeaderCompiler());
ImmutableList<Artifact> outputs = ImmutableList.of(outputJar, outputDepsProto);
NestedSet<Artifact> directInputs =
NestedSet<Artifact> baseInputs =
NestedSetBuilder.<Artifact>stableOrder()
.addTransitive(javabaseInputs)
.addAll(bootclasspathEntries)
.addAll(sourceJars)
.addAll(sourceFiles)
.addTransitive(directJars)
.addAll(tools)
.build();

if (useDirectClasspath && disableJavacFallback) {
// use a regular SpawnAction to invoke turbine with direct deps only,
// and no fallback to javac-turbine
return new ActionAnalysisMetadata[] {
new SpawnAction(
ruleContext.getActionOwner(),
tools,
directInputs,
outputs,
LOCAL_RESOURCES,
directCommandLine,
false,
JavaCompileAction.UTF8_ENVIRONMENT,
/*executionInfo=*/ ImmutableSet.<String>of(),
getProgressMessage(),
"Turbine")
};
boolean noFallback =
ruleContext.getFragment(JavaConfiguration.class).headerCompilationDisableJavacFallback();
// The action doesn't require annotation processing and either javac-turbine fallback is
// disabled, or the action doesn't distinguish between direct and transitive deps, so
// use a plain SpawnAction to invoke turbine.
if ((noFallback || directJars.isEmpty()) && !requiresAnnotationProcessing) {
SpawnAction.Builder builder = new SpawnAction.Builder();
NestedSet<Artifact> classpath;
if (!directJars.isEmpty() || classpathEntries.isEmpty()) {
classpath = directJars;
} else {
classpath = classpathEntries;
// Transitive classpath actions may exceed the command line length limit.
builder.alwaysUseParameterFile(ParameterFileType.SHELL_QUOTED);
}
CustomCommandLine.Builder commandLine =
baseCommandLine(CustomCommandLine.builder(), classpath);
if (noFallback) {
commandLine.add("--nojavac_fallback");
}
return builder
.addTools(tools)
.addTransitiveInputs(baseInputs)
.addTransitiveInputs(classpath)
.addOutputs(outputs)
.setCommandLine(commandLine.build())
.setJarExecutable(
ruleContext.getHostConfiguration().getFragment(Jvm.class).getJavaExecutable(),
javaToolchain.getHeaderCompiler(),
ImmutableList.<String>builder()
.add("-Xbootclasspath/p:" + javacJar.getExecPath())
.addAll(javaToolchain.getJvmOptions())
.build())
.setMnemonic("Turbine")
.setProgressMessage(getProgressMessage())
.build(ruleContext);
}

CommandLine transitiveParams = transitiveCommandLine();
Expand All @@ -400,16 +414,17 @@ private ActionAnalysisMetadata[] buildInternal(JavaToolchainProvider javaToolcha
getBaseArgs(javaToolchain).addPaths("@%s", paramsFile.getExecPath()).build();
NestedSet<Artifact> transitiveInputs =
NestedSetBuilder.<Artifact>stableOrder()
.addTransitive(directInputs)
.addTransitive(baseInputs)
.addTransitive(classpathEntries)
.addTransitive(processorPath)
.addTransitive(compileTimeDependencyArtifacts)
.add(paramsFile)
.build();
if (!useDirectClasspath) {
// If direct classpaths are disabled (e.g. because the compilation uses API-generating
// annotation processors) skip the custom action implementation and just use SpawnAction.
return new ActionAnalysisMetadata[] {

if (requiresAnnotationProcessing) {
// turbine doesn't support API-generating annotation processors, so skip the two-tiered
// turbine/javac-turbine action and just use SpawnAction to invoke javac-turbine.
return new Action[] {
parameterFileWriteAction,
new SpawnAction(
ruleContext.getActionOwner(),
Expand All @@ -421,11 +436,23 @@ private ActionAnalysisMetadata[] buildInternal(JavaToolchainProvider javaToolcha
false,
JavaCompileAction.UTF8_ENVIRONMENT,
/*executionInfo=*/ ImmutableSet.<String>of(),
getProgressMessage(),
getProgressMessageWithAnnotationProcessors(),
"JavacTurbine")
};
}
return new ActionAnalysisMetadata[] {

// The action doesn't require annotation processing, javac-turbine fallback is enabled, and
// the target distinguishes between direct and transitive deps. Try a two-tiered spawn
// the invokes turbine with direct deps, and falls back to javac-turbine on failures to
// produce better diagnostics. (At the cost of slower failed actions and a larger
// cache footprint.)
// TODO(cushon): productionize --nojavac_fallback and remove this path
checkState(!directJars.isEmpty());
NestedSet<Artifact> directInputs =
NestedSetBuilder.fromNestedSet(baseInputs).addTransitive(directJars).build();
CustomCommandLine directCommandLine =
baseCommandLine(getBaseArgs(javaToolchain), directJars).build();
return new Action[] {
parameterFileWriteAction,
new JavaHeaderCompileAction(
ruleContext.getActionOwner(),
Expand All @@ -439,6 +466,17 @@ private ActionAnalysisMetadata[] buildInternal(JavaToolchainProvider javaToolcha
};
}

private String getProgressMessageWithAnnotationProcessors() {
List<String> shortNames = new ArrayList<>();
for (String name : processorNames) {
shortNames.add(name.substring(name.lastIndexOf('.') + 1));
}
return getProgressMessage()
+ " and running annotation processors ("
+ Joiner.on(", ").join(shortNames)
+ ")";
}

private String getProgressMessage() {
return String.format(
"Compiling Java headers %s (%d files)",
Expand All @@ -458,7 +496,8 @@ private CustomCommandLine.Builder getBaseArgs(JavaToolchainProvider javaToolchai
* Adds the command line arguments shared by direct classpath and transitive classpath
* invocations.
*/
private CustomCommandLine.Builder baseCommandLine(CustomCommandLine.Builder result) {
private CustomCommandLine.Builder baseCommandLine(
CustomCommandLine.Builder result, NestedSet<Artifact> classpathEntries) {
result.addExecPath("--output", outputJar);

if (outputDepsProto != null) {
Expand Down Expand Up @@ -492,13 +531,14 @@ private CustomCommandLine.Builder baseCommandLine(CustomCommandLine.Builder resu
result.add("@" + targetLabel);
}
}
result.addExecPaths("--classpath", classpathEntries);
return result;
}

/** Builds a transitive classpath command line. */
private CommandLine transitiveCommandLine() {
CustomCommandLine.Builder result = CustomCommandLine.builder();
baseCommandLine(result);
baseCommandLine(result, classpathEntries);
if (!processorNames.isEmpty()) {
result.add("--processors", processorNames);
}
Expand All @@ -514,28 +554,7 @@ private CommandLine transitiveCommandLine() {
result.addExecPaths("--deps_artifacts", compileTimeDependencyArtifacts);
}
}
result.addExecPaths("--classpath", classpathEntries);
return result.build();
}

/** Returns true if the header compilation classpath should only include direct deps. */
boolean useDirectClasspath() {
if (directJars.isEmpty() && !classpathEntries.isEmpty()) {
// the compilation doesn't distinguish direct deps, e.g. because it doesn't support strict
// java deps
return false;
}
if (!processorNames.isEmpty()) {
// the compilation uses API-generating annotation processors and has to fall back to
// javac-turbine, which doesn't support direct classpaths
return false;
}
JavaConfiguration javaConfiguration = ruleContext.getFragment(JavaConfiguration.class);
if (!javaConfiguration.headerCompilationDirectClasspath()) {
// the experiment is disabled
return false;
}
return true;
}
}
}
10 changes: 0 additions & 10 deletions src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -416,15 +416,6 @@ public OneVersionEnforcementLevelConverter() {
)
public boolean strictDepsJavaProtos;

@Option(
name = "java_header_compilation_direct_classpath",
defaultValue = "true",
optionUsageRestrictions = OptionUsageRestrictions.UNDOCUMENTED,
help = "Experimental option to limit the header compilation classpath to direct deps.",
oldName = "experimental_java_header_compilation_direct_classpath"
)
public boolean headerCompilationDirectClasspath;

@Option(
name = "experimental_java_header_compilation_disable_javac_fallback",
defaultValue = "false",
Expand Down Expand Up @@ -463,7 +454,6 @@ public FragmentOptions getHost(boolean fallback) {
// incremental build performance is important.
host.useIjars = useIjars;
host.headerCompilation = headerCompilation;
host.headerCompilationDirectClasspath = headerCompilationDirectClasspath;
host.headerCompilationDisableJavacFallback = headerCompilationDisableJavacFallback;

host.javaDeps = javaDeps;
Expand Down

0 comments on commit 0aa176a

Please sign in to comment.