From ea74978da1aca9fbb1fe11456065ce114456afd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Sol=C3=B3rzano?= Date: Fri, 8 Dec 2023 03:20:04 +0100 Subject: [PATCH] [MCOMPILER-381] - Refactor incremental detection (#181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jorge Solórzano --- pom.xml | 14 +- .../verify.groovy | 2 +- .../plugin/compiler/AbstractCompilerMojo.java | 236 ++++++++++-------- .../maven/plugin/compiler/DeltaList.java | 54 ++++ 4 files changed, 187 insertions(+), 119 deletions(-) create mode 100644 src/main/java/org/apache/maven/plugin/compiler/DeltaList.java diff --git a/pom.xml b/pom.xml index f0f049d4..b6284ef8 100644 --- a/pom.xml +++ b/pom.xml @@ -80,16 +80,6 @@ under the License. org.apache.maven.plugins.compiler.its - - - - - com.thoughtworks.qdox - qdox - 2.0.3 - - - org.apache.maven.plugin-tools @@ -162,6 +152,10 @@ under the License. ${plexusCompilerVersion} runtime + + org.codehaus.plexus + plexus-utils + org.apache.maven.plugin-testing diff --git a/src/it/MCOMPILER-522-unresolvable-dependency/verify.groovy b/src/it/MCOMPILER-522-unresolvable-dependency/verify.groovy index b2198cd4..fcb0d735 100644 --- a/src/it/MCOMPILER-522-unresolvable-dependency/verify.groovy +++ b/src/it/MCOMPILER-522-unresolvable-dependency/verify.groovy @@ -24,4 +24,4 @@ def buildLog = logFile.getText('UTF-8') assert buildLog.contains( "Caused by: org.apache.maven.plugin.MojoExecutionException: " + "Resolution of annotationProcessorPath dependencies failed: " ) assert buildLog.contains( - "Could not find artifact org.apache.maven.plugins.compiler.it:annotation-processor-non-existing:jar:1.0-SNAPSHOT" ) + "The POM for org.apache.maven.plugins.compiler.it:annotation-processor-non-existing:jar:1.0-SNAPSHOT is missing, no dependency information available" ) diff --git a/src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java b/src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java index e9467651..b24e6577 100644 --- a/src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java +++ b/src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java @@ -27,6 +27,8 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.time.Instant; +import java.time.temporal.ChronoUnit; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -59,11 +61,7 @@ import org.apache.maven.project.MavenProject; import org.apache.maven.shared.incremental.IncrementalBuildHelper; import org.apache.maven.shared.incremental.IncrementalBuildHelperRequest; -import org.apache.maven.shared.utils.ReaderFactory; import org.apache.maven.shared.utils.StringUtils; -import org.apache.maven.shared.utils.io.DirectoryScanResult; -import org.apache.maven.shared.utils.io.DirectoryScanner; -import org.apache.maven.shared.utils.io.FileUtils; import org.apache.maven.shared.utils.logging.MessageBuilder; import org.apache.maven.shared.utils.logging.MessageUtils; import org.apache.maven.toolchain.Toolchain; @@ -83,6 +81,7 @@ import org.codehaus.plexus.compiler.util.scan.mapping.SuffixMapping; import org.codehaus.plexus.languages.java.jpms.JavaModuleDescriptor; import org.codehaus.plexus.languages.java.version.JavaVersion; +import org.codehaus.plexus.util.FileUtils; import org.eclipse.aether.RepositorySystem; import org.eclipse.aether.artifact.Artifact; import org.eclipse.aether.artifact.ArtifactTypeRegistry; @@ -560,12 +559,11 @@ public abstract class AbstractCompilerMojo extends AbstractMojo { /** * File extensions to check timestamp for incremental build. - * Default contains only class and jar. * * @since 3.1 */ - @Parameter - private List fileExtensions; + @Parameter(defaultValue = "class,jar") + private Set fileExtensions; /** *

to enable/disable incremental compilation feature.

@@ -897,37 +895,30 @@ public void execute() throws MojoExecutionException, CompilationFailureException incrementalBuildHelperRequest = new IncrementalBuildHelperRequest().inputFiles(sources); - DirectoryScanResult dsr = computeInputFileTreeChanges(incrementalBuildHelper, sources); - - boolean immutableOutputFile = compiler.getCompilerOutputStyle() - .equals(CompilerOutputStyle.ONE_OUTPUT_FILE_FOR_ALL_INPUT_FILES) - && !canUpdateTarget; - boolean dependencyChanged = isDependencyChanged(); - boolean sourceChanged = isSourceChanged(compilerConfiguration, compiler); - boolean inputFileTreeChanged = hasInputFileTreeChanged(dsr); - // CHECKSTYLE_OFF: LineLength - if (immutableOutputFile || dependencyChanged || sourceChanged || inputFileTreeChanged) - // CHECKSTYLE_ON: LineLength - { - String cause = immutableOutputFile - ? "immutable single output file" - : (dependencyChanged - ? "changed dependency" - : (sourceChanged ? "changed source code" : "added or removed source files")); - getLog().info("Recompiling the module because of " + cause + "."); - if (showCompilationChanges) { - for (String fileAdded : dsr.getFilesAdded()) { - getLog().info("\t+ " + fileAdded); - } - for (String fileRemoved : dsr.getFilesRemoved()) { - getLog().info("\t- " + fileRemoved); - } - } - + // Strategies used to detect modifications. + String immutableOutputFile = (compiler.getCompilerOutputStyle() + .equals(CompilerOutputStyle.ONE_OUTPUT_FILE_FOR_ALL_INPUT_FILES) + && !canUpdateTarget) + ? "immutable single output file" + : null; + String dependencyChanged = isDependencyChanged() ? "changed dependency" : null; + String sourceChanged = isSourceChanged(compilerConfiguration, compiler) ? "changed source code" : null; + String inputFileTreeChanged = hasInputFileTreeChanged(incrementalBuildHelper, sources) + ? "added or removed source files" + : null; + + // Get the first cause for the rebuild compilation detection. + String cause = Stream.of(immutableOutputFile, dependencyChanged, sourceChanged, inputFileTreeChanged) + .filter(Objects::nonNull) + .findFirst() + .orElse(null); + + if (cause != null) { + getLog().info("Recompiling the module because of " + + MessageUtils.buffer().strong(cause) + "."); compilerConfiguration.setSourceFiles(sources); } else { getLog().info("Nothing to compile - all classes are up to date."); - return; } } catch (CompilerException e) { @@ -958,8 +949,7 @@ public void execute() throws MojoExecutionException, CompilationFailureException } if (staleSources.isEmpty()) { - getLog().info("Nothing to compile - all classes are up to date"); - + getLog().info("Nothing to compile - all classes are up to date."); return; } @@ -1166,7 +1156,8 @@ public void execute() throws MojoExecutionException, CompilationFailureException // ---------------------------------------------------------------------- if (StringUtils.isEmpty(compilerConfiguration.getSourceEncoding())) { - getLog().warn("File encoding has not been set, using platform encoding " + ReaderFactory.FILE_ENCODING + getLog().warn("File encoding has not been set, using platform encoding " + + MessageUtils.buffer().strong(Charset.defaultCharset()) + ", i.e. build is platform dependent!"); } @@ -1229,13 +1220,17 @@ public void execute() throws MojoExecutionException, CompilationFailureException List errors = new ArrayList<>(); List others = new ArrayList<>(); for (CompilerMessage message : compilerResult.getCompilerMessages()) { - if (message.getKind() == CompilerMessage.Kind.ERROR) { - errors.add(message); - } else if (message.getKind() == CompilerMessage.Kind.WARNING - || message.getKind() == CompilerMessage.Kind.MANDATORY_WARNING) { - warnings.add(message); - } else { - others.add(message); + switch (message.getKind()) { + case ERROR: + errors.add(message); + break; + case WARNING: + case MANDATORY_WARNING: + warnings.add(message); + break; + default: + others.add(message); + break; } } @@ -1280,11 +1275,9 @@ public void execute() throws MojoExecutionException, CompilationFailureException case OTHER: getLog().info(message.toString()); break; - case ERROR: getLog().error(message.toString()); break; - case MANDATORY_WARNING: case WARNING: default: @@ -1409,20 +1402,21 @@ private Set getCompileSources(Compiler compiler, CompilerConfiguration com /** * @param compilerConfiguration * @param compiler - * @return true if at least a single source file is newer than it's class file + * @return {@code true} if at least a single source file is newer than it's class file */ - private boolean isSourceChanged(CompilerConfiguration compilerConfiguration, Compiler compiler) - throws CompilerException, MojoExecutionException { - Set staleSources = - computeStaleSources(compilerConfiguration, compiler, getSourceInclusionScanner(staleMillis)); + private boolean isSourceChanged(CompilerConfiguration compilerConfiguration, Compiler compiler) { + Set staleSources = Collections.emptySet(); + try { + staleSources = computeStaleSources(compilerConfiguration, compiler, getSourceInclusionScanner(staleMillis)); + } catch (MojoExecutionException | CompilerException ex) { + // we cannot detect Stale Sources, so don't do anything beside logging + getLog().warn("Cannot detect stale sources."); + return false; + } if (getLog().isDebugEnabled() || showCompilationChanges) { for (File f : staleSources) { - if (showCompilationChanges) { - getLog().info("Stale source detected: " + f.getAbsolutePath()); - } else { - getLog().debug("Stale source detected: " + f.getAbsolutePath()); - } + getLog().info("\tStale source detected: " + f.getAbsolutePath()); } } return !staleSources.isEmpty(); @@ -1438,9 +1432,14 @@ protected int getRequestThreadCount() { } protected Date getBuildStartTime() { - MavenExecutionRequest request = session.getRequest(); - Date buildStartTime = request == null ? new Date() : request.getStartTime(); - return buildStartTime == null ? new Date() : buildStartTime; + return getBuildStartTimeInstant().map(Date::from).orElseGet(Date::new); + } + + private Optional getBuildStartTimeInstant() { + return Optional.ofNullable(session.getRequest()) + .map(MavenExecutionRequest::getStartTime) + .map(Date::toInstant) + .map(i -> i.truncatedTo(ChronoUnit.MILLIS)); } private String getMemoryValue(String setting) { @@ -1576,36 +1575,41 @@ private static List removeEmptyCompileSourceRoots(List compileSo * generated classes and if we got a file which is >= the build-started timestamp, then we caught a file which * got changed during this build. * - * @return true if at least one single dependency has changed. + * @return {@code true} if at least one single dependency has changed. */ protected boolean isDependencyChanged() { - if (session == null) { + final Instant buildStartTime = getBuildStartTimeInstant().orElse(null); + if (buildStartTime == null) { // we just cannot determine it, so don't do anything beside logging - getLog().info("Cannot determine build start date, skipping incremental build detection."); + getLog().debug("Cannot determine build start time, skipping incremental build detection."); return false; } if (fileExtensions == null || fileExtensions.isEmpty()) { - fileExtensions = Collections.unmodifiableList(Arrays.asList("class", "jar")); + fileExtensions = new HashSet<>(Arrays.asList("class", "jar")); } - Date buildStartTime = getBuildStartTime(); - List pathElements = new ArrayList<>(); pathElements.addAll(getClasspathElements()); pathElements.addAll(getModulepathElements()); for (String pathElement : pathElements) { - File artifactPath = new File(pathElement); - if (artifactPath.isDirectory() || artifactPath.isFile()) { - if (!artifactPath.equals(getOutputDirectory()) && hasNewFile(artifactPath, buildStartTime)) { - if (showCompilationChanges) { - getLog().info("New dependency detected: " + artifactPath.getAbsolutePath()); - } else { - getLog().debug("New dependency detected: " + artifactPath.getAbsolutePath()); + Path artifactPath = Paths.get(pathElement); + + // Search files only on dependencies (other modules), not on the current project, + if (Files.isDirectory(artifactPath) + && !artifactPath.equals(getOutputDirectory().toPath())) { + try (Stream walk = Files.walk(artifactPath)) { + if (walk.anyMatch(p -> hasNewFile(p, buildStartTime))) { + return true; } - return true; + } catch (IOException ex) { + // we just cannot determine it, so don't do anything beside logging + getLog().warn("I/O error walking the path: " + ex.getMessage()); + return false; } + } else if (hasNewFile(artifactPath, buildStartTime)) { + return true; } } @@ -1614,25 +1618,27 @@ protected boolean isDependencyChanged() { } /** - * @param classPathEntry entry to check + * @param file entry to check * @param buildStartTime time build start * @return if any changes occurred */ - private boolean hasNewFile(File classPathEntry, Date buildStartTime) { - if (!classPathEntry.exists()) { - return false; - } - - if (classPathEntry.isFile()) { - return classPathEntry.lastModified() >= buildStartTime.getTime() - && fileExtensions.contains(FileUtils.getExtension(classPathEntry.getName())); - } - - File[] children = classPathEntry.listFiles(); - - for (File child : children) { - if (hasNewFile(child, buildStartTime)) { - return true; + private boolean hasNewFile(Path file, Instant buildStartTime) { + if (Files.isRegularFile(file) + && fileExtensions.contains( + FileUtils.extension(file.getFileName().toString()))) { + try { + Instant lastModifiedTime = Files.getLastModifiedTime(file) + .toInstant() + .minusMillis(staleMillis) + .truncatedTo(ChronoUnit.MILLIS); + boolean hasChanged = lastModifiedTime.isAfter(buildStartTime); + if (hasChanged && (getLog().isDebugEnabled() || showCompilationChanges)) { + getLog().info("\tNew dependency detected: " + file.toAbsolutePath()); + } + return hasChanged; + } catch (IOException ex) { + // we just cannot determine it, so don't do anything beside logging + getLog().warn("I/O error reading the lastModifiedTime: " + ex.getMessage()); } } @@ -1790,36 +1796,50 @@ private String getMavenCompilerPluginVersion() { return pomProperties.getProperty("version"); } - private DirectoryScanResult computeInputFileTreeChanges(IncrementalBuildHelper ibh, Set inputFiles) - throws MojoExecutionException { - File mojoConfigBase = ibh.getMojoStatusDirectory(); - File mojoConfigFile = new File(mojoConfigBase, INPUT_FILES_LST_FILENAME); - - String[] oldInputFiles = new String[0]; + private boolean hasInputFileTreeChanged(IncrementalBuildHelper ibh, Set inputFiles) { + Path mojoConfigBase; + try { + mojoConfigBase = ibh.getMojoStatusDirectory().toPath(); + } catch (MojoExecutionException e) { + // we cannot get the mojo status dir, so don't do anything beside logging + getLog().warn("Error reading mojo status directory."); + return false; + } + Path mojoConfigFile = mojoConfigBase.resolve(INPUT_FILES_LST_FILENAME); - if (mojoConfigFile.exists()) { + List oldInputFiles = Collections.emptyList(); + if (Files.isRegularFile(mojoConfigFile)) { try { - oldInputFiles = FileUtils.fileReadArray(mojoConfigFile); + oldInputFiles = Files.readAllLines(mojoConfigFile); } catch (IOException e) { - throw new MojoExecutionException("Error reading old mojo status " + mojoConfigFile, e); + // we cannot read the mojo config file, so don't do anything beside logging + getLog().warn("Error while reading old mojo status: " + mojoConfigFile); + return false; } } - String[] inputFileNames = inputFiles.stream().map(File::getAbsolutePath).toArray(String[]::new); - - DirectoryScanResult dsr = DirectoryScanner.diffFiles(oldInputFiles, inputFileNames); + List newInputFiles = + inputFiles.stream().sorted().map(File::getAbsolutePath).collect(Collectors.toList()); try { - FileUtils.fileWriteArray(mojoConfigFile, inputFileNames); + Files.write(mojoConfigFile, newInputFiles); } catch (IOException e) { - throw new MojoExecutionException("Error while storing the mojo status", e); + // we cannot write the mojo config file, so don't do anything beside logging + getLog().warn("Error while writing new mojo status: " + mojoConfigFile); + return false; } - return dsr; - } + DeltaList inputTreeChanges = new DeltaList<>(oldInputFiles, newInputFiles); + if (getLog().isDebugEnabled() || showCompilationChanges) { + for (String fileAdded : inputTreeChanges.getAdded()) { + getLog().info("\tInput tree files (+): " + fileAdded); + } + for (String fileRemoved : inputTreeChanges.getRemoved()) { + getLog().info("\tInput tree files (-): " + fileRemoved); + } + } - private boolean hasInputFileTreeChanged(DirectoryScanResult dsr) { - return dsr.getFilesAdded().length > 0 || dsr.getFilesRemoved().length > 0; + return inputTreeChanges.hasChanged(); } public void setTarget(String target) { diff --git a/src/main/java/org/apache/maven/plugin/compiler/DeltaList.java b/src/main/java/org/apache/maven/plugin/compiler/DeltaList.java new file mode 100644 index 00000000..18943fab --- /dev/null +++ b/src/main/java/org/apache/maven/plugin/compiler/DeltaList.java @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.plugin.compiler; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; + +/** + * Show the modifications between two lists. + */ +final class DeltaList { + + private final List added; + private final List removed; + private final boolean hasChanged; + + DeltaList(Collection oldList, Collection newList) { + this.added = new ArrayList<>(newList); + this.removed = new ArrayList<>(oldList); + added.removeAll(oldList); + removed.removeAll(newList); + this.hasChanged = !added.isEmpty() || !removed.isEmpty(); + } + + Collection getAdded() { + return Collections.unmodifiableCollection(added); + } + + Collection getRemoved() { + return Collections.unmodifiableCollection(removed); + } + + boolean hasChanged() { + return hasChanged; + } +}