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.

Sign up for GitHub

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

[MCOMPILER-548] JDK 21 throws annotations processing warning that can not be turned off #200

Merged
merged 2 commits into from
Mar 6, 2024
Merged

[MCOMPILER-548] JDK 21 throws annotations processing warning that can not be turned off #200

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f91043b
[MCOMPILER-548] JDK 21 throws annotations processing warning that can…
hgschmie Oct 4, 2023
eb0b643
Add property for proc parameter
slawekjaranowski Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,13 +294,16 @@ public abstract class AbstractCompilerMojo extends AbstractMojo {
* <ul>
* <li><code>none</code> - no annotation processing is performed.</li>
* <li><code>only</code> - only annotation processing is done, no compilation.</li>
* <li><code>full</code> - annotation processing and compilation.</li>
* </ul>
*
* <code>full</code> is the default. Starting with JDK 21, this option must be set explicitly.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if I do not set it? This seems to contradict the wording from line 276:

If not set, both compilation and annotation processing are performed at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you don't set it on JDK 21, you will be greeted with a warning that "soon, not setting proc:full will result in no longer doing annotation processing".

It is a very unsatisfying situation:

JDK < 21: proc:full is not allowed, results in error: invalid flag: -proc:full
JDK == 21: proc:full works, a warning is reported without it
JDK > 21: proc:full is required at some point, no more annotation processing without it

which puts the compiler plugin in the unfortunate situation of having to check the version of javac first before either not passing in -proc:full (because it is JDK < 21 and it is set by default) or passing it in for JDK >= 21.

That's why the comment reads "requires more thought". I have not found time to deal with it. Right now, I have a conditional profile in my poms... 😵‍💫

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, just hit the same: I explicitly wanted to disable proc (as am not using it, but unintentionally there ARE APs present on classpath, like Sisu AP is), so I set in TLP proc=none. Cool so far. Then in one module I needed AP, but as parent had proc=none, I had to enable, so I did proc=full, just to figure, that it breaks everything below Java 21...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another solution (which is probably intended by the JDK folks):
I used to do two executions in my projects:

  • generate-sources: execute with proc:only
  • default-compile: execute with proc:none

Worked like a charm and only changed it because Tamas hinted me to do so (because with this setup, some things like caching, incremental compilation etc. won't work that well or not at all).
However, this solution would not run into those problems at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and I keep forgetting about this, and for me this sounds like "most correct" solution as well: split steps and use existing phases even we provide.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use conditions depending on the javac version

fact is we don't know with javac version what is supported since several vendors did backport full support.

we can add a property for proc parameter and property can be overridden in child modules with empty value.

It is already the case AFAIK (side note being adding properties for compiler args is quite misleading and lead to broken configuration more than it helps IMHO).

-> https://maven.apache.org/plugins/maven-compiler-plugin/compile-mojo.html#proc

Think we don't have much choice to split the default compilation in 2 runs if there is at least one processor available (path or serviceloader discovery) and proc is not configured in compiler properties or args.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this allow to override apache/maven-parent#157

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we can use a property for that.

My suggestion:
Leave it like in this PR for m-c-p 3. Detection is "smart" and will many builds, because they of course will have something like this already configured.

For m-c-p 4.x use two phases all the time, even if there's no APT. An empty run (no sources) doesn't hurt.

We can define properties for those as well, of course.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, shouldnt we revert the parent change? except it is a breaking change it is also unexpected IMHO.

side note: this does not allow more than compilerArgs but enables to get conflicting args now ;).

Agree with you @bmarwell if v4 arrives at ~the same time than v3.x+1 since 21 starts to be used and everybody creates the verbose way in the pom which is very unlikely IMHO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that we talk about one line in documentation too much 😄

*
* @since 2.2
* @see <a href="https://docs.oracle.com/en/java/javase/17/docs/specs/man/javac.html#option-proc">javac -proc</a>
* @see <a href="https://docs.oracle.com/en/java/javase/17/docs/specs/man/javac.html#annotation-processing">javac Annotation Processing</a>
*/
@Parameter
@Parameter(property = "maven.compiler.proc")
private String proc;

/**
Expand Down