Skip to content
This repository was 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions maven-javadoc-plugin/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Loading

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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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