Skip to content
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

[MJAVADOC-544] - Changed behaviour of Javadoc for temporary files encoding (options, argfile, ...) #10

Merged
merged 1 commit into from Dec 4, 2018

Conversation

michael-st
Copy link
Contributor

mvn -Prun-its verify was run with JDK9 on windows without errors

@michael-st
Copy link
Contributor Author

Due to changes to readFile in b78042e conflicts were introduced.

I rebased the PR and fixed some style-issues in the tests.

Copy link
Contributor

@rfscholte rfscholte left a comment

Choose a reason for hiding this comment

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

Can you squash the commits? And you can add yourself as contributor in the root pom.

@@ -4368,9 +4378,15 @@ private void addCommandLineOptions( Commandline cmd, List<String> arguments, Fil
options.append( StringUtils.join( arguments.iterator(),
SystemUtils.LINE_SEPARATOR ) );

/* default to platform encoding */
String encoding = null;
if ( JAVA_VERSION.compareTo( SINCE_JAVADOC_9 ) >= 0 )
Copy link
Contributor

Choose a reason for hiding this comment

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

Please switch to isAtLeast() or isBefore(), which are better readable.

@@ -4414,9 +4430,15 @@ private void addCommandLineArgFile( Commandline cmd, File javadocOutputDirectory
cmd.createArg().setValue( "@" + FILES_FILE_NAME );
}

/* default to platform encoding */
String encoding = null;
if ( JAVA_VERSION.compareTo( SINCE_JAVADOC_9 ) >= 0 )
Copy link
Contributor

Choose a reason for hiding this comment

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

Please switch to isAtLeast() or isBefore(), which are better readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to squash your commits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I addressed your points and squashed the commits.

Ran mvn -Prun-its verify with JDK8 and JDK9 on Windows successfully.

@rfscholte
Copy link
Contributor

@michael-st
Copy link
Contributor Author

too bad...

The error message occurs when compiling the file "/src/test/resources/unit/argfileumlautencoding-test/argfileumlautencoding/test/Appäöü�.java".
The compiler complains about a name mismatch between class "Appäöüß" and the filename "AppäöüÃ�.java".
This is a charset-problem, which occurs only in the linux environment.

I tried to reproduce the problem:

The build passes when I execute the maven build in a docker container (image: maven:3-jdk-9-slim) in which the LANG is set to C.UTF-8.
If I change the LANG-environment variable in the container ("export LANG=en_US.ISO-8859-1") - the build fails.

FROM maven:3-jdk-9-slim
RUN echo $LANG
> C.UTF-8
RUN apt update && apt install -y git && echo dirty no cleanup of apt
RUN git clone https://github.com/michael-st/maven-javadoc-plugin.git
RUN cd maven-javadoc-plugin
RUN git checkout MJAVADOC-544
RUN mvn -Dtest=org.apache.maven.plugins.javadoc.JavadocReportTest#testArgfileUmlautEncoding clean test
> build passes
RUN export LANG=en_US.ISO-8859-1
RUN echo $LANG
> en_US.ISO-8859-1
RUN mvn -Dtest=org.apache.maven.plugins.javadoc.JavadocReportTest#testArgfileUmlautEncoding clean test
> build fails

I tried to convert the file itself and the corresponding .xml-file to ISO-8859-1, but that did not go well either.

At the moment I'm out of ideas and I don't think that I can find a working solution for this (besides setting the LANG to ...UTF-8 in the build environment).

My initial problem was only related to the options file, which would be fixed with this pull request.
For this all tests should pass (hopefully).

So, for this issue, I have removed the special handling for argfile, which would only be needed if someone is using special characters in java class names.
I also modified the commit message to reflect this change.

Is this course of action okay?

@rfscholte rfscholte merged commit b9ec67b into apache:master Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants