[3.9.x] MavenPluginJavaPrerequisiteChecker: Handle 8/1.8 Java version in ranges as well#11577
Conversation
Java is consistent when reporting versions, but users are not. Fixes apache#11575
| } | ||
| VersionConstraint constraint; | ||
| try { | ||
| constraint = versionScheme.parseVersionConstraint(requiredVersion.equals("8") ? "1.8" : requiredVersion); |
There was a problem hiding this comment.
two lines above you substring currentVersion here it's about requiredVersion. Is it intended or are you accidently mixing variables?
| @@ -30,7 +30,22 @@ public class MavenPluginJavaPrerequisiteCheckerTest { | |||
| public void testMatchesVersion() { | |||
There was a problem hiding this comment.
Use Parameterized tests here
There was a problem hiding this comment.
Did the change as you asked, but now fails with checkstyle:
Error: src/test/java/org/apache/maven/plugin/internal/MavenPluginJavaPrerequisiteCheckerTest.java:[67,19] (design) VisibilityModifier: Variable 'requiredVersion' must be private and have accessor methods.
Error: src/test/java/org/apache/maven/plugin/internal/MavenPluginJavaPrerequisiteCheckerTest.java:[70,19] (design) VisibilityModifier: Variable 'currentVersion' must be private and have accessor methods.
Error: src/test/java/org/apache/maven/plugin/internal/MavenPluginJavaPrerequisiteCheckerTest.java:[73,20] (design) VisibilityModifier: Variable 'expected' must be private and have accessor methods.
There was a problem hiding this comment.
Clearly the two are not aligned: as if I make field private or just non-public, I get
Cannot set parameter 'requiredVersion'. Ensure that the field 'requiredVersion' is public.
So one want this, other want that.
There was a problem hiding this comment.
do it simple, you can try to add suppress for checkstyle, by the way I'm working on move test to JUnit 5 in #11547
This reverts commit 8c00268.
slawekjaranowski
left a comment
slawekjaranowski
left a comment
There was a problem hiding this comment.
I would like to change a PR title to be more descriptive what is fixed 😄
|
What about Maven 4? Shouldn't the versions be interpreted the same? |
Maven4 is Java 17+, hence it does not need to handle 8/1.8 (both are less than 17). Maven 3 can run on Java 8 and higher, and the problem is really ONLY when it runs on Java 8 (as it reports 1.8 as version), but users are inconsistent, and may use "8" or "1.8", depending on their mood. |
cstamas
changed the title
4 participants
Java is consistent when reporting versions, but users are not. No need to go below 1.8, as Maven 3.9 is Java 8 only. This PR makes "8/1.8" work for ranges as well, not only for standalone version.
Fixes #11575