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

Support running on JDK 17 #1281

Merged
merged 22 commits into from
Jun 27, 2023
Merged

Support running on JDK 17 #1281

merged 22 commits into from
Jun 27, 2023

Conversation

msridhar
Copy link
Member

With this change, WALA's regression tests pass on JDK 17, and WALA can also load and process some JDK 17 bytecodes. We have not thoroughly tested recent bytecode features and they may still not work, but whatever is exercised by current regression tests seems to be working. Getting things working on JDK 17 required various minor changes.


// On JDK 17, deprecation errors in ECJ cannot be disabled when compiling JLex code. So, we disable
// the ECJ task.
tasks.named("compileTestJavaUsingEcj") { enabled = false }
Copy link
Member Author

Choose a reason for hiding this comment

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

@liblit this is a JDK 17 specific issue, and I have no idea how to fix it. If you have a better thought here let me know. If you like we can conditionally disable this task only on JDKs with version greater than 11, or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to keep this task enabled on JDKs where we can make it work. There's still some value in the extra checks that ECJ will apply to those builds.

In consideration of your eagerness to move this pull request forward, I will approve it in its current form. However, I would appreciate it if you could follow up with a pull request that restores this task in at least some subset of our builds. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

I have limited disabling the task to JDK 17+: bc4a19c

@@ -23,7 +23,7 @@ org.eclipse.jdt.core.compiler.problem.assertIdentifier=error
org.eclipse.jdt.core.compiler.problem.autoboxing=ignore
org.eclipse.jdt.core.compiler.problem.comparingIdentical=error
org.eclipse.jdt.core.compiler.problem.deadCode=warning
org.eclipse.jdt.core.compiler.problem.deprecation=error
org.eclipse.jdt.core.compiler.problem.deprecation=disabled
Copy link
Member Author

Choose a reason for hiding this comment

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

Here, somehow ECJ is not getting the release parameter we pass to the Java compiler, so it is reporting warnings for methods deprecated after JDK 11. Maybe there is a way to tell ECJ which Java version we are targeting, and then it won't report such errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know of a way to do that offhand, but I haven't actively investigated. If you think there's a decent chance that I'll find something you missed, feel free to create a new issue as a reminder and assign it to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this one is just safe to leave as is. We already compile util with -Werror so any true deprecation issues should cause javac itself to fail.

@msridhar msridhar requested a review from liblit June 24, 2023 23:04
@msridhar
Copy link
Member Author

Before landing, we (I) should updated the list of required CI jobs to include the JDK 17 job.

liblit
liblit previously approved these changes Jun 27, 2023

// On JDK 17, deprecation errors in ECJ cannot be disabled when compiling JLex code. So, we disable
// the ECJ task.
tasks.named("compileTestJavaUsingEcj") { enabled = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to keep this task enabled on JDKs where we can make it work. There's still some value in the extra checks that ECJ will apply to those builds.

In consideration of your eagerness to move this pull request forward, I will approve it in its current form. However, I would appreciate it if you could follow up with a pull request that restores this task in at least some subset of our builds. Thanks!

@@ -23,7 +23,7 @@ org.eclipse.jdt.core.compiler.problem.assertIdentifier=error
org.eclipse.jdt.core.compiler.problem.autoboxing=ignore
org.eclipse.jdt.core.compiler.problem.comparingIdentical=error
org.eclipse.jdt.core.compiler.problem.deadCode=warning
org.eclipse.jdt.core.compiler.problem.deprecation=error
org.eclipse.jdt.core.compiler.problem.deprecation=disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know of a way to do that offhand, but I haven't actively investigated. If you think there's a decent chance that I'll find something you missed, feel free to create a new issue as a reminder and assign it to me.

@msridhar msridhar enabled auto-merge (squash) June 27, 2023 20:26
@msridhar msridhar merged commit c6f20aa into wala:master Jun 27, 2023
@msridhar msridhar deleted the Java-17 branch June 27, 2023 21:50
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.

2 participants