-
Notifications
You must be signed in to change notification settings - Fork 181
Various bug fixes in the handling of --patch-module
#1002
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
base: master
Are you sure you want to change the base?
Conversation
--patch-module
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.
Pull request overview
This pull request refactors the handling of --patch-module options to fix several bugs in multi-release and multi-module Maven projects. The changes eliminate a workaround that temporarily deleted module-info.class files during test compilation and improve module path resolution by scanning output directories instead of relying solely on source elements.
Key changes:
- Refactored module patching logic to properly track and replace source directories with compiled output directories across multiple compilation phases
- Changed dependency collections from
ListtoDeque/Collectionto support efficient prepending/removal operations needed for patch management - Improved module directory scanning to handle modules that exist only in higher Java version directories
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| WorkaroundForPatchModule.java | Changed to LinkedHashMap/LinkedHashSet for predictable iteration order; simplified setLocationForModule logic |
| ToolExecutorForTest.java | Major refactoring: removed hack for deleting module-info.class; added methods to scan main output directories for modules; improved module path resolution |
| ToolExecutor.java | Changed dependencies from List to Deque; added methods for handling previous phase outputs and module directory resolution; refactored compile method to properly handle patch-module across versions |
| TestCompilerMojo.java | Simplified createExecutor to pass mainModulePath as parameter; renamed getTestModuleName to moduleNameFromPackageHierarchy |
| SourcesForRelease.java | Updated dependency type from List to Collection |
| SourceDirectory.java | Made parse method package-private; added META_INF constant |
| ForkedToolSources.java | Added isAbsent helper method and moduleSources factory method |
| DiagnosticLogger.java | Added null check for directory field before relativizing paths |
| CompilationTaskSources.java | Deleted entire file as no longer needed after removing the module-info compilation workaround |
| AbstractCompilerMojo.java | Updated method signatures to use Collection instead of List |
| Test files | Updated assertions and removed toolchain requirements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugin/compiler/ToolExecutor.java
Outdated
Show resolved
Hide resolved
…multi-release projects. * Fix a compilation errors when a module exists only for a Java version higher than the base version. * Add the base version in the module path for avoiding recompilation of classes of the base version. * Determine the modules to patch by scanning the output directory instead of the <source> elements. * Change dependency collections from List to Deque for efficient prepending/removal operations. * Enable a test which was previously disabled because of toolchain.
…ss` when overwriting that file. It stay not recommended (compiler options should be used instead), but the new way that options such as `--patch-module` are set seem to avoid this need.
Bug fixes in the handling of
--patch-moduleoption for tests and for multi-release projects, in particular when used in multi-module projects. The main issues fixed by this pull request are:<source>elements. It allows to take in account the outputs of generated sources (for example).module-infoin test replaces the mainmodule-info(I advise against this practice, but we nevertheless need to support that for compatibility reasons), remove the hack that consisted in temporarily deleting themodule-info.classfile of the main class.Those bug fixes are resolved by a refactoring of the code that handle
--patch-module. The new code uses these options more properly. The removal of the hack mentioned in the last above point became possible as a side-effect of this cleanup.Instead of adding a new test case, this pull request modifies an existing one for doing more checks. It also enables another test which was previously disabled because of toolchain. This is also tested against a real project.