-
Notifications
You must be signed in to change notification settings - Fork 8
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
base: master
Are you sure you want to change the base?
Conversation
22d7211
to
3eef81a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is separate component we need separate jira issue 😄 to correctly build a release notes.
I know that unit tests are missing here ... can you try add first one for your change?
/** | ||
* 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping
} | ||
|
||
/** | ||
* Set the DirectoryScanner which shall get used by this build helper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which shall get used by this build helper --> that scans the target/generated sources directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops yeah just copy/pasted the javadocs. I will update them.
* or create new a DirectoryScanner if none is yet set. | ||
* The DirectoryScanner is used for detecting changes in a directory | ||
*/ | ||
public DirectoryScanner getGeneratedSourcesDirectoryScanner() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these methods need to be public? It looks like they can be package protected/default access
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are copy-pasted from the getDirectoryScanner(). It isn't clear to me why that one is public. I made this one public to be consistent.
* Set the DirectoryScanner which shall get used by this build helper. | ||
* @param generatedSourcesDirectoryScanner | ||
*/ | ||
public void setGeneratedSourcesDirectoryScanner( DirectoryScanner generatedSourcesDirectoryScanner ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why these methods (or this class for that matter) is needed. It seems a directory scanner could be easily created without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I agree entirely with you that this class seems pointless. But I have no idea who uses it so it doesn't seem safe to change? Or would you prefer that I delete/inline this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though this class is literally the only thing in maven-shared-incremental
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change existing public methods in this PR, but also don't compound problems by introducing new ones we don't need.
throw new MojoExecutionException( "Error while storing the mojo status", e ); | ||
} | ||
writeChangedFiles( | ||
getDirectoryScanner(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
|
||
// in case of clean compile the file is not created so next compile won't see it | ||
// we mus create it here | ||
File mojoConfigFile; |
There was a problem hiding this comment.
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
DirectoryScanner directoryScanner, | ||
String[] filesBeforeAction, | ||
File mojoConfigBase, | ||
String createdFilesLstFilename ) throws MojoExecutionException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lst --> List
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
private static void writeChangedFiles( | ||
DirectoryScanResult outputDirectoryScanResult, | ||
File mojoConfigBase, | ||
String createdFilesLstFilename ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lst --> List
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
throws IOException | ||
{ | ||
File createdFiles = new File( mojoConfigBase, createdFilesLstFilename ); | ||
FileUtils.fileWriteArray( createdFiles, outputDirectoryScanResult.getFilesAdded() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is deprecated. Use
java.nio.files.Files.write(file.toPath(), data.getBytes(encoding), StandardOpenOption.CREATE)
and specify UTF-8 encoding
|
||
private static String[] deleteFiles( File fileNameIndex, File parent ) throws IOException | ||
{ | ||
String[] oldFiles = FileUtils.fileReadArray( fileNameIndex ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deprecated. use java.nio.files.Files.readAllLines() instead and specify UTF-8
Should that still be MCOMPILER?
Uhhhhhhhh... I'm not sure it is practical for me to write the unit tests for this file. If you are able to whip together a quick one I can add one for this change? |
…ctory The generatedSourcesDirectory (by default target/generated-sources/annotations) contains source code generated by annotation processors. These generated sources are also outputs of the compiler, and thus should be cleaned along with the class files in the outputDirectory, in order to correctly do incremental compilations. If this isn't done, then javac may not correctly compile the generated code, since it was there before. See https://docs.oracle.com/en/java/javase/17/docs/specs/man/javac.html#annotation-processing See https://github.com/marquiswang/incremental-compile-dagger-test for a reproducing example.
@elharo please review again |
@@ -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. |
There was a problem hiding this comment.
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?
private DirectoryScanner directoryScanner = new DirectoryScanner(); | ||
|
||
/** | ||
* Used for detecting changes in the generated sources directory between the Mojo execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
/** | ||
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping
throw new MojoExecutionException( "Error while storing the mojo status", e ); | ||
} | ||
writeChangedFiles( | ||
getDirectoryScanner(), |
There was a problem hiding this comment.
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
DirectoryScanner directoryScanner, | ||
String[] filesBeforeAction, | ||
File mojoConfigBase, | ||
String createdFilesListFileName ) throws MojoExecutionException |
There was a problem hiding this comment.
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
|
||
public void setGeneratedSourcesDirectory( File generatedSourcesDirectory ) | ||
{ | ||
this.generatedSourcesDirectory = generatedSourcesDirectory; |
There was a problem hiding this comment.
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 ) | ||
{ | ||
this.generatedSourcesDirectory = generatedSourcesDirectory; |
There was a problem hiding this comment.
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?
@@ -73,4 +75,20 @@ public IncrementalBuildHelperRequest outputDirectory( File outputDirectory ) | |||
this.outputDirectory = outputDirectory; | |||
return this; | |||
} | |||
|
|||
public File getGeneratedSourcesDirectory() |
There was a problem hiding this comment.
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 ) |
There was a problem hiding this comment.
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; | ||
} | ||
|
||
public IncrementalBuildHelperRequest generatedSourcesDirectory( File generatedSourcesDirectory ) |
There was a problem hiding this comment.
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.
The generatedSourcesDirectory (by default
target/generated-sources/annotations) contains source code generated by annotation processors. These generated sources are also outputs of the compiler, and thus should be cleaned along with the class files in the outputDirectory, in order to correctly do incremental compilations.
If this isn't done, then javac may not correctly compile the generated code, since it was there before. See
https://docs.oracle.com/en/java/javase/17/docs/specs/man/javac.html#annotation-processing
See https://github.com/marquiswang/incremental-compile-dagger-test for a reproducing example.