You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/12/17 01:42:32 UTC
[GitHub] [maven-compiler-plugin] ccorsi opened a new pull request #36: [MCOMPILER-447] Unable to compile modularized test for a multi-version jar that is not a java module by default
ccorsi opened a new pull request #36:
URL: https://github.com/apache/maven-compiler-plugin/pull/36
The following changes resolves two different issues. The first is the case that
the we are creating a multi-version jar file that by default is not modularized.
If tests are created that will be testing the modularized part of the multi-version
jar file. Then the build was generating an UnsupportedOperationException because
it assumed that we were using a non-modularized jar file with a modularized test.
The second issue was related to the fact that the test are using dependency jars
that are not modularized. The generated javac command was only producing the
required module path and nulled out the classpath. This was causing the building
of the test sources to fail. The was caused because those non-modularized jar
files were not part of the classpath.
The current set of changes solves the first issue by determining if the project
jar file is a multi-version jar file. If it is, it will then set the main module
descriptor to the latest version less than or equal to the defined release
attribute.
It solves the second issue by adding all non-modularized jar files to the
class path. It does not remove them from the defined module path to be
consistent with how it was working.
Finally an integration test was created that reproduced the above issues.
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] 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.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[MPH-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MPH-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.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] 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.
- [ ] 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)
- [ ] 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] bmarwell commented on a change in pull request #36: [MCOMPILER-447] Unable to compile modularized test for a multi-version jar that is not a java module by default
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #36:
URL: https://github.com/apache/maven-compiler-plugin/pull/36#discussion_r545168225
##########
File path: src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
##########
@@ -27,11 +27,13 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
+import java.util.Iterator;
Review comment:
Please fix this breaking issue:
`Error: src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java:[30,8] (imports) UnusedImports: Unused import - java.util.Iterator.`
##########
File path: src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
##########
@@ -319,7 +352,42 @@ else if ( Double.valueOf( getTarget() ) < Double.valueOf( MODULE_INFO_TARGET ) )
if ( testModuleDescriptor != null )
{
modulepathElements = testPath;
- classpathElements = Collections.emptyList();
+ classpathElements = null;
+ for ( String path : modulepathElements )
+ {
+ File file = new File( path );
+ if ( file.exists() && ! file.isDirectory() && file.getName().endsWith( ".jar" ) )
+ {
+ // check if the jar file contains the module-info.class
+ try
+ {
+ JarFile jarFile = new JarFile( file );
+ if ( jarFile.getJarEntry( "module-info.class" ) == null )
+ {
+ if ( getLog().isDebugEnabled() )
+ {
+ getLog().debug( "Add non-module jar dependency " + path
+ + " to class path." );
+ }
+ if ( classpathElements == null )
+ {
+ classpathElements = new ArrayList<String>();
Review comment:
Please use the diamond operator available in Java 7.
##########
File path: src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
##########
@@ -319,7 +352,42 @@ else if ( Double.valueOf( getTarget() ) < Double.valueOf( MODULE_INFO_TARGET ) )
if ( testModuleDescriptor != null )
{
modulepathElements = testPath;
- classpathElements = Collections.emptyList();
+ classpathElements = null;
+ for ( String path : modulepathElements )
+ {
+ File file = new File( path );
+ if ( file.exists() && ! file.isDirectory() && file.getName().endsWith( ".jar" ) )
+ {
+ // check if the jar file contains the module-info.class
+ try
+ {
+ JarFile jarFile = new JarFile( file );
+ if ( jarFile.getJarEntry( "module-info.class" ) == null )
+ {
+ if ( getLog().isDebugEnabled() )
+ {
+ getLog().debug( "Add non-module jar dependency " + path
+ + " to class path." );
+ }
+ if ( classpathElements == null )
+ {
+ classpathElements = new ArrayList<String>();
+ }
+ // add the path to the classpathElements list.
+ classpathElements.add( path );
+ }
Review comment:
Can you refactor the code a bit? Too many levels of indentation.
##########
File path: src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
##########
@@ -227,6 +229,37 @@ protected void preparePaths( Set<File> sourceFiles )
}
}
+ // It is possible that we are building a multi-version jar file in which the main classes are not module based.
+ if ( ! mainModuleDescriptorClassFile.exists() && release != null )
+ {
+ int version = Integer.valueOf( release );
+ while ( version >= 9 )
+ {
+ String releaseOutputDirectory = String.format( "%s%sMETA-INF%sversions%s%d",
+ getProject().getBuild().getOutputDirectory(), File.separator, File.separator,
+ File.separator, version );
Review comment:
Please use `File metaInfDir = new File(getProject().getBuild().getOutputDirectory(), "META-INF/versions/" + version)`. It will do the conversion to system-specific separators for you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] bmarwell commented on a change in pull request #36: [MCOMPILER-447] Unable to compile modularized test for a multi-version jar that is not a java module by default
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #36:
URL: https://github.com/apache/maven-compiler-plugin/pull/36#discussion_r546799029
##########
File path: src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
##########
@@ -319,7 +352,42 @@ else if ( Double.valueOf( getTarget() ) < Double.valueOf( MODULE_INFO_TARGET ) )
if ( testModuleDescriptor != null )
{
modulepathElements = testPath;
- classpathElements = Collections.emptyList();
+ classpathElements = null;
+ for ( String path : modulepathElements )
+ {
+ File file = new File( path );
+ if ( file.exists() && ! file.isDirectory() && file.getName().endsWith( ".jar" ) )
+ {
+ // check if the jar file contains the module-info.class
+ try
+ {
+ JarFile jarFile = new JarFile( file );
+ if ( jarFile.getJarEntry( "module-info.class" ) == null )
+ {
+ if ( getLog().isDebugEnabled() )
+ {
+ getLog().debug( "Add non-module jar dependency " + path
+ + " to class path." );
+ }
+ if ( classpathElements == null )
+ {
+ classpathElements = new ArrayList<String>();
+ }
+ // add the path to the classpathElements list.
+ classpathElements.add( path );
+ }
Review comment:
Yes that is what I had in mind. A small comment why stuff happens there would be helpful as well. I would start a little above in the "try" statement with the comment: ` // check if the jar file contains the module-info.class.`.
I think the whole try statement would make a nice method. A simple comment why the jars are explicitly added to the classpath would absolutely suffice.
Also:
`new ArrayList<String>();` -- you can just use the diamond operator: `new ArrayList<>();`. We are on Java 7, not 5. ;-)
Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] rfscholte commented on pull request #36: [MCOMPILER-447] Unable to compile modularized test for a multi-version jar that is not a java module by default
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #36:
URL: https://github.com/apache/maven-compiler-plugin/pull/36#issuecomment-913206621
MCOMPILER-447 is closed as `Won't fix`
--
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
[GitHub] [maven-compiler-plugin] ccorsi commented on a change in pull request #36: [MCOMPILER-447] Unable to compile modularized test for a multi-version jar that is not a java module by default
Posted by GitBox <gi...@apache.org>.
ccorsi commented on a change in pull request #36:
URL: https://github.com/apache/maven-compiler-plugin/pull/36#discussion_r546883926
##########
File path: src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
##########
@@ -319,7 +352,42 @@ else if ( Double.valueOf( getTarget() ) < Double.valueOf( MODULE_INFO_TARGET ) )
if ( testModuleDescriptor != null )
{
modulepathElements = testPath;
- classpathElements = Collections.emptyList();
+ classpathElements = null;
+ for ( String path : modulepathElements )
+ {
+ File file = new File( path );
+ if ( file.exists() && ! file.isDirectory() && file.getName().endsWith( ".jar" ) )
+ {
+ // check if the jar file contains the module-info.class
+ try
+ {
+ JarFile jarFile = new JarFile( file );
+ if ( jarFile.getJarEntry( "module-info.class" ) == null )
+ {
+ if ( getLog().isDebugEnabled() )
+ {
+ getLog().debug( "Add non-module jar dependency " + path
+ + " to class path." );
+ }
+ if ( classpathElements == null )
+ {
+ classpathElements = new ArrayList<String>();
+ }
+ // add the path to the classpathElements list.
+ classpathElements.add( path );
+ }
Review comment:
I went a step further, I ended up extracting the for loop into a separate method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] bmarwell commented on a change in pull request #36: [MCOMPILER-447] Unable to compile modularized test for a multi-version jar that is not a java module by default
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #36:
URL: https://github.com/apache/maven-compiler-plugin/pull/36#discussion_r546900913
##########
File path: src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
##########
@@ -319,7 +352,42 @@ else if ( Double.valueOf( getTarget() ) < Double.valueOf( MODULE_INFO_TARGET ) )
if ( testModuleDescriptor != null )
{
modulepathElements = testPath;
- classpathElements = Collections.emptyList();
+ classpathElements = null;
+ for ( String path : modulepathElements )
+ {
+ File file = new File( path );
+ if ( file.exists() && ! file.isDirectory() && file.getName().endsWith( ".jar" ) )
+ {
+ // check if the jar file contains the module-info.class
+ try
+ {
+ JarFile jarFile = new JarFile( file );
+ if ( jarFile.getJarEntry( "module-info.class" ) == null )
+ {
+ if ( getLog().isDebugEnabled() )
+ {
+ getLog().debug( "Add non-module jar dependency " + path
+ + " to class path." );
+ }
+ if ( classpathElements == null )
+ {
+ classpathElements = new ArrayList<String>();
+ }
+ // add the path to the classpathElements list.
+ classpathElements.add( path );
+ }
Review comment:
Well, now you have the same amount of indentation in a loop. But I will not complain anymore about indentation unless another committer does. ;-)
The comment in the new method is helpful. Nit: You could have added it as Javadoc while you were at it.
I will check it out locally in a few days to inspect the changes further.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] rfscholte closed pull request #36: [MCOMPILER-447] Unable to compile modularized test for a multi-version jar that is not a java module by default
Posted by GitBox <gi...@apache.org>.
rfscholte closed pull request #36:
URL: https://github.com/apache/maven-compiler-plugin/pull/36
--
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
[GitHub] [maven-compiler-plugin] ccorsi commented on a change in pull request #36: [MCOMPILER-447] Unable to compile modularized test for a multi-version jar that is not a java module by default
Posted by GitBox <gi...@apache.org>.
ccorsi commented on a change in pull request #36:
URL: https://github.com/apache/maven-compiler-plugin/pull/36#discussion_r546706179
##########
File path: src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
##########
@@ -319,7 +352,42 @@ else if ( Double.valueOf( getTarget() ) < Double.valueOf( MODULE_INFO_TARGET ) )
if ( testModuleDescriptor != null )
{
modulepathElements = testPath;
- classpathElements = Collections.emptyList();
+ classpathElements = null;
+ for ( String path : modulepathElements )
+ {
+ File file = new File( path );
+ if ( file.exists() && ! file.isDirectory() && file.getName().endsWith( ".jar" ) )
+ {
+ // check if the jar file contains the module-info.class
+ try
+ {
+ JarFile jarFile = new JarFile( file );
+ if ( jarFile.getJarEntry( "module-info.class" ) == null )
+ {
+ if ( getLog().isDebugEnabled() )
+ {
+ getLog().debug( "Add non-module jar dependency " + path
+ + " to class path." );
+ }
+ if ( classpathElements == null )
+ {
+ classpathElements = new ArrayList<String>();
+ }
+ // add the path to the classpathElements list.
+ classpathElements.add( path );
+ }
Review comment:
Not sure exactly how you want me to refactor this code. The build checkstyle definition forced me into this code style. I did combine the debug statement within a single line. The only other option is to extract this set of code and create a private method. That would reduce the indentation. Is this what you were thinking?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] ccorsi commented on a change in pull request #36: [MCOMPILER-447] Unable to compile modularized test for a multi-version jar that is not a java module by default
Posted by GitBox <gi...@apache.org>.
ccorsi commented on a change in pull request #36:
URL: https://github.com/apache/maven-compiler-plugin/pull/36#discussion_r546704328
##########
File path: src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
##########
@@ -27,11 +27,13 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
+import java.util.Iterator;
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] ccorsi commented on a change in pull request #36: [MCOMPILER-447] Unable to compile modularized test for a multi-version jar that is not a java module by default
Posted by GitBox <gi...@apache.org>.
ccorsi commented on a change in pull request #36:
URL: https://github.com/apache/maven-compiler-plugin/pull/36#discussion_r546704475
##########
File path: src/main/java/org/apache/maven/plugin/compiler/TestCompilerMojo.java
##########
@@ -319,7 +352,42 @@ else if ( Double.valueOf( getTarget() ) < Double.valueOf( MODULE_INFO_TARGET ) )
if ( testModuleDescriptor != null )
{
modulepathElements = testPath;
- classpathElements = Collections.emptyList();
+ classpathElements = null;
+ for ( String path : modulepathElements )
+ {
+ File file = new File( path );
+ if ( file.exists() && ! file.isDirectory() && file.getName().endsWith( ".jar" ) )
+ {
+ // check if the jar file contains the module-info.class
+ try
+ {
+ JarFile jarFile = new JarFile( file );
+ if ( jarFile.getJarEntry( "module-info.class" ) == null )
+ {
+ if ( getLog().isDebugEnabled() )
+ {
+ getLog().debug( "Add non-module jar dependency " + path
+ + " to class path." );
+ }
+ if ( classpathElements == null )
+ {
+ classpathElements = new ArrayList<String>();
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org