Skip to content

Commit

Permalink
Fix fast run/debug manual compilation failing with mismatched java ve…
Browse files Browse the repository at this point in the history
…rsions.

There were two problems:
- the FastBuildJavac interface included classes which changed between java versions, so we couldn't map between the two instances in the classpath
- we weren't specifying the boot classpath during compilation, so if core jdk classes referenced in the files being compiled had changed, recompilation failed

Remove all classes which might change between java versions from the FastBuildJavac interface, so that it's safe to share between the two classloaders present at runtime.

PiperOrigin-RevId: 270030568
  • Loading branch information
brendandouglas authored and copybara-github committed Sep 19, 2019
1 parent 77ec9b7 commit 0c783de
Show file tree
Hide file tree
Showing 8 changed files with 195 additions and 71 deletions.
5 changes: 5 additions & 0 deletions aspect/fast_build_info.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,19 @@ def _fast_build_info_impl(target, ctx):
)
for datadep in ctx.rule.attr.data
]

if hasattr(target, "java_toolchain"):
write_output = True
toolchain = target.java_toolchain
javac_jars = []
if hasattr(toolchain, "tools"):
javac_jars = [artifact_location(f) for f in toolchain.tools.to_list()]
bootclasspath_jars = []
if hasattr(toolchain, "bootclasspath"):
bootclasspath_jars = [artifact_location(f) for f in toolchain.bootclasspath.to_list()]
info["java_toolchain_info"] = struct_omit_none(
javac_jars = javac_jars,
bootclasspath_jars = bootclasspath_jars,
source_version = toolchain.source_version,
target_version = toolchain.target_version,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,23 +218,35 @@ public abstract Builder setAnnotationProcessorClasspath(
abstract static class JavaToolchainInfo {
public abstract ImmutableList<ArtifactLocation> javacJars();

public abstract ImmutableList<ArtifactLocation> bootClasspathJars();

public abstract String sourceVersion();

public abstract String targetVersion();

static JavaToolchainInfo create(
ImmutableList<ArtifactLocation> javacJars, String sourceVersion, String targetVersion) {
ImmutableList<ArtifactLocation> javacJars,
ImmutableList<ArtifactLocation> bootJars,
String sourceVersion,
String targetVersion) {
return new AutoValue_FastBuildBlazeData_JavaToolchainInfo(
javacJars, sourceVersion, targetVersion);
javacJars, bootJars, sourceVersion, targetVersion);
}

static JavaToolchainInfo fromProto(FastBuildInfo.JavaToolchainInfo javaToolchainInfo) {
ImmutableList<ArtifactLocation> javacJars =
javaToolchainInfo.getJavacJarsList().stream()
.map(ArtifactLocation::fromProto)
.collect(toImmutableList());
ImmutableList<ArtifactLocation> bootJars =
javaToolchainInfo.getBootclasspathJarsList().stream()
.map(ArtifactLocation::fromProto)
.collect(toImmutableList());
return create(
javacJars, javaToolchainInfo.getSourceVersion(), javaToolchainInfo.getTargetVersion());
javacJars,
bootJars,
javaToolchainInfo.getSourceVersion(),
javaToolchainInfo.getTargetVersion());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import com.google.idea.blaze.base.sync.data.BlazeProjectDataManager;
import com.google.idea.blaze.java.fastbuild.FastBuildBlazeData.JavaToolchainInfo;
import com.google.idea.blaze.java.fastbuild.FastBuildCompiler.CompileInstructions;
import com.google.idea.blaze.java.fastbuild.FastBuildJavac.CompilerOutput;
import com.google.idea.blaze.java.fastbuild.FastBuildJavac.DiagnosticLine;
import com.google.idea.blaze.java.fastbuild.FastBuildLogDataScope.FastBuildLogOutput;
import com.intellij.ide.plugins.IdeaPluginDescriptor;
import com.intellij.ide.plugins.PluginManager;
Expand All @@ -59,8 +61,6 @@
import java.util.function.Supplier;
import javax.tools.Diagnostic;
import javax.tools.Diagnostic.Kind;
import javax.tools.DiagnosticListener;
import javax.tools.JavaFileObject;

final class FastBuildCompilerFactoryImpl implements FastBuildCompilerFactory {

Expand Down Expand Up @@ -106,8 +106,11 @@ public FastBuildCompiler getCompilerFor(Label label, Map<Label, FastBuildBlazeDa
checkState(projectData != null, "not a blaze project");
List<File> javacJars =
projectData.getArtifactLocationDecoder().decodeAll(javaToolchain.javacJars());
List<File> bootJars =
projectData.getArtifactLocationDecoder().decodeAll(javaToolchain.bootClasspathJars());
Javac javac = createCompiler(javacJars);
return new JavacRunner(javac, javaToolchain.sourceVersion(), javaToolchain.targetVersion());
return new JavacRunner(
javac, bootJars, javaToolchain.sourceVersion(), javaToolchain.targetVersion());
}

private JavaToolchainInfo getJavaToolchain(Label label, Map<Label, FastBuildBlazeData> blazeData)
Expand Down Expand Up @@ -143,7 +146,6 @@ boolean compile(

private Javac createCompiler(List<File> javacJars) throws FastBuildException {
try {

Class<?> javacClass =
loadJavacClass(
FAST_BUILD_JAVAC_CLASS,
Expand All @@ -160,8 +162,10 @@ private Javac createCompiler(List<File> javacJars) throws FastBuildException {
FastBuildJavac.class, new MatchingMethodInvocationHandler(javacClass, javacInstance));
return (context, javacArgs, files, writer) -> {
Stopwatch timer = Stopwatch.createStarted();
boolean result =
javaCompiler.compile(javacArgs, files, new ProblemsViewDiagnosticListener(context));
Object[] rawOutput = javaCompiler.compile(javacArgs, files);
CompilerOutput output = CompilerOutput.decode(rawOutput);
processDiagnostics(context, output);
boolean result = output.result;
Command command =
Command.builder()
.setExecutable(javacJars.get(0).getPath())
Expand Down Expand Up @@ -200,11 +204,14 @@ private Class<?> loadJavacClass(String javaCompilerClass, List<File> jars)
private static class JavacRunner implements FastBuildCompiler {

private final Javac javac;
private final List<File> bootClassPathJars;
private final String sourceVersion;
private final String targetVersion;

private JavacRunner(Javac javac, String sourceVersion, String targetVersion) {
private JavacRunner(
Javac javac, List<File> bootClassPathJars, String sourceVersion, String targetVersion) {
this.javac = javac;
this.bootClassPathJars = bootClassPathJars;
this.sourceVersion = sourceVersion;
this.targetVersion = targetVersion;
}
Expand All @@ -223,6 +230,11 @@ public void compile(BlazeContext context, CompileInstructions instructions)
.add("-cp")
.add(instructions.classpath().stream().map(File::getPath).collect(joining(":")))
.add("-g");
if (!bootClassPathJars.isEmpty()) {
argsBuilder
.add("-bootclasspath")
.add(bootClassPathJars.stream().map(File::getPath).collect(joining(":")));
}
if (instructions.annotationProcessorClassNames().isEmpty()) {
// Without this, it will find all the annotation processors in the classpath and run them.
// We only want to run them if the BUILD file asked for it.
Expand Down Expand Up @@ -340,45 +352,34 @@ public Object invoke(Object o, Method method, Object[] objects) throws Throwable
}
}

private static class ProblemsViewDiagnosticListener
implements DiagnosticListener<JavaFileObject> {

private final BlazeContext context;
private static void processDiagnostics(BlazeContext context, CompilerOutput output) {
output.diagnostics.forEach(d -> processDiagostic(context, d));
}

private ProblemsViewDiagnosticListener(BlazeContext context) {
this.context = context;
private static void processDiagostic(BlazeContext context, DiagnosticLine line) {
IssueOutput.Builder output = IssueOutput.issue(toCategory(line.kind), line.message);
if (line.uri != null) {
output.inFile(new File(line.uri));
}

@Override
public void report(Diagnostic<? extends JavaFileObject> diagnostic) {

IssueOutput.Builder output =
IssueOutput.issue(toCategory(diagnostic.getKind()), diagnostic.getMessage(null));
if (diagnostic.getSource() != null) {
output.inFile(new File(diagnostic.getSource().toUri()));
}
if (diagnostic.getPosition() != Diagnostic.NOPOS) {
output
.onLine((int) diagnostic.getLineNumber())
.inColumn((int) diagnostic.getColumnNumber());
}
context.output(output.build());
context.output(new PrintOutput(diagnostic.toString()));
if (line.lineNumber != Diagnostic.NOPOS) {
output.onLine((int) line.lineNumber).inColumn((int) line.columnNumber);
}
context.output(output.build());
context.output(new PrintOutput(line.formattedMessage));
}

private Category toCategory(Kind kind) {
switch (kind) {
case ERROR:
return Category.ERROR;
case MANDATORY_WARNING:
case WARNING:
return Category.WARNING;
case NOTE:
return Category.NOTE;
case OTHER:
return Category.INFORMATION;
}
throw new AssertionError("Unknown Kind " + kind);
private static Category toCategory(Kind kind) {
switch (kind) {
case ERROR:
return Category.ERROR;
case MANDATORY_WARNING:
case WARNING:
return Category.WARNING;
case NOTE:
return Category.NOTE;
case OTHER:
return Category.INFORMATION;
}
throw new AssertionError("Unknown Kind " + kind);
}
}
104 changes: 98 additions & 6 deletions java/src/com/google/idea/blaze/java/fastbuild/FastBuildJavac.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,115 @@
package com.google.idea.blaze.java.fastbuild;

import java.io.File;
import java.net.URI;
import java.util.Collection;
import java.util.List;
import javax.tools.DiagnosticListener;
import javax.tools.JavaFileObject;
import java.util.stream.Collectors;
import javax.tools.Diagnostic.Kind;

/**
* A small wrapper around javac.
*
* <p>This interface is used by two classloaders (the plugin's and the compiler's) whose only shared
* classes are those from the JRE, so it mustn't import anything besides that.
*
* <p>Additionally, the compiler and plugin can have different versions of java, so we try to limit
* the interface to classes which don't change (javax.tools.DiagnosticListener is out), at the
* expense of code cleanliness and type safety.
*/
// TODO(plumpy): have FastBuildJavacImpl implement JavaCompiler instead, which would get rid of the
// need for this class. It's a lot more complicated to do that, however.
interface FastBuildJavac {

boolean compile(
List<String> args,
Collection<File> sources,
DiagnosticListener<? super JavaFileObject> listener);
/** Returns an encoded version of CompilerOutput. Call {@link CompilerOutput#decode} to decode. */
Object[] compile(List<String> args, Collection<File> sources);

final class CompilerOutput {
final boolean result;
final List<DiagnosticLine> diagnostics;

CompilerOutput(boolean result, List<DiagnosticLine> diagnostics) {
this.result = result;
this.diagnostics = diagnostics;
}

Object[] encode() {
return new Object[] {
result, diagnostics.stream().map(DiagnosticLine::encode).collect(Collectors.toList())
};
}

@SuppressWarnings("unchecked")
public static CompilerOutput decode(Object[] rawOutput) {
boolean result = (boolean) rawOutput[0];
List<DiagnosticLine> diagnostics =
((List<Object[]>) rawOutput[1])
.stream().map(DiagnosticLine::decode).collect(Collectors.toList());
return new CompilerOutput(result, diagnostics);
}
}

final class DiagnosticLine {
final Kind kind;
final String message;
final String formattedMessage;
/** uri is nullable (but we can't add a dependency on jsr305 annotations) */
final URI uri;

final long lineNumber;
final long columnNumber;

DiagnosticLine(
Kind kind,
String message,
String formattedMessage,
URI uri,
long lineNumber,
long columnNumber) {
this.kind = kind;
this.message = message;
this.formattedMessage = formattedMessage;
this.uri = uri;
this.lineNumber = lineNumber;
this.columnNumber = columnNumber;
}

Object[] encode() {
return new Object[] {
kind.toString(),
message,
formattedMessage,
uri == null ? null : uri.toString(),
lineNumber,
columnNumber
};
}

@SuppressWarnings("unchecked")
static DiagnosticLine decode(Object[] encoded) {
return new DiagnosticLine(
decodeKind((String) encoded[0]),
(String) encoded[1],
(String) encoded[2],
decodeUri((String) encoded[3]),
(long) encoded[4],
(long) encoded[5]);
}

private static Kind decodeKind(String kind) {
try {
return Kind.valueOf(kind);
} catch (IllegalArgumentException e) {
return Kind.OTHER;
}
}

private static URI decodeUri(String uri) {
try {
return uri == null ? null : URI.create(uri);
} catch (IllegalArgumentException e) {
return null;
}
}
}
}
Loading

0 comments on commit 0c783de

Please sign in to comment.