-
Notifications
You must be signed in to change notification settings - Fork 382
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
Point to the latest JDT and asm #9884
Conversation
3e12be6
to
25e9b91
Compare
Do you think it would make sense to also update SourceLevel to accept 17 as a value, and pass that to JDT? That should at least get us text blocks and sealed classes "for free" (though of course we still need tests). I think that makes sense to include in this PR, and give that a quick test so that we know that the new jdt actually does something. |
f5e28a1
to
9b37765
Compare
This version should also already support Java 21 (except some preview features). eclipse-jdt/eclipse.jdt.core#890 Most interesting would be record patterns and pattern matching for switch. |
@jnehlmeier unfortunately Ahmad has found that builds of JDT past 4.27 require a minimum of Java 17 to compile and run, so if we're dropping just Java 8 for now, we can't update to 4.29. Additionally, there is something funny happening with a new required dependency of the jdt core, and shading it into the gwt-dev.jar seems to prevent classes from being loaded in that jar correctly. If you want to push for going all the way to Java 21 now, you could take that up, discuss dropping both Java 8 and 11 on the mailing list, and work up a fix for the new batch dependency? Otherwise I continue to be in favor of a more incremental approach (especially as record/pattern matching is likely going to require more custom codegen to make sense in js). |
4db8576
to
f904c46
Compare
f904c46
to
608dc02
Compare
d430a04
to
46dd5c7
Compare
faa9aaf
to
1cf0047
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think this through - but I think you're going to need to re-introduce the isJava8 flag as isJava11 so that the Java 17 test won't try to be compiled with Java 11. LMK if you want a hand, this Ant wiring is a pain.
user/test-super/com/google/gwt/dev/jjs/super/com/google/gwt/dev/jjs/test/Java17Test.java
Show resolved
Hide resolved
1cf0047
to
012efff
Compare
Cherry-pick that last commit to its own PR? Looks like it will need to have at least one line wrapped too, to fix stylecheck. |
5b61a45
to
3aa51cb
Compare
user/test-super/com/google/gwt/dev/jjs/super/com/google/gwt/dev/jjs/test/Java17Test.java
Show resolved
Hide resolved
bb97ac6
to
b6ce395
Compare
b6ce395
to
eae2c83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some last minute cleanup. After #9905 is merged let's do one more CI run (to be sure github actions is happy to run on 11/17/21), then merge?
.github/workflows/full-check.yml
Outdated
@@ -29,7 +30,8 @@ jobs: | |||
- name: Checkout GWT tools into a sibling directory | |||
uses: actions/checkout@v4 | |||
with: | |||
repository: 'gwtproject/tools' | |||
repository: 'vegegoku/tools' | |||
ref: 'java-17-support' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the tools patch merged, revert this (in both yml files)
.github/workflows/full-check.yml
Outdated
@@ -1,4 +1,4 @@ | |||
# Run all tests and builds all aspects of GWT using Java 11 and 17. Runs | |||
# Run all tests and builds all aspects of GWT using Java 8, 11, and 17. Runs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Run all tests and builds all aspects of GWT using Java 8, 11, and 17. Runs | |
# Run all tests and builds all aspects of GWT using Java 11, 17, and 21. Runs |
dev/build.xml
Outdated
@@ -2,7 +2,7 @@ | |||
<property name="gwt.root" location=".."/> | |||
<property name="project.tail" value="dev"/> | |||
<property name="test.args" value="-ea"/> | |||
<property name="test.jvmargs" value="-ea"/> | |||
<property name="test.jvmargs" value="-ea -agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=8005"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert
Update build scripts to point to the latest compatible JDT [3.32.0.v20221108-1853]/[4.26] and the latest asm 9.6 fix gwtproject#9871 related to gwtproject#9869
Point CI to the forked tools repository that has java 17 support changes. fix gwtproject#9871 related to gwtproject#9869
Write tests to confirm the code compiles to javascript too, add a test to guard for using records until support is added. fix gwtproject#9871 related to gwtproject#9869
eae2c83
to
9c9b874
Compare
Update build scripts to point to the latest JDT [3.36.0.v20231115-1055] and the latest asm 9.6
fix #9871
related to #9869