Major issues with excludes and directory parsing on 4.0.rc1

[hazendaz]

Release 4.0.rc1 is really buggy in general. The logic used to parse the location of the license file fails if it uses full path to header as it flips the path arguments between windows / *nix style. That logic is easy to overcome by setting use of 'basedir' from the project instead and just simple name for header file. It needs fixed though.

The worst issue I've found so far is that excludes doesn't work properly at all. The new license set way plus the old way are required to work properly. It should be either. I suspect the plugin is wiping out changes when loading the data. I've tried both ways. The only way to get it to work is to add both. For me that exclusion list is super long so this adds excessive bloat.

I don't see either of these have been reported and overall there wasn't that much in general changed IMHO since 2016. So this in theory should be easy to find, add a test case for and fix. I can try to get more data on this but cannot provide the test output where I'm using this at the moment. So hopefully enough is noted here to be helpful.

[adamretter]

Release 4.0.rc1 is really buggy in general.

I am sorry to hear that. The number of test cases was increased between 3.0 and 4.0.rc1. 4.0.rc1 passes all tests that 3.0 passed, and more! However, that is not to say that we might not need some more tests ;-)

The logic used to parse the location of the license file fails if it uses full path to header as it flips the path arguments between windows / *nix style. That logic is easy to overcome by setting use of 'basedir' from the project instead and just simple name for header file. It needs fixed though.

Does this also happen with the previous 3.0 version? Can you provide an example?

The worst issue I've found so far is that excludes doesn't work properly at all.

Again can you provide an example to reproduce this please?

[hazendaz]

Hi @adamretter I'll try to spend a few cycles later tonight to get a demo up on open source project that is similar in my use case.

Prior version 3.0 worked perfectly. The issues seem to be in the split for deprecated items vs new license sets. I took some time looking at code. I sort of think I know what the problem is but havent' seen that in the code yet. I'll get this cloned tonight and start taking a deeper look.

On a plus I was able to overcome all the issues though and have been testing it since. While configuration is not ideal at the moment, I am able to move forwards on some larger code bases to simply get a baseline of testing. And personally I wanted a few of the things this brought along :)

[hazendaz]

I suspect the issues is in the convertLegacyConfigToLicenseSet method. I do not have a demo going yet to prove it but I do suspect maven sets header in both license set and abstract license mojo. They are both called the same name from the property argument as 'license.header'. Therefore I don't see how maven would know which is which really needed and probably seeds them both. The check in the method convertLegacyConfigToLicenseSet is only checking if it is null and thus I believe will fall into the logic. That logic is not protected at all. So it would if incorrectly encountered do exactly as I'm seeing in behaviour and wipe the exclusions added to the license set among other items. At the very least, a quick log to confirm what the legacy values are would be helpful. Maybe that is already there, I'll check my work build tomorrow to see.

To my first issue, it's clear the logic to find the file location is wrong. The build does not build as-is from clone out on windows and run using powershell. It blows up right away when trying to run either of these tests.

Tests in error:
ResourceFinderTest.test_load_absolute_file:47 » MojoFailure Resource C:\Users...
ResourceFinderTest.test_load_from_URL:74 » MojoFailure Resource file:///C:/Use...

I haven't looked in detail at that one but it probably would be best to raise the bar on the repo overall and require java 7 at least, dump all the code that is designed here that guava and java 7 already support and use those instead. Going further, I find it odd to be targetting java 6 still when java 7 is out of support entirely. So it would make more sense to just go to java 8 and use most all the utils directly from the various java 8 new classes and simply remove a lot of that old logic. Regardless, cross platform needs to behave properly here and I suspect it's a platform issue .

One other note, I used a maven profiler to see what was worst in processing. By far, this plugin performed the poorest of all the ones I use which is 100+. I know that is mostly because it's writing things but it doesn't perform any sort of caching and is just duplicating work it already did or at least that is what I believe based on looking through all the code. Other plugins such as formatter-maven-plugin cache to avoid constantly touching files again and again. It might be worthwhile to work towards something similar and possible count files processed vs skipped.

Sorry not much more to go on tonight and I've broadened the topic quite a bit. I'm still getting familiar with the code base and expect I'll be able to fully address the main issue here sometime this week.

[mathieucarbou]

Thanks a lot @hazendaz for your report. Let us know when you have something more concrete we can use.

