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

Use the latest available jdk #59788

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Use the latest available jdk #59788

wants to merge 8 commits into from

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jan 9, 2025

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jan 9, 2025
Copy link
Contributor

Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jan 9, 2025

@wtgodbe @BrennanConroy this will fix your builds (everything is broken right now). I opened dotnet/source-build#4816 to track how to best handle this going forward, including your local script to install a jdk.

We patched the VMR with this change to unblock builds.

@ViktorHofer ViktorHofer marked this pull request as ready for review January 9, 2025 12:57
@wtgodbe wtgodbe mentioned this pull request Jan 9, 2025
@@ -24,7 +24,7 @@
"xcopy-msbuild": "17.8.5"
},
"native-tools": {
"jdk": "11.0.24"
"jdk": "latest"
Copy link
Member

Choose a reason for hiding this comment

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

This would break

if (-not $JdkVersion) {
$globalJson = Get-Content "$repoRoot\global.json" | ConvertFrom-Json
$JdkVersion = $globalJson.'native-tools'.jdk
}
when we don't pass in a version - @BrennanConroy @halter73 maybe we could just default to https://aka.ms/download-jdk/microsoft-jdk-11.0.25-windows-x64.zip for now?

Copy link
Member

Choose a reason for hiding this comment

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

Since we're likely changing the mechanism, let's just do whatever is needed to get this working.

Copy link
Member

Choose a reason for hiding this comment

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

@ViktorHofer can you just change the block I linked to default to 11.0.24 for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

@wtgodbe feel free to push into this branch

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I think I got it right.

@wtgodbe
Copy link
Member

wtgodbe commented Jan 9, 2025

Screenshot 2025-01-09 at 1 19 32 PM

Whoa, github AI automatically populated an accurate commit description for my commit. Neat

@wtgodbe wtgodbe enabled auto-merge (squash) January 9, 2025 21:21
@@ -22,7 +22,7 @@ allprojects {
version project.findProperty('packageVersion') ?: "99.99.99-dev"

java {
sourceCompatibility = 1.8
sourceCompatibility = 1.9
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed?

invalid source release: 9

Copy link
Member

Choose a reason for hiding this comment

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

See dotnet/sdk#45822 (comment)

We started hitting an error w/ version 8 when updating to 11.0.25:

EXEC : error : [options] source value 8 is obsolete and will be removed in a future release [D:\a\_work\1\vmr\src\aspnetcore\src\SignalR\clients\java\signalr\core\signalr.client.java.core.javaproj]

I'm not sure how to proceed if the underlying machines are in a mixed state, as they seem to be (some have 11.0.24, which is incompatible w/ 9, and some have 11.0.25, which is incompatible w/ 8)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, never mind, it's Helix, not the build machines. Do you know why non-windows is on JDK 10 & windows is on 11 here? The Helix failure was only on Mac.

<HelixPreCommand Condition="'$(IsWindowsHelixQueue)' == 'true'" Include="call RunPowershell.cmd InstallJdk.ps1 11.0.3 %25HELIX_CORRELATION_PAYLOAD%25\jdk &amp;&amp; set %22JAVA_HOME=%25HELIX_CORRELATION_PAYLOAD%25\jdk%22" />
<HelixPreCommand Condition="'$(IsWindowsHelixQueue)' != 'true' AND '$(IsMacHelixQueue)' != 'true'" Include="./installjdk.sh 10.0.2 x64 &amp;&amp; if [ &quot;%24JAVA_HOME&quot; = &quot;&quot; ]%3B then export JAVA_HOME=%24PWD/java%3B fi" />

Copy link
Member

Choose a reason for hiding this comment

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

I thought all machines were supposed to have moved to 21.X?

Copy link
Member

Choose a reason for hiding this comment

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

Probably should remove those lines now? Wonder if we used to have to install the jdk ourselves 5 years ago.

Copy link
Member

@wtgodbe wtgodbe Jan 9, 2025

Choose a reason for hiding this comment

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

Oh, I totally missed that. So it looks like whatever version of macos-openjdk is pre-installed on the MacOS-13 queue isn't compatible w/ 1.9. @dotnet/dnceng does anyone know how we can check what version is installed on OSX.13.amd64.open? helix.dot.net doesn't list one.

Copy link
Member

Choose a reason for hiding this comment

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

Related to this ask?

Copy link
Member

Choose a reason for hiding this comment

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

dotnet/dnceng#4554

all macOS machines will have OpenJDK 8 and 21 after they are next refreshed

So maybe the machines haven't been refreshed?

Copy link
Member

Choose a reason for hiding this comment

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

Now we have

ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH.

On Windows 🙄

Copy link
Member

Choose a reason for hiding this comment

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

Should probably revert 3b86147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants