[
https://issues.apache.org/jira/browse/MCOMPILER-333?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17729703#comment-17729703
]
ASF GitHub Bot commented on MCOMPILER-333:
------------------------------------------
elharo commented on code in PR #23:
URL:
https://github.com/apache/maven-shared-incremental/pull/23#discussion_r1219477223
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -61,17 +69,28 @@
private MavenProject mavenProject;
/**
- * Used for detecting changes between the Mojo execution.
+ * Used for detecting changes in the output directory between the Mojo
execution.
Review Comment:
something's off with "between the Mojo execution". possibly should be
"between Mojo executions" or perhaps something else is meant?
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -323,25 +335,23 @@ public String[] beforeRebuildExecution(
IncrementalBuildHelperRequest incrementa
public void afterRebuildExecution( IncrementalBuildHelperRequest
incrementalBuildHelperRequest )
throws MojoExecutionException
{
- DirectoryScanner diffScanner = getDirectoryScanner();
- // now scan the same directory again and create a diff
- diffScanner.scan();
- DirectoryScanResult scanResult = diffScanner.diffIncludedFiles(
filesBeforeAction );
-
File mojoConfigBase = getMojoStatusDirectory();
- File mojoConfigFile = new File( mojoConfigBase,
CREATED_FILES_LST_FILENAME );
- try
- {
- FileUtils.fileWriteArray( mojoConfigFile,
scanResult.getFilesAdded() );
- }
- catch ( IOException e )
- {
- throw new MojoExecutionException( "Error while storing the mojo
status", e );
- }
+ writeChangedFiles(
+ getDirectoryScanner(),
Review Comment:
no need to call a getter method in the same class, just use the
directoryScanner field directly
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
+ throws IOException
+ {
+ File createdFiles = new File( mojoConfigBase, createdFilesListFileName
);
+ String[] filesAdded = outputDirectoryScanResult.getFilesAdded();
+ String filesAddedAsString = Stream.of( filesAdded ).collect(
Collectors.joining( "\n" ) );
+
+ Files.write(
+ createdFiles.toPath(),
+ filesAddedAsString.getBytes( StandardCharsets.UTF_8 ),
+ StandardOpenOption.CREATE );
+ }
+
+ private static DirectoryScanResult scanDirectoryDiff(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction )
+ {
+ // now scan the same directory again and create a diff
+ directoryScanner.scan();
+ DirectoryScanResult outputScanResult =
+ directoryScanner.diffIncludedFiles( filesBeforeAction );
+ return outputScanResult;
+ }
+
+ private static List<String> deleteFiles( File fileNameIndex, File parent )
throws IOException
+ {
+ List<String> oldFiles = Files.readAllLines( fileNameIndex.toPath() );
+ for ( String oldFileName : oldFiles )
+ {
+ File oldFile = new File( parent, oldFileName );
+ oldFile.delete();
Review Comment:
what if this method returns false? i.e. delete failed. Do we care?
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelperRequest.java:
##########
@@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File
outputDirectory )
this.outputDirectory = outputDirectory;
return this;
}
+
+ public File getGeneratedSourcesDirectory()
+ {
+ return generatedSourcesDirectory;
+ }
+
+ public void setGeneratedSourcesDirectory( File generatedSourcesDirectory )
Review Comment:
needs Javadoc
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -61,17 +69,28 @@
private MavenProject mavenProject;
/**
- * Used for detecting changes between the Mojo execution.
+ * Used for detecting changes in the output directory between the Mojo
execution.
* @see #getDirectoryScanner();
*/
- private DirectoryScanner directoryScanner;
+ private DirectoryScanner directoryScanner = new DirectoryScanner();
+
+ /**
+ * Used for detecting changes in the generated sources directory between
the Mojo execution.
Review Comment:
ditto
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
Review Comment:
This is a field in the same class so no need to pass it as an argument
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
Review Comment:
This is a constant in the same class so no need to pass it as an argument
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
+ throws IOException
+ {
+ File createdFiles = new File( mojoConfigBase, createdFilesListFileName
);
+ String[] filesAdded = outputDirectoryScanResult.getFilesAdded();
+ String filesAddedAsString = Stream.of( filesAdded ).collect(
Collectors.joining( "\n" ) );
+
+ Files.write(
+ createdFiles.toPath(),
+ filesAddedAsString.getBytes( StandardCharsets.UTF_8 ),
+ StandardOpenOption.CREATE );
+ }
+
+ private static DirectoryScanResult scanDirectoryDiff(
+ DirectoryScanner directoryScanner,
Review Comment:
make non-static and use the directoryScanner field
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
+ throws IOException
+ {
+ File createdFiles = new File( mojoConfigBase, createdFilesListFileName
);
+ String[] filesAdded = outputDirectoryScanResult.getFilesAdded();
+ String filesAddedAsString = Stream.of( filesAdded ).collect(
Collectors.joining( "\n" ) );
Review Comment:
Just use String.join or StringJoiner here
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
+ throws IOException
+ {
+ File createdFiles = new File( mojoConfigBase, createdFilesListFileName
);
+ String[] filesAdded = outputDirectoryScanResult.getFilesAdded();
+ String filesAddedAsString = Stream.of( filesAdded ).collect(
Collectors.joining( "\n" ) );
+
+ Files.write(
+ createdFiles.toPath(),
+ filesAddedAsString.getBytes( StandardCharsets.UTF_8 ),
+ StandardOpenOption.CREATE );
+ }
+
+ private static DirectoryScanResult scanDirectoryDiff(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction )
+ {
+ // now scan the same directory again and create a diff
+ directoryScanner.scan();
+ DirectoryScanResult outputScanResult =
+ directoryScanner.diffIncludedFiles( filesBeforeAction );
+ return outputScanResult;
+ }
+
+ private static List<String> deleteFiles( File fileNameIndex, File parent )
throws IOException
+ {
+ List<String> oldFiles = Files.readAllLines( fileNameIndex.toPath() );
+ for ( String oldFileName : oldFiles )
+ {
+ File oldFile = new File( parent, oldFileName );
+ oldFile.delete();
+ }
+ return oldFiles;
+ }
+
+ private String[] scanDirectory( DirectoryScanner directoryScanner, File
directory )
Review Comment:
make non-static and use the directoryScanner field
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
Review Comment:
This is a constant in the same class so no need to pass it as an argument
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelperRequest.java:
##########
@@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File
outputDirectory )
this.outputDirectory = outputDirectory;
return this;
}
+
+ public File getGeneratedSourcesDirectory()
+ {
+ return generatedSourcesDirectory;
+ }
+
+ public void setGeneratedSourcesDirectory( File generatedSourcesDirectory )
+ {
+ this.generatedSourcesDirectory = generatedSourcesDirectory;
Review Comment:
Is it worth checking here that the file exists and is a directory? Throw an
IllegalArgumentException if not.
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelperRequest.java:
##########
@@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File
outputDirectory )
this.outputDirectory = outputDirectory;
return this;
}
+
+ public File getGeneratedSourcesDirectory()
+ {
+ return generatedSourcesDirectory;
+ }
+
+ public void setGeneratedSourcesDirectory( File generatedSourcesDirectory )
+ {
+ this.generatedSourcesDirectory = generatedSourcesDirectory;
+ }
+
+ public IncrementalBuildHelperRequest generatedSourcesDirectory( File
generatedSourcesDirectory )
+ {
+ this.generatedSourcesDirectory = generatedSourcesDirectory;
Review Comment:
Is it worth checking here that the file exists and is a directory?
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelperRequest.java:
##########
@@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File
outputDirectory )
this.outputDirectory = outputDirectory;
return this;
}
+
+ public File getGeneratedSourcesDirectory()
Review Comment:
needs Javadoc that clearly explains what this directory is.
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -61,17 +69,28 @@
private MavenProject mavenProject;
/**
- * Used for detecting changes between the Mojo execution.
+ * Used for detecting changes in the output directory between the Mojo
execution.
* @see #getDirectoryScanner();
*/
- private DirectoryScanner directoryScanner;
+ private DirectoryScanner directoryScanner = new DirectoryScanner();
Review Comment:
This class is deprecated. Consider using use java.nio.file.DirectoryStream
or java.nio.Files.walkFileTree() and related classes instead. Those should
definitely be safer in a multi-threaded environment.
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -358,6 +368,79 @@ public void afterRebuildExecution(
IncrementalBuildHelperRequest incrementalBuil
}
+ private void writeChangedFiles(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction,
+ File mojoConfigBase,
+ String createdFilesListFileName ) throws MojoExecutionException
+ {
+ if ( directoryScanner.getBasedir() == null )
+ {
+ return;
+ }
+
+ DirectoryScanResult outputDirectoryScanResult = scanDirectoryDiff(
+ directoryScanner, filesBeforeAction );
+
+ try
+ {
+ writeChangedFiles( outputDirectoryScanResult, mojoConfigBase,
createdFilesListFileName );
+ }
+ catch ( IOException e )
+ {
+ throw new MojoExecutionException( "Error while storing the mojo
status", e );
+ }
+ }
+
+ private static void writeChangedFiles(
+ DirectoryScanResult outputDirectoryScanResult,
+ File mojoConfigBase,
+ String createdFilesListFileName )
+ throws IOException
+ {
+ File createdFiles = new File( mojoConfigBase, createdFilesListFileName
);
+ String[] filesAdded = outputDirectoryScanResult.getFilesAdded();
+ String filesAddedAsString = Stream.of( filesAdded ).collect(
Collectors.joining( "\n" ) );
+
+ Files.write(
+ createdFiles.toPath(),
+ filesAddedAsString.getBytes( StandardCharsets.UTF_8 ),
+ StandardOpenOption.CREATE );
+ }
+
+ private static DirectoryScanResult scanDirectoryDiff(
+ DirectoryScanner directoryScanner,
+ String[] filesBeforeAction )
+ {
+ // now scan the same directory again and create a diff
+ directoryScanner.scan();
+ DirectoryScanResult outputScanResult =
+ directoryScanner.diffIncludedFiles( filesBeforeAction );
+ return outputScanResult;
+ }
+
+ private static List<String> deleteFiles( File fileNameIndex, File parent )
throws IOException
+ {
+ List<String> oldFiles = Files.readAllLines( fileNameIndex.toPath() );
+ for ( String oldFileName : oldFiles )
+ {
+ File oldFile = new File( parent, oldFileName );
+ oldFile.delete();
+ }
+ return oldFiles;
+ }
+
+ private String[] scanDirectory( DirectoryScanner directoryScanner, File
directory )
+ {
+ directoryScanner.setBasedir( directory );
Review Comment:
This is a little worrisome. I didn't realize directoryScanner was mutable.
This might suggest not using a directoryScanner field at all and just creating
one when we need it.
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelperRequest.java:
##########
@@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File
outputDirectory )
this.outputDirectory = outputDirectory;
return this;
}
+
+ public File getGeneratedSourcesDirectory()
+ {
+ return generatedSourcesDirectory;
+ }
+
+ public void setGeneratedSourcesDirectory( File generatedSourcesDirectory )
+ {
+ this.generatedSourcesDirectory = generatedSourcesDirectory;
+ }
+
+ public IncrementalBuildHelperRequest generatedSourcesDirectory( File
generatedSourcesDirectory )
Review Comment:
needs Javadoc that clearly explains what this directory is.
##########
src/main/java/org/apache/maven/shared/incremental/IncrementalBuildHelper.java:
##########
@@ -66,12 +71,24 @@
*/
private DirectoryScanner directoryScanner;
+ /**
+ * Used for detecting changes between the Mojo execution.
+ * @see #getGeneratedSourcesDirectoryScanner() ;
+ */
+ private DirectoryScanner generatedSourcesDirectoryScanner;
+
/**
* Once the {@link
#beforeRebuildExecution(org.apache.maven.shared.incremental.IncrementalBuildHelperRequest)}
got
* called, this will contain the list of files in the build directory.
*/
private String[] filesBeforeAction = new String[0];
+ /**
+ * Once the {@link
#beforeRebuildExecution(org.apache.maven.shared.incremental.IncrementalBuildHelperRequest)}
got
Review Comment:
ping
> Incremental compilation causes "mvn clean compile compile" to fail
> ------------------------------------------------------------------
>
> Key: MCOMPILER-333
> URL: https://issues.apache.org/jira/browse/MCOMPILER-333
> Project: Maven Compiler Plugin
> Issue Type: Bug
> Affects Versions: 3.7.0
> Reporter: Anthony Vanelverdinghe
> Priority: Major
>
> With <useIncrementalCompilation>true</useIncrementalCompilation>, my build
> fails if I do "mvn clean compile compile". From the
> [comment|https://issues.apache.org/jira/browse/MCOMPILER-205?focusedCommentId=16390065&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16390065]
> in MCOMPILER-205, I understand that incremental compilation deletes the
> "classes" directory. But it seems that it doesn't delete the
> "generated-sources" directory.
> When I do "mvn clean compile" and "mvn compile", the second build fails.
> However, if I delete the "generated-sources" directory between the 2 builds,
> they both succeed.
> So I guess the bug here is that incremental compilation doesn't delete the
> "generated-sources" directory.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
