Copilot commented on code in PR #1002:
URL:
https://github.com/apache/maven-compiler-plugin/pull/1002#discussion_r2597145327
##########
src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java:
##########
@@ -577,31 +630,52 @@ public boolean compile(final JavaCompiler compiler, final
Options configuration,
if (!generatedSourceDirectories.isEmpty()) {
fileManager.setLocationFromPaths(StandardLocation.SOURCE_OUTPUT,
generatedSourceDirectories);
}
+ final String moduleNameFromPackageHierarchy =
moduleNameFromPackageHierarchy();
Review Comment:
The variables `isClasspathProject` and `isModularProject` are declared
outside the outer loop but are set inside the nested loop (lines 669-671, 674).
Once set to `true`, they remain `true` for all subsequent iterations of the
outer loop. This could cause incorrect behavior when compiling multi-release
projects where different release versions might have different module
configurations. These variables should either be reset at the start of each
outer loop iteration or their scope should be reconsidered.
```suggestion
final String moduleNameFromPackageHierarchy =
moduleNameFromPackageHierarchy();
// These variables must be reset for each release version
iteration to avoid stale values.
```
##########
src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java:
##########
@@ -453,30 +500,36 @@ private void setDependencyPaths(final
StandardJavaFileManager fileManager) throw
* @param first the path to put first
* @return the new paths for the given type, as a modifiable list
*/
- protected List<Path> prependDependency(final PathType pathType, final Path
first) {
- List<Path> paths = dependencies(pathType);
- paths.add(0, first);
+ protected Deque<Path> prependDependency(final PathType pathType, final
Path first) {
+ Deque<Path> paths = dependencies(pathType);
+ paths.addFirst(first);
return paths;
}
/**
- * Ensures that the given value is non-null, replacing null values by the
latest version.
+ * Removes the first <var>n</var> elements of the given collection.
+ * This is used for removing {@code --patch-module} items that were added
as source directories.
+ * The callers should replace the removed items by the output directory of
these source files.
+ *
+ * @param paths the paths from which to remove the first elements
+ * @param count number of elements to remove, or {@code null} if none
+ * @return whether at least one item has been removed
*/
- private static SourceVersion nonNullOrLatest(SourceVersion release) {
- return (release != null) ? release : SourceVersion.latest();
+ private static boolean removeFirsts(Deque<Path> paths, Integer count) {
+ boolean changed = false;
+ if (count != null) {
+ for (int i = count; --i >= 9; ) {
Review Comment:
The loop condition `--i >= 9` appears to be a typo and should be `--i >= 0`.
This bug would cause the method to only remove elements when `count` is 10 or
greater, and even then it would skip removing the first 9 elements. This would
break the `--patch-module` path replacement logic.
```suggestion
for (int i = count; --i >= 0; ) {
```
##########
src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java:
##########
@@ -611,59 +685,97 @@ public boolean compile(final JavaCompiler compiler, final
Options configuration,
fileManager.setLocationFromPaths(StandardLocation.SOURCE_PATH, sourcePaths);
} else {
fileManager.setLocationForModule(StandardLocation.MODULE_SOURCE_PATH,
moduleName, sourcePaths);
+ modulesNotPresentInNewVersion.put(moduleName,
Boolean.FALSE);
}
- outputForRelease = outputDirectory; // Modified below if
compiling a non-base release.
- if (isVersioned) {
- outputForRelease =
Files.createDirectories(SourceDirectory.outputDirectoryForReleases(
- isModularProject, outputForRelease,
unit.release));
- if (isClasspathProject) {
- /*
- * For a non-modular project, this block is
executed at most once par compilation unit.
- * Add the paths to the classes compiled for
previous versions.
- */
- List<Path> classpath =
prependDependency(JavaPathType.CLASSES, latestOutputDirectory);
-
fileManager.setLocationFromPaths(StandardLocation.CLASS_PATH, classpath);
- } else {
- /*
- * For a modular project, this block can be
executed an arbitrary number of times
- * (once per module).
- */
- Path latestOutputForModule =
latestOutputDirectory.resolve(moduleName);
- JavaPathType.Modular pathType =
JavaPathType.patchModule(moduleName);
- List<Path> paths = prependDependency(pathType,
latestOutputForModule);
+ /*
+ * When compiling for the base Java version, the
configuration for current module is finished.
+ * The remaining of this loop is executed only for target
Java versions after the base version.
+ * In those cases, we need to add the paths to the classes
compiled for the previous version.
+ * For a non-modular project, always add the paths to the
class-path. For a modular project,
+ * add the paths to the module-path only the first time.
After, we need to use patch-module.
+ */
+ if (latestOutputDirectory != null) {
+ if (canAddLatestOutputToPath) {
+ canAddLatestOutputToPath = isClasspathProject; //
For next iteration.
+ JavaPathType pathType = isClasspathProject ?
JavaPathType.CLASSES : JavaPathType.MODULES;
+ Deque<Path> paths = prependDependency(pathType,
latestOutputDirectory);
+
fileManager.setLocationFromPaths(pathType.location().get(), paths);
+ }
+ /*
+ * For a modular project, following block can be
executed an arbitrary number of times
+ * We need to declare that the sources that we are
compiling are for patching a module.
+ * But we also need to remember that these sources
will need to be removed in the next
+ * iteration, because they will be replaced by the
compiled classes (the above block).
+ */
+ if (isModularProject) {
+ final Deque<Path> paths =
dependencies(JavaPathType.patchModule(moduleName));
+ removeFirsts(paths,
modulesWithSourcesAsPatches.put(moduleName, sourcePaths.size()));
+ Path latestOutput =
resolveModuleOutputDirectory(latestOutputDirectory, moduleName);
+ if (Files.exists(latestOutput)) {
+ paths.addFirst(latestOutput);
+ }
+ sourcePaths.forEach(paths::addFirst);
fileManager.setLocationForModule(StandardLocation.PATCH_MODULE_PATH,
moduleName, paths);
}
}
}
+ /*
+ * If there are any module compiled for the previous Java
version which are not compiled again
+ * for the current version, we need to clear the source paths
which were declared for leftover
+ * modules.
+ */
+ for (var iterator =
modulesNotPresentInNewVersion.entrySet().iterator(); iterator.hasNext(); ) {
+ Map.Entry<String, Boolean> entry = iterator.next();
+ if (entry.getValue()) {
+ String moduleName = entry.getKey();
+ Deque<Path> paths =
dependencies(JavaPathType.patchModule(moduleName));
+ if (removeFirsts(paths,
modulesWithSourcesAsPatches.remove(moduleName))) {
+
paths.addFirst(latestOutputDirectory.resolve(moduleName));
+ } else if (paths.isEmpty()) {
+ // Not sure why the following is needed, but it
has been observed in real projects.
+ paths.add(outputDirectory.resolve(moduleName));
+ }
+
fileManager.setLocationForModule(StandardLocation.PATCH_MODULE_PATH,
moduleName, paths);
+
fileManager.setLocationForModule(StandardLocation.MODULE_SOURCE_PATH,
moduleName, Set.of());
+ iterator.remove();
+ } else {
+ entry.setValue(Boolean.TRUE); // For compilation of
next target version (if any).
+ }
+ }
/*
* At this point, we finished to set the source paths. We have
also modified the class-path or
* patched the modules with the output directories of codes
compiled for lower Java releases.
- * The `defaultModuleName` is an adjustment done when the
project is a Java module, but still
- * organized in a package source hierarchy instead of a module
source hierarchy. Updating the
- * `unit.roots` map is not needed for this class, but done in
case a `target/javac.args` file
- * will be written after the compilation.
+ * The adjustment done below if for when the project is a Java
module, but still organized in
Review Comment:
Typo in comment: "if for when" should be "is for when".
```suggestion
* The adjustment done below is for when the project is a
Java module, but still organized in
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
