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

Only build and include svm-foreign when building with JDK > 21 #398

Closed
wants to merge 1 commit into from
Closed

Only build and include svm-foreign when building with JDK > 21 #398

wants to merge 1 commit into from

Conversation

zakkak
Copy link
Collaborator

@zakkak zakkak commented Jan 26, 2024 •
edited
Loading

Since oracle/graal#7980 the foreign API is only
available for JDK >= 22 builds.

Fixes error message:

dependency named SVM_FOREIGN was removed:
  distribution SVM_FOREIGN was removed as all its dependencies were removed:
    project com.oracle.svm.hosted.foreign was removed as JDK 22 is not available

When trying to build with JDK 21.

If accepted I will backport it to 24.0 as well.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jan 26, 2024
@zakkak zakkak requested a review from jerboaa January 26, 2024 13:40
jerboaa
jerboaa reviewed Jan 26, 2024
View reviewed changes
Copy link
Collaborator

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

I'm not convinced we need JDK 21 build support, but your mileage may vary.

build.java Show resolved Hide resolved
@jerboaa
Copy link
Collaborator

jerboaa commented Jan 26, 2024

Also note that IMO it's an (upstream) bug to add attempt to add svm-foreign.jar if --enable-preview is specified. I'm pretty sure a Graal master build with labsjdk 21 and --enable-preview will fail since in that case the native image generator unconditionally attempts to add the dir containing svm-foreign.jar (not present) to the class/module path.

@zakkak
Copy link
Collaborator Author

zakkak commented Jan 26, 2024

Also note that IMO it's an (upstream) bug to add attempt to add svm-foreign.jar if --enable-preview is specified. I'm pretty sure a Graal master build with labsjdk 21 and --enable-preview will fail since in that case the native image generator unconditionally attempts to add the dir containing svm-foreign.jar (not present) to the class/module path.

I think that's fixed in oracle/graal#7980

@zakkak zakkak force-pushed the 2024-01-26-only-handle-foreign-after-21 branch 2 times, most recently from 3ed73c9 to 81a8e4e Compare January 26, 2024 14:49
@jerboaa
Copy link
Collaborator

jerboaa commented Jan 26, 2024

CI fails due to too new JDK 23:

Error: Static modifier mismatch: private void com.oracle.svm.core.thread.Target_java_lang_VirtualThread.notifyJvmtiHideFrames(boolean), private static native void java.lang.VirtualThread.notifyJvmtiHideFrames(boolean)

Fixed once this PR merges: oracle/graal#8262

@jerboaa
Copy link
Collaborator

jerboaa commented Jan 26, 2024

Also note that IMO it's an (upstream) bug to add attempt to add svm-foreign.jar if --enable-preview is specified. I'm pretty sure a Graal master build with labsjdk 21 and --enable-preview will fail since in that case the native image generator unconditionally attempts to add the dir containing svm-foreign.jar (not present) to the class/module path.

I think that's fixed in oracle/graal#7980

You are right. Thinking some more about this, we don't need the SVM_FOREIGN dependency on JDK 23 either. Not sure why the build doesn't outright fail...

@zakkak
Copy link
Collaborator Author

zakkak commented Jan 31, 2024

we don't need the SVM_FOREIGN dependency on JDK 23 either. Not sure why the build doesn't outright fail...

Why is that? All the checks in GraalVM are for >= 22.

@zakkak
Only build and include svm-foreign when building with JDK > 21
f332451 
Since oracle/graal#7980 the foreign API is only
available for JDK >= 22 builds.
@zakkak zakkak force-pushed the 2024-01-26-only-handle-foreign-after-21 branch from 81a8e4e to f332451 Compare January 31, 2024 12:29
jerboaa
jerboaa requested changes Feb 1, 2024
View reviewed changes
Copy link
Collaborator

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

It makes little sense to me to try to keep JDK 21 compatibility in master. The entire JEP 454 story in GraalVM seems not 100% ready yet. See graalvm/mandrel#678

build.java
@@ -855,7 +858,7 @@ class Mx
Pattern.compile("\"version\"\\s*:\\s*\"([0-9.]*)\"");

static final List<BuildArgs> BUILD_JAVA_STEPS = List.of(
BuildArgs.of("--no-native", "--dependencies", "SVM,SVM_FOREIGN,GRAAL_SDK,SVM_DRIVER,SVM_AGENT,SVM_DIAGNOSTICS_AGENT")
BuildArgs.of("--no-native", "--dependencies", "SVM,GRAAL_SDK,SVM_DRIVER,SVM_AGENT,SVM_DIAGNOSTICS_AGENT" + (Runtime.version().feature() > 21 ? ",SVM_FOREIGN" : ""))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if it makes sense to conditionalize on this for JDK 21. IIRC, this will only work with JDK 22+ Between JDK 21 and JDK 22 the API changed slightly, so I'd be surprised if this still worked. I think it's also something the GraalVM team is aware of:
oracle/graal#8012 (see the note about "Foreign Function and Memory API support")

We have the 23.1 branch for JDK 21 compatible builds. Not sure if we should fix it in master.

build.java
if (Runtime.version().feature() > 21)
{
logger.debugf("Copy svm-preview...");
final Path svmForeign = mandrelJavaHome.resolve(Path.of("lib", "svm-preview", "builder", "svm-foreign.jar"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

svm-preview folder is wrong as a target for JDK 22+. See graalvm/mandrel#678 for some details.

@jerboaa
Copy link
Collaborator

jerboaa commented Feb 1, 2024

we don't need the SVM_FOREIGN dependency on JDK 23 either. Not sure why the build doesn't outright fail...

Why is that? All the checks in GraalVM are for >= 22.

Sorry, I was wrong. However, there is a different problem with getting svm-foreign.jar properly installed for JDK 23. I'll take a look.

@jerboaa
Copy link
Collaborator

jerboaa commented Feb 5, 2024

we don't need the SVM_FOREIGN dependency on JDK 23 either. Not sure why the build doesn't outright fail...

Why is that? All the checks in GraalVM are for >= 22.

Sorry, I was wrong. However, there is a different problem with getting svm-foreign.jar properly installed for JDK 23. I'll take a look.

See #399, which should fix the svm-foreign.jar installation.

@zakkak
Copy link
Collaborator Author

zakkak commented Feb 6, 2024

It makes little sense to me to try to keep JDK 21 compatibility in master. The entire JEP 454 story in GraalVM seems not 100% ready yet. See graalvm/mandrel#678

The primary reason for opening this was to prevent build failures when trying to build with JDK 21 so that we can see the actual blocker/issue which is graalvm/mandrel#598

Closing this for now as there are no plans to build master with JDK 21, see graalvm/mandrel#598 (comment)

@zakkak zakkak closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerboaa jerboaa jerboaa requested changes

Assignees
No one assigned
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zakkak @jerboaa