-
-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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 ARM version #68946
openjdk: add ARM version #68946
Conversation
a05a528
to
80791f0
Compare
Looks like you need homebrew-core/Formula/openjdk.rb Lines 38 to 42 in 3eb4055
? |
Formula/openjdk.rb
Outdated
url "https://download.java.net/java/GA/jdk14.0.2/205943a0976c4ed48cb16f1043c5c647/12/GPL/openjdk-14.0.2_osx-x64_bin.tar.gz" | ||
sha256 "386a96eeef63bf94b450809d69ceaa1c9e32a97230e0a120c1b41786b743ae84" | ||
if Hardware::CPU.arm? | ||
url "https://download.java.net/java/early_access/jdk16/29/GPL/openjdk-16-ea+29_osx-x64_bin.tar.gz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting an outstanding task from #65670:
Before merge, [we'll] need a reliable 16-ea URL or can just hard-link any 16 build if @VladimirKempik thinks it'll be OK.
@VladimirKempik replied with the following information:
jep391 branch will soon be merged with upstream and become 17-ea
I'm not sure how soon this would happen, but working from upstream (and not a sandbox) is obviously preferred, but we'll need a boot-jdk that won't be pulled down days or weeks after this PR is merged.
@VladimirKempik I'm sorry if I asked this before, but is it absolutely mandatory that the boot-jdk has JEP-391 code? If not, we can probably find a more reliable link.
If it is mandatory that we have a JEP-391 enabled boot-jdk, I'd need some guidance from Homebrew as to where to retrieve this from. From my experience with other formulae, I understand volatile links are not uncommon, but I don't know if it's OK here? Once the bottles are created, it would only affect the source builds, I'm just not sure if that's acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often would the link break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boot-jdk has to be either jdkX or jdkX-1 ( where X is jdk version you build)
build-jdk has to be the same version of jdk you build
both doesn't have to have jep-391 code in it.
so you can just have some fixed version of upstream(jdk/jdk) to be used as boot/build jdk. this will exist forever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often would the link break?
It broke while the old PR was open (26-29, now 31), so they change frequently, I'm not sure how long they maintain the old URLs for though.
Although any JDK16 version will do, I'm unsure where to find a stable link. Of the commercial providers, Zulu has an EA link that we could potentially borrow -- just for boot-jdk purposes, but I assume this is frowned upon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've switched it to the latest (openjdk-16-ea+31
) and I've mirror it on our side.
I thought I hadn't changed anything on Intel, but we get:
|
Agreed, that doesn't appear to be newly introduced with this PR. I'll fire a vanilla source build on Intel and see if the same error occurs. |
Hmm... oddly, it doesn't fail on the vanilla formula.
|
you build jdk15 using jdk14 as boot jdk - this is fine when you try to build jdk16 using jdk14 as boot jdk - it will fail |
Where does the formula do this? I don't see it... |
jdk15u-jdk-15.0.1-ga/boot-jdk/Contents/Home is incorrect JDK version (openjdk version "14.0.2" 2020-07-14); ignoring |
@VladimirKempik this is not building jdk16, it's building jdk15 (and it definitely used to work) |
yes, but where in the formula code is this coming from? |
excuse me, I missed one word there: configure: Potential Build JDK found at /private/tmp/openjdk-20210113-63129-k3mi50/jdk15u-jdk-15.0.1-ga/boot-jdk/Contents/Home is incorrect JDK version (openjdk version "14.0.2" 2020-07-14); ignoring build jdk has to match the version of jdk you building but one doesnt need build-jdk if not cross-compiling |
@VladimirKempik's right, good catch. I added that for all builds, it's a mistake and should be moved to the arm-only block. This is what master looks like, notice there's no homebrew-core/Formula/openjdk.rb Line 88 in fcef5e5
|
@tresf I think you mean Updated, and pushed for a new round of testing. |
632d575
to
d7b7565
Compare
Are we not doing ARM caveats like with |
|
You may find out more about this predicament here: #65670 (comment) |
I've restarted CI now that Homebrew/brew#10320 is merged. |
Thanks @jonchang you rock! |
.max | ||
raise "cannot find build number in .hg_archival.txt" if build.nil? | ||
if Hardware::CPU.arm? | ||
build = File.read(".hgtags") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was testing this approach on 15.0.1 (to allow us to clean up the comment above) and it reports incorrectly. It will report 15.0.1+8
instead of 15.0.1+9
. @VladimirKempik moving forward, should we add +1
to the last tag in .hgtags
or is this handled automatically in newer releases? How should we calculate this value moving forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much of a deal-breaker is that? Testing has been progressing nicely so far, so I planned to merge this tomorrow. Should I wait for a solution to this, or is it a minor issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much of a deal-breaker is that? Testing has been progressing nicely so far, so I planned to merge this tomorrow. Should I wait for a solution to this, or is it a minor issue?
I was thinking the same thing when I saw the "ready to merge" tag. I suppose it can be tackled at some future time when this is consolidated, but I'd still like to know moving forward as the comment suggests the old versioning information will eventually go away.
The ARM testing was stuck on |
Oops. https://github.com/Homebrew/homebrew-core/runs/1705106711?check_suite_focus=true
So we actually can't have arch-dependent version. |
First few builds of openjdk-dependent formulas on ARM are a mixed bag:
|
@tresf @VladimirKempik regarding openjdk@8 and openjdk@11, I understand there may not be a backport to the official openjdk (I've read the redhat position on that, which you posted on the previous PR). However, for each of those two versions: is there a set of patches, or a fork (or downstream distribution, I don't know how they're called) that we could switch to? That would be on ARM only, we would keep the official source on Intel. The idea being that for our ARM users, it's better to distribute a patch or branch from a well-known vendor, than nothing from the official branch. (Excuse my ignorance about the whole openjdk ecosystem…) |
Bottle installs! However I get the error
during the Pouring step, and postinstall does not complete successfully, though I don't see an obvious error:
|
You will need to wait for the answer here while we will think about it. |
@unitof weird, can you please file an issue about it, and provide all the requested information? I'll try to debug that |
@VladimirKempik kindly provided ARM64 patches for openjdk@11 here: #65670 (comment) (the patch is > 4K lines, it won't apply cleanly as he's warned). I haven't seen any publicly available patches for openjdk@8 land yet. |
Trying to rework #65670 into the direction decided by the TSC (see #65670 (comment) and below)