Make the default include/exclude syntax compatible with the behavior of Maven 3 #945
Conversation
…features. It allows the include/exclude patterns to reproduce the behavior of Maven 3 when no syntax is specified. Before this commit the default syntax was glob, which is not identical to Maven 3 behavior. A future version should use the `PathSelector` from maven-impl directly. But it may require to move that class in another module.
| switch (includes.length) { | ||
| case 0: | ||
| return INCLUDES_ALL; | ||
| case 1: |
There was a problem hiding this comment.
This case makes the thing like module-info.java not match the module-info.java in the compile source root. Once the case 1: is omitted, everything works correctly. Might be an unnecessary and wrong optimization, since the includes is not the only part of this. There are also directory includes, directory excludes and so on.
There was a problem hiding this comment.
Ah, good catch. I suspect that it is because when using the includes[0] matcher directly, baseDirectory.relativize(path) is not invoked.
There was a problem hiding this comment.
Yeah, and the optimization is not worth it besides being wrong. The case 0 is what we want, which now could be replaced by simple "if (includes.length == 0) return INCLUDES_ALL;"
There was a problem hiding this comment.
This would be something to change in maven 4 too, I guess. At least, here, all the separate module-info.java files were not compiled because of this. I mean, the situation when one compiles with release 8 the whole project and then compiles only the module-info.java in another execution. Those are typically cases when includes.length == 1. When I changed it to **/module-info.java, the logic made it two matchers and the simplify() was basically pass-through.
There was a problem hiding this comment.
It seems to me that the case 1 is the most common case, because the default matcher for compiling Java sources is **/*.java. The call to baseDirectory.relativize(path) is not necessary when the matcher starts with **/, so we could add that check in the case 1. It would cover all compilations with the default configuration. I do not know if the difference would be noticeable, but since it would cover millions of compilations around the world, it may be not that bad to have this small optimization.
There was a problem hiding this comment.
You are the one doing the PR. So, you get to decide. Just more complicated optimizations, more possibilities to get the thing wrong. If it was on me, I would go for code simplicity. But as I said, not my call :)
There was a problem hiding this comment.
But whatever decision, exactly the same thing should be done in maven, so that there is no subtle difference that will take again a day to understand :)
There was a problem hiding this comment.
Yes, it is my plan to port the change to Maven 4. There is already another bug fix (a NullPointerException) which needs to be ported anyway.
… no need to relativize. Opportunistically check if there is a need to relativize the rest of the time as well.
|
@desruisseaux Please assign appropriate label to PR according to the type of change. |
This pull request temporarily copies the
PathSelectorclass from Maven core with removal of unused features. A future version should use directly thePathSelectorclass from Maven core instead, but it may require that we move the latter in another ("shared"?) module.The use of
PathSelectorallows the include/exclude patterns to reproduce the behavior of Maven 3 when no syntax is specified. Before this commit the default syntax was "glob", which is not identical to Maven 3 behavior.