Skip to content

Commit

Permalink
Clean up turbine action creation
Browse files Browse the repository at this point in the history 
Support disabling javac fallback for actions without a direct
classpath, and only use the 'JavacTurbine' mnemonic for spawns
that require javac-turbine due to annotation processing to make
it easier to collect metrics on that.

Finally, remove --java_header_compilation_direct_classpath now
that it has been productionized and enabled by default.

PiperOrigin-RevId: 158359858
  • Loading branch information
@cushon @katre
cushon authored and katre committed Jun 8, 2017
1 parent 5596d3b commit 923d7df
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 64 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
102 changes: 55 additions & 47 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,10 +14,12 @@

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;
Expand Down Expand Up @@ -342,41 +344,42 @@ 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
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) {
NestedSet<Artifact> classpath = !directJars.isEmpty() ? directJars : classpathEntries;
CustomCommandLine.Builder commandLine =
baseCommandLine(getBaseArgs(javaToolchain), classpath);
if (noFallback) {
commandLine.add("--nojavac_fallback");
}
return new ActionAnalysisMetadata[] {
new SpawnAction(
ruleContext.getActionOwner(),
tools,
directInputs,
NestedSetBuilder.fromNestedSet(baseInputs).addTransitive(classpath).build(),
outputs,
LOCAL_RESOURCES,
directCommandLine,
commandLine.build(),
false,
JavaCompileAction.UTF8_ENVIRONMENT,
/*executionInfo=*/ ImmutableSet.<String>of(),
Expand All @@ -402,15 +405,16 @@ 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.

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 ActionAnalysisMetadata[] {
parameterFileWriteAction,
new SpawnAction(
Expand All @@ -423,10 +427,22 @@ private ActionAnalysisMetadata[] buildInternal(JavaToolchainProvider javaToolcha
false,
JavaCompileAction.UTF8_ENVIRONMENT,
/*executionInfo=*/ ImmutableSet.<String>of(),
getProgressMessage(),
getProgressMessageWithAnnotationProcessors(),
"JavacTurbine")
};
}

// 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 ActionAnalysisMetadata[] {
parameterFileWriteAction,
new JavaHeaderCompileAction(
Expand All @@ -441,6 +457,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 @@ -460,7 +487,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 @@ -494,13 +522,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 @@ -516,28 +545,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 923d7df

Please sign in to comment.