Skip to content

[Xamarin.Android.Build.Tasks] Bump MinimumSupportedJavaVersion=17 #9257

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
Merged
jonpryor merged 1 commit into from
Aug 29, 2024
Merged

[Xamarin.Android.Build.Tasks] Bump MinimumSupportedJavaVersion=17 #9257

jonpryor merged 1 commit into from
Aug 29, 2024

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Aug 29, 2024

Context: #9159 (comment)

Bump $(MinimumSupportedJavaVersion) to 17.0, and base this value on the Configurables.Defaults.MicrosoftOpenJDK17Version value so that if (when) we bump the JDK we build against, the major version value of $(MinimumSupportedJavaVersion) follows suit.

Additionally, remove the $(MinimumSupportedJavaVersion) definition in Microsoft.Android.Sdk.DefaultProperties.targets so that it's only defined in one location.

@jonpryor
[Xamarin.Android.Build.Tasks] Bump MinimumSupportedJavaVersion=17
14e86f1 
Context: #9159 (comment)

Bump `$(MinimumSupportedJavaVersion)` to 17.0, and base this value on
the `Configurables.Defaults.MicrosoftOpenJDK17Version` value so that
if (when) we bump the JDK we build against, the major version value
of `$(MinimumSupportedJavaVersion)` follows suit.

Additionally, remove the `$(MinimumSupportedJavaVersion)` definition
in `Microsoft.Android.Sdk.DefaultProperties.targets` so that it's
only defined in one location.
jonathanpeppers
jonathanpeppers reviewed Aug 29, 2024
View reviewed changes
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

Is this raising the min JDK to 17? for our whole product?

Should we consider:

  • JDK 11 minimum for .NET 9
  • JDK 17 minimum for .NET 10

Just wondering what might break if this goes out in RC 2.

@jonpryor
Copy link
Contributor Author

jonpryor commented Aug 29, 2024

@jonathanpeppers asked:

Is this raising the min JDK to 17? for our whole product?

Yes.

Which may sound bananas, but:

TODO? What did/does VSMac provision? (And should we even care?)

Thus, who could/would still be using JDK-11? Who could this break?

Then there's the "real" reason I'm suggesting this: bumping $(MinimumSupportedJavaVersion) means it's consistent with $(JavaSdkVersion) and @(JavaDependency). Without this change, we'll have a possible scenario of:

  1. JDK-11 is installed and preferred (which is my current machine, via $HOME/.config/xbuild/monodroid-config.xml)

  2. I run dotnet build -t:InstallAndroidDependencies …, on .NET 9, and it errors out (now! with .NET 9 P7):

    error : Unable to install the Java SDK into `/Library/Java/JavaVirtualMachines/microsoft-11.jdk/Contents/Home`. Please set `$(JavaSdkDirectory)` to a valid non-administrator path.

    It errors out because InstallAndroidDependencies wants to provision JDK 17, but my current system configured JDK is JDK 11.

Bumping $(MinimumSupportedJavaVersion) is the "easy and straightforward" way to resolve this. It would instead cause my machine to produce a XA5300 error that my JDK is too old.

@jonpryor
Copy link
Contributor Author

jonpryor commented Aug 29, 2024

Also, I don't think any of our unit tests have used JDK-11 for quite some time (please correct me if I'm wrong), so it's entirely plausible that we don't fully work on JDK-11 now…

jonathanpeppers
jonathanpeppers approved these changes Aug 29, 2024
View reviewed changes
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

If it breaks someone, they have the workaround for their project:

<MinimumSupportedJavaVersion>11.0</MinimumSupportedJavaVersion>

So, I think we should just put this in RC 2 release notes, and see if anything goes wrong.

@jonpryor jonpryor merged commit df68c20 into main Aug 29, 2024
@jonpryor jonpryor deleted the dev/jonp/jonp-bump-min-jdk-to-17 branch August 29, 2024 19:05
@jonpryor jonpryor mentioned this pull request Aug 29, 2024
Open
@github-actions github-actions bot locked and limited conversation to collaborators Sep 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@grendello grendello grendello approved these changes

@dellis1972 dellis1972 dellis1972 approved these changes

@jonathanpeppers jonathanpeppers jonathanpeppers approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jonpryor @grendello @dellis1972 @jonathanpeppers