Skip to content

use maven.compiler.release instead of .target#165

Closed
bmarwell wants to merge 1 commit intomasterfrom
MNG-8061_java17
Closed

use maven.compiler.release instead of .target#165
bmarwell wants to merge 1 commit intomasterfrom
MNG-8061_java17

Conversation

@bmarwell
Copy link
Contributor

@bmarwell bmarwell commented Feb 27, 2024

Change the properties as mentioned by Sylvester, so we have #224 for the actual Java 17 change

@bmarwell
Copy link
Contributor Author

Will redo this on maven-core first.

@olamy
Copy link
Member

olamy commented Feb 28, 2024

this means all plugins will be 17 required.
so all plugins upgrading to to this parent will not work for users of maven 3.x and jdk 8/11.
sounds an issue to solve as it would mean eventually having branches for plugins in case of major bugs and/or security issues.

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

We can not bump JDK in parent ... it will have an impact for all plugins ... but plugins should stay at JDK 8

@olamy
Copy link
Member

olamy commented Feb 28, 2024

We can not bump JDK in parent ... it will have an impact for all plugins ... but plugins should stay at JDK 8

plugins parent can have

    <javaVersion>8</javaVersion>
    <maven.compiler.source>${javaVersion}</maven.compiler.source>
    <maven.compiler.target>${javaVersion}</maven.compiler.target>

but anyway this need more residents as I guess some plugins will now maven4 API directly so there will be branches...

@slachiewicz
Copy link
Member

can we split this PR to 2 - one to replace parameters to use .target - with the latest m-compiler-p it no longer requires special profile. That is only technical cleanup.
And then dedicated - only to bump java to 17 as in vote?

@hboutemy
Copy link
Member

and can we avoid MNG issue for a change that is related to Maven parent POM?

@bmarwell
Copy link
Contributor Author

and can we avoid MNG issue for a change that is related to Maven parent POM?

Sure, both works for me. Will update soon.

@bmarwell bmarwell changed the title [MNG-8061] Maven: Require Java 17 Require Java 17 Dec 30, 2024
@bmarwell bmarwell force-pushed the MNG-8061_java17 branch 2 times, most recently from 457ab8e to a82b8eb Compare December 30, 2024 10:03
@bmarwell
Copy link
Contributor Author

bmarwell commented Dec 30, 2024

can we split this PR to 2 - one to replace parameters to use .target - with the latest m-compiler-p it no longer requires special profile. That is only technical cleanup.

What exactly are you thinking of?

// Edit: got it :)

@bmarwell bmarwell changed the title Require Java 17 use maven.compiler.release instead of .target Dec 30, 2024
@bmarwell bmarwell marked this pull request as ready for review December 30, 2024 10:18
@bmarwell bmarwell requested a review from slachiewicz December 30, 2024 10:18
Copy link
Contributor

@Bukama Bukama left a comment

Choose a reason for hiding this comment

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

Thanks for updating this dormant PR :)

+1 (nb)

@slachiewicz slachiewicz added this to the MAVEN-44 milestone Dec 30, 2024
<javaVersion>8</javaVersion>
<maven.compiler.source>${javaVersion}</maven.compiler.source>
<maven.compiler.target>${javaVersion}</maven.compiler.target>
<maven.compiler.release>${javaVersion}</maven.compiler.release>
Copy link
Member

Choose a reason for hiding this comment

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

Should we left old properties and allow to m-compiler-p to use proper one?

What java version will be used by m-compiler-p when we have only maven.compiler.release and build on JDK 8?

Copy link
Member

Choose a reason for hiding this comment

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

Since version 3.13.0 of the Compiler Plugin together with the default javac compilerId you don't need conditional parametrisation of release. The release parameter will only be effective for Java 9 or above, otherwise the source and target will be passed to the compiler.

Copy link
Member

Choose a reason for hiding this comment

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

yes, leave original .source and .target properties.

Copy link
Contributor Author

@bmarwell bmarwell Jan 2, 2025

Choose a reason for hiding this comment

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

Done for main properties, not sure whether we need them in other places, too

Copy link
Member

Choose a reason for hiding this comment

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

in ale place should be the same

@bmarwell
Copy link
Contributor Author

Do we want/need an issue? Merge now or later?

@slawekjaranowski
Copy link
Member

I think that we need preserve also source and target - it will be used for jdk8

Comment on lines -133 to -134
<invoker.maven.compiler.source>${maven.compiler.source}</invoker.maven.compiler.source>
<invoker.maven.compiler.target>${maven.compiler.target}</invoker.maven.compiler.target>
Copy link
Member

