Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MCOMPILER-333] Clean generatedSourcesDirectory along with outputDirectory #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,14 @@

import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.StandardOpenOption;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

/**
* Various helper methods to support incremental builds
Expand All @@ -46,6 +53,7 @@ public class IncrementalBuildHelper
*/
private static final String MAVEN_STATUS_ROOT = "maven-status";
public static final String CREATED_FILES_LST_FILENAME = "createdFiles.lst";
public static final String GENERATED_FILES_LST_FILENAME = "generatedFiles.lst";
private static final String INPUT_FILES_LST_FILENAME = "inputFiles.lst";

private static final String[] EMPTY_ARRAY = new String[0];
Expand All @@ -61,17 +69,28 @@ public class IncrementalBuildHelper
private MavenProject mavenProject;

/**
* Used for detecting changes between the Mojo execution.
* Used for detecting changes in the output directory between the Mojo execution.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something's off with "between the Mojo execution". possibly should be "between Mojo executions" or perhaps something else is meant?

* @see #getDirectoryScanner();
*/
private DirectoryScanner directoryScanner;
private DirectoryScanner directoryScanner = new DirectoryScanner();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Used for detecting changes in the generated sources directory between the Mojo execution.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

*/
private DirectoryScanner generatedSourcesDirectoryScanner = new DirectoryScanner();

/**
* 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the {@link #beforeRebuildExecution(org.apache.maven.shared.incremental.IncrementalBuildHelperRequest)} is
* called, this contains the list of files in the build directory.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping

* called, this will contain the list of files in the generated sources directory.
*/
private String[] generatedSourcesBeforeAction = new String[0];

public IncrementalBuildHelper( MojoExecution mojoExecution, MavenSession mavenSession )
{
this( mojoExecution, getMavenProject( mavenSession ) );
Expand Down Expand Up @@ -112,11 +131,6 @@ private static MavenProject getMavenProject( MavenSession mavenSession )
*/
public DirectoryScanner getDirectoryScanner()
{
if ( directoryScanner == null )
{
directoryScanner = new DirectoryScanner();
}

return directoryScanner;
}

Expand Down Expand Up @@ -280,34 +294,32 @@ public String[] beforeRebuildExecution( IncrementalBuildHelperRequest incrementa
throws MojoExecutionException
{
File mojoConfigBase = getMojoStatusDirectory();
File mojoConfigFile = new File( mojoConfigBase, CREATED_FILES_LST_FILENAME );
File mojoCreatedFiles = new File( mojoConfigBase, CREATED_FILES_LST_FILENAME );
File mojoGeneratedFile = new File( mojoConfigBase, GENERATED_FILES_LST_FILENAME );

String[] oldFiles;
List<String> deletedFiles = new ArrayList<>();

try
{
oldFiles = FileUtils.fileReadArray( mojoConfigFile );
for ( String oldFileName : oldFiles )
{
File oldFile = new File( incrementalBuildHelperRequest.getOutputDirectory(), oldFileName );
oldFile.delete();
}
deletedFiles.addAll(
deleteFiles( mojoCreatedFiles,
incrementalBuildHelperRequest.getOutputDirectory() ) );
deletedFiles.addAll(
deleteFiles( mojoGeneratedFile,
incrementalBuildHelperRequest.getGeneratedSourcesDirectory() ) );
}
catch ( IOException e )
{
throw new MojoExecutionException( "Error reading old mojo status", e );
}

// we remember all files which currently exist in the output directory
DirectoryScanner diffScanner = getDirectoryScanner();
diffScanner.setBasedir( incrementalBuildHelperRequest.getOutputDirectory() );
if ( incrementalBuildHelperRequest.getOutputDirectory().exists() )
{
diffScanner.scan();
filesBeforeAction = diffScanner.getIncludedFiles();
}
filesBeforeAction = scanDirectory( getDirectoryScanner(),
incrementalBuildHelperRequest.getOutputDirectory() );
generatedSourcesBeforeAction = scanDirectory( generatedSourcesDirectoryScanner,
incrementalBuildHelperRequest.getGeneratedSourcesDirectory() );

return oldFiles;
return deletedFiles.toArray( new String[0] );
}

/**
Expand All @@ -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(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These getter methods don't seem to help. They're an unnecessary layer of indirection. Just use the constructor directly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yeah I was just going for consistency with the existing API!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That only makes sense if the existing API is correct. Consistency with bad code is not a goal.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to call a getter method in the same class, just use the directoryScanner field directly

filesBeforeAction,
mojoConfigBase,
CREATED_FILES_LST_FILENAME );

writeChangedFiles(
generatedSourcesDirectoryScanner,
generatedSourcesBeforeAction,
mojoConfigBase,
GENERATED_FILES_LST_FILENAME );

// in case of clean compile the file is not created so next compile won't see it
// we mus create it here
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mus --> must

File mojoConfigFile;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declare on same line as initialize

mojoConfigFile = new File( mojoConfigBase, INPUT_FILES_LST_FILENAME );
if ( !mojoConfigFile.exists() )
{
Expand All @@ -358,6 +368,79 @@ public void afterRebuildExecution( IncrementalBuildHelperRequest incrementalBuil

}

private void writeChangedFiles(
DirectoryScanner directoryScanner,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a field in the same class so no need to pass it as an argument

String[] filesBeforeAction,
File mojoConfigBase,
String createdFilesListFileName ) throws MojoExecutionException
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a constant in the same class so no need to pass it as an argument

{
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 )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a constant in the same class so no need to pass it as an argument

throws IOException
{
File createdFiles = new File( mojoConfigBase, createdFilesListFileName );
String[] filesAdded = outputDirectoryScanResult.getFilesAdded();
String filesAddedAsString = Stream.of( filesAdded ).collect( Collectors.joining( "\n" ) );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use String.join or StringJoiner here


Files.write(
createdFiles.toPath(),
filesAddedAsString.getBytes( StandardCharsets.UTF_8 ),
StandardOpenOption.CREATE );
}

private static DirectoryScanResult scanDirectoryDiff(
DirectoryScanner directoryScanner,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make non-static and use the directoryScanner field

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();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if this method returns false? i.e. delete failed. Do we care?

}
return oldFiles;
}

private String[] scanDirectory( DirectoryScanner directoryScanner, File directory )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make non-static and use the directoryScanner field

{
directoryScanner.setBasedir( directory );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ( directory != null && directory.exists() )
{
directoryScanner.scan();
return directoryScanner.getIncludedFiles();
}
return new String[0];
}

private String[] toArrayOfPath( Set<File> files )
{
return ( files == null || files.isEmpty() )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ public class IncrementalBuildHelperRequest

private File outputDirectory;

private File generatedSourcesDirectory;

public IncrementalBuildHelperRequest()
{
// no op
Expand Down Expand Up @@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File outputDirectory )
this.outputDirectory = outputDirectory;
return this;
}

public File getGeneratedSourcesDirectory()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs Javadoc that clearly explains what this directory is.

{
return generatedSourcesDirectory;
}

public void setGeneratedSourcesDirectory( File generatedSourcesDirectory )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs Javadoc

{
this.generatedSourcesDirectory = generatedSourcesDirectory;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth checking here that the file exists and is a directory? Throw an IllegalArgumentException if not.

}

public IncrementalBuildHelperRequest generatedSourcesDirectory( File generatedSourcesDirectory )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs Javadoc that clearly explains what this directory is.

{
this.generatedSourcesDirectory = generatedSourcesDirectory;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth checking here that the file exists and is a directory?

return this;
}
}