Eclipse compiler does not process multiple --add-exports correctly

[eilensm]

Hi,

I experienced a problem with mulitple --add-exports during my Maven build and created an issue in the Maven compiler plugin (see https://issues.apache.org/jira/browse/MCOMPILER-279).
Since the plugin works with the normal Java compiler and also the Eclipse compiler itself understands multiple add-exports arguments, he pointed me to the EclipseJavaCompiler here.

The problem is that in line https://github.com/codehaus-plexus/plexus-compiler/blob/master/plexus-compilers/plexus-compiler-eclipse/src/main/java/org/codehaus/plexus/compiler/eclipse/EclipseJavaCompiler.java#L179, the custom compiler arguments are processed as entries of a map. But here, the entries should be processed as real entry pairs. Therefore the method config.getCustomCompilerArgumentsEntries() should be used. But there is also a failing test thus a further change becomes necessary. Thus the change for line #179 would be following:

for ( Entry<String, String> entry : config.getCustomCompilerArgumentsEntries() )
{
    // omit errorsAsWarnings options which already have been handled above
    if ( entry.getKey().equals("errorsAsWarnings") || entry.getKey().equals("-errorsAsWarnings") )
    {
	    continue;
    }

I don't know exactly how to provide a patch or a pull request and I'm totally unsure of how to provide a unit test verifying the correct behaviour after the fix. I had a look at the existing unit tests but didn't find anything I could really base a new test case upon.

But I would give a PR a try if someone can point me to a howto, etc....

[tomaswolf]

Good catch. Here's what I'd do:

• Indeed use config.getCustomCompilerArgumentsEntries().
• Move the extra handling of [-]errorsAsWarnings and -properties into the loop.
• Also move the handling of -proceedOnError from below the loop into the loop.
• Now the extrasvariable should be gone.
• Now extract the whole loop into a method static boolean processCustomArguments(CompilerConfiguration config, List<String>args) (adds arguments from the config to args, and returns true if errorsAsWarnings was encountered (and skipped)). Where the loop was, you'll just have this.errorAsWarnings = processCustomArguments(config, args).
• Then write a unit test for just this new static, package-visible method, using CompilerConfigurationTest (in plexus-compiler-api) as a model: set up a configuration, call EclipseJavaCompiler.processCustomArguments() directly, and verify that the list does contain the expected items.

[eilensm]

Hi @tomaswolf ,

I just had some time for doing the proposed refactoring and changes. Also have some unit tests, though I had to place them in a the plexus-compiler-eclipse module instead in the api module due to accessing EclipseJavaCompiler class.

As stated earlier, I have my changes in a local branch. How can I upload the stuff and create a PR for a review???

Thanks,
Marc

[pzygielo]

How can I upload the stuff and create a PR for a review???

Start with: Fork a repo

[tomaswolf]

Hi Marc, that commit looks pretty good. Just the indentation on haveSourceOrReleaseArgument is off.

You should now create a PR from that commit, so that it can be reviewed and eventually merged. See Creating a pull request from a fork for how to do that.

[eilensm]

Hi @tomaswolf ,

I fixed the formatting issue and did a minor refactoring concerning the usage of the option and optionValue inside the loop.
There's already a PR: #113

There are some problems with the build, but for me, it seems to be a problem with JDK16, and not really with my changes.