Thanks @adamretter for jumping in ;-) What do you think of updating the travis file to add multi OS support for the build ? We could ignore the windows failures until the tests are fixed. To my knowledge, we have never ran the test suite in a continuous windows build.

Ref: https://docs.travis-ci.com/user/multi-os/

[adamretter]

@mathieucarbou I am have sent a PR to add multiple platforms to Travis CI - #176

I am just wondering what you think about @hazendaz idea of moving to a newer minimum Java version, i.e. JDK 8?

[mathieucarbou]

@mathieucarbou I am have sent a PR to add multiple platforms to Travis CI - #176

I am just wondering what you think about @hazendaz idea of moving to a newer minimum Java version, i.e. JDK 8?

Yes, completely agree! JDK is a good choice 👍

[hazendaz]

I'm hoping to bring a nice caching solution this way soon. I rewrote what we use in formatter-maven-plugin which under already formatted situation drops time to process to almost nothing. I'm going to try to apply that in full here. The essential idea is that the files are hashed with google guava and added to cache file. Then before trying to add in the header, the file is checked to confirm the hash changed. Only if it changed, will it write. That should take the espensive write operations out under majority of cycles. All the profiling I've ran thus far show this plugin in the few dozen I have in about a 12k file project as running the hottest and running twice during the build.

[adamretter]

@hazendaz Any further luck on some details of how to reproduce this issue you opened?

[hazendaz]

The first issue with inability to find the header file, config as is on existing project simply no longer works.

clone https://github.com/Waffle/waffle, update to latest version in 'source/jna/pom.xml'. Results are as follows.

[ERROR] Failed to execute goal com.mycila:license-maven-plugin:4.0.rc1:format (default) on project waffle-parent: Execution default of goal com.mycila:license-maven-plugin:4.0.rc1:format failed: Cannot read header document C:\Users\Jeremy\Documents\GitHub\waffle/license.txt. Cause: Resource C:\Users\Jeremy\Documents\GitHub\waffle/license.txt not found in file system, classpath or URL: The filename, directory name, or volume label syntax is incorrect -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginExecutionException

Configuration is this

                <plugin>
                    <groupId>com.mycila</groupId>
                    <artifactId>license-maven-plugin</artifactId>
                    <version>${license.plugin}</version>
                    <configuration>
                        <header>${main.basedir}/license.txt</header>
                        <excludes>
                            <exclude>.factorypath</exclude>
                            <exclude>.gitattributes</exclude>
                            <exclude>license.txt</exclude>
                        </excludes>
                        <mapping>
                            <conf>DOUBLESLASH_STYLE</conf>
                            <policy>DOUBLESLASH_STYLE</policy>
                        </mapping>
                    </configuration>
                    <dependencies>
                        <dependency>
                            <groupId>com.mycila</groupId>
                            <artifactId>license-maven-plugin-git</artifactId>
                            <version>${license.plugin}</version>
                        </dependency>
                    </dependencies>
                </plugin>

Changing configuration break 'header' into basedir and header solves the issue. This is the first issue that needs to be addressed as I don't think it states this behaviour change. I'd expect to use same configuration as 3.0 with 4.0.x and it work as it originally did. I believe this issue is related to why the build doesn't run for license plugin within windows as it is failing to properly check per system and that is kind of evident in slashes going multiple directions in output here.

C:\Users\Jeremy\Documents\GitHub\waffle/license.txt

The backslashes would need escaped and really no reason to use windows based even on windows. I haven't looked deeper yet at that specific issue or why it works when I simply break basedir out of header. I'm using the above project noted to test with as it is rather large and gets a good overall usage of license plugin. I'm just now getting to look into this so this is first thing team can take a better look at as it is easy to replicate on existing project.

[hazendaz]

Sample above also completely forgets to exclude the 3 noted exclusions. That IMO is due to using same configuration for both legacy and new in annotations so maven should be populating both and as a result checking for legacy vs new is checking one of the items and then overriding all. Thus it fails to work unless defined twice.

[WARNING] Unknown file extension: C:/Users/Jeremy/Documents/GitHub/waffle/Source/JNA/waffle-spring-boot2/waffle-spring-boot-autoconfigure2/.factorypath

[hazendaz]

Looks like last master resolved the issue I had on windows. Subsequently, the work around I was trying to use was reprocessing all files in the path leading to slow times. So now that the main issue is solved, I am closing this as completed from my perspective.