Disallow duplicate srcjars in javac-turbine

Also improve srcjar extraction to use zipfs instead of a temp dir,
remove the now unused temp dir handling, and use JSR 199 in a slightly
more idiomatic way.

This fixes a bug affecting srcjar entries that differ only in case
when compiling on platforms with case-insensitive filesystems.

--
PiperOrigin-RevId: 149175693
MOS_MIGRATED_REVID=149175693

6 files changed, 98 insertions, 120 deletions

diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java
index e57ecb853c..382e3f4f90 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbine.java
@@ -37,22 +37,19 @@ import java.io.IOException;
 import java.io.OutputStream;
 import java.io.OutputStreamWriter;
 import java.io.PrintWriter;
+import java.nio.file.FileSystem;
+import java.nio.file.FileSystems;
 import java.nio.file.FileVisitResult;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.SimpleFileVisitor;
-import java.nio.file.StandardCopyOption;
 import java.nio.file.attribute.BasicFileAttributes;
-import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
-import java.util.Enumeration;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.regex.Pattern;
-import java.util.zip.ZipEntry;
-import java.util.zip.ZipFile;
 import java.util.zip.ZipOutputStream;
 import javax.tools.StandardLocation;
 import org.objectweb.asm.ClassReader;
@@ -119,15 +116,15 @@ public class JavacTurbine implements AutoCloseable {
   private final TurbineOptions turbineOptions;
   @VisibleForTesting Context context;
 
+  /** Cache of opened zip filesystems for srcjars. */
+  private final Map<Path, FileSystem> filesystems = new HashMap<>();
+
   public JavacTurbine(PrintWriter out, TurbineOptions turbineOptions) {
     this.out = out;
     this.turbineOptions = turbineOptions;
   }
 
   Result compile() throws IOException {
-    Path tmpdir = Paths.get(turbineOptions.tempDir());
-    Files.createDirectories(tmpdir);
-
     ImmutableList.Builder<String> argbuilder = ImmutableList.builder();
 
     argbuilder.addAll(JavacOptions.removeBazelSpecificFlags(turbineOptions.javacOpts()));
@@ -160,14 +157,15 @@ public class JavacTurbine implements AutoCloseable {
       processorpath = ImmutableList.of();
     }
 
-    List<String> sources = new ArrayList<>();
-    sources.addAll(turbineOptions.sources());
-    sources.addAll(extractSourceJars(turbineOptions, tmpdir));
-
-    argbuilder.addAll(sources);
+    ImmutableList<Path> sources =
+        ImmutableList.<Path>builder()
+            .addAll(asPaths(turbineOptions.sources()))
+            .addAll(getSourceJarEntries(turbineOptions))
+            .build();
 
     JavacTurbineCompileRequest.Builder requestBuilder =
         JavacTurbineCompileRequest.builder()
+            .setSources(sources)
             .setJavacOptions(argbuilder.build())
             .setBootClassPath(asPaths(turbineOptions.bootClassPath()))
             .setProcessorClassPath(processorpath);
@@ -208,9 +206,6 @@ public class JavacTurbine implements AutoCloseable {
 
     if (!compileResult.success() && hasRecognizedError(compileResult.output())) {
       // fall back to transitive classpath
-      deleteRecursively(tmpdir);
-      extractSourceJars(turbineOptions, tmpdir);
-
       actualClasspath = originalClasspath;
       requestBuilder.setClassPath(asPaths(actualClasspath));
       compileResult = JavacTurbineCompiler.compile(requestBuilder.build());
@@ -362,31 +357,36 @@ public class JavacTurbine implements AutoCloseable {
     return result.build();
   }
 
-  /** Extra sources in srcjars to disk. */
-  private static List<String> extractSourceJars(TurbineOptions turbineOptions, Path tmpdir)
+  /** Returns paths to the source jar entries to compile. */
+  private ImmutableList<Path> getSourceJarEntries(TurbineOptions turbineOptions)
       throws IOException {
-    if (turbineOptions.sourceJars().isEmpty()) {
-      return Collections.emptyList();
-    }
-
-    ArrayList<String> extractedSources = new ArrayList<>();
+    ImmutableList.Builder<Path> sources = ImmutableList.builder();
     for (String sourceJar : turbineOptions.sourceJars()) {
-      try (ZipFile zf = new ZipFile(sourceJar)) {
-        Enumeration<? extends ZipEntry> entries = zf.entries();
-        while (entries.hasMoreElements()) {
-          ZipEntry ze = entries.nextElement();
-          if (!ze.getName().endsWith(".java")) {
-            continue;
-          }
-          Path dest = tmpdir.resolve(ze.getName());
-          Files.createDirectories(dest.getParent());
-          // allow overlapping source jars for compatibility with JavaBuilder (see b/26688023)
-          Files.copy(zf.getInputStream(ze), dest, StandardCopyOption.REPLACE_EXISTING);
-          extractedSources.add(dest.toAbsolutePath().toString());
-        }
+      for (Path root : getJarFileSystem(Paths.get(sourceJar)).getRootDirectories()) {
+        Files.walkFileTree(
+            root,
+            new SimpleFileVisitor<Path>() {
+              @Override
+              public FileVisitResult visitFile(Path path, BasicFileAttributes attrs)
+                  throws IOException {
+                String fileName = path.getFileName().toString();
+                if (fileName.endsWith(".java")) {
+                  sources.add(path);
+                }
+                return FileVisitResult.CONTINUE;
+              }
+            });
       }
     }
-    return extractedSources;
+    return sources.build();
+  }
+
+  private FileSystem getJarFileSystem(Path sourceJar) throws IOException {
+    FileSystem fs = filesystems.get(sourceJar);
+    if (fs == null) {
+      filesystems.put(sourceJar, fs = FileSystems.newFileSystem(sourceJar, null));
+    }
+    return fs;
   }
 
   private static final Pattern MISSING_PACKAGE =
@@ -409,27 +409,8 @@ public class JavacTurbine implements AutoCloseable {
   @Override
   public void close() throws IOException {
     out.flush();
-    deleteRecursively(Paths.get(turbineOptions.tempDir()));
-  }
-
-  private static void deleteRecursively(final Path dir) throws IOException {
-    Files.walkFileTree(
-        dir,
-        new SimpleFileVisitor<Path>() {
-          @Override
-          public FileVisitResult visitFile(Path path, BasicFileAttributes attrs)
-              throws IOException {
-            Files.delete(path);
-            return FileVisitResult.CONTINUE;
-          }
-
-          @Override
-          public FileVisitResult postVisitDirectory(Path path, IOException exc) throws IOException {
-            if (!path.equals(dir)) {
-              Files.delete(path);
-            }
-            return FileVisitResult.CONTINUE;
-          }
-        });
+    for (FileSystem fs : filesystems.values()) {
+      fs.close();
+    }
   }
 }
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompileRequest.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompileRequest.java
index 29a9a4dc88..76bc0f0be5 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompileRequest.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompileRequest.java
@@ -24,6 +24,7 @@ import javax.annotation.Nullable;
 /** The input to a {@link JavacTurbineCompiler} compilation. */
 class JavacTurbineCompileRequest {
 
+  private final ImmutableList<Path> sources;
   private final ImmutableList<Path> classPath;
   private final ImmutableList<Path> bootClassPath;
   private final ImmutableList<Path> processorClassPath;
@@ -31,11 +32,13 @@ class JavacTurbineCompileRequest {
   @Nullable private final StrictJavaDepsPlugin strictJavaDepsPlugin;
 
   JavacTurbineCompileRequest(
+      ImmutableList<Path> sources,
       ImmutableList<Path> classPath,
       ImmutableList<Path> bootClassPath,
       ImmutableList<Path> processorClassPath,
       ImmutableList<String> javacOptions,
       @Nullable StrictJavaDepsPlugin strictJavaDepsPlugin) {
+    this.sources = checkNotNull(sources);
     this.classPath = checkNotNull(classPath);
     this.bootClassPath = checkNotNull(bootClassPath);
     this.processorClassPath = checkNotNull(processorClassPath);
@@ -43,6 +46,11 @@ class JavacTurbineCompileRequest {
     this.strictJavaDepsPlugin = strictJavaDepsPlugin;
   }
 
+  /** The sources to compile. */
+  ImmutableList<Path> sources() {
+    return sources;
+  }
+
   /** The class path; correspond's to javac -classpath. */
   ImmutableList<Path> classPath() {
     return classPath;
@@ -76,6 +84,7 @@ class JavacTurbineCompileRequest {
   }
 
   static class Builder {
+    private ImmutableList<Path> sources;
     private ImmutableList<Path> classPath;
     private ImmutableList<Path> bootClassPath;
     private ImmutableList<Path> processorClassPath;
@@ -86,7 +95,12 @@ class JavacTurbineCompileRequest {
 
     JavacTurbineCompileRequest build() {
       return new JavacTurbineCompileRequest(
-          classPath, bootClassPath, processorClassPath, javacOptions, strictDepsPlugin);
+          sources, classPath, bootClassPath, processorClassPath, javacOptions, strictDepsPlugin);
+    }
+
+    Builder setSources(ImmutableList<Path> sources) {
+      this.sources = sources;
+      return this;
     }
 
     Builder setClassPath(ImmutableList<Path> classPath) {
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompiler.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompiler.java
index c2fab7e719..16dfcb610b 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompiler.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineCompiler.java
@@ -14,16 +14,15 @@
 
 package com.google.devtools.build.java.turbine.javac;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.buildjar.javac.plugins.dependency.StrictJavaDepsPlugin;
 import com.google.devtools.build.java.turbine.javac.JavacTurbineCompileResult.Status;
 import com.google.devtools.build.java.turbine.javac.ZipOutputFileManager.OutputFileObject;
+import com.sun.source.util.JavacTask;
+import com.sun.tools.javac.api.JavacTool;
 import com.sun.tools.javac.file.CacheFSInfo;
-import com.sun.tools.javac.main.Arguments;
-import com.sun.tools.javac.main.CommandLine;
-import com.sun.tools.javac.main.JavaCompiler;
 import com.sun.tools.javac.util.Context;
-import com.sun.tools.javac.util.Log;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.io.StringWriter;
@@ -32,7 +31,6 @@ import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import javax.annotation.Nullable;
-import javax.tools.JavaFileManager;
 import javax.tools.StandardLocation;
 
 /** Performs a javac-based turbine compilation given a {@link JavacTurbineCompileRequest}. */
@@ -46,46 +44,32 @@ public class JavacTurbineCompiler {
     Context context = new Context();
 
     try (PrintWriter pw = new PrintWriter(sw)) {
-      ZipOutputFileManager.preRegister(context, files);
       setupContext(context, request.strictJavaDepsPlugin());
       CacheFSInfo.preRegister(context);
-
-      context.put(Log.outKey, pw);
-
-      ZipOutputFileManager fm = (ZipOutputFileManager) context.get(JavaFileManager.class);
-      fm.setLocationFromPaths(StandardLocation.SOURCE_PATH, Collections.<Path>emptyList());
-      fm.setLocationFromPaths(StandardLocation.CLASS_PATH, request.classPath());
-      fm.setLocationFromPaths(StandardLocation.PLATFORM_CLASS_PATH, request.bootClassPath());
-      fm.setLocationFromPaths(
-          StandardLocation.ANNOTATION_PROCESSOR_PATH, request.processorClassPath());
-
-      String[] javacArgArray = request.javacOptions().toArray(new String[0]);
-      javacArgArray = CommandLine.parse(javacArgArray);
-
-      Arguments args = Arguments.instance(context);
-      args.init("turbine", javacArgArray);
-
-      fm.setContext(context);
-      fm.handleOptions(args.getDeferredFileManagerOptions());
-
-      if (!args.validate()) {
-        return new JavacTurbineCompileResult(
-            ImmutableMap.<String, OutputFileObject>of(), Status.ERROR, sw, context);
-      }
-
-      JavaCompiler comp = JavaCompiler.instance(context);
-      if (request.strictJavaDepsPlugin() != null) {
-        request.strictJavaDepsPlugin().init(context, Log.instance(context), comp);
-      }
-
-      try {
-        comp.compile(args.getFileObjects(), args.getClassNames(), null);
-        status = comp.errorCount() == 0 ? Status.OK : Status.ERROR;
+      try (ZipOutputFileManager fm = new ZipOutputFileManager(files)) {
+        JavacTask task =
+            JavacTool.create()
+                .getTask(
+                    pw,
+                    fm,
+                    null /*diagnostics*/,
+                    request.javacOptions(),
+                    ImmutableList.of() /*classes*/,
+                    fm.getJavaFileObjectsFromPaths(request.sources()),
+                    context);
+
+        fm.setContext(context);
+        fm.setLocationFromPaths(StandardLocation.SOURCE_PATH, Collections.<Path>emptyList());
+        fm.setLocationFromPaths(StandardLocation.CLASS_PATH, request.classPath());
+        fm.setLocationFromPaths(StandardLocation.PLATFORM_CLASS_PATH, request.bootClassPath());
+        fm.setLocationFromPaths(
+            StandardLocation.ANNOTATION_PROCESSOR_PATH, request.processorClassPath());
+
+        status = task.call() ? Status.OK : Status.ERROR;
       } catch (Throwable t) {
         t.printStackTrace(pw);
         status = Status.ERROR;
       }
-      comp.close();
     }
 
     return new JavacTurbineCompileResult(ImmutableMap.copyOf(files), status, sw, context);
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineJavaCompiler.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineJavaCompiler.java
index f44cca00bc..b17cde85bd 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineJavaCompiler.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/JavacTurbineJavaCompiler.java
@@ -21,6 +21,7 @@ import com.sun.tools.javac.comp.Env;
 import com.sun.tools.javac.main.JavaCompiler;
 import com.sun.tools.javac.tree.JCTree.JCCompilationUnit;
 import com.sun.tools.javac.util.Context;
+import com.sun.tools.javac.util.Log;
 import java.util.Queue;
 import javax.annotation.Nullable;
 import javax.tools.JavaFileObject;
@@ -39,6 +40,10 @@ class JavacTurbineJavaCompiler extends JavaCompiler implements AutoCloseable {
   public JavacTurbineJavaCompiler(Context context, @Nullable StrictJavaDepsPlugin strictJavaDeps) {
     super(context);
     this.strictJavaDeps = strictJavaDeps;
+    JavaCompiler comp = JavaCompiler.instance(context);
+    if (strictJavaDeps != null) {
+      strictJavaDeps.init(context, Log.instance(context), this);
+    }
   }
 
   @Override
diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/ZipOutputFileManager.java b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/ZipOutputFileManager.java
index 813eb54b2a..7bbe35b086 100644
--- a/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/ZipOutputFileManager.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/javac/ZipOutputFileManager.java
@@ -17,7 +17,6 @@ package com.google.devtools.build.java.turbine.javac;
 import com.sun.tools.javac.api.ClientCodeWrapper.Trusted;
 import com.sun.tools.javac.file.JavacFileManager;
 import com.sun.tools.javac.util.Context;
-
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.FileNotFoundException;
@@ -32,10 +31,8 @@ import java.nio.charset.CharsetDecoder;
 import java.nio.charset.CodingErrorAction;
 import java.nio.charset.StandardCharsets;
 import java.util.Map;
-
 import javax.annotation.Nullable;
 import javax.tools.FileObject;
-import javax.tools.JavaFileManager;
 import javax.tools.JavaFileObject;
 import javax.tools.SimpleJavaFileObject;
 import javax.tools.StandardLocation;
@@ -46,11 +43,15 @@ public class ZipOutputFileManager extends JavacFileManager {
 
   private final Map<String, OutputFileObject> files;
 
-  public ZipOutputFileManager(Context context, Map<String, OutputFileObject> files) {
-    super(context, true, StandardCharsets.UTF_8);
+  public ZipOutputFileManager(Map<String, OutputFileObject> files) {
+    super(new Context(), false, StandardCharsets.UTF_8);
     this.files = files;
   }
 
+  public void setContext(Context context) {
+    super.setContext(context);
+  }
+
   /**
    * Returns true if the file manager owns this location; otherwise it delegates to the underlying
    * implementation.
@@ -162,17 +163,6 @@ public class ZipOutputFileManager extends JavacFileManager {
     }
   }
 
-  public static void preRegister(Context context, final Map<String, OutputFileObject> files) {
-    context.put(
-        JavaFileManager.class,
-        new Context.Factory<JavaFileManager>() {
-          @Override
-          public JavaFileManager make(Context c) {
-            return new ZipOutputFileManager(c, files);
-          }
-        });
-  }
-
   @Override
   protected ClassLoader getClassLoader(URL[] urls) {
     // mask turbine classes from the processor classpath to avoid version skew
diff --git a/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java b/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java
index 85f6ce760f..b4196a0840 100644
--- a/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java
+++ b/src/java_tools/buildjar/javatests/com/google/devtools/build/java/turbine/javac/JavacTurbineTest.java
@@ -995,10 +995,14 @@ public class JavacTurbineTest {
 
     optionsBuilder.setSourceJars(ImmutableList.of(sourceJar2.toString(), sourceJar1.toString()));
 
-    compile();
-
-    Map<String, byte[]> outputs = collectOutputs();
-    assertThat(outputs.keySet()).containsExactly("Hello.class");
+    StringWriter errOutput = new StringWriter();
+    Result result;
+    try (JavacTurbine turbine =
+        new JavacTurbine(new PrintWriter(errOutput, true), optionsBuilder.build())) {
+      result = turbine.compile();
+    }
+    assertThat(result).isEqualTo(Result.ERROR);
+    assertThat(errOutput.toString()).contains("duplicate class: Hello");
   }
 
   @Test