jboss / jboss-parent-pom

JBoss Parent POM
27 stars 56 forks source link

Prefer maven.compiler.release over the other compiler properties #363

Open jamezp opened 3 weeks ago

jamezp commented 3 weeks ago

The maven.compiler.release property has been available for some time now. In Java 22+ I get the following warning when compiling:

[WARNING] location of system modules is not set in conjunction with -source 11
  not setting the location of system modules may lead to class files that cannot run on JDK 11
    --release 11 is recommended instead of -source 11 -target 11 because it sets the location of system modules automatically

This is remedied by setting <maven.compiler.release>11</maven.compiler.release> in the properties.

Over all this is a fairly simple and straight forward change.

That said, I wonder if we should take this a step further and break a couple things. Should we delete these properties?

<maven.compiler.target>11</maven.compiler.target>
<maven.compiler.source>11</maven.compiler.source>
<maven.compiler.testTarget>${maven.compiler.target}</maven.compiler.testTarget>
<maven.compiler.testSource>${maven.compiler.source}</maven.compiler.testSource>

<!--
    Options to override the compiler arguments directly on the compiler argument line to separate between what
    the IDE understands as the source level and what the Maven compiler actually use.
-->
<maven.compiler.argument.target>${maven.compiler.target}</maven.compiler.argument.target>
<maven.compiler.argument.source>${maven.compiler.source}</maven.compiler.argument.source>
<maven.compiler.argument.testTarget>${maven.compiler.testTarget}</maven.compiler.argument.testTarget>
<maven.compiler.argument.testSource>${maven.compiler.testSource}</maven.compiler.argument.testSource>

And the following from the maven-compiler-plugin configuration:

<source>${maven.compiler.argument.source}</source>
<target>${maven.compiler.argument.target}</target>
<testSource>${maven.compiler.argument.testSource}</testSource>
<testTarget>${maven.compiler.argument.testTarget}</testTarget>

Note, these are set if there is a ${basedir}/build-release-xx file exists. However, I'm not sure if this is working as expected. If I add the file, my effective-pom still doesn't not show <release>xx</release>. Possibly an issue with the help:effective-pom , but strange. I also wonder if every module in a multi-module project would need the same file.

I don't know how many projects use the old maven.compiler.argumen.* versions of the properties, but it seems time to remove them :)

One other minor thing to note. In IDEA I have to override the maven.compiler.source and maven.compiler.target properties it it complains when I use newer Java features. I don't yet know if this is because they are defined to 11 or it's just a bug in IDEA, but it's something to look at if we decide to remove the maven.compiler.source and maven.compiler.target properties.

jamezp commented 3 weeks ago

I meant to add. I don't mind doing the work here, but I figured we should discuss it first about the preferred approach.

dmlloyd commented 3 weeks ago

I generally support this idea. It would be good to meet these requirements:

It might be the case that we have to set source and target as well to make IDE integration work. If so, these shall be set from the release version. It would be nice if they can be non-overridable but alas, this isn't possible that I know of.

jamezp commented 3 weeks ago

Given that there is a property for <release/> we don't actually need to set it in the compiler plugins configuration. IoW

<configuration>
    <release>${maven.compiler.release}</release>
</configuration>

is not needed. It would really just be about overriding the property in the <properties/> section.

The IDE testing will take some checking, but yeah simply doing something like:

<maven.compiler.release>11</maven.compiler.release>
<maven.compiler.target>${maven.compiler.release}</maven.compiler.target>
<maven.compiler.source>${maven.compiler.release}</maven.compiler.source>

is pretty simple. We could just add a comment as to why it's added.

dmlloyd commented 3 weeks ago

OK. It would be nice to get to the bottom of the problem you saw with the effective POM though. Also we need to make sure that setting that property isn't overriding all of the release set in the MR JAR profiles (test building e.g. jboss-marshalling to check that). If it is, then we have a problem.

jamezp commented 3 weeks ago

Okay cool. I'll do some digging then. I started making changes, then decided we should discuss it first.