Choose a reason for hiding this comment

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

please also preserver here

Comment on lines -153 to -154
<maven.compiler.source>${invoker.maven.compiler.source}</maven.compiler.source>
<maven.compiler.target>${invoker.maven.compiler.target}</maven.compiler.target>
Copy link
Member

Choose a reason for hiding this comment

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

please also preserver here

Comment on lines -180 to -181
<invoker.maven.compiler.source>${maven.compiler.source}</invoker.maven.compiler.source>
<invoker.maven.compiler.target>${maven.compiler.target}</invoker.maven.compiler.target>
Copy link
Member

Choose a reason for hiding this comment

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

please also preserver here

Comment on lines -200 to -201
<maven.compiler.source>${invoker.maven.compiler.source}</maven.compiler.source>
<maven.compiler.target>${invoker.maven.compiler.target}</maven.compiler.target>
Copy link
Member

Choose a reason for hiding this comment

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

please also preserver here

<javaVersion>8</javaVersion>
<maven.compiler.source>${javaVersion}</maven.compiler.source>
<maven.compiler.target>${javaVersion}</maven.compiler.target>
<maven.compiler.release>${javaVersion}</maven.compiler.release>
Copy link
Member

Choose a reason for hiding this comment

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

in ale place should be the same

@slawekjaranowski slawekjaranowski added the waiting-for-feedback Waiting for 90 days until issues or pull request will be closed label Feb 16, 2025
@slawekjaranowski slawekjaranowski removed this from the MAVEN-44 milestone Mar 2, 2025
@github-actions
Copy link

github-actions bot commented May 2, 2025

This pull request is stale because it has been waiting for feedback for 60 days. Remove the stale label or comment on this PR, or it will be automatically closed in 30 days.

@github-actions github-actions bot added the Stale label May 2, 2025
@desruisseaux
Copy link

Should we left old properties and allow to m-compiler-p to use proper one?
I think that we need preserve also source and target - it will be used for jdk8

Actually, if using version 4 of maven-compiler-plugin, the old source and target properties would need to be removed. The reason is because that plugin tries to be more straightforward regarding the mapping of configuration options to javac parameters (e.g. less of Maven's own opinion of what the default values should have been), and javac does not accept source or target to be specified in same time as release.

@github-actions github-actions bot removed the waiting-for-feedback Waiting for 90 days until issues or pull request will be closed label May 2, 2025
@Bukama
Copy link
Contributor

Bukama commented May 2, 2025

Should we left old properties and allow to m-compiler-p to use proper one?
I think that we need preserve also source and target - it will be used for jdk8

Actually, if using version 4 of maven-compiler-plugin, the old source and target properties would need to be removed. The reason is because that plugin tries to be more straightforward regarding the mapping of configuration options to javac parameters (e.g. less of Maven's own opinion of what the default values should have been), and javac does not accept source or target to be specified in same time as release.

But it can still apply source or target when compiling against Java 8?

@desruisseaux
Copy link

But it can still apply source or target when compiling against Java 8?

Yes, but this is not useful since the plugin requires Java 17 for execution anyway. Therefore, user should specify --release 8 instead, except when using forked JVM.

If we keep default values for source and target, then in the current version of compiler plugin 4, the configuration needs to look like that:

<source/>  <!-- Erase the source configuration inherited from the parent pom -->
<target/>
<release>8</release>

@Bukama
Copy link
Contributor

Bukama commented May 3, 2025

I guess I was not clear what I mean. As of kno (3.x) you can specifiy release8 and the compiler plugin turns that into source/target if you are on JDK 8, which does not know release. But with Maven 4 and JDK 17 this is only a thing if you use another JDK via toolchains or something, right?

so I'm okay to remove sourge/target parameters in the 4x plugin

@slachiewicz
Copy link
Member

Maven-parent is used for other Maven projects that we maintain, where the base is java 8. If we remove it now - we need to adjust around 100 repos. Please think also about backward compatibility for us but also for users (m-compiler-p) as they usually expect that the project should build smoothly with Maven 3 and 4 should be also without issues

@slawekjaranowski slawekjaranowski added the waiting-for-feedback Waiting for 90 days until issues or pull request will be closed label Jul 19, 2025
@github-actions github-actions bot removed the Stale label Jul 20, 2025
@bmarwell bmarwell closed this Aug 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-feedback Waiting for 90 days until issues or pull request will be closed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants