From 2e0eec7ec50a04f480a85050f797275bc64d874b Mon Sep 17 00:00:00 2001 From: elenairina Date: Fri, 8 Jun 2018 01:42:20 -0700 Subject: [PATCH] Make JavaBuilder use a unique coverage metadata directory for each test instead of the same directory for all the tests. The previous implementation was using one directory for instrumenting the classes of a jar. For each each jar the metadata directory was deleted if it already existed. This is problematic for local execution when multiple tests are run in parallel because some threads will try to delete the directory and some will try to perform read/write operations on it. This is an important fix for Bazel coverage users. Fixes #4398. RELNOTES: Java coverage works now with multiple jobs. PiperOrigin-RevId: 199764483 --- .../buildjar/SimpleJavaLibraryBuilder.java | 6 ++- .../JacocoInstrumentationProcessor.java | 44 +++++++++++-------- 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/SimpleJavaLibraryBuilder.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/SimpleJavaLibraryBuilder.java index 50c936ac43360e..483b871d518c29 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/SimpleJavaLibraryBuilder.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/SimpleJavaLibraryBuilder.java @@ -148,17 +148,21 @@ public BlazeJavacResult run(JavaLibraryBuildRequest build) throws Exception { public void buildJar(JavaLibraryBuildRequest build) throws IOException { JarCreator jar = new JarCreator(build.getOutputJar()); + JacocoInstrumentationProcessor processor = null; try { jar.setNormalize(true); jar.setCompression(build.compressJar()); jar.addDirectory(build.getClassDir()); jar.setJarOwner(build.getTargetLabel(), build.getInjectingRuleKind()); - JacocoInstrumentationProcessor processor = build.getJacocoInstrumentationProcessor(); + processor = build.getJacocoInstrumentationProcessor(); if (processor != null) { processor.processRequest(build, processor.isNewCoverageImplementation() ? jar : null); } } finally { jar.execute(); + if (processor != null) { + processor.cleanup(); + } } } diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java index d3b37d257abf57..995c95d408d02f 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/instrumentation/JacocoInstrumentationProcessor.java @@ -37,24 +37,23 @@ public final class JacocoInstrumentationProcessor { public static JacocoInstrumentationProcessor create(List args) throws InvalidCommandLineException { - if (args.size() < 2) { + if (args.size() < 1) { throw new InvalidCommandLineException( - "Number of arguments for Jacoco instrumentation should be 2+ (given " + "Number of arguments for Jacoco instrumentation should be 1+ (given " + args.size() - + ": metadataOutput metadataDirectory [filters*]."); + + ": metadataOutput [filters*]."); } // ignoring filters, they weren't used in the previous implementation // TODO(bazel-team): filters should be correctly handled - return new JacocoInstrumentationProcessor(args.get(1), args.get(0)); + return new JacocoInstrumentationProcessor(args.get(0)); } - private final String metadataDir; + private Path instrumentedClassesDirectory; private final String coverageInformation; private final boolean isNewCoverageImplementation; - private JacocoInstrumentationProcessor(String metadataDir, String coverageInfo) { - this.metadataDir = metadataDir; + private JacocoInstrumentationProcessor(String coverageInfo) { this.coverageInformation = coverageInfo; // This is part of the new Java coverage implementation where JacocoInstrumentationProcessor // receives a file that includes the relative paths of the uninstrumented Java files, instead @@ -71,15 +70,10 @@ public boolean isNewCoverageImplementation() { * jacocoMetadataDir, to be zipped up in the output file jacocoMetadataOutput. */ public void processRequest(JavaLibraryBuildRequest build, JarCreator jar) throws IOException { - // Clean up jacocoMetadataDir to be used by postprocessing steps. This is important when - // running JavaBuilder locally, to remove stale entries from previous builds. - if (metadataDir != null) { - Path workDir = Paths.get(metadataDir); - if (Files.exists(workDir)) { - recursiveRemove(workDir); - } - Files.createDirectories(workDir); - } + // Use a directory for coverage metadata that is unique to each built jar. Avoids + // multiple threads performing read/write/delete actions on the instrumented classes directory. + instrumentedClassesDirectory = getMetadataDirRelativeToJar(build.getOutputJar()); + Files.createDirectories(instrumentedClassesDirectory); if (jar == null) { jar = new JarCreator(coverageInformation); } @@ -87,14 +81,26 @@ public void processRequest(JavaLibraryBuildRequest build, JarCreator jar) throws jar.setCompression(build.compressJar()); Instrumenter instr = new Instrumenter(new OfflineInstrumentationAccessGenerator()); instrumentRecursively(instr, build.getClassDir()); - jar.addDirectory(metadataDir); + jar.addDirectory(instrumentedClassesDirectory); if (isNewCoverageImplementation) { jar.addEntry(coverageInformation, coverageInformation); } else { jar.execute(); + cleanup(); + } + } + + public void cleanup() throws IOException { + if (Files.exists(instrumentedClassesDirectory)) { + recursiveRemove(instrumentedClassesDirectory); } } + // Return the path of the coverage metadata directory relative to the output jar path. + private static Path getMetadataDirRelativeToJar(Path outputJar) { + return outputJar.resolveSibling(outputJar + "-coverage-metadata"); + } + /** * Runs Jacoco instrumentation processor over all .class files recursively, starting with root. */ @@ -119,9 +125,9 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) if (isNewCoverageImplementation) { Path absoluteUninstrumentedCopy = Paths.get(file + ".uninstrumented"); uninstrumentedCopy = - Paths.get(metadataDir).resolve(root.relativize(absoluteUninstrumentedCopy)); + instrumentedClassesDirectory.resolve(root.relativize(absoluteUninstrumentedCopy)); } else { - uninstrumentedCopy = Paths.get(metadataDir).resolve(root.relativize(file)); + uninstrumentedCopy = instrumentedClassesDirectory.resolve(root.relativize(file)); } Files.createDirectories(uninstrumentedCopy.getParent()); Files.move(file, uninstrumentedCopy);