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

Add microsoft distribution of the JDK. #252

Merged
merged 12 commits into from Dec 8, 2021
Merged

Conversation

brendandburns
Copy link
Contributor

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
Copy link
Contributor Author

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

@brendandburns
Copy link
Contributor Author

cc @brunoborges

README.md Outdated
@@ -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)

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@brunoborges
Copy link

@ethomson this is a great PR.

@ethomson
Copy link
Contributor

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

@brendandburns
Copy link
Contributor Author

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

@brendandburns
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Copy link

@dikehtaw dikehtaw left a comment

Choose a reason for hiding this comment

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

gh pr checkout 252

README.md Outdated
@@ -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)

Choose a reason for hiding this comment

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

brendandburns:main

@dmitry-shibanov
Copy link
Contributor

dmitry-shibanov commented Nov 23, 2021

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

README.md Outdated
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

import fs from 'fs';
import path from 'path';

const supportedPlatform = `'linux', 'macos', 'windows'`;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

.github/workflows/e2e-versions.yml Outdated Show resolved Hide resolved
.github/workflows/e2e-versions.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@brcrista brcrista left a comment

Choose a reason for hiding this comment

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

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

brendandburns and others added 2 commits Nov 30, 2021
Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com>
Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com>
@brcrista
Copy link
Contributor

Thanks @brendanburns , just kicked off the CI

@brcrista
Copy link
Contributor

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
Copy link
Contributor

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

@brendandburns
Copy link
Contributor Author

@brcrista Thanks! I merged your PR.

@@ -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']

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Ah, yes. That'd be better indeed.

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

Choose a reason for hiding this comment

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

Should we move up to 17?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above.

Choose a reason for hiding this comment

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

Noted.

README.md Outdated
@@ -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)

Choose a reason for hiding this comment

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

Correct name is Microsoft Build of OpenJDK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (apologies, this got lost in my rebasing)

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

No, it's fine. See previous comment.

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

Choose a reason for hiding this comment

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

See previous comments regarding making 16 available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as above.

@brendandburns
Copy link
Contributor Author

Comments addressed, please re-check.

__tests__/distributors/microsoft-installer.test.ts Outdated Show resolved Hide resolved
src/distributions/microsoft/installer.ts Outdated Show resolved Hide resolved
src/distributions/microsoft/installer.ts Outdated Show resolved Hide resolved
src/distributions/microsoft/models.ts Outdated Show resolved Hide resolved
@dmitry-shibanov
Copy link
Contributor

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

@brcrista
Copy link
Contributor

brcrista commented Dec 3, 2021

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

image

@brendandburns
Copy link
Contributor Author

@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!

@brcrista brcrista mentioned this pull request Dec 8, 2021
2 tasks
Copy link
Contributor

@brcrista brcrista left a comment

Choose a reason for hiding this comment

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

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!

@brcrista brcrista merged commit db2f350 into actions:main Dec 8, 2021
147 of 148 checks passed
@brendandburns
Copy link
Contributor Author

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

@brunoborges
Copy link

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

@ethomson
Copy link
Contributor

ethomson commented Dec 8, 2021

Thanks so much @brendanburns!

@sullis
Copy link

sullis commented Dec 12, 2021

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.

image

Can somebody tag a new release of setup-java?

cc: @ethomson @brendandburns @dmitry-shibanov

@brunoborges
Copy link

@sullis you can reference the commit hash as a tag

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.

None yet

8 participants