Improve AspectJ build

[kriegaex]

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

• 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.

@odrotbohm, feel free to accept the PR if it helps you straighten out the situation.

[kriegaex]

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

1. ANTLR v4 plugin creates new sources
2. MC sees the changed generated sources by ANTLR and therefore runs annotation processing
3. AJM sees both the changed generated sources by ANTLR and MC (JPA stuff) and therefore runs AJC.

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:

[INFO] --- compiler:3.11.0:compile (java-compile) @ spring-data-jpa ---
[INFO] Nothing to compile - all classes are up to date
[INFO]
[INFO] --- aspectj:1.14-SNAPSHOT:compile (aspectj-compile) @ spring-data-jpa ---
[INFO] No modifications found skipping aspectJ compile

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.

[kriegaex]

Until there is an improved version of A4M, I have an idea for a Maven workaround,

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.

[mp911de]

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 mvn clean compile, then mvn compile fails with cannot find symbol. Without the replacement optimization, the entire build runs within 6 to 9 seconds (if there are no other changes).

Rolling back the changes in the replacer path and adding annotation processors to the Maven plugin helps with optimization.

[mp911de]

Regarding the test scope of AspectJ: This is an optional dependency for us, spring-aspects is already optional and we'd like to keep it that way. Instead of forcing functionality on users, we rather prefer opt-in so fixing test scope into optional=true makes more sense here.

[mp911de]

Thank you for your contribution. That's merged, polished, and backported now.

[kriegaex]

The replacer is in place because ANTLR doesn't have means to generate package-private classes

@mp911de, I thought so, because I saw what it does. I never questioned that it was necessary, only under which circumstances.

Running various combinations mvn clean compile, then mvn compile fails with cannot find symbol.

Of course, because AspectJ Maven Plugin is configured to run a bit later in phase process-classes, i.e. you needed to test with mvn clean process-classes and then mvn process-classes. I did not change that, it was like that before my change. I.e., mvn [clean] compile would have failed before, if any subsequent build step would have needed AuditingEntityListener. With just compile, the AspectJ build step would always have been missing.

Without the replacement optimization, the entire build runs within 6 to 9 seconds (if there are no other changes).

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 mvn clean compile, but the consequence would be that the lexical order of plugins in the POM now determines if MC or AJM run first. In my solution, the phases make that stable.

Regarding the test scope of AspectJ: This is an optional dependency for us, spring-aspects is already optional and we'd like to keep it that way. Instead of forcing functionality on users, we rather prefer opt-in so fixing test scope into optional=true makes more sense here.

I did not know it was optional, because also aspectjweaver before my change was not optional, but test-scoped. Under these circumstances, I agree with your change if, and only if you can guarantee that none of the aspect-enhanced classes is ever loaded, if it needs classes from aspectjrt. If you can guarantee that, optional scope is fine. As far as I can see, only class AuditingEntityListener is affected by aspects, but that one definitely needs AspectJ classes. Therefore, you need to guarantee what I described for that class. Can you?

[mp911de]

Thanks a lot for reviewing the merge.

Reinstating the Antlr generated sources target via antlr4.dir leads to compile errors on subsequent compilation, that is why I reverted that part.

Running various combinations mvn clean compile, then mvn compile fails with cannot find symbol.

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.

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 mvn clean compile, but the consequence would be that the lexical order of plugins in the POM now determines if MC or AJM run first. In my solution, the phases make that stable.

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 replacer entirely.

[mp911de]

By using some trickery (ANTLR first tries to resolve a template via new File(…) and then falls back into classpath lookup) I was able to get rid of post-processing ANTLR-generated files. Now the Maven build reports (for a repeated mvn verify command):

[INFO] --------------< org.springframework.data:spring-data-jpa >--------------
[INFO] Building Spring Data JPA 3.2.2-SNAPSHOT                            [2/4]
[INFO]   from spring-data-jpa/pom.xml
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- enforcer:3.3.0:enforce (enforce-maven-version) @ spring-data-jpa ---
[INFO] 
[INFO] --- antlr4:4.13.0:antlr4 (default) @ spring-data-jpa ---
[INFO] No grammars to process
[INFO] ANTLR 4: Processing source directory /Users/mpaluch/git/data/spring-data-jpa/spring-data-jpa/src/main/antlr4
[INFO] 
[INFO] --- resources:3.3.1:resources (default-resources) @ spring-data-jpa ---
[INFO] Copying 15 resources from src/main/resources to target/classes
[INFO] 
[INFO] --- kotlin:1.9.21:compile (compile) @ spring-data-jpa ---
[INFO] No sources to compile
[INFO] 
[INFO] --- compiler:3.11.0:compile (java-compile) @ spring-data-jpa ---
[INFO] Nothing to compile - all classes are up to date
[INFO] 
[INFO] --- aspectj:1.14.0:compile (aspectj-compile) @ spring-data-jpa ---
[INFO] No modifications found skipping aspectJ compile
[INFO] 
[INFO] --- resources:3.3.1:testResources (default-testResources) @ spring-data-jpa ---
[INFO] Copying 43 resources from src/test/resources to target/test-classes
[INFO] 
[INFO] --- kotlin:1.9.21:test-compile (test-compile) @ spring-data-jpa ---
[INFO] No sources to compile
[INFO] 
[INFO] --- compiler:3.11.0:testCompile (java-test-compile) @ spring-data-jpa ---
[INFO] Nothing to compile - all classes are up to date
[INFO] 
[INFO] --- aspectj:1.14.0:test-compile (aspectj-test-compile) @ spring-data-jpa ---
[INFO] No modifications found skipping aspectJ compile
[INFO] 
[INFO] --- surefire:3.0.0:test (default-test) @ spring-data-jpa ---
[INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider

Let me know whether the build behavior meets the intended outcome.

[kriegaex]

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.

[kriegaex]

Reinstating the Antlr generated sources target via antlr4.dir leads to compile errors on subsequent compilation, that is why I reverted that part.

No, it does not! Like I said, you ran mvn compile where you should have run mvn process-classes. The same errors will happen now after your own ANTLR optimisation. Try for yourself. Hence one of the commits in my follow-up PR, moving AspectJ phases to the defaults most users might expect.

Running various combinations mvn clean compile, then mvn compile fails with cannot find symbol.

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.

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.

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 mvn clean compile, but the consequence would be that the lexical order of plugins in the POM now determines if MC or AJM run first. In my solution, the phases make that stable.

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.

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 mvn compile instead of mvn process-classes or mvn test-compile instead of mvn process-test-classes while testing. I also had to remind myself to use the right phases to ensure AspectJ compilation.

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 replacer entirely.

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.

[kriegaex]

Relates to antlr/antlr4#4504.