Skip to content
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

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

Closed
philippe-granet opened this issue Mar 14, 2024 · 4 comments · Fixed by #294
Assignees
Labels
bug Something isn't working
Milestone

Comments

@philippe-granet
Copy link

philippe-granet commented Mar 14, 2024

When using https://github.com/mojohaus/aspectj-maven-plugin with an invalid JRE version, aspectj silently return without any error in MessageHolder:

if (SourceVersion.latest().ordinal() < MINIMAL_JRE_VERSION) {
System.err.println(MINIMAL_JRE_VERSION_ERROR);
if (doExit)
System.exit(-1);
return;

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 added a commit to philippe-granet/aspectj that referenced this issue Mar 14, 2024
@philippe-granet
Copy link
Author

MR proposed: #293

@kriegaex
Copy link
Contributor

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 kriegaex self-assigned this Mar 15, 2024
@kriegaex kriegaex changed the title aspectj-maven-plugin not fail if minimal JRE version is incorrect In-process compilation with less than minimal JRE version does not fail after printing warning Mar 15, 2024
kriegaex added a commit that referenced this issue Mar 15, 2024
When running AJC, throw an AbortException(MINIMAL_JRE_VERSION) instead
of just logging an error, if the minimal JRE version requirement is
violated. Otherwise, in-process compilation would not fail due to the
skipped System.exit(-1) that was used before. In-process compilation is,
for example, relevant for AspectJ Maven Plugin, but also for non-forked
executions of Plexus AspectJ via Maven Compiler.

I am not 100% sure that AbortException is the appropriate exception
type, because it was designed for an aborted compilation process and
here compilation has not even started yet, but it seems to work fine.

Relates to #269.
Fixes #292.

Signed-off-by: Alexander Kriegisch <[email protected]>
@kriegaex kriegaex added the bug Something isn't working label Mar 15, 2024
@kriegaex kriegaex added this to the 1.9.22 milestone Mar 15, 2024
@kriegaex
Copy link
Contributor

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

kriegaex added a commit that referenced this issue Mar 15, 2024
When running AJC, throw an AbortException(MINIMAL_JRE_VERSION) instead
of just logging an error, if the minimal JRE version requirement is
violated. Otherwise, in-process compilation would not fail due to the
skipped System.exit(-1) that was used before. In-process compilation is,
for example, relevant for AspectJ Maven Plugin, but also for non-forked
executions of Plexus AspectJ via Maven Compiler.

I am not 100% sure that AbortException is the appropriate exception
type, because it was designed for an aborted compilation process and
here compilation has not even started yet, but it seems to work fine.

Relates to #269.
Fixes #292.

Signed-off-by: Alexander Kriegisch <[email protected]>
@philippe-granet
Copy link
Author

philippe-granet commented Mar 15, 2024

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

Yes your alternative is OK for me, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants