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

GitHub Issue #59: JakartaEE9 source and target level #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

markus183
Copy link

maven compiler plugin - set source and target properties in parent pom.xml

Signed-off-by: Markus Stauffer [email protected]

maven compiler plugin - set source and target properties in parent pom.xml

Signed-off-by: Markus Stauffer <[email protected]>
@@ -97,6 +97,8 @@
</mailingLists>

<properties>
<maven.compiler.source>1.8</maven.compiler.source>
<maven.compiler.target>1.8</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.

wouldn't setting up release flag be better option? How is this going to guarantee that no SE 9+ APIs are used in the code to compiled?

Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't setting up release flag be better option?

I think it wouldn't for following reasons combined:

  • if release property is set, compiler-plugin uses it to call compiler regardless of actual JDK used, hence for JDK8 this happens:
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project org.eclipse.persistence.asm: Fatal error compiling: invalid flag: --release
  • not all children migrated to/enforce JDK > 1.8 yet.

Copy link
Member

Choose a reason for hiding this comment

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

  • not all children migrated to/enforce JDK > 1.8 yet.

how is this useful for those who did then? What are those having default compile with release 11 and additional compile step with release 8 supposed to do to adopt this change?

Copy link
Author

Choose a reason for hiding this comment

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

good point @lukasj . The release flag seems to be the better option if all children are building with jdk11. But I wonder if they do and/or are going to migrate?

Copy link
Member

Choose a reason for hiding this comment

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

Well I think it's better to 'force' those relying on JDK 8 to move forward than those on JDK 11 already to move backward

Copy link
Contributor

@pzygielo pzygielo Jun 3, 2020

Choose a reason for hiding this comment

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

how is this useful for those who did then?

There is no impact on them in such case. But using m*.c*.release would impact those that are not build-with-JDK9+ ready.

What are those having default compile with release 11 and additional compile step with release 8 supposed to do to adopt this change?

Nothing, as they already configured compiler plugin in its configuration section or with properties (which override the ones from parent should some be defined).

Adding m*.c*.source/target will mainly serve informational role as I see it. Because - I guess, children already configure compiler plugin which by default uses source/target=1.6 if not commanded otherwise.

Using --release is not enough to guarantee that no SE 9+ APIs are used in the code, because Cross-Compilation Options for javac says so, and some other steps have to be done, beside setting that property.

So, having <maven.compiler.target>1.8</maven.compiler.target>
could be JakartaEE declaration of class level, but I do not like idea of <maven.compiler.release>1.8</maven.compiler.release> now.

Choose a reason for hiding this comment

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

Using --release is not enough to guarantee that no SE 9+ APIs are used in the code, because Cross-Compilation Options for javac says so, and some other steps have to be done, beside setting that property.

This is false. On the same page, the description of --release reads:

Compiles against the public, supported and documented API for a specific VM version.

There are 2 possibilities:

  • the legacy way: -source+-target+-bootclasspath+-extdirs
  • the modern way: --release

As @lukasj already pointed out, just setting -source and -target is not sufficient to prevent usage of Java SE 9+ APIs.

Copy link
Contributor

@pzygielo pzygielo Jun 4, 2020

Choose a reason for hiding this comment

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

There are 2 possibilities:
* the legacy way: -source+-target+-bootclasspath+-extdirs
* the modern way: --release

Ouch, now I see. I was wrong. Thanks.
I agree - release is better 😄 then (once all children are prepared or it's agreed to force them to migrate to newer JDK).

Copy link
Author

Choose a reason for hiding this comment

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

https://openjdk.java.net/jeps/247
I think we have the following options for the parent pom.xml:

  1. Do nothing and abandon this PR. Handle everything in the child projects.
  2. Add maven.compiler.release=8 to the parent pom and enforce building with jdk9+ in all child projects.
  3. Add and activate two maven profiles for child projects building with either jdk8 or jdk9+.
    For example:
    3.1 profile for jdk9+ includes property release=8.
    3.2 profile for jdk8 includes source=8, target=8.

I don't understand why -bootclasspath and -extdirs is needed in case 3.2. There is no cross compilation happening. (I am curious btw if option 3 was envisioned like this for the ecosystem.)

Copy link
Contributor

@hboutemy hboutemy Dec 19, 2020

Choose a reason for hiding this comment

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

I don't understand why -bootclasspath and -extdirs is needed in case 3.2. There is no cross compilation happening.

because if you don't set these options, compiler may introduce a reference to a class that does not exist in the target JDK: this is one key thing that --release does = enforce classes from target JDK

one option often used to avoid setting -bootclasspath and -extdirs, but detect if something wrong happens is to use AnimalSniffer: https://www.mojohaus.org/animal-sniffer/

@ivargrimstad
Copy link
Member

I think this is outdated, so I suggest closing this

@pzygielo
Copy link
Contributor

pzygielo commented Nov 6, 2022

Today I think that this parent is not a good place to configure m-c-p. It should be limited to EF infra-related and release-time settings (like repository location, perhaps m-gpg-p, nexus-staging plugin, but not much more).

It seems that this parent doesn't share its lifecycle with Jakarta EE releases, so statement for Java level should be left to children.

@hboutemy
Copy link
Contributor

hboutemy commented Nov 6, 2022

the question of defining a default target JDK in parent POM remains open: is it something the project would find useful or not? (currently not done)

then the next question discussed here: JDK 8 or less or JDK 9 or more to build? in the latter case, it's better to set release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants