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

Build SVM with JDK 19. #263

Closed
wants to merge 1 commit into from
Closed

Build SVM with JDK 19. #263

wants to merge 1 commit into from

Conversation

oraluben
Copy link
Contributor

@oraluben oraluben commented Jun 6, 2022 •
edited
Loading

@oraluben oraluben mentioned this pull request Jun 6, 2022
Closed
@oraluben oraluben changed the title Build SVM with Loom JDK. Build SVM with JDK 19. Jun 17, 2022
@oraluben
Copy link
Contributor Author

Hi @dougxc , the current version of requiresConcealed seems to have issue.
IIUC it's checking if the current jdk.javaCompliance is in the range of java_compliance, i.e.,

if java_compliance.__contains__(jdk.javaCompliance)

, i.e.,

if jdk.javaCompliance in java_compliance

. Instead mx now has (negation is not important here):

mx/mx_javamodules.py

Line 1121 in c900245

if java_compliance not in jdk.javaCompliance:

I met this issue while having something like:
https://github.com/oracle/graal/blob/e0f7b9735df345077373cdf82df499e923ca30d3/substratevm/mx.substratevm/suite.py#L229-L236

Does this fix look good to you: e4128a5

dougxc
dougxc reviewed Jun 27, 2022
View reviewed changes
mx.py
@@ -7670,6 +7670,8 @@ def prepare(self, sourceFiles, project, outputDir, classPath, processorPath, sou
javacArgs += ['-proc:none']
c = str(compliance)
javacArgs += ['-target', c, '-source', c]
if c == '19' and self.jdk.javaCompliance == '19':
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be >= instead of ==. What happens when we get to Java 20 and loom is still in preview?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is not ready for review, and this is a temporary solution for local tests. Besides, I think the preview feature in JDK is very tricky for Graal, E.g. jdk.internal.access.foreign.MemorySegmentProxy has been moved to a preview feature in JDK-8282191, which affects some substitutions in Target_java_nio_DirectByteBuffer_JDK17OrLater.java.

dougxc
dougxc reviewed Jun 27, 2022
View reviewed changes
mx_javacompliance.py Outdated
len(other._parts) == 1 and
(
other._high_bound() is None or
other._high_bound() == other.value
Copy link
Member

Choose a reason for hiding this comment

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

How can this happen? Can you please add extra tests to _test() in mx_javacompliance.py to cover this case as well as any other cases made relevant by your changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll add tests for range checks.

@oraluben oraluben mentioned this pull request Jun 27, 2022
Closed
@oraluben oraluben closed this Jul 2, 2022
@oraluben oraluben reopened this Jul 2, 2022
@oraluben oraluben force-pushed the build-with-19-loom branch 2 times, most recently from 994e34a to 358705b Compare July 7, 2022 06:07
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jul 21, 2022
@oraluben oraluben closed this Aug 22, 2022
@oraluben oraluben deleted the build-with-19-loom branch October 17, 2022 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougxc dougxc dougxc left review comments

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@oraluben @dougxc @graalvmbot