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

supporting JDK17 #179

Merged
merged 2 commits into from
Oct 31, 2021
Merged

supporting JDK17 #179

merged 2 commits into from
Oct 31, 2021

Conversation

li0nr
Copy link
Collaborator

@li0nr li0nr commented Oct 26, 2021

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Hey,
I am opening this PR to allow Oak to work with JDK>=8, with minimal effort and without having to change a lot of code.
While still using Unsafe and other sealed packages, though it is not encouraged anymore, and there exists some new alternatives that could replace them.

my changes introduce a new profile to the poms that we in order compile with JDK>= 8 and to run with JDK>=11
two tests fail when ran with JDK17, due to the changes introduced to the JDK past JDK8, which I need your help with (I still think there would be minimal effort to fix them)

core/pom.xml Outdated
</plugins>
</build>
</profile>
</profiles>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please adjust the XML formatting

@sanastas sanastas merged commit 9e8d79c into yahoo:master Oct 31, 2021
Comment on lines +255 to +262
<profile>
<activation>
<jdk>[9,)</jdk>
</activation>
<properties>
<maven.compiler.release>17</maven.compiler.release>
</properties>
</profile>
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails the compilation if you don't have Java 17 installed. Why is that necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

it fails if you have something JDK>= 9 and it is not 17.
if you compile it with JDK8 it runs as usual.
my intention in the PR is to make Oak work with JDK 17 and that why I put 17 in line 260.

if you have JDK 11 you may change the line from 17 to 11 and then it will work with JDK 11.
Or if you wanna just run with 11 and compile 8, then we can change the activation value from [9, to [17,
then this effect will take place just when we detect system with JDK>= 17

Copy link
Contributor

Choose a reason for hiding this comment

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

So why is it necessary? If you compile on JDK17, then it will compile for Java 17 by default, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand it, if this is not present even if we JDK=17 installed
since the maven source-target is set to 8, it will still compile with 8.
In my changes it will run and compile with 17

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. This is indeed true. I suggest that for now, when compiling for a specific JVM version, simply change JAVA_HOME to the appropriate value.

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.

4 participants