You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "jorsol (via GitHub)" <gi...@apache.org> on 2023/02/17 17:17:07 UTC
[GitHub] [maven-compiler-plugin] jorsol opened a new pull request, #181: [MCOMPILER-381] - Refactor incremental detection
jorsol opened a new pull request, #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181
Following this checklist to help us incorporate your
contribution quickly and easily:
- [X] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MCOMPILER) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [X] Format the pull request title like `[MCOMPILER-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MCOMPILER-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [X] You have run the integration tests successfully (`mvn -Prun-its clean verify`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [X] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#issuecomment-1843238062
Well, I think this should be ready to be merged if there is no other comment.
BTW, looking at the open issues, this should also fix [MCOMPILER-333](https://issues.apache.org/jira/browse/MCOMPILER-333) so it can be linked to this and declare it fixed for 3.12.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#issuecomment-1844892061
> `mvn spotless:apply` should help here ;)
Done! 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404817879
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1539,36 +1546,41 @@ private static List<String> removeEmptyCompileSourceRoots(List<String> 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 <code>true</code> 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) {
+ private boolean isDependencyChanged() {
Review Comment:
In any case, I don't mind reverting those methods to protected if you like, it doesn't make a difference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#issuecomment-1835652536
@olamy - any more remarks on it, I would like to merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404865372
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1577,31 +1589,35 @@ 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(getFileExtension(file))) {
+ try {
+ Instant lastModifiedTime =
+ Files.getLastModifiedTime(file).toInstant().truncatedTo(ChronoUnit.MILLIS);
+ boolean isChanged = lastModifiedTime.isAfter(buildStartTime);
+ if (isChanged && (getLog().isDebugEnabled() || showCompilationChanges)) {
+ getLog().info("\tNew dependency detected: " + file.toAbsolutePath());
+ }
+ return isChanged;
+ } 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());
}
}
return false;
}
+ private static String getFileExtension(Path path) {
Review Comment:
Don't we have already some plexus utils for that? Or the dependency have been removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#issuecomment-1839543867
Hi @olamy, is there any other feedback?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404816887
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1400,10 +1406,11 @@ protected int getRequestThreadCount() {
return session.getRequest().getDegreeOfConcurrency();
}
- protected Date getBuildStartTime() {
- MavenExecutionRequest request = session.getRequest();
- Date buildStartTime = request == null ? new Date() : request.getStartTime();
- return buildStartTime == null ? new Date() : buildStartTime;
+ private Optional<Instant> getBuildStartTime() {
Review Comment:
try a github search and you will see https://github.com/search?q=extends+AbstractCompilerMojo&type=code
Even if it's maybe a bad idea this was not private from start but it is what it is we have to live (almost forever0 with that.
by the way there is no problem marking those methods as deprecated and make them using the private one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404815615
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1539,36 +1546,41 @@ private static List<String> removeEmptyCompileSourceRoots(List<String> 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 <code>true</code> 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) {
+ private boolean isDependencyChanged() {
Review Comment:
Same, this should have never been protected, is an internal method.
A good practice is to always start with the most restricted modifier (private) and then if needed open it to package-protected, then protected and finally public.
Again, this should not break anything since this class is not intended to be subclassed externally (unless I'm missing a weird use case and maven plugins can be used as public APIs).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404864669
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1156,6 +1151,16 @@ public void execute() throws MojoExecutionException, CompilationFailureException
incrementalBuildHelper.beforeRebuildExecution(incrementalBuildHelperRequest);
+ // Cleanup the generated sources directory
+ if (getGeneratedSourcesDirectory() != null) {
Review Comment:
Oh right the method name or even field is kind of wrong but we can't change that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1406583464
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -876,37 +874,33 @@ 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.
+ Supplier<String> immutableOutputFile = () -> (compiler.getCompilerOutputStyle()
+ .equals(CompilerOutputStyle.ONE_OUTPUT_FILE_FOR_ALL_INPUT_FILES)
+ && !canUpdateTarget)
+ ? "immutable single output file"
+ : null;
+ Supplier<String> dependencyChanged = () -> isDependencyChanged() ? "changed dependency" : null;
+ Supplier<String> sourceChanged =
+ () -> isSourceChanged(compilerConfiguration, compiler) ? "changed source code" : null;
+ Supplier<String> inputFileTreeChanged =
+ () -> hasInputFileTreeChanged(computeInputFileTreeChanges(incrementalBuildHelper, sources))
+ ? "added or removed source files"
+ : null;
+
+ // Lazy evaluation of the incremental compilation detection.
+ String cause = Stream.of(immutableOutputFile, dependencyChanged, sourceChanged, inputFileTreeChanged)
Review Comment:
Will need to undo the lazy evaluation, I plan to make more changes to the dependency detection and I need to store the status of the detection, if I lazily run this it will not execute and store the file and that means that the next execution could be recompiled again. 😞 Also the inputFileTreeChanged is affected since this already stores the state of files, running this lazily will just postpone the run in a second or third run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy merged PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1372383137
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -876,37 +874,33 @@ 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.
+ Supplier<String> immutableOutputFile = () -> (compiler.getCompilerOutputStyle()
+ .equals(CompilerOutputStyle.ONE_OUTPUT_FILE_FOR_ALL_INPUT_FILES)
+ && !canUpdateTarget)
+ ? "immutable single output file"
+ : null;
+ Supplier<String> dependencyChanged = () -> isDependencyChanged() ? "changed dependency" : null;
+ Supplier<String> sourceChanged =
+ () -> isSourceChanged(compilerConfiguration, compiler) ? "changed source code" : null;
+ Supplier<String> inputFileTreeChanged =
+ () -> hasInputFileTreeChanged(computeInputFileTreeChanges(incrementalBuildHelper, sources))
+ ? "added or removed source files"
+ : null;
+
+ // Lazy evaluation of the incremental compilation detection.
+ String cause = Stream.of(immutableOutputFile, dependencyChanged, sourceChanged, inputFileTreeChanged)
Review Comment:
+1 for lazy evaluation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404881234
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1577,31 +1589,35 @@ 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(getFileExtension(file))) {
+ try {
+ Instant lastModifiedTime =
+ Files.getLastModifiedTime(file).toInstant().truncatedTo(ChronoUnit.MILLIS);
+ boolean isChanged = lastModifiedTime.isAfter(buildStartTime);
+ if (isChanged && (getLog().isDebugEnabled() || showCompilationChanges)) {
+ getLog().info("\tNew dependency detected: " + file.toAbsolutePath());
+ }
+ return isChanged;
+ } 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());
}
}
return false;
}
+ private static String getFileExtension(Path path) {
Review Comment:
Plexus-utils is not included, there is one in maven-shared-utils but it's deprecated in favor of commons-io, yet commons-io is an indirect dependency, so we can make commons-io a direct dependency for that single method or simply duplicate the method here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#issuecomment-1843931085
> > Well, I think this should be ready to be merged if there is no other comment.
> > BTW, looking at the open issues, this should also fix [MCOMPILER-333](https://issues.apache.org/jira/browse/MCOMPILER-333) so it can be linked to this and declare it fixed for 3.12.0
>
> it's usually better to have a PR per change, especially in case of non-related changes. But as you already hijacked this PR #186 here, it would be good to have this in the PR title and commits for history. And obviously, update https://issues.apache.org/jira/browse/MCOMPILER-333
Sure, will create a new PR for that change. the fix is simpler than the one from #186.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#issuecomment-1843943578
@jorsol thanks for creating another PR with the other change!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#issuecomment-1780080413
@khmarbaise looks like you reported issue which this PR try to fix ... can you look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404812699
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1400,10 +1406,11 @@ protected int getRequestThreadCount() {
return session.getRequest().getDegreeOfConcurrency();
}
- protected Date getBuildStartTime() {
- MavenExecutionRequest request = session.getRequest();
- Date buildStartTime = request == null ? new Date() : request.getStartTime();
- return buildStartTime == null ? new Date() : buildStartTime;
+ private Optional<Instant> getBuildStartTime() {
Review Comment:
I might be missing something, but I don't expect that anyone extend from AbstractCompilerMojo externally, I have never seen maven plugins as public APIs that can be extended, in other words protected works for subclasses that are on a different package, so this should not break anything.
The only consumer is m-compiler-p itself, and even if this method is used in a subclass it should be package-protected (no modifier) since the subclasses CompilerMojo and TestCompilerMojo are on the same package.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404816887
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1400,10 +1406,11 @@ protected int getRequestThreadCount() {
return session.getRequest().getDegreeOfConcurrency();
}
- protected Date getBuildStartTime() {
- MavenExecutionRequest request = session.getRequest();
- Date buildStartTime = request == null ? new Date() : request.getStartTime();
- return buildStartTime == null ? new Date() : buildStartTime;
+ private Optional<Instant> getBuildStartTime() {
Review Comment:
try a github search and you will see https://github.com/search?q=extends+AbstractCompilerMojo&type=code
Even if it's maybe a bad idea this was not private from start but it is what it is we have to live (almost forever0 with that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404817131
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1539,36 +1546,41 @@ private static List<String> removeEmptyCompileSourceRoots(List<String> 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 <code>true</code> 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) {
+ private boolean isDependencyChanged() {
Review Comment:
same comment as here https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404816887
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1406583464
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -876,37 +874,33 @@ 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.
+ Supplier<String> immutableOutputFile = () -> (compiler.getCompilerOutputStyle()
+ .equals(CompilerOutputStyle.ONE_OUTPUT_FILE_FOR_ALL_INPUT_FILES)
+ && !canUpdateTarget)
+ ? "immutable single output file"
+ : null;
+ Supplier<String> dependencyChanged = () -> isDependencyChanged() ? "changed dependency" : null;
+ Supplier<String> sourceChanged =
+ () -> isSourceChanged(compilerConfiguration, compiler) ? "changed source code" : null;
+ Supplier<String> inputFileTreeChanged =
+ () -> hasInputFileTreeChanged(computeInputFileTreeChanges(incrementalBuildHelper, sources))
+ ? "added or removed source files"
+ : null;
+
+ // Lazy evaluation of the incremental compilation detection.
+ String cause = Stream.of(immutableOutputFile, dependencyChanged, sourceChanged, inputFileTreeChanged)
Review Comment:
Will need to undo the lazy evaluation, I plan to make more changes to the dependency detection and I need to store the status of the detection, if I lazily run this it will not execute and store the file and that means that the next execution could be recompiled again. 😞
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -876,37 +874,33 @@ 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.
+ Supplier<String> immutableOutputFile = () -> (compiler.getCompilerOutputStyle()
+ .equals(CompilerOutputStyle.ONE_OUTPUT_FILE_FOR_ALL_INPUT_FILES)
+ && !canUpdateTarget)
+ ? "immutable single output file"
+ : null;
+ Supplier<String> dependencyChanged = () -> isDependencyChanged() ? "changed dependency" : null;
+ Supplier<String> sourceChanged =
+ () -> isSourceChanged(compilerConfiguration, compiler) ? "changed source code" : null;
+ Supplier<String> inputFileTreeChanged =
+ () -> hasInputFileTreeChanged(computeInputFileTreeChanges(incrementalBuildHelper, sources))
+ ? "added or removed source files"
+ : null;
+
+ // Lazy evaluation of the incremental compilation detection.
+ String cause = Stream.of(immutableOutputFile, dependencyChanged, sourceChanged, inputFileTreeChanged)
Review Comment:
Will need to undo the lazy evaluation, I plan to make more changes to the dependency detection and I need to store the status of the detection, if I lazily run this it will not execute and store the file and that means that the next execution could be recompiled again. 😞
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#issuecomment-1843859913
> Well, I think this should be ready to be merged if there is no other comment.
>
> BTW, looking at the open issues, this should also fix [MCOMPILER-333](https://issues.apache.org/jira/browse/MCOMPILER-333) so it can be linked to this and declare it fixed for 3.12.0
it's usually better to have a PR per change, especially in case of non-related changes.
But as you already hijacked this PR https://github.com/apache/maven-compiler-plugin/pull/186 here, it would be good to have this in the PR title and commits for history.
And obviously, update https://issues.apache.org/jira/browse/MCOMPILER-333
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404858082
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1156,6 +1151,16 @@ public void execute() throws MojoExecutionException, CompilationFailureException
incrementalBuildHelper.beforeRebuildExecution(incrementalBuildHelperRequest);
+ // Cleanup the generated sources directory
+ if (getGeneratedSourcesDirectory() != null) {
Review Comment:
Yes, let me explain, the generatedSourcesDirectory is actually the directory for the annotation processor's generated source code:
https://github.com/apache/maven-compiler-plugin/blob/e5375fd10f00e4db5471bdc69ebdc4ba20f7bbdc/src/main/java/org/apache/maven/plugin/compiler/CompilerMojo.java#L114-L122
Since the "incremental" feature is actually a rebuild feature, this directory is generated by the javac compiler, it needs to be cleaned before the rebuild of the module or the build will fail with strange errors, this probably fixes many bugs related to the "incremental" compilation and annotation processors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404882575
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1577,31 +1589,35 @@ 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(getFileExtension(file))) {
+ try {
+ Instant lastModifiedTime =
+ Files.getLastModifiedTime(file).toInstant().truncatedTo(ChronoUnit.MILLIS);
+ boolean isChanged = lastModifiedTime.isAfter(buildStartTime);
+ if (isChanged && (getLog().isDebugEnabled() || showCompilationChanges)) {
+ getLog().info("\tNew dependency detected: " + file.toAbsolutePath());
+ }
+ return isChanged;
+ } 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());
}
}
return false;
}
+ private static String getFileExtension(Path path) {
Review Comment:
I don't mind to make commons-io a direct dependency so will change this ASAP.
I'm not on a computer right now, so I will need some time to make all the requested changes.
Thanks for all the feedback! 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404715210
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1539,36 +1546,41 @@ private static List<String> removeEmptyCompileSourceRoots(List<String> 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 <code>true</code> 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) {
+ private boolean isDependencyChanged() {
Review Comment:
breaking backward compat protected -> private
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1400,10 +1406,11 @@ protected int getRequestThreadCount() {
return session.getRequest().getDegreeOfConcurrency();
}
- protected Date getBuildStartTime() {
- MavenExecutionRequest request = session.getRequest();
- Date buildStartTime = request == null ? new Date() : request.getStartTime();
- return buildStartTime == null ? new Date() : buildStartTime;
+ private Optional<Instant> getBuildStartTime() {
Review Comment:
uhm you're changing a protected method. this is breaking any backward compat.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404714195
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1156,6 +1151,16 @@ public void execute() throws MojoExecutionException, CompilationFailureException
incrementalBuildHelper.beforeRebuildExecution(incrementalBuildHelperRequest);
+ // Cleanup the generated sources directory
+ if (getGeneratedSourcesDirectory() != null) {
Review Comment:
Why cleaning this directory? I don't understand the reason. Can you please explain?
reading the change as it it looks this will remove the generated-sources added via the phase `generate-sources` or any phase before compile.
Maybe I read the change in a wrong way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "jorsol (via GitHub)" <gi...@apache.org>.
jorsol commented on code in PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#discussion_r1404827601
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1400,10 +1406,11 @@ protected int getRequestThreadCount() {
return session.getRequest().getDegreeOfConcurrency();
}
- protected Date getBuildStartTime() {
- MavenExecutionRequest request = session.getRequest();
- Date buildStartTime = request == null ? new Date() : request.getStartTime();
- return buildStartTime == null ? new Date() : buildStartTime;
+ private Optional<Instant> getBuildStartTime() {
Review Comment:
Actually there are many false-positive results, searching by https://github.com/search?q=%22import+org.apache.maven.plugin.compiler.AbstractCompilerMojo%22&type=code gives a closer look of what is actually using this class, there are a few results, so yes, there are external consumers.
I will revert them to protected since it actually doesn't make a difference for what this PR is fixing.
BTW, forever is a long time, I expect this plugin and others to be rewritten someday and drop the legacy baggage ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-381] - Refactor incremental detection [maven-compiler-plugin]
Posted by "olamy (via GitHub)" <gi...@apache.org>.
olamy commented on PR #181:
URL: https://github.com/apache/maven-compiler-plugin/pull/181#issuecomment-1844352429
`mvn spotless:apply` should help here ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org