-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improve AspectJ build #3282
Improve AspectJ build #3282
Conversation
Aspect-enhanced classes need aspectjrt on the class path. If it is not, the AspectJ Compiler usually complains, either warning or even failing the build, also via AspectJ Maven. The aspectjweaver, however, is for load-time weaving. It is a superset of aspectjrt, but bigger than necessary. Also, the weaver was only test-scoped, but also during normal runtime aspectjrt is necessary. Without it, it can only work by the lucky chance that Spring already depends on aspectjweaver, which is not good dependency management. Each Maven module should be self-consistent. Aspectjrt was a plugin dependency for AJ Maven, which is also superfluous, because aspectjtools already is a superset of aspectjweaver, i.e. it also contains aspectjrt. Hence, aspectjtools is all AJ Maven needs, but the compiled application is who really needs aspectjrt.
by separation of concerns: Let - Maven Compiler do annotation processing without compilation and - AspectJ Maven compilation of all Java sources and aspects without annotation processing. Actually, we could let AJ Maven do all the work, but it would be difficult to configure everything correctly in JDK 9+, because AJ Maven is incomplete regarding automatically putting everything on the right module paths. so, this separation of concerns saves tedious configuration work. Relates to mojohaus/aspectj-maven-plugin#15.
Oh, one more thing: I debugged into AspectJ Maven (AJM) a bit to find out why, just like Maven Compiler (MC) before, it always recompiles after my change. The reason is simple: Your two code generation steps (ANTLR v4 and the two JPA annotation processors) always regenerate sources, no matter whether their input data have changed or not. AJM (and probably also MC) check if any sources have more recent time stamps than the last build file, which is always true. Therefore, they recompile. I do not know anything about your Gradle Enterprise Maven Extension, but maybe that one can be configured in such a way that it skips the code generation steps, if not the timestamps but the checksums are unchanged. BTW, Maven Replacer is not helping either. I am assuming, that even if there is nothing to replace, it will rewrite the files. Unless at some point in the future Maven and its plugins are changed to use checksums rather than timestamps, there is not much you can do. Moreover, for the reason described in the PR description and the corresponding commit, we use both MC and AJC in that order. I.e., we have a causal chain
I think, if you open an issue for ANTLR4 Maven Plugin (A4M), requesting to add an option to skip execution if no input files have changed, then you get what you want. Currently, no such option exists, as far as I can see. I tested my hypothesis by experimentally commenting out A4M after the clean build. When I run one more build after that, of course it recompiles because the POM has changed, which is also checked by at AJM and MC. Build config changes trigger rebuilds. But if I recompile again after that, I get:
This confirms my hypothesis. Until there is an improved version of A4M, I have an idea for a Maven workaround, but it will be a little bit ugly. Stay tuned for an update of this PR, if I get it working. |
This is a workaround for the fact that ANTLR4 Maven Plugin always generates source files, even if its input files have not changed, which in turn leads to changed source files and then to unnecessary compilation of same. A similar workaround makes sure that in the same situation Maven Replacer Plugin does not find any files to replace text in, because that would also alter the timestamps of the target files. Maven Build Helper Plugin is used to compare ANTLR4 input and output directories, determining if the latter are up-to-date. If so, the two plugins mentioned above will be fed a dummy directory name, otherwise a real one. Along the way, Maven Replacer was upgraded from 1.4.1 to its last release 1.5.3, before it was retired on Google Code. The upgrade also led to renaming the plugin, probably because the word "plugin" is already in its group ID. But it is, in fact, the same plugin. The upgrade also fixes a bug, enabling the plugin to understand absolute directories, i.e. now we can use ''${project.build.directory}' instead of 'target' as a base directory. Relates to mojohaus/aspectj-maven-plugin#15.
I must admit, that this was way more difficult than I anticipated. I thought, I could do something like auto-activating profiles based on properties based on Maven Build Helper (MBH) goals, but it seems that the POM needs to be evaluated before MBH kicks in. Anyway, I was able to use MBH in another way to address this issue. Please read the long commit message of d12a20b for more details. As a result, non-clean rebuilds across all stages are now lightning-fast, really not doing anything anymore across the whole tool chain of parser generation, text replacements in the generated parser classes, annotation processing and Java + AspectJ compilation. I also verified this without the Gradle Enterprise build extension to ensure that it also works without it trying to optimise anything, possibly hiding problems in my approach. Oh, and BTW: Just in case you do not like my separation of concerns (annotation processing vs. compilation) and somehow feel better to let MC do all the work except for the one file affected by the aspect and use your old explicit include for AJM (and hopefully the forgotten exclude for MC so as not to undermine quick rebuilds), I am expecting this workaround to also work for that scenario. |
Thanks a lot for looking into this. The replacer is in place because ANTLR doesn't have means to generate package-private classes, see antlr/antlr4#972. Running various combinations Rolling back the changes in the replacer path and adding annotation processors to the Maven plugin helps with optimization. |
Regarding the test scope of AspectJ: This is an optional dependency for us, |
Aspect-enhanced classes need aspectjrt on the class path. If it is not, the AspectJ Compiler usually complains, either warning or even failing the build, also via AspectJ Maven. The aspectjweaver, however, is for load-time weaving. It is a superset of aspectjrt, but bigger than necessary. Also, the weaver was only test-scoped, but also during normal runtime aspectjrt is necessary. Without it, it can only work by the lucky chance that Spring already depends on aspectjweaver, which is not good dependency management. Each Maven module should be self-consistent. Aspectjrt was a plugin dependency for AJ Maven, which is also superfluous, because aspectjtools already is a superset of aspectjweaver, i.e. it also contains aspectjrt. Hence, aspectjtools is all AJ Maven needs, but the compiled application is who really needs aspectjrt. See #3282
by separation of concerns: Let - Maven Compiler do annotation processing without compilation and - AspectJ Maven compilation of all Java sources and aspects without annotation processing. Actually, we could let AJ Maven do all the work, but it would be difficult to configure everything correctly in JDK 9+, because AJ Maven is incomplete regarding automatically putting everything on the right module paths. so, this separation of concerns saves tedious configuration work. Relates to mojohaus/aspectj-maven-plugin#15. See #3282
Move AspectJ dependency from test into optional. Add annotation processor paths to compiler plugin for discovery by Gradle Enterprise. Rollback replacer/conditional antlr paths. See #3282
Aspect-enhanced classes need aspectjrt on the class path. If it is not, the AspectJ Compiler usually complains, either warning or even failing the build, also via AspectJ Maven. The aspectjweaver, however, is for load-time weaving. It is a superset of aspectjrt, but bigger than necessary. Also, the weaver was only test-scoped, but also during normal runtime aspectjrt is necessary. Without it, it can only work by the lucky chance that Spring already depends on aspectjweaver, which is not good dependency management. Each Maven module should be self-consistent. Aspectjrt was a plugin dependency for AJ Maven, which is also superfluous, because aspectjtools already is a superset of aspectjweaver, i.e. it also contains aspectjrt. Hence, aspectjtools is all AJ Maven needs, but the compiled application is who really needs aspectjrt. See #3282
by separation of concerns: Let - Maven Compiler do annotation processing without compilation and - AspectJ Maven compilation of all Java sources and aspects without annotation processing. Actually, we could let AJ Maven do all the work, but it would be difficult to configure everything correctly in JDK 9+, because AJ Maven is incomplete regarding automatically putting everything on the right module paths. so, this separation of concerns saves tedious configuration work. Relates to mojohaus/aspectj-maven-plugin#15. See #3282
Move AspectJ dependency from test into optional. Add annotation processor paths to compiler plugin for discovery by Gradle Enterprise. Rollback replacer/conditional antlr paths. See #3282
Thank you for your contribution. That's merged, polished, and backported now. |
@mp911de, I thought so, because I saw what it does. I never questioned that it was necessary, only under which circumstances.
Of course, because AspectJ Maven Plugin is configured to run a bit later in phase
Yes, but it forces recompilation where it is unnecessary. My whole optimisation started only, because @odrotbohm had complained about Maven Compiler (MC) and AspectJ Maven (AJM) recompiling unnecessarily, but one of the problems was the Replacer run, updating source timestamps. The other was ANTLR. The problem always was in this build configuration, never in MC or AJM. Your revert defeats that optimisation. May I suggest to reinstate it? See #3284. If you like, it is possible to remove the phases (plural, because now it also compiles tests) from AspectJ Maven, reverting to default phases. Then you can run
I did not know it was optional, because also |
See this comment, clarifying the reviewer's misunderstanding : spring-projects#3282 (comment) This optimisation is necessary to avoid rebuilds due to ANTLR and Replacer regenerating and modifying sources which have already been generated and modified before identically.
Thanks a lot for reviewing the merge. Reinstating the Antlr generated sources target via
This wasn't about AspectJ but regarding ANTLR sources being generated into a different place and hence these couldn't be found by the compiler.
I'm fine keeping the phases to enforce ordering, we ran already into plugin ordering issues in other places so it is good to be explicit. Let's stick with this pull request for the time being. Meanwhile, I'm searching for an approach where we could omit the need for |
By using some trickery (ANTLR first tries to resolve a template via
Let me know whether the build behavior meets the intended outcome. |
Yes, it does. Now MC and AJM only do work when they are supposed to, because no unintended source code changes due to ANTLR and the now obsolete Replacer happen anymore. |
No, it does not! Like I said, you ran
Again, no. They were generated into the right place. You simply stopped your build one phase too early. Sorry to correct you all the time, but your statements are just wrong. My PR was working 100%, the problem sat in front of the computer.
But is it, though? You are still trying to explain non-existent bugs in my PR to me. I think, my commit from the other PR would help you avoid this build pitfall in the future. My long comment in the POM would avoid other people from messing up the POM, I guess. Even I ran into the compilation problems several times while testing the PR, because I also used
Part of my second PR is now obsolete (making ANTLR and Replacer runs conditional), of course, because your new ANTLR template solved the problem of repeated source generation and makes Replacer obsolete, too. But I still recommend to cherry-pick the commit changing AspectJ phases. I can also drop the first commit from the other PR for you, so you can just review and merge normally. |
Relates to antlr/antlr4#4504. |
Compile-time weaving requires aspectjrt, not aspectjweaver
Aspect-enhanced classes need aspectjrt on the class path. If it is not, the AspectJ Compiler usually complains, either warning or even failing the build, also via AspectJ Maven. The aspectjweaver, however, is for load-time weaving. It is a superset of aspectjrt, but bigger than necessary. Also, the weaver was only test-scoped, but also during normal runtime aspectjrt is necessary. Without it, it can only work by the lucky chance that Spring already depends on aspectjweaver, which is not good dependency management. Each Maven module should be self-consistent.
Aspectjrt was a plugin dependency for AJ Maven, which is also superfluous, because aspectjtools already is a superset of aspectjweaver, i.e. it also contains aspectjrt. Hence, aspectjtools is all AJ Maven needs, but the compiled application is who really needs aspectjrt.
Get rid of 'forceAjcCompile' workaround and special includes
by separation of concerns: Let
Actually, we could let AJ Maven do all the work, but it would be difficult to configure everything correctly in JDK 9+, because AJ Maven is incomplete regarding automatically putting everything on the right module paths. so, this separation of concerns saves tedious configuration work.
Relates to mojohaus/aspectj-maven-plugin#15.
@odrotbohm, feel free to accept the PR if it helps you straighten out the situation.