feat: Add a property to limit plugin to a list of files #876
Conversation
…iles usage: mvn license:check -Dlicense.filesToCheck="src/main/org/acme/MyClass.java,src/main/org/acme/MyEnum.java"
|
Thanks! We'll look! |
|
From a quick reading I see that the property impacts all goals so I would rename the property to something like: Also, after reading #536, it feels like this is much more a big fix ? Right @hazendaz ? Passing properties as proposed here #536 (comment) should work... It is also documented in the plugin javadoc (from memory) |
As you understood, the intent is to replace the file scan by giving a list of files to run the plugin goals on. The property should reflect this, it is not a filter per se.
The proposed solutions did not work for me |
It's neither a "check" (ref: Like said: or an equivalent name not bound to any goal but more to the fact that this actually replaces the file scan.
That is my real concerne here.... Before merging this PR, I would like to understand why this is not working and whether we should make it work... These properties being documented, I would expect them to work when passed as parameter. They were also working before the move to a list structure. |
I'll try to create a test to reproduce this but this is the debug redacted output of a local license:check configuration This line is triggering: The call was made with |
|
Right, there is no toString() representation sadly - one should be added, which displays the object content. |
|
@mathieucarbou, I've created a new PR with a failing IT test: #878, finding a way to make this test work would eliminate the need for this PR. |
|
Proposition for the name of the property to set the files to replace the scan of the
@mathieucarbou and @hazendaz, do you have any preferences ? |
|
@mathieu I haven't looked at this yet but seems like something we are looking for in mybatis. We are looking to add the git commit hook plugin for any formatting tools because its too frequent that folks use the IDE to make changes, test them, etc but then never run the full build and miss formatting only to make junk commits later. I've done extensive profiling on more than 100 plugins and this by far when messing with the copy right dates is the slowest plugin that exists. That said, this seems interesting to get working so it can work off a diff only as no reason to check all files constantly. Another PR currently out to better handle the slow git commands seems like a double punch to make this plugin go from last to somewhere reasonable. I'm going to try to review all you have up including the alternative this weekend but may take a little longer to fully understand all the changes. I think 3 in your list seems appropriate in case I'm thinking but before doing any more, give us some time to review fully so we better understand and test this against mybatis where we already have an open PR to add in the git commit hook plugin (without license right now). Currently doing that there ends up taking 8 minutes on the contributors machine, mine takes far less but I have a bulked up machine that is serious overkill for this. At work though, I'm on a dev ops team supporting some 2k+ repos and due to performance on this plugin, we have not really promoted its usage but want to. I think if all these current pull requests work out, it will allow us to further promote that. It may not be a silver bullet but hopefully its enough to make it viable for using a hook to address as code is committed without killing developer experience. |
|
@mathieu @hazendaz : I was thinking about this feature this morning. Like @mathieu said, the goal is to globally pre-filter the set of files that the plugin will use to work with (pre-commit, etc), which is current done by replacing the scanner. I think we can do better. The main idea is to reduce the selection on which the plugin will work, but the way to define this selection, we already have something we could re-use in terms of plugin: the concept of includes/excludes with ant-like patterns, which is even more flexible than defining specific file paths. This concept is also hidden in the name proposals here: #876 (comment): In maven, we already have the concepts in other plugins (i.e. fileSets and source for assembly - https://maven.apache.org/plugins/maven-assembly-plugin/assembly.html). So why not re-use this concept and introduce something like this. It cannot be a global But it could be something really conveying the fact that:
If set, then, we can use the scanner as usual to list the files we should work with, but this list, which forms a virtual workspace, then has to be filtered again using the include/exclude of each license set. Some thing like (adapting an example from the project and using <build>
<plugins>
<plugin>
<groupId>com.mycila</groupId>
<artifactId>license-maven-plugin</artifactId>
<version>@project.version@</version>
<configuration>
<workspace>
<includes>
<include>specific/file.java</include>
<include>all/in/this/folder/.java</include>
<includes>
<excludes>
<exclude>all/in/this/folder/DoNotTouch.java</exclude>
<excludes>
<workspace>
</fileSets>
<licenseSets>
<licenseSet>
<header>mock-license-1.txt</header>
<excludes>
<exclude>invoker.properties</exclude>
<exclude>pom.xml</exclude>
<exclude>*.groovy</exclude>
<exclude>**/*.bak</exclude>
<exclude>*.log</exclude>
<exclude>mock-license-*</exclude>
<exclude>**/Unformatted2.java</exclude>
<exclude>**/Unformatted3.java</exclude>
</excludes>
</licenseSet>
<licenseSet>
<header>mock-license-2.txt</header>
<excludes>
<exclude>invoker.properties</exclude>
<exclude>pom.xml</exclude>
<exclude>*.groovy</exclude>
<exclude>**/*.bak</exclude>
<exclude>*.log</exclude>
<exclude>mock-license-*</exclude>
<exclude>**/Unformatted1.java</exclude>
<exclude>**/Unformatted3.java</exclude>
</excludes>
</licenseSet>
<licenseSet>
<header>mock-license-3.txt</header>
<includes>
<include>**/Unformatted3.java</include>
</includes>
</licenseSet>
</licenseSets>
</configuration>
</plugin>
</plugins>
</build> |
|
So using this proposal from pre-commit would mean something like this ? mvn license:check -Dlicense.workspace.includes=src/file1.txt,src/file2.txt |
Yes! it would be easier to use it like that and also with patterns. Also, if you look at #878, it works actually, but more a pain to setup in a commit hook. I think there is value in being able to reduce the scope of the global workspace, I just don't know yet how to efficiently implement that considering that there are 2 scans involved or 1 big scan and a post-scan filtering. |
|
@hazendaz : what do you think ? Should we release with the latest pr merged or we wait to have the work for this one done ? But on the other end, as shown, there is currently a way to use a commit hook to only format the committed files, although not efficient. |
I think we should wait for this. I'm also refocusing over here to do some other cleanup. Since we are going into our first version 5 release on java 11 now its already going to show as a breaking change in terms of the 4.x stream so multiple bigger feature improvements seems appropriate in this particular case even if we are mostly sticking as close to underlying current api as possible. |
You're right! Forgot about that! |
|
@mathieu : are you willing to update your PR to try the proposal about with the workspace idea ? Also, <workspace>
<baseDir></baseDir>
<includes>
<include>specific/file.java</include>
<include>all/in/this/folder/.java</include>
<includes>
<excludes>
<exclude>all/in/this/folder/DoNotTouch.java</exclude>
<excludes>
<workspace> |
|
@mathieucarbou , I'll need some more context on your vision of the |
This concept exists already but is implicit:
The idea is not make it explicitly configurable.
As I see things for includes/excludes:
For backward compat', In the case of a commit hook, then the only property to add would be:
This behind will be transferred to licenseset.includes and will have at the end the same effect of my comment in this PR: #878 (comment) Also, this "design" does not change the scanner logic, which is still taking place but based on a refinement of the baseDir / includes / excludes. @mathieucarbou @hazendaz @frawa : let me know what you think of that ;-) |
|
In other words, we do not bypass the |
No because if you bypass it, you cannot support ant patterns. Example: And it is also less code to not bypass it and reuse the include / exclude mechanism. Also, if the file list is given as complete path, the directory scanner anyway will be pretty fast. |
|
I'll close this PR and open an issue (#887) for the workspace feature. I'll open a new PR with updates for it |
The idea is to be able to use this plugin from a pre-commit hook (only run on a set of files)
usage: mvn license:check -Dlicense.filesToCheck="src/main/org/acme/MyClass.java,src/main/org/acme/MyEnum.java"
ref #536