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 testing with JDK 17 #1108

Closed
wants to merge 6 commits into from
Closed

Conversation

jonpryor
Copy link
Member

Context: dotnet/android#8029

Update <JdkInfo/> task to emit a new $(Java*MajorVersion) MSBuild property. This is used by src/Java.Base so that it knows which JDK version it's binding.

Begin explicitly qualifying all $(Jdk*) and $(Java*) MSBuild properties to explicitly disambiguate between JDK 1.8 and JDK-11 contexts. Eventually we may "swap" things so that properties without a version are for e.g. JDK 11 (17, etc.) and $(Jdk8*) is for JDK 1.8-requiring contexts.

Note: to get this to build we had to update src/Java.Base to support binding against JDK-17, which was an expected diversion. This "JDK-17 Java.Base binding" was a "time limited" effort.

Context: dotnet/android#8029

Update `<JdkInfo/>` task to emit a new `$(Java*MajorVersion)`
MSBuild property.  This is used by `src/Java.Base` so that it knows
which JDK version it's binding.

Begin explicitly qualifying all `$(Jdk*)` and `$(Java*)` MSBuild
properties to explicitly disambiguate between JDK 1.8 and JDK-11
contexts.  Eventually we may "swap" things so that properties without
a version are for e.g. JDK 11 (17, etc.) and `$(Jdk8*)` is for
JDK 1.8-requiring contexts.

Note: to get this to *build* we had to update `src/Java.Base` to
support binding against JDK-17, which was an expected diversion.
This "JDK-17 Java.Base binding" was a "time limited" effort.
Unit tests "manually" read the `JdkInfo*.props` files, and since
the file names and property names changed, things broke.  Oops.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 11, 2023
Java.Interop needs JDK-17 support first!
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 11, 2023
Java.Interop needs JDK-17 support first!
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 12, 2023
Java.Interop needs JDK-17 support first!
Use Gradle 8.1.1, fix `build.gradle` so it works with Gradle 8.1.1.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 12, 2023
Java.Interop needs JDK-17 support first!
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 12, 2023
Java.Interop needs JDK-17 support first!
JdkInfo.props rename strikes again!
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 12, 2023
Java.Interop needs JDK-17 support first!
Looks like tools/java-source-utils tests still require JDK 1.8
to pass on CI?
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 12, 2023
Java.Interop needs JDK-17 support first!
@jonpryor
Copy link
Member Author

"Random *argh!*s"

  • JDK-17 doesn't work with Gradle 5.1.1, so we needed to update Gradle. OK, but…
  • …our build.gradle files were not compatible with Gradle 8.1.1. Figuring out how to fix that was "fun".
  • Renaming JdkInfo.props to JdkInfo-8.props and $(JdkJvmPath) to $(JdkJvm8Path)/etc. blew things up good. I expected breakage, but not quite this much. See also [xaprepare] Provision Microsoft OpenJDK 17.0.7 android#8029, much of which is dealing with these filename & property name changes.
  • That said, I like the change, in principal. Consider tools/java-source-utils, in which we "knew" that some tests required a JDK 1.8 environment to pass, but this requirement was very easy to overlook and skip. Having explicit $(Java8SdkDirectory) and $(Java11SdkDirectory) variables makes things clearer to me.
  • …but the naming is now "wack", because for this experiment we're using JDK-17, but using the $(Java11SdkDirectory)/etc. variables. The numbers don't match.
  • We should drop support for JDK 1.8 at some point…
  • …but I think we'll still want to support using multiple JDKs "at the same time", specifically JDK-11 + JDK-17. Why? For src/Java.Base (groan), so that the API it produces doesn't "float" (and require additional effort whenever we want to use a newer JDK for everything else).
    • Though maybe the fix for this would be to checkin the "known good" api.xml built against JDK-11, instead of requiring that we process JDK-11's java.base.jmod file. Checking in api.xml would make things easier, but also means we don't have a "full stack parsing + generator" project in the tree.

`<SetEnvironmentVariable/>` was failing because `JI_JVM_PATH` was
being set to an empty string.
@jonpryor jonpryor marked this pull request as ready for review May 13, 2023 00:38
@jonpryor jonpryor requested a review from jpobst May 13, 2023 00:38
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 13, 2023
@jonpryor jonpryor marked this pull request as draft May 15, 2023 19:20
@jonpryor
Copy link
Member Author

Superseded by PR #1141.

@jonpryor jonpryor closed this Sep 11, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant