search

eclipse-aspectj / aspectj

Other
291 stars 84 forks source link

In-process compilation with less than minimal JRE version does not fail after printing warning #292

Closed philippe-granet closed 6 months ago

philippe-granet commented 6 months ago

When using https://github.com/mojohaus/aspectj-maven-plugin with an invalid JRE version, aspectj silently return without any error in MessageHolder: https://github.com/eclipse-aspectj/aspectj/blob/d27943749cc35864ff4565a444b21851915307ab/org.aspectj.ajdt.core/src/main/java/org/aspectj/tools/ajc/Main.java#L247-L251

In maven logs:

[INFO] --- aspectj:1.15.0:compile (default) @ my-project ---
[INFO] Showing AJC message detail for messages of types: [error, warning, fail]
The AspectJ compiler needs at least Java runtime 17
[INFO] --- resources:3.3.1:testResources (default-testResources) @ my-project ---
[INFO] 
...

-> build continue and not failing

philippe-granet commented 6 months ago

MR proposed: https://github.com/eclipse-aspectj/aspectj/pull/293

kriegaex commented 6 months ago

Thanks for the PR, @philippe-granet. I will review and possibly merge it, with or without modification.

Note to myself: The actual problem is that AJ Maven does not fork but runs AJC in-process, i.e. the exit code -1 which would signal an error is not there in that situation. Before introducing the warning, AJC would just try to start compilation and run into an exception thrown by the JVM due to unknown class file versions or by JDT Core due to missing constants there, whichever would occur first depending on the AspectJ version. By trying to print a more comprehensive error message instead of throwing an exception, AspectJ lost the ability to signal the fatal error in the in-process situation, which I think is what really needs to be fixed. Logging a compiler error instead as proposed here feels wrong, because there was no compilation attempt in the first place. A good old exception like before, just with a clear error message, might be not just the simplest but also the best course of action.

kriegaex commented 6 months ago

@philippe-granet, while writing the note to myself - a technique I sometimes use when there is no other "rubber duck" to talk to - I realised that I actually prefer to throw an exception, if the minimum JRE requirement is unmet, which is why I created another PR superseding yours.

Having said that, I am still impressed that you created your own PR and managed to get it done with the complex message handling approach. By all means, feel encouraged to contribute more to AspectJ. Your PRs are always welcome, even though this particular one ended up unmerged. But I hope that my alternative fix also works for you.

philippe-granet commented 6 months ago

But I hope that my alternative fix also works for you.

Yes your alternative is OK for me, thanks!