chore: close the analyzer log, widen the tests, drop dead build config

README.md

@@ -8,6 +8,12 @@ to detect style violations and potential bugs early in the development process.
   <img src=".github/assets/finoid-codequality-maven-plugin.jpg" width="256">
 </div>
 
+## Requirements
+
+* **Java 21 or later.** The plugin itself is compiled for Java 21, and the Error Prone releases it defaults to require a
+  Java 21 compiler to run.
+* Maven 3.9.6 or later.
+
 ## Supported code quality tools
 
 * Checkstyle – Analyzes Java code for style guideline violations, helping enforce consistent formatting and naming

pom.xml

@@ -494,19 +494,6 @@
                     </execution>
                 </executions>
             </plugin>
-            <plugin>
-                <groupId>org.codehaus.plexus</groupId>
-                <artifactId>plexus-component-metadata</artifactId>
-                <version>2.2.0</version>
-                <executions>
-                    <execution>
-                        <id>process-annotations</id>
-                        <goals>
-                            <goal>generate-metadata</goal>
-                        </goals>
-                    </execution>
-                </executions>
-            </plugin>
             <plugin>
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-compiler-plugin</artifactId>

src/main/java/io/github/finoid/maven/plugins/codequality/MojoLogDecoratorExecutionListener.java

@@ -101,12 +101,24 @@ public void beforeMojoExecution(final MojoExecutionEvent event) {
 
     @Override
     public void afterMojoExecutionSuccess(final MojoExecutionEvent event) {
-        // No-op
+        closeDecoratedLog(event);
     }
 
     @Override
     public void afterExecutionFailure(final MojoExecutionEvent event) {
-        // No-op
+        closeDecoratedLog(event);
+    }
+
+    /**
+     * Releases the file the decorated log of the given mojo writes to.
+     * <p>
+     * The log is read back from the mojo rather than remembered, so that concurrently executing modules cannot close
+     * each other's file. Mojos this listener did not decorate are left alone.
+     */
+    private static void closeDecoratedLog(final MojoExecutionEvent event) {
+        if (event.getMojo().getLog() instanceof LogAndFileAppender appender) {
+            appender.close();
+        }
     }
 
     private static boolean isMojoOfType(final MojoExecutionEvent event, final String type) {

src/main/java/io/github/finoid/maven/plugins/codequality/log/LogAndFileAppender.java

@@ -5,6 +5,7 @@
 import org.codehaus.plexus.logging.Logger;
 import org.jspecify.annotations.Nullable;
 
+import java.io.Closeable;
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
@@ -16,13 +17,17 @@
  * This class wraps an existing {@link Logger} instance and appends log messages to a specified file,
  * making it useful for cases where logs need to be both written to the standard logging system
  * and persisted to a file. This implementation is inspired by {@link org.apache.maven.monitor.logging.DefaultLog}.
+ * <p>
+ * Holds the file open until {@link #close()} is called, which
+ * {@link io.github.finoid.maven.plugins.codequality.MojoLogDecoratorExecutionListener} does once the decorated mojo has
+ * finished. A reactor of many modules would otherwise keep one file handle open per analyzed module.
  */
-public class LogAndFileAppender implements Log {
+public class LogAndFileAppender implements Log, Closeable {
     private final Logger logger;
     private final LogLevel logLevel;
     private final PrintStream printStream;
 
-    @SuppressWarnings("required.method.not.called") // the FileOutputStream will be implicitly closed when the jvm exits
+    @SuppressWarnings("required.method.not.called") // ownership is transferred to the PrintStream, which close() closes
     public LogAndFileAppender(final Logger logger, final File file, final LogLevel logLevel) throws FileNotFoundException {
         this.logger = Precondition.nonNull(logger, "Logger shouldn't be null");
         this.logLevel = Precondition.nonNull(logLevel, "LogLevel shouldn't be null");
@@ -131,6 +136,14 @@ public void error(final Throwable error) {
         printStream.println(error);
     }
 
+    /**
+     * Closes the underlying file. Subsequent writes are discarded by the {@link PrintStream} rather than throwing.
+     */
+    @Override
+    public void close() {
+        printStream.close();
+    }
+
     @Override
     public boolean isDebugEnabled() {
         return logger.isDebugEnabled();

src/main/java/io/github/finoid/maven/plugins/codequality/step/CheckerFrameworkStep.java

@@ -8,6 +8,7 @@
 import io.github.finoid.maven.plugins.codequality.report.CheckerFrameworkViolationLogParser;
 import io.github.finoid.maven.plugins.codequality.report.Violation;
 import io.github.finoid.maven.plugins.codequality.util.CollectorUtils;
+import io.github.finoid.maven.plugins.codequality.util.ExceptionUtils;
 import io.github.finoid.maven.plugins.codequality.util.MojoUtils.ElementUtils;
 import io.github.finoid.maven.plugins.codequality.util.MojoUtils.PluginUtils;
 import io.github.finoid.maven.plugins.codequality.util.Precondition;
@@ -142,7 +143,12 @@ private List<Violation> executeStep(
 
             return parseViolations(context);
         } catch (final Exception e) {
-            throw new CodeQualityException("Error during execution of CheckerFramework step", e);
+            // The forked compiler reports through its own log, which the plugin redirects to a file, so the reason a
+            // step failed is regularly only in that file. Both the file and the deepest cause are named here, the
+            // wrapping exceptions of a forked mojo say little on their own.
+            throw new CodeQualityException(String.format(
+                "Error during execution of CheckerFramework step. Cause: %s. The output of the forked compiler was captured in %s",
+                ExceptionUtils.rootCauseMessage(e), checkerFrameworkOutputFilePath(currentProject)), e);
         }
     }
 

src/main/java/io/github/finoid/maven/plugins/codequality/step/ErrorProneStep.java

@@ -8,6 +8,7 @@
 import io.github.finoid.maven.plugins.codequality.log.ErrorProneViolationLogParser;
 import io.github.finoid.maven.plugins.codequality.report.Violation;
 import io.github.finoid.maven.plugins.codequality.util.CollectorUtils;
+import io.github.finoid.maven.plugins.codequality.util.ExceptionUtils;
 import io.github.finoid.maven.plugins.codequality.util.MojoUtils.ElementUtils;
 import io.github.finoid.maven.plugins.codequality.util.MojoUtils.PluginUtils;
 import io.github.finoid.maven.plugins.codequality.util.Precondition;
@@ -134,7 +135,12 @@ private List<Violation> executeStep(final CodeQualityConfiguration codeQualityCo
 
             return parseViolations(context);
         } catch (final Exception e) {
-            throw new CodeQualityException("Error during execution of ErrorProne step", e);
+            // The forked compiler reports through its own log, which the plugin redirects to a file, so the reason a
+            // step failed is regularly only in that file. Both the file and the deepest cause are named here, the
+            // wrapping exceptions of a forked mojo say little on their own.
+            throw new CodeQualityException(String.format(
+                "Error during execution of ErrorProne step. Cause: %s. The output of the forked compiler was captured in %s",
+                ExceptionUtils.rootCauseMessage(e), errorProneOutputFilePath(currentProject)), e);
         }
     }
 

src/main/java/io/github/finoid/maven/plugins/codequality/util/ExceptionUtils.java

@@ -0,0 +1,59 @@
+package io.github.finoid.maven.plugins.codequality.util;
+
+import lombok.experimental.UtilityClass;
+import org.jspecify.annotations.Nullable;
+
+/**
+ * Utility class for extracting the interesting part of an exception chain.
+ */
+@UtilityClass
+public class ExceptionUtils {
+    /**
+     * A cause chain is not guaranteed to be acyclic, so the traversal is bounded.
+     */
+    private static final int MAX_DEPTH = 20;
+
+    /**
+     * Resolves the message of the deepest cause which has one.
+     * <p>
+     * The steps fork other mojos, so the reason a step failed regularly sits several wrappers down - a
+     * {@code MojoExecutionException} wrapping a {@code CompilationFailureException} wrapping the actual failure - while
+     * the wrappers themselves say little. Falls back to the simple name of the deepest cause when none of them carries a
+     * message.
+     *
+     * @param throwable the throwable to unwrap
+     * @return the message of the deepest cause which has one, never blank
+     */
+    public static String rootCauseMessage(final Throwable throwable) {
+        Throwable deepest = throwable;
+        String message = messageOrNull(throwable);
+
+        for (int depth = 0; depth < MAX_DEPTH; depth++) {
+            final Throwable cause = deepest.getCause();
+
+            if (cause == null || cause == deepest) {
+                break;
+            }
+
+            deepest = cause;
+
+            final String causeMessage = messageOrNull(cause);
+            if (causeMessage != null) {
+                message = causeMessage;
+            }
+        }
+
+        return message != null ? message : deepest.getClass().getSimpleName();
+    }
+
+    @Nullable
+    private static String messageOrNull(final Throwable throwable) {
+        final String message = throwable.getMessage();
+
+        if (message == null || message.isBlank()) {
+            return null;
+        }
+
+        return message.strip();
+    }
+}

src/test/java/io/github/finoid/maven/plugins/codequality/CodeQualityReactorIT.java

@@ -10,6 +10,7 @@
 import org.junit.jupiter.api.Test;
 
 import java.io.IOException;
+import java.io.InputStream;
 import java.io.UncheckedIOException;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.FileVisitResult;
@@ -22,6 +23,7 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Properties;
 import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -46,19 +48,22 @@
 class CodeQualityReactorIT {
     private static final String CHECKSTYLE_FIXTURE = "it/multi-module-reactor";
     private static final String ERROR_PRONE_FIXTURE = "it/error-prone-reactor";
+    private static final String CHECKER_FRAMEWORK_FIXTURE = "it/checker-framework-reactor";
 
     private static final Map<String, String> EXPECTED_CHECKSTYLE_VIOLATIONS_BY_PATH = Map.of(
         "module-a/src/main/java/it/alpha/Alpha.java", "Unused import - java.util.List.",
         "module-b/src/main/java/it/beta/Beta.java", "Empty statement.",
         "module-c/src/main/java/it/gamma/Gamma.java", "Literal Strings should be compared using equals(), not '=='."
     );
 
-    private static final Set<String> EXPECTED_ERROR_PRONE_PATHS = Set.of(
+    private static final Set<String> EXPECTED_COMPILER_STEP_PATHS = Set.of(
         "module-a/src/main/java/it/alpha/Alpha.java",
         "module-b/src/main/java/it/beta/Beta.java",
         "module-c/src/main/java/it/gamma/Gamma.java"
     );
 
+    private static final List<String> MODULES = List.of("module-a", "module-b", "module-c");
+
     private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
 
     private static String pluginVersion;
@@ -145,21 +150,79 @@ void givenErrorProneReactor_whenVerifyInParallel_thenViolationsOfEveryModuleAreR
 
         final List<ReportedViolation> violations = aggregatedViolations(basedir);
 
-        Assertions.assertEquals(EXPECTED_ERROR_PRONE_PATHS, pathsOf(violations),
+        Assertions.assertEquals(EXPECTED_COMPILER_STEP_PATHS, pathsOf(violations),
             () -> "Every module is expected to contribute its own Error Prone violations, but got " + violations);
 
         // Every module has to have been analyzed by Error Prone, rather than a single module three times over
         violations.forEach(violation -> Assertions.assertTrue(violation.description().startsWith("ErrorProne: "),
             () -> "Unexpected non Error Prone violation: " + violation));
 
-        // The log file of every module has to have been written next to that module, not next to a pinned one
-        for (final String module : List.of("module-a", "module-b", "module-c")) {
-            final Path log = basedir.resolve(module + "/target/errorprone-" + module + ".txt");
+        assertPerModuleOutput(basedir, "errorprone");
+        assertReportedOnce(basedir);
+    }
 
-            Assertions.assertTrue(Files.isRegularFile(log), () -> "Missing Error Prone output of " + module + ": " + log);
-        }
+    /**
+     * The Checker Framework step forks the compiler exactly like Error Prone does, and reads its findings back from the
+     * same kind of per module log file, so it is prone to the same cross module mix ups.
+     */
+    @Test
+    @DisplayName("A parallel built reactor reports the Checker Framework violations of every module")
+    void givenCheckerFrameworkReactor_whenVerifyInParallel_thenViolationsOfEveryModuleAreReported() throws Exception {
+        // Not under this project's own target directory, see copyFixtureOutsideBuildDirectory
+        final Path basedir = copyFixtureOutsideBuildDirectory(CHECKER_FRAMEWORK_FIXTURE, "checker-framework-parallel");
 
+        final Verifier verifier = verifier(basedir);
+        verifier.addCliOption("-T");
+        verifier.addCliOption("4");
+        // Keeps checker-qual, which the step requires on the class path, in step with the checker the plugin runs
+        verifier.addCliOption("-Dcq.it.checker.version=" + checkerFrameworkVersion());
+        verifier.executeGoal("verify");
+        verifier.resetStreams();
+
+        final List<ReportedViolation> violations = aggregatedViolations(basedir);
+
+        Assertions.assertEquals(EXPECTED_COMPILER_STEP_PATHS, pathsOf(violations),
+            () -> "Every module is expected to contribute its own Checker Framework violations, but got " + violations);
+
+        violations.forEach(violation -> Assertions.assertTrue(violation.description().startsWith("CheckerFramework: "),
+            () -> "Unexpected non Checker Framework violation: " + violation));
+
+        assertPerModuleOutput(basedir, "checkerframework");
         assertReportedOnce(basedir);
+
+        // Only on success, a failed run is worth keeping around to look at
+        deleteRecursively(basedir.getParent());
+    }
+
+    /**
+     * Asserts that the output of the forked compiler of every module was written next to that module, rather than next
+     * to whichever module a pinned project happened to be.
+     */
+    private void assertPerModuleOutput(final Path basedir, final String prefix) {
+        for (final String module : MODULES) {
+            final Path log = basedir.resolve(module + "/target/" + prefix + "-" + module + ".txt");
+
+            Assertions.assertTrue(Files.isRegularFile(log), () -> "Missing analyzer output of " + module + ": " + log);
+        }
+    }
+
+    /**
+     * The Checker Framework version the plugin defaults to, read from the properties the build filters it into.
+     */
+    private static String checkerFrameworkVersion() throws IOException {
+        final Properties properties = new Properties();
+
+        try (InputStream stream = CodeQualityReactorIT.class.getResourceAsStream("/checkerframework-versions.properties")) {
+            Assertions.assertNotNull(stream, "Missing checkerframework-versions.properties on the test class path");
+
+            properties.load(stream);
+        }
+
+        final String version = properties.getProperty("checkerframework.version");
+
+        Assertions.assertNotNull(version, "Missing checkerframework.version property");
+
+        return version;
     }
 
     private Verifier verifier(final Path basedir) throws VerificationException {
@@ -235,14 +298,26 @@ private String logOf(final Path basedir) throws IOException {
     }
 
     private Path copyFixture(final String fixture, final String name) throws IOException {
+        return copyFixtureTo(fixture, Paths.get(System.getProperty("basedir", ""), "target", "it", name).toAbsolutePath());
+    }
+
+    /**
+     * Copies a fixture to a working directory outside the build directory of this project.
+     * <p>
+     * The Checker Framework step passes {@code -AskipFiles=/target/} to keep generated sources out of the analysis, and
+     * that pattern is matched against the whole path of every file. A fixture below this project's own {@code target}
+     * directory would therefore be skipped in its entirety and the analyzer would report nothing at all.
+     */
+    private Path copyFixtureOutsideBuildDirectory(final String fixture, final String name) throws IOException {
+        return copyFixtureTo(fixture, Files.createTempDirectory("codequality-it-").resolve(name));
+    }
+
+    private Path copyFixtureTo(final String fixture, final Path target) throws IOException {
         final Path fixtureRoot = Paths.get(System.getProperty("basedir", ""), "target", "test-classes", fixture)
             .toAbsolutePath();
 
         Assertions.assertTrue(Files.isDirectory(fixtureRoot), () -> "Missing integration test fixture: " + fixtureRoot);
 
-        final Path target = Paths.get(System.getProperty("basedir", ""), "target", "it", name)
-            .toAbsolutePath();
-
         deleteRecursively(target);
 
         Files.createDirectories(target);