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

Jump to bottom

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

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

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions parent/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

Copy link

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 •
edited
Loading

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 •
edited
Loading

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/

<sonatypeOssDistMgmtNexusUrl>https://jakarta.oss.sonatype.org/</sonatypeOssDistMgmtNexusUrl>
<sonatypeOssDistMgmtSnapshotsUrl>${sonatypeOssDistMgmtNexusUrl}content/repositories/snapshots/</sonatypeOssDistMgmtSnapshotsUrl>
<sonatypeOssDistMgmtStagingUrl>${sonatypeOssDistMgmtNexusUrl}content/repositories/staging/</sonatypeOssDistMgmtStagingUrl>
Expand Down