Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

MJAVADOC-485 Upgrade to commons-lang3 #119

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

MJAVADOC-485 Upgrade to commons-lang3 #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions maven-javadoc-plugin/pom.xml
View file
Open in desktop
Expand Up @@ -193,9 +193,9 @@ under the License.

<!-- misc -->
<dependency>
<groupId>commons-lang</groupId>
<artifactId>commons-lang</artifactId>
<version>2.6</version>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<version>3.5</version>
</dependency>
<dependency>
<groupId>commons-io</groupId>
Expand Down
2 changes: 1 addition & 1 deletion ...-javadoc-plugin/src/main/java/org/apache/maven/plugin/javadoc/AbstractFixJavadocMojo.java
View file
Open in desktop
Expand Up @@ -32,7 +32,7 @@
import com.thoughtworks.qdox.model.TypeVariable;
import com.thoughtworks.qdox.parser.ParseException;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.ClassUtils;
import org.apache.commons.lang3.ClassUtils;
import org.apache.maven.artifact.Artifact;
import org.apache.maven.artifact.DependencyResolutionRequiredException;
import org.apache.maven.artifact.repository.ArtifactRepository;
Expand Down
21 changes: 12 additions & 9 deletions maven-javadoc-plugin/src/main/java/org/apache/maven/plugin/javadoc/AbstractJavadocMojo.java
View file
Open in desktop
Expand Up @@ -53,8 +53,9 @@
import java.util.Set;
import java.util.StringTokenizer;

import org.apache.commons.lang.ClassUtils;
import org.apache.commons.lang.SystemUtils;
import org.apache.commons.lang3.ClassUtils;
import org.apache.commons.lang3.JavaVersion;
import org.apache.commons.lang3.SystemUtils;
import org.apache.maven.artifact.Artifact;
import org.apache.maven.artifact.factory.ArtifactFactory;
import org.apache.maven.artifact.handler.ArtifactHandler;
Expand Down Expand Up @@ -288,6 +289,8 @@ public abstract class AbstractJavadocMojo
*/
private static final float SINCE_JAVADOC_1_8 = 1.8f;

private static final float JAVA_VERSION_FLOAT = JavadocUtil.parseJavadocVersion(SystemUtils.JAVA_VERSION);
Copy link

@Tunaki Tunaki Jul 1, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises an exception when building the project on 9+175 (with mvn clean verify -Prun-its for example)

Caused by: java.util.regex.PatternSyntaxException: Unrecognized version of Javadoc: '9' near index 47
(?s).*?[^a-zA-Z]([0-9]+\.?[0-9]*)(\.([0-9]+))?.*
                                               ^
	at org.apache.maven.plugin.javadoc.JavadocUtil.parseJavadocVersion(JavadocUtil.java:674)

This method is intended to parse the output of javadoc -J-fullversion and it works for Javadoc 9+175 (output being java full version "9+175"). I think a new float parseJavaVersion(String) is needed, that works for the output of java -version. Or perhaps we should get rid of all those float comparisons and rely on Commons Lang JavaVersion (and atLeast).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tunaki Unfortunately, JavaVersion works with floats too. How will this make it any better? Moreover, on needs to extract the version string first before passing to JavaVersion.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michael-o I just noticed there is a new Runtime.Version class in Java 9 which can parse and compare Java version strings. Do you think we can use with reflection in place of JavaVersion?

But as far as upgrading to commons-lang3, having a new method float parseJavaVersion(String), that is similar to parseJavadocVersion but with the regex being ([0-9]+\\.?[0-9]*)(\\.([0-9]+)), looks like the way to go. We can look at using Runtime.Version or not later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use it for two reasons:

  1. I don't like reflection personally, it is just ugly code
  2. It work 9+ only

We can of course add more logic to Commons Lang parsing arbitrary strings from 6 to 9.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tunaki refactoring to JavaVersion would also have been my preference but that's tricky for two reasons

  1. AbstractJavadocMojo.version is Maven container injected, I don't know how we could customise the injection process Mapping Complex Objects does not seem to be a solution
  2. Unfortunately both the parsing code and the code for accessing the parsed float is not public in commons-lang3. We ultimately need the parsed float in AbstractJavadocMojo.addStandardDocletOptions.

Initially I copy and pasted the parsing code from commons-lang 2.6 but that was quite a bit of code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to get rid of the float, in the end it will cause issues (java versions were never floats). Instead we need a Comparable JavaVersion (just like Aether/Artifact Resolver has). Is should be as simple as parsing the String to a JavaVersion instance and use its compareTo.


// ----------------------------------------------------------------------
// Mojo components
// ----------------------------------------------------------------------
Expand Down Expand Up @@ -3632,7 +3635,7 @@ private String getJavadocExecutable()
}
// For Apple's JDK 1.6.x (and older?) on Mac OSX
// CHECKSTYLE_OFF: MagicNumber
else if ( SystemUtils.IS_OS_MAC_OSX && SystemUtils.JAVA_VERSION_FLOAT < 1.7f )
else if ( SystemUtils.IS_OS_MAC_OSX && !SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_1_7) )
// CHECKSTYLE_ON: MagicNumber
{
javadocExe = new File( SystemUtils.getJavaHome() + File.separator + "bin", javadocCommand );
Expand Down Expand Up @@ -3693,27 +3696,27 @@ private void setFJavadocVersion( File jExecutable )
if ( getLog().isWarnEnabled() )
{
getLog().warn( "Unable to find the javadoc version: " + e.getMessage() );
getLog().warn( "Using the Java version instead of, i.e. " + SystemUtils.JAVA_VERSION_FLOAT );
getLog().warn( "Using the Java version instead of, i.e. " + JAVA_VERSION_FLOAT );
}
jVersion = SystemUtils.JAVA_VERSION_FLOAT;
jVersion = JAVA_VERSION_FLOAT;
}
catch ( CommandLineException e )
{
if ( getLog().isWarnEnabled() )
{
getLog().warn( "Unable to find the javadoc version: " + e.getMessage() );
getLog().warn( "Using the Java version instead of, i.e. " + SystemUtils.JAVA_VERSION_FLOAT );
getLog().warn( "Using the Java version instead of, i.e. " + JAVA_VERSION_FLOAT );
}
jVersion = SystemUtils.JAVA_VERSION_FLOAT;
jVersion = JAVA_VERSION_FLOAT;
}
catch ( IllegalArgumentException e )
{
if ( getLog().isWarnEnabled() )
{
getLog().warn( "Unable to find the javadoc version: " + e.getMessage() );
getLog().warn( "Using the Java version instead of, i.e. " + SystemUtils.JAVA_VERSION_FLOAT );
getLog().warn( "Using the Java version instead of, i.e. " + JAVA_VERSION_FLOAT );
}
jVersion = SystemUtils.JAVA_VERSION_FLOAT;
jVersion = JAVA_VERSION_FLOAT;
}

if ( StringUtils.isNotEmpty( javadocVersion ) )
Expand Down
2 changes: 1 addition & 1 deletion maven-javadoc-plugin/src/main/java/org/apache/maven/plugin/javadoc/JavadocUtil.java
View file
Open in desktop
Expand Up @@ -19,7 +19,7 @@
* under the License.
*/

import org.apache.commons.lang.SystemUtils;
import org.apache.commons.lang3.SystemUtils;
import org.apache.http.HttpHost;
import org.apache.http.HttpResponse;
import org.apache.http.HttpStatus;
Expand Down
2 changes: 1 addition & 1 deletion maven-javadoc-plugin/src/test/java/org/apache/maven/plugin/javadoc/JavadocReportTest.java
View file
Open in desktop
Expand Up @@ -31,7 +31,7 @@
import java.util.List;
import java.util.Map;

import org.apache.commons.lang.SystemUtils;
import org.apache.commons.lang3.SystemUtils;
import org.apache.maven.execution.MavenSession;
import org.apache.maven.model.Plugin;
import org.apache.maven.plugin.LegacySupport;
Expand Down
2 changes: 1 addition & 1 deletion maven-javadoc-plugin/src/test/java/org/apache/maven/plugin/javadoc/JavadocUtilTest.java
View file
Open in desktop
Expand Up @@ -30,7 +30,7 @@
import java.util.Map;
import java.util.regex.PatternSyntaxException;

import org.apache.commons.lang.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.maven.plugin.javadoc.ProxyServer.AuthAsyncProxyServlet;
import org.apache.maven.settings.Proxy;
import org.apache.maven.settings.Settings;
Expand Down