Revert back to old behavior when jar_spool_mode=itermediate_to_disk

Summary:
So the good folk at Uber use
jar_spool_mode=intermediate_to_disk, and have their CI runs use
--code-coverage. Since CI runs have --code-coverage, the new mode of
unzipping all the jar files (serially) adds a fair amount of
additional overhead.

Let's solve this.

Test Plan:
Unit tests. + on an external repo, ran `buck test
--code-coverage` for both options of jar_spool_mode. Verified the
coverage output looked fine, and verified that the
parameters.properties file generated looked reasonable.

Reviewed By: illicitonion

fbshipit-source-id: cc0cda9

src/com/facebook/buck/cli/TestRunning.java

@@ -19,6 +19,7 @@
 import com.facebook.buck.event.BuckEventBus;
 import com.facebook.buck.event.ConsoleEvent;
 import com.facebook.buck.io.ProjectFilesystem;
+import com.facebook.buck.jvm.java.DefaultJavaLibrary;
 import com.facebook.buck.jvm.java.DefaultJavaPackageFinder;
 import com.facebook.buck.jvm.java.GenerateCodeCoverageReportStep;
 import com.facebook.buck.jvm.java.JacocoConstants;
@@ -27,6 +28,7 @@
 import com.facebook.buck.jvm.java.JavaLibraryWithTests;
 import com.facebook.buck.jvm.java.JavaRuntimeLauncher;
 import com.facebook.buck.jvm.java.JavaTest;
+import com.facebook.buck.jvm.java.JavacOptions;
 import com.facebook.buck.log.Logger;
 import com.facebook.buck.model.BuildTarget;
 import com.facebook.buck.model.HasBuildTarget;
@@ -429,14 +431,15 @@ public void onFailure(Throwable e) {
     // Generate the code coverage report.
     if (options.isCodeCoverageEnabled() && !rulesUnderTestForCoverage.isEmpty()) {
       try {
+        JavaBuckConfig javaBuckConfig = params.getBuckConfig().getView(JavaBuckConfig.class);
         DefaultJavaPackageFinder defaultJavaPackageFinder =
-            params.getBuckConfig().getView(JavaBuckConfig.class).createDefaultJavaPackageFinder();
+            javaBuckConfig.createDefaultJavaPackageFinder();
         stepRunner.runStepForBuildTarget(
             executionContext,
             getReportCommand(
                 rulesUnderTestForCoverage,
                 defaultJavaPackageFinder,
-                params.getBuckConfig().getView(JavaBuckConfig.class)
+                javaBuckConfig
                     .getDefaultJavaOptions()
                     .getJavaRuntimeLauncher(),
                 params.getCell().getFilesystem(),
@@ -445,6 +448,8 @@ public void onFailure(Throwable e) {
                 JacocoConstants.getJacocoOutputDir(params.getCell().getFilesystem()),
                 options.getCoverageReportFormat(),
                 options.getCoverageReportTitle(),
+                javaBuckConfig.getDefaultJavacOptions().getSpoolMode() ==
+                     JavacOptions.SpoolMode.INTERMEDIATE_TO_DISK,
                 options.getCoverageIncludes(),
                 options.getCoverageExcludes()),
             Optional.empty());
@@ -779,6 +784,7 @@ private static Step getReportCommand(
       Path outputDirectory,
       CoverageReportFormat format,
       String title,
+      boolean useIntermediateClassesDir,
       Optional<String> coverageIncludes,
       Optional<String> coverageExcludes) {
     ImmutableSet.Builder<String> srcDirectories = ImmutableSet.builder();
@@ -791,11 +797,17 @@ private static Step getReportCommand(
       if (!sourceFolderPath.isEmpty()) {
         srcDirectories.addAll(sourceFolderPath);
       }
-      Path jarFile = rule.getPathToOutput();
-      if (jarFile == null) {
+      Path classesItem;
+
+      if (useIntermediateClassesDir) {
+        classesItem = DefaultJavaLibrary.getClassesDir(rule.getBuildTarget(), filesystem);
+      } else {
+        classesItem = rule.getPathToOutput();
+      }
+      if (classesItem == null) {
         continue;
       }
-      pathsToJars.add(jarFile);
+      pathsToJars.add(classesItem);
     }
 
     return new GenerateCodeCoverageReportStep(

src/com/facebook/buck/jvm/java/GenerateCodeCoverageReportStep.java

@@ -88,19 +88,25 @@ public String getShortName() {
   @Override
   public StepExecutionResult execute(ExecutionContext context)
       throws IOException, InterruptedException {
+    Set<Path> tempDirs = new HashSet<>();
     Set<Path> extractedClassesDirectories = new HashSet<>();
     for (Path jarFile : jarFiles) {
-      Path extractClassDir = Files.createTempDirectory("extractedClasses");
-      populateClassesDir(
-          jarFile,
-          extractClassDir);
-      extractedClassesDirectories.add(extractClassDir);
+      if (filesystem.isDirectory(jarFile)) {
+        extractedClassesDirectories.add(jarFile);
+      } else {
+        Path extractClassDir = Files.createTempDirectory("extractedClasses");
+        populateClassesDir(
+            jarFile,
+            extractClassDir);
+        extractedClassesDirectories.add(extractClassDir);
+        tempDirs.add(extractClassDir);
+      }
     }
 
     try {
       return executeInternal(context, extractedClassesDirectories);
     } finally {
-      for (Path tempDir : extractedClassesDirectories) {
+      for (Path tempDir : tempDirs) {
         MoreFiles.deleteRecursively(tempDir);
       }
     }
@@ -176,11 +182,13 @@ protected ImmutableList<String> getShellCommandInternal(ExecutionContext context
    * ReportGenerator.java needs a class-directory to work with, so if
    * we instead have a jar file we extract it first.
    */
-  public static void populateClassesDir(
+  private void populateClassesDir(
       Path outputJar,
       Path classesDir) {
     try {
-      Preconditions.checkState(outputJar.toFile().exists());
+      Preconditions.checkState(
+          filesystem.exists(outputJar),
+          String.valueOf(outputJar) + " does not exist");
       Unzip.extractZipFile(
           outputJar,
           classesDir,

test/com/facebook/buck/jvm/java/GenerateCodeCoverageReportStepTest.java

@@ -156,6 +156,35 @@ StepExecutionResult executeInternal(
     assertFalse(extractedDir[1].exists());
   }
 
+  @Test
+  public void testClassesDirIsUntouched() throws Throwable {
+    final File classesDir = tmp.newFolder("classesDir");
+    jarFiles.clear();
+    jarFiles.add(classesDir.toPath());
+
+    step = new GenerateCodeCoverageReportStep(
+        new ExternalJavaRuntimeLauncher("/baz/qux/java"),
+        filesystem,
+        SOURCE_DIRECTORIES,
+        jarFiles,
+        Paths.get(OUTPUT_DIRECTORY),
+        CoverageReportFormat.HTML,
+        "TitleFoo",
+        Optional.empty(),
+        Optional.empty()) {
+        @Override
+        StepExecutionResult executeInternal(
+            ExecutionContext context,
+            Set<Path> jarFiles) {
+          assertEquals(1, jarFiles.size());
+          assertEquals(classesDir.toPath(), jarFiles.iterator().next());
+          return null;
+        }
+      };
+
+    step.execute(TestExecutionContext.newInstance());
+    assertTrue(classesDir.exists());
+  }
 
   @Test
   public void testSaveParametersToPropertyFile() throws IOException {