Copilot commented on code in PR #959: URL: https://github.com/apache/maven-compiler-plugin/pull/959#discussion_r2404644352
########## src/main/java/org/apache/maven/plugin/compiler/WorkaroundForPatchModule.java: ########## @@ -0,0 +1,237 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.plugin.compiler; + +import javax.tools.FileObject; +import javax.tools.ForwardingJavaFileManager; +import javax.tools.JavaFileManager; +import javax.tools.JavaFileObject; +import javax.tools.StandardJavaFileManager; +import javax.tools.StandardLocation; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +import org.apache.maven.api.JavaPathType; + +/** + * Workaround for a {@code javax.tools} method which seems not yet supported on all compilers. + * At least with OpenJDK 24, an {@link UnsupportedOperationException} may occur during the call to + * {@code fileManager.setLocationForModule(StandardLocation.PATCH_MODULE_PATH, moduleName, paths)}. + * The workaround is to format the paths in a {@code --path-module} option instead. Review Comment: Corrected 'path-module' to 'patch-module' in comment. ```suggestion * The workaround is to format the paths in a {@code --patch-module} option instead. ``` ########## src/main/java/org/apache/maven/plugin/compiler/SourcesForRelease.java: ########## @@ -74,6 +76,19 @@ final class SourcesForRelease implements Closeable { */ private SourceDirectory lastDirectoryAdded; + /** + * Snapshot of {@link ToolExecutor#dependencies}. + * This information is saved in case a {@code target/javac.args} debug file needs to be written. + */ + Map<PathType, List<Path>> dependencySnapshot; + + /** + * The output directory for the release. This is either base base output directory or a sub-directory Review Comment: Corrected duplicated word 'base base' to 'base' in comment. ```suggestion * The output directory for the release. This is either base output directory or a sub-directory ``` ########## src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java: ########## @@ -254,26 +260,21 @@ protected ToolExecutor(final AbstractCompilerMojo mojo, DiagnosticListener<? sup /* * Get the dependencies. If the module-path contains any automatic (filename-based) * dependency and the MOJO is compiling the main code, then a warning will be logged. - * - * NOTE: this code assumes that the map and the list values are modifiable. - * This code performs a deep copies for safety. They are unnecessary copies when - * the implementation is `org.apache.maven.impl.DefaultDependencyResolverResult`, - * but we assume for now that it is not worth an optimization. The copies also - * protect `dependencyResolution` from changes in `dependencies`. */ dependencyResolution = mojo.resolveDependencies(hasModuleDeclaration); if (dependencyResolution != null) { dependencies.putAll(dependencyResolution.getDispatchedPaths()); - dependencies.entrySet().forEach((e) -> e.setValue(new ArrayList<>(e.getValue()))); + copyDependencyValues(); } mojo.resolveProcessorPathEntries(dependencies); } /** - * {@return the source files to compile}. + * Copies all values of the dependency map in unmodifiable lists. + * This is used for creating a snapshot of the current state of the dependency map. */ - public Stream<Path> getSourceFiles() { - return sourceFiles.stream().map((s) -> s.file); + private void copyDependencyValues() { + dependencies.entrySet().forEach((entry) -> entry.setValue(List.copyOf(entry.getValue()))); Review Comment: Using `List.copyOf()` creates immutable lists which may cause issues later when `dependencies(PathType)` method tries to modify them. Consider using `new ArrayList<>(entry.getValue())` for consistency. ```suggestion dependencies.entrySet().forEach((entry) -> entry.setValue(new ArrayList<>(entry.getValue()))); ``` ########## src/main/java/org/apache/maven/plugin/compiler/IncrementalBuild.java: ########## @@ -624,9 +624,8 @@ String inputFileTreeChanges() throws IOException { * Each given root can be either a regular file (typically a JAR file) or a directory. * Directories are scanned recursively. * - * @param directories files or directories to scan + * @param dependencies files or directories to scan * @param fileExtensions extensions of the file to check (usually "jar" and "class") - * @param changeTime the time at which a file is considered as changed * @return {@code null} if the project does not need to be rebuilt, otherwise a message saying why to rebuild Review Comment: Parameter documentation is incomplete - missing @param for changeTime which is referenced in the implementation but removed from the method signature. ########## src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java: ########## @@ -613,8 +665,24 @@ public boolean compile(final JavaCompiler compiler, final Options configuration, JavaCompiler.CompilationTask task; for (CompilationTaskSources c : toCompilationTasks(unit)) { Iterable<? extends JavaFileObject> sources = fileManager.getJavaFileObjectsFromPaths(c.files); - task = compiler.getTask(otherOutput, fileManager, listener, configuration.options, null, sources); + StandardJavaFileManager workaround = fileManager; + boolean workaroundNeedsClose = false; + if (WorkaroundForPatchModule.ENABLED) { // Test alone for making clear everything inside is a hack. Review Comment: The comment should be clearer about the purpose. Consider: 'Check flag separately to clearly indicate this entire block is a workaround hack.' ```suggestion // Check flag separately to clearly indicate this entire block is a workaround hack for patching the module system. if (WorkaroundForPatchModule.ENABLED) { ``` ########## src/it/multirelease-with-modules/pom.xml: ########## @@ -0,0 +1,65 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ Licensed to the Apache Software Foundation (ASF) under one + ~ or more contributor license agreements. See the NOTICE file + ~ distributed with this work for additional information + ~ regarding copyright ownership. The ASF licenses this file + ~ to you under the Apache License, Version 2.0 (the + ~ "License"); you may not use this file except in compliance + ~ with the License. You may obtain a copy of the License at + ~ + ~ http://www.apache.org/licenses/LICENSE-2.0 + ~ + ~ Unless required by applicable law or agreed to in writing, + ~ software distributed under the License is distributed on an + ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + ~ KIND, either express or implied. See the License for the + ~ specific language governing permissions and limitations + ~ under the License. + --> +<project xmlns="http://maven.apache.org/POM/4.1.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.1.0 http://maven.apache.org/xsd/maven-4.1.0.xsd";> + <modelVersion>4.1.0</modelVersion> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>multirelease-with-modules</artifactId> + <version>1.0-SNAPSHOT</version> + <packaging>jar</packaging> + <name>Mulirelease with modules</name> Review Comment: Corrected spelling of 'Mulirelease' to 'Multirelease'. ```suggestion <name>Multirelease with modules</name> ``` -- 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]
