Add microsoft distribution of the JDK. #252

brendandburns · 2021-11-18T17:25:31Z

Description:
Initial add of the Microsoft OpenJDK distribution

Related issue:
N/A

Check list:

Mark if documentation changes are required.
Mark if tests were added or updated to cover the changes.

brendandburns · 2021-11-18T17:26:21Z

This was based off of #225 thanks for making it easy!

brendandburns · 2021-11-18T18:13:08Z

cc @brunoborges

brunoborges · 2021-11-18T19:20:23Z

README.md

@@ -58,6 +58,7 @@ Currently, the following distributions are supported:
 | `zulu` | Zulu OpenJDK | [Link](https://www.azul.com/downloads/zulu-community/?package=jdk) | [Link](https://www.azul.com/products/zulu-and-zulu-enterprise/zulu-terms-of-use/) |
 | `adopt` or `adopt-hotspot` | Adopt OpenJDK Hotspot | [Link](https://adoptopenjdk.net/) | [Link](https://adoptopenjdk.net/about.html) |
 | `adopt-openj9` | Adopt OpenJDK OpenJ9 | [Link](https://adoptopenjdk.net/) | [Link](https://adoptopenjdk.net/about.html)
+| `microsoft` | Microsoft OpenJDK | [Link](https://www.microsoft.com/openjdk) | [Link](https://docs.microsoft.com/en-us/java/openjdk/faq)


Thanks @brendanburns! One quick suggestion here on the name: Microsoft Build of OpenJDK

brunoborges · 2021-11-18T20:32:02Z

@ethomson this is a great PR.

ethomson · 2021-11-18T20:44:59Z

Thanks @brendandburns! We'll take a look. 👀

brendandburns · 2021-11-18T22:55:26Z

Comment addressed and I fixed formatting to follow prettier style guidelines.

brendandburns · 2021-11-19T16:45:00Z

Hold on this for a second, I realized that the Windows URLs aren't right, I need to substitute ".zip" for "tar.gz" I will get to that sometime today.

brendandburns · 2021-11-19T19:04:21Z

Ok, this is now fixed and I believe should be ready for final review.

dikehtaw

gh pr checkout 252

dikehtaw · 2021-11-20T20:24:22Z

README.md

@@ -58,7 +58,7 @@ Currently, the following distributions are supported:
 | `zulu` | Zulu OpenJDK | [Link](https://www.azul.com/downloads/zulu-community/?package=jdk) | [Link](https://www.azul.com/products/zulu-and-zulu-enterprise/zulu-terms-of-use/) |
 | `adopt` or `adopt-hotspot` | Adopt OpenJDK Hotspot | [Link](https://adoptopenjdk.net/) | [Link](https://adoptopenjdk.net/about.html) |
 | `adopt-openj9` | Adopt OpenJDK OpenJ9 | [Link](https://adoptopenjdk.net/) | [Link](https://adoptopenjdk.net/about.html)
-| `microsoft` | Microsoft OpenJDK | [Link](https://www.microsoft.com/openjdk) | [Link](https://docs.microsoft.com/en-us/java/openjdk/faq)


brendandburns:main

dmitry-shibanov · 2021-11-23T12:08:00Z

Hello @brendanburns. Thank you for your pull request. Will you add logic to automatically grab all available versions ?

brcrista · 2021-11-29T21:03:06Z

README.md

@@ -58,6 +58,7 @@ Currently, the following distributions are supported:
 | `zulu` | Zulu OpenJDK | [Link](https://www.azul.com/downloads/zulu-community/?package=jdk) | [Link](https://www.azul.com/products/zulu-and-zulu-enterprise/zulu-terms-of-use/) |
 | `adopt` or `adopt-hotspot` | Adopt OpenJDK Hotspot | [Link](https://adoptopenjdk.net/) | [Link](https://adoptopenjdk.net/about.html) |
 | `adopt-openj9` | Adopt OpenJDK OpenJ9 | [Link](https://adoptopenjdk.net/) | [Link](https://adoptopenjdk.net/about.html)
+| `microsoft` | Microsoft Build of OpenJDK | [Link](https://www.microsoft.com/openjdk) | [Link](https://docs.microsoft.com/en-us/java/openjdk/faq)


Suggested change

| `microsoft` | Microsoft Build of OpenJDK | [Link](https://www.microsoft.com/openjdk) | [Link](https://docs.microsoft.com/en-us/java/openjdk/faq)

| `microsoft` | Microsoft Build of OpenJDK | [Link](https://www.microsoft.com/openjdk) | [Link](https://docs.microsoft.com/java/openjdk/faq)

I believe docs.microsoft.com will redirect to the appropriate page for the user's language setting

brcrista · 2021-11-29T21:11:52Z

src/distributions/microsoft/installer.ts

+import fs from 'fs';
+import path from 'path';
+
+const supportedPlatform = `'linux', 'macos', 'windows'`;


The URL in my browser shows macOS, but I see macos works as well (IIS? 🙂)

I don't think there's anything needed here. (Aren't all URLs case-insensitive?)

I believe the path part of a URL is usually case-sensitive: https://webmasters.stackexchange.com/questions/90339/why-are-urls-case-sensitive

It seems to work though, so no not a big deal.

src/distributions/microsoft/installer.ts

.github/workflows/e2e-versions.yml

brcrista

Looks good! This workflow is failing; I'm looking into why that is ~~(and why it doesn't report on the Conversation page in the PR)~~ must be because of the merge conflict on that file

Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com>

brcrista · 2021-11-30T21:44:29Z

Thanks @brendanburns , just kicked off the CI

brcrista · 2021-11-30T21:46:25Z

Looks like there are some unresolved merge conflicts in .github/workflows/e2e-versions.yml and linting is failing. I can push fixes for those issues, if you don't mind.

brcrista · 2021-11-30T22:31:29Z

brendandburns#1 should fix the CI runs. Once those are green we can merge 🚢

brendandburns · 2021-11-30T23:14:32Z

@brcrista Thanks! I merged your PR.

brunoborges · 2021-12-01T06:20:58Z

.github/workflows/e2e-versions.yml

@@ -20,8 +20,11 @@ jobs:
      fail-fast: false
      matrix:
        os: [macos-latest, windows-latest, ubuntu-latest]
-        distribution: ['temurin', 'adopt', 'adopt-openj9', 'zulu', 'liberica'] # internally 'adopt-hotspot' is the same as 'adopt'
+        distribution: ['temurin', 'adopt', 'adopt-openj9', 'zulu', 'liberica', 'microsoft' ] # internally 'adopt-hotspot' is the same as 'adopt'
        version: ['8', '11', '16']


Can we replace 16 with 17 here, since 17 is the latest LTS now?

Probably worth doing in a different PR since it is unrelated to adding MSFT.

Ah, yes. That'd be better indeed.

brunoborges · 2021-12-01T06:21:33Z

.github/workflows/e2e-versions.yml

        os: [windows-latest, ubuntu-latest]
-        distribution: ['zulu', 'liberica']
+        distribution: ['liberica', 'microsoft', 'zulu']
        version: ['11']


Should we move up to 17?

brunoborges · 2021-12-01T06:21:49Z

README.md

@@ -59,6 +59,7 @@ Currently, the following distributions are supported:
 | `adopt` or `adopt-hotspot` | Adopt OpenJDK Hotspot | [Link](https://adoptopenjdk.net/) | [Link](https://adoptopenjdk.net/about.html) |
 | `adopt-openj9` | Adopt OpenJDK OpenJ9 | [Link](https://adoptopenjdk.net/) | [Link](https://adoptopenjdk.net/about.html) |
 | `liberica` | Liberica JDK | [Link](https://bell-sw.com/) | [Link](https://bell-sw.com/liberica_eula/) |
+| `microsoft` | Microsoft OpenJDK | [Link](https://www.microsoft.com/openjdk) | [Link](https://docs.microsoft.com/java/openjdk/faq)


Correct name is Microsoft Build of OpenJDK

Done (apologies, this got lost in my rebasing)

brunoborges · 2021-12-01T06:23:38Z

.github/workflows/e2e-versions.yml

        version: ['8', '11', '16']
+        exclude:


I think we should also exclude Microsoft's version 16, as it is not an LTS release and we will no longer provide updates.

I think for now, since Microsoft hosts it, we should keep it in the spirit of meeting users where they are. Many users haven't updated their CI/CD for JDK 17.

Yeah, that makes sense. We are not exposing support timelines for any of the distros through the GH Action anyways, so, it is up to the end-user to check the documentation of each distro.

brunoborges · 2021-12-01T06:24:08Z

__tests__/distributors/microsoft-installer.test.ts

+      'https://aka.ms/download-jdk/microsoft-jdk-17.0.1.12.1-{{OS_TYPE}}-x64.{{ARCHIVE_TYPE}}'
+    ],
+    [
+      '16.0.x',


Given 16 is not an LTS release of MS Build of OpenJDK, I think we should not list it here.

as above, but if you feel strongly I will remove it.

No, it's fine. See previous comment.

brunoborges · 2021-12-01T06:26:20Z

src/distributions/microsoft/installer.ts

+        fullVersion: [17, 0, 1, 12, 1]
+      },
+      {
+        majorVersion: 16,


See previous comments regarding making 16 available.

brendandburns · 2021-12-01T18:19:09Z

Comments addressed, please re-check.

__tests__/distributors/microsoft-installer.test.ts

src/distributions/microsoft/installer.ts

src/distributions/microsoft/models.ts

dmitry-shibanov · 2021-12-02T09:27:49Z

Could you please revert the changes for package-lock.json and use these commands npm ci -> npm run build.

brcrista · 2021-12-03T18:27:28Z

Looking at my fork, looks like there will be a couple more issues:

https://github.com/brcrista/setup-java/runs/4408213781?check_suite_focus=true#step:4:7

Found java version: __tests__/verify-java.sh: line 24: /c/hostedtoolcache/windows/Java_Microsoft_jdk/11.0.13/aarch64/bin/java: cannot execute binary file: Exec format error
Error: Unexpected version

Similar issue for Ubuntu as well.

@brendanburns if you want to allow edits from maintainers on this PR then we can push fixes for these issues

brendandburns · 2021-12-07T21:27:12Z

@dmitry-shibanov comments addressed.

@brcrista not sure where the aarch is coming from but it seems that actions doesn't support that architecture? I have ticked the "allow edits from maintainers" box, so any fixes or pointers would be great.

Thanks!

.github/workflows/e2e-versions.yml

brcrista

Looks like the Licensed workflow is failing, but that's not related to these changes https://github.com/actions/setup-java/actions/runs/1555484816

Thank you @brendanburns and @brunoborges, this is a huge help!

brendandburns · 2021-12-08T22:29:05Z

@brcrista thank you for your help getting this across the finish line!!

brunoborges · 2021-12-08T22:31:51Z

Indeed, excited to see Microsoft's OpenJDK now on the Setup Java action!

ethomson · 2021-12-08T22:35:52Z

Thanks so much @brendanburns!

sullis · 2021-12-12T13:30:00Z

I just tried using the Microsoft JDK in a GitHub Actions workflow. It did not work. My workflow uses 'setup-java@v2' and there hasn't been a release that incorporates the new Microsoft JDK code.

Can somebody tag a new release of setup-java?

cc: @ethomson @brendandburns @dmitry-shibanov

brunoborges · 2021-12-12T15:50:51Z

@sullis you can reference the commit hash as a tag

brendandburns force-pushed the main branch from c28745f to 4dd2998 Compare November 18, 2021 17:30

brunoborges reviewed Nov 18, 2021

View reviewed changes

dikehtaw approved these changes Nov 20, 2021

View reviewed changes

dikehtaw reviewed Nov 20, 2021

View reviewed changes

brendandburns force-pushed the main branch from fc7c917 to aa7837f Compare November 22, 2021 21:16