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

openjdk: Add support for Apple silicon #65670

Closed
wants to merge 11 commits into from
Closed

Conversation

tresf
Copy link
Contributor

@tresf tresf commented Nov 25, 2020

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Adds openjdk-16ea as a viable openjdk dependency, thanks to the help from @VladimirKempik here.

PR is in draft because I have a feeling the Ruby syntax needs improvement.

  • Need advice on do macos for URL magic.
    • (Done) Removed URL magic, bumped formula to exclusively use JEP-391
  • Need to add the XCode 12 patches back in as conditional.
    • (Done) Kept patches, moved to versioned formula

Click to expand formula mirror
brew tap tresf/tresf
brew install tresf/tresf/openjdk

@tresf
Copy link
Contributor Author

tresf commented Nov 25, 2020

it might be better to use build-jdk built from same sources as the jdk you are trying to build ( jep391 branch in this case)

Thanks. In this case, the boot-jdk (build-jdk) is an Intel version that's openjdk16-ea, which was the closest I could find. Are you aware of any JEP-391 builds? I'm aware of the Azul and Microsoft ARM64 builds but I'm hesitant to link those, since Homebrew has historically leveraged tarballs directly from openjdk so I'm inclined to leave it unless issues arrise.

while I haven't seen any issues so far if doing it same way as you,

😅

@VladimirKempik
Copy link

VladimirKempik commented Nov 25, 2020

No intel builds, sorry, it has to be built manually, but many vm options can be disabled to speed that build up, for example:
--with-jvm-features="-aot -cds -graal -jfr -serialgc -shenandoahgc -zgc -parallelgc -epsilongc -jvmti"

but i'm talking onbly about build-jdk, the boot-jdk in your schema is absolutely right.

@SMillerDev
Copy link
Member

So, the main problem I have with this is confusion and compatibility.

What version should brew info report when an arm user installs this?

How many packages in homebrew actually work with JDK 16?

@tresf
Copy link
Contributor Author

tresf commented Nov 26, 2020

What version should brew info report when an arm user installs this?

It should report 16.0, perhaps with additional + and/or ea information afterward. Since it's a WIP branch, there's no solid version information associated from the .hgtags file. If needed, I can add a condition to supplement this information in the PR.

How many packages in homebrew actually work with JDK 16?

Running unit tests will start to help us understand this number. I can start running these, but I'll need help satisfying the openjdk dependency. Currently brew test maven errors with:

- Error: maven is missing test dependencies: openjdk

(Speculative) So packages that may potentially work with openjdk16: 6 major blocking projects, 184 others.*

  • *This doesn't include projects which obtain openjdk dependency as a child dependency (e.g. maven, gradle).

How I obtained this number:

  • 9 major dependencies listed in wireshark 2.2.3 #7857 as Re-check when openjdk
    • Of these 9, 3 specifically list an LTS JDK (i.e. @11) and almost certainly will NOT work in their current form. My experience is very limited, but since most packages have a basic unit test, that's a good initial litmus test.
  • 266 formula depending on openjdk at any version grep -rl "depends_on \"openjdk" . |wc -l
    • Of these 266, 76 formula depend on openjdk at a specific version and almost certainly will NOT work in their current form. grep -rl "depends_on \"openjdk@" . |wc -l

Anecdotally, I use ant, and it's working very well. Also, my downstream Java app normally targets openjdk@11 but does mostly work with openjdk@16-ea.

Since maven is a dependency of most larger Java projects, I decided to use try a small test using Apache's PDFBOX project:

brew install --ignore-dependencies maven
export JAVA_HOME=$(brew --prefix openjdk)
git clone https://github.com/apache/pdfbox
cd pdfbox
mvn clean install

The result is PDFBOX compiles and passes all unit tests.

Even testing it interactively works to a physical printer.

java -jar ~pdfbox/app/target/pdfbox-app-3.0.0-SNAPSHOT.jar PrintPDF sample.pdf

This is just one project, but I hope it speaks a bit to the viability of an early access JDK as a stop-gap solution.

@SMillerDev
Copy link
Member

It should report 16.0, perhaps with additional + and/or ea information afterward. Since it's a WIP branch, there's no solid version information associated from the .hgtags file. If needed, I can add a condition to supplement this information in the PR.

The formula version would only be 16 for them though. For everyone else it would be 15. I think 16 should really be a separate formula here because it'll get really hard for users to determine what they're actually using when they install this.

@tresf
Copy link
Contributor Author

tresf commented Nov 26, 2020

The formula version would only be 16 for them though. For everyone else it would be 15. I think 16 should really be a separate formula here because it'll get really hard for users to determine what they're actually using when they install this.

I'm in no position to argue that, but when someone types:

brew intall maven

... it would be beneficial to offer a path to dependency resolution. If you can help me understand how to make that possible using Homebrew, please do, because backports may not come for a while and the stop-gap will significantly speed up viability of dependent projects work with this new hardware without resorting to brute-force techniques. If not, the formula which depend on openjdk will continue to be blocked by this dependency.

Quoting @VladimirKempik Homebrew/brew#7857 (comment)

Redhat ( who maintains openjdk11) already suggested that they won't let mac_arm backport go into openjdk11 upstream repo any time soon - https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-October/009734.html

so your wait could be long

@tresf
Copy link
Contributor Author

tresf commented Nov 26, 2020

If instead it must live as a tap for the foreseeable future, I can close this PR. The downside I see to that approach is it currently results in a lot of unsupported --ignore-dependencies installs. As an example, without --ignore-depencies, maven currently attempts to build jdk14.0, which doesn't yet offer aarch64 support.

Any advice forward is appreciated.

@tresf
Copy link
Contributor Author

tresf commented Nov 27, 2020

Spitballing here, but does brew support a conditional dependency stub? e.g. openjdk.rb -> depends openjdk@16ea if apple silicon?

If yes, then perhaps we can keep the @16ea in a dedicated formula and not overly pollute or convolute the openjdk@14.

If no, is there another stop-gap measure used in the past when a bleeding-edge formula was needed?

@SMillerDev
Copy link
Member

I think linuxbrew does a lot of depends_on "" if OS.mac so it definitely can be done. I think it would make sense to split the versioned formula and only depend on it when needed.

@tresf
Copy link
Contributor Author

tresf commented Nov 28, 2020

I think it would make sense to split the versioned formula and only depend on it when needed.

Happy to. Do you have advice on a name? e.g. openjdk@16ea or something referencing JEP391? Should I guard it from being used on Linux?

I think linuxbrew does a lot of depends_on "" if OS.mac

I've been studying formulas and I can't find any that specifically stub off into a secondary versioned formula. Do you remember any formula specifically so that I can study how it should be done or pointers on how to grep for one? I also am unsure how to prevent the unnecessary jdk14 download as a conditional. Would it be normal to just let it download and not use it? Any pointers are appreciated.

@SMillerDev
Copy link
Member

Do you have advice on a name? e.g. openjdk@16ea or something referencing JEP391?

I'd say just openjdk@16.

As for the conditional include, I don't have experience with this but I'd expect this to work

# arm or whatever the identifier is
if Arch.arm
  depends_on "openjdk@16"
else
  depends_on "openjdk" 
end

@VladimirKempik
Copy link

I'd say just openjdk@16.

Maybe it's better to not mention openjdk16 here
openjdk16 will be forked into stabilization branch in few weeks and jep-391 won't be part of it due to time constrains.
jep-391 will likely come in openjdk17

@SMillerDev
Copy link
Member

Maybe it's better to not mention openjdk16 here
openjdk16 will be forked into stabilization branch in few weeks and jep-391 won't be part of it due to time constrains.
jep-391 will likely come in openjdk17

we can rename it once the source changes to be build on the OpenJDK 17 source. But we're going to need some way to distinguish it from the other openjdk formulae and I think the version is the clearest way to do this.

@tresf
Copy link
Contributor Author

tresf commented Dec 1, 2020

Would it be normal to just let it download and not use it? Any pointers are appreciated.

I still require direction before making this formula. Is the project OK with a recipe downloading a tarball (e.g. openjdk@14) that it has no intention on using? Are there any examples of this out in use to base the PR off of?

Maybe it's better to not mention openjdk16 here

I sort of agree, it's not openjdk@16 as much as it's openjdk@16@JEP391. It's ultimately up to the Homebrew team how to store this recipe, I'm happy to construct the PR in a manner in which it'll be accepted. I can comment the formula to warn that it's a stop-gap openjdk@16 until JEP-391 is merged into openjdk@17.

I'll start preparing the PR with a the redundantly downloaded tarball intact, but direction is appreciated.

@SMillerDev
Copy link
Member

As a maintainer and user I'd say having a separate formula for an unreleased version and making other formula conditionally depend on it is the version with the least magic and the nicest. @Homebrew/tsc any opinions?

@fxcoudert
Copy link
Member

I am of the opinion that, being the only openjdk version usable on Apple Silicon, it should be the openjdk formula. It is what will cause the least confusion.

It's not ideal, but none of the solutions are. And it is probably what causes the smaller amount of disruption to all the other formulas. And even more importantly, for the migration when the next release ships.

@tresf
Copy link
Contributor Author

tresf commented Dec 1, 2020

I am of the opinion that, being the only openjdk version usable on Apple Silicon, it should be the openjdk formula. It is what will cause the least confusion.

Not trying to jump the gun while the @tsc chimes in, but if this is the case, I assume archiving the current formula off into openjdk@14 is desired, then Intel-only formula that rely on this exact version can hard-code the version?

This also means that Intel and Linux builds will be forced to use this JEP-391 snapshot until upstream lands it on an official branch. Interested to hear others' thoughts. I have a proof of concept for depends_on in my local, but it downloads everything (jdk14, boot_jdk for jdk14 and patches for jdk14) that never get used so I'll await some more feedback before modifying the PR.

@iMichka
Copy link
Member

iMichka commented Dec 1, 2020

On linuxbrew our gcc formula installs gcc5 instead of gcc10. So openjdk formula could install a different openjdk version based on the arch. It works. But it's a dirty hack, and I am trying to get rid of it.

# arm or whatever the identifier is
if Arch.arm
  depends_on "openjdk@16"
else
  depends_on "openjdk" 
end

I also do not like that idea ... it's basically what we have been doing on linuxbrew; and have tried to reduce that kind of thing to the bare minimum.

So we have the choice between two "ugly" solutions. Why not just disable openjdk on ARM until upstream has sorted it out?

We are adding a new platform here: if it was PPC or linux ARM, the stock answer would be: create a fork of homebrew-core and play around until you have something stable. Especially as we do not have CI for ARM (and it may take some time to have it).

For something as important as openjdk, I would rather wait for an official release, as I am worried that running from some WIP or experimental branch should be avoided.

@reitermarkus
Copy link
Member

I am of the opinion that, being the only openjdk version usable on Apple Silicon, it should be the openjdk formula. It is what will cause the least confusion.

Yes, given that it is the only working version, I agree that this should be the openjdk formula. The versions will eventually converge to 17 I guess.

@fxcoudert
Copy link
Member

I would rather wait for an official release, as I am worried that running from some WIP or experimental branch should be avoided

It is not a random patch, it is an upstream branch, which is a contribution by Apple: if we trust them enough to run the OS, we probably can extend that to their other contributions as well :)

@MikeMcQuaid
Copy link
Member

Yes, given that it is the only working version, I agree that this should be the openjdk formula. The versions will eventually converge to 17 I guess.

Agreed. I'm not terribly happy about the situation but Homebrew should aim to be useful here; people running Homebrew on ARM are already spammed with warnings and this will open quite a bit of the dependency tree for us that's currently blocked.

@tresf
Copy link
Contributor Author

tresf commented Dec 2, 2020

which is a contribution by Apple

Slight clarification, from the press releases, I believe it's Azul and Microsoft that have been predominantly spearheading this effort.

I'll begin working this PR to be JEP391-based openjdk.rb (~@16) while moving the current formula to openjdk@15.rb. I'm sure there will be projects that will chose to go back to @15 (forcing them to stick to Intel and Linux only), but as anecdotal evidence with Apache PDFBOX and maven, this new JEP391 version should "just work" for many projects.

Adds openjdk16 with JEP391 patches
Moves openjdk -> openjdk@15
@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Dec 2, 2020
@tresf tresf marked this pull request as ready for review December 2, 2020 18:25
@tresf
Copy link
Contributor Author

tresf commented Dec 24, 2020

@VladimirKempik the 16-ea links broke, (patched via 3eb4055) I assume this is intentional, but will make the formula fail when they change again. Are there more reliable links I can use for the boot-jdk?

@fxcoudert
Copy link
Member

We now have 70% of formulas bottled for Apple Silicon, and openjdk is the only major blocker remaining at this stage. I'd like us to determine collectively what is needed to finalise this.

We have 3 major formulas for where Apple Silicon support is only available in a beta-type release, or in this case, a snapshot of a branch under development: rust, go, and openjdk.

  1. We move all users to the beta version, as is proposed here. Upside: everything is in sync. Downside: we break our promise to users, which is to ship only stable released versions
  2. We move only ARM users to the beta version, by having a formula where the source url (and checksum) depends on ARM/Intel. We keep Intel users on the stable version, and explain to ARM users that we're shipping the only version available for them, and it's beta.

For rust and go, we went with option 2. You can see here 467c932 and here 95c403c how it looks like. This allows us not to expose users to a beta version when there actually is a stable version available for their platform. So, if technically feasible, it is probably a better option for users.

I am not sure, however, whether that could work for openjdk, and I would like your opinion on this. Some issues I can see if we were to do that:

  • some openjdk-based formulas will work with one version and not the other, and so will fail on ARM or Intel
  • the build options of openjdk 15 and 16 might be too different, making a single formula (with 15 on Intel and 16 on ARM) unreadable and unmaintainable

@tresf what is your opinion on this? Would “option 2” be feasible for openjdk, or not, in your opinion?

@tresf
Copy link
Contributor Author

tresf commented Dec 27, 2020

@tresf what is your opinion on this? Would “option 2” be feasible for openjdk, or not, in your opinion?

If that's the desired, path, I'd be happy to start that process. Before merge, I'll need a reliable 16-ea URL or can just hard-link any 16 build if @VladimirKempik thinks it'll be OK.

In regards to formula impact, I did some quick investigations in November and found the following:

  • 9 major dependencies listed in wireshark 2.2.3 #7857 as Re-check when openjdk
    • Of these 9, 3 specifically list an LTS JDK (i.e. @11) and almost certainly will NOT work in their current form. My experience is very limited, but since most packages have a basic unit test, that's a good initial litmus test.
  • 266 formula depending on openjdk at any version grep -rl "depends_on \"openjdk" . |wc -l
    • Of these 266, 76 formula depend on openjdk at a specific version and almost certainly will NOT work in their current form. grep -rl "depends_on \"openjdk@" . |wc -l

So assuming Homebrew is OK with the 80/20 rule (focusing on the majority), it's my high-level educated guess that most formula which relies on openjdk(@latest) will be OK.

If the steering committee's recommendation is to create a "[potentially unreadable and unmaintainable] single formula (with 15 on Intel and 16 on ARM" I can start this.

@VladimirKempik
Copy link

jep391 branch will soon be merged with upstream and become 17-ea

@fxcoudert
Copy link
Member

@tresf thanks for your response, I'm asking for feedback from other maintainers @Homebrew/tsc

The goal would be to get this into mergeable state over the next few days. Having a formula that remains at the same version on Intel (option 2) has the added benefit that we won't be regressing things for formulas that wouldn't support 16-ea, so unless there is a argument I haven't thought of against it, I expect us to go in that direction.

@MikeMcQuaid
Copy link
Member

The goal would be to get this into mergeable state over the next few days. Having a formula that remains at the same version on Intel (option 2) has the added benefit that we won't be regressing things for formulas that wouldn't support 16-ea, so unless there is a argument I haven't thought of against it, I expect us to go in that direction.

Yes, I would rather than direction too 👍🏻

@fxcoudert
Copy link
Member

If the steering committee's recommendation is to create a "[potentially unreadable and unmaintainable] single formula (with 15 on Intel and 16 on ARM" I can start this

@tresf I can confirm other members of the steering committee are in agreement with that, and we would like to go that way for openjdk (as we did for rust and go). Thanks again for your time and help with this!

Copy link

@vlw vlw 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. Please, merge it faster as possible

hannesa2 added a commit to MikeOrtiz/TouchImageView that referenced this pull request Jan 3, 2021
As it looks like, Apple M1 will receive Java 16 in brew Homebrew/homebrew-core#65670 (comment)
hannesa2 added a commit to MikeOrtiz/TouchImageView that referenced this pull request Jan 3, 2021
As it looks like, Apple M1 will receive Java 16 in brew Homebrew/homebrew-core#65670 (comment)
hannesa2 added a commit to MikeOrtiz/TouchImageView that referenced this pull request Jan 3, 2021
As it looks like, Apple M1 will receive Java 16 in brew Homebrew/homebrew-core#65670 (comment)
hannesa2 added a commit to MikeOrtiz/TouchImageView that referenced this pull request Jan 3, 2021
As it looks like, Apple M1 will receive Java 16 in brew Homebrew/homebrew-core#65670 (comment)
@jonchang jonchang mentioned this pull request Jan 4, 2021
5 tasks
@tresf
Copy link
Contributor Author

tresf commented Jan 12, 2021

Sorry for the delay, this is behind a few other tasks currently.

@fxcoudert
Copy link
Member

@tresf I've tried giving it a shot at #68946

@tresf
Copy link
Contributor Author

tresf commented Jan 13, 2021

@tresf I've tried giving it a shot at #68946

Thanks. I'll close this out for now. Happy to reopen if needed. Note, this PR also added Linux support, which is a bit off-topic. I'll leave this task open (for myself) to at least follow-up with Linux support as not to get lost.

@tresf tresf closed this Jan 13, 2021
@fxcoudert
Copy link
Member

I welcome your guidance and help on #68946 when you have time, of course!

@kostapsimoulis
Copy link

kostapsimoulis commented Jan 23, 2021

@tresf I have just tried to install it using the latest master branch, it works but with some minor issues:

a) When I run java it says openjdk 16, even though it downloaded 15. That seems a bit strange

kosta@Konstantinoss-Mac-mini ~ % /opt/homebrew/opt/openjdk/bin/java -version
openjdk version "16" 2021-03-16
OpenJDK Runtime Environment (build 16+14)
OpenJDK 64-Bit Server VM (build 16+14, mixed mode)

b) Installation gives an error:

Error: Failed applying an ad-hoc signature to /opt/homebrew/Cellar/openjdk/15.0.1/libexec/openjdk.jdk/Contents/MacOS/libjli.dylib

Full log:

kosta@Konstantinoss-Mac-mini ~ % brew install openjdk
==> Homebrew has enabled anonymous aggregate formula and cask analytics.
Read the analytics documentation (and how to opt-out) here:
  https://docs.brew.sh/Analytics
No analytics have been recorded yet (or will be during this `brew` run).

==> Homebrew is run entirely by unpaid volunteers. Please consider donating:
  https://github.com/Homebrew/brew#donations
==> Auto-updated Homebrew!
Updated 1 tap (homebrew/core).
==> New Formulae
ansible@2.9
==> Updated Formulae
Updated 1 formula.

==> Downloading https://homebrew.bintray.com/bottles/openjdk-15.0.1.arm64_big_sur.bottle.1.tar.gz
==> Downloading from https://d29vzk4ow07wi7.cloudfront.net/64da5d70d63d617def59e16bd411f236b822626f130a6b16576eb
######################################################################## 100.0%
==> Pouring openjdk-15.0.1.arm64_big_sur.bottle.1.tar.gz
Error: Failed applying an ad-hoc signature to /opt/homebrew/Cellar/openjdk/15.0.1/libexec/openjdk.jdk/Contents/MacOS/libjli.dylib
==> Caveats
For the system Java wrappers to find this JDK, symlink it with
  sudo ln -sfn /opt/homebrew/opt/openjdk/libexec/openjdk.jdk /Library/Java/JavaVirtualMachines/openjdk.jdk
This is a beta version of openjdk for Apple Silicon
(openjdk 16 preview).

openjdk is keg-only, which means it was not symlinked into /opt/homebrew,
because it shadows the macOS `java` wrapper.

If you need to have openjdk first in your PATH run:
  echo 'export PATH="/opt/homebrew/opt/openjdk/bin:$PATH"' >> ~/.zshrc

For compilers to find openjdk you may need to set:
  export CPPFLAGS="-I/opt/homebrew/opt/openjdk/include"

==> Summary
🍺  /opt/homebrew/Cellar/openjdk/15.0.1: 612 files, 305.4MB

Let me know if you would like me to test a different branch. Thank you

@carlocab
Copy link
Member

There's an open issue for the codesigning issue at #69100.

When I run java it says openjdk 16, even though it downloaded 15. That seems a bit strange

This is explained in the caveats that you pasted above:

This is a beta version of openjdk for Apple Silicon
(openjdk 16 preview).

openjdk 15 is not compatible with Apple Silicon.

hannesa2 added a commit to MikeOrtiz/TouchImageView that referenced this pull request May 5, 2021
As it looks like, Apple M1 will receive Java 16 in brew Homebrew/homebrew-core#65670 (comment)
hannesa2 added a commit to MikeOrtiz/TouchImageView that referenced this pull request May 5, 2021
As it looks like, Apple M1 will receive Java 16 in brew Homebrew/homebrew-core#65670 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge-skip `brew pr-automerge` will skip this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.