treewide: standardize JDKs on darwin #375212
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
6.topic: java
|
formatting: my mortal enemy |
Samasaur1
force-pushed
the
push-sxnxmpuoupyt
branch
from
faac810 to
a2dc519
Compare
nix-owners
bot
requested review from
chayleaf,
Infinidoge,
tomodachi94 and
FliegendeWurst
|
|
|
forgot to install rosetta 2, no wonder the x86_64-darwin builds all failed |
|
As best I can tell, all the packages broken on The |
|
|
Out of those 151 reported failing packages on x86_64-darwin, hydra-check only reports the following as successful:
|
|
I suspect that they may be Rosetta 2 errors |
|
I'm able to build |
|
|
Samasaur1
force-pushed
the
push-sxnxmpuoupyt
branch
from
a2dc519 to
e87ff4c
Compare
All four of these build (off the pre-rebase commit) on a native x86_64-darwin machine |
|
|
Ran
|
|
I still don't think these failures are caused by this PR. For example, I started investigating |
|
Could it be related to the vendor of JDK? Certificate ripper for example needs to be built with GraalVM JDK. The script in the NixOS pkgs has also the instruction to built a native executable instead of a fat jar being bundled with a JRE. See here for the script nixpkgs/pkgs/by-name/ce/certificate-ripper/package.nix Lines 50 to 71 in f21847f |
|
@Hakky54 I don't think that's an issue here. I didn't touch GraalVM and the changes that I did make were just moving output files around to standardize locations |
Infinidoge
approved these changes
Mar 12, 2025
There was a problem hiding this comment.
Code looks good to me. I'm not a darwin user, nor do I have a setup to test darwin (and admittedly, nor do I really want to set one up), so I leave in-practice testing to others.
Minor concern about the scoping of rec and a complaint about darwin, that's all.
Thank you for the standardization, definitely necessary.
Comment on lines
43
to
55
| mv $sourceRoot $out | ||
| mkdir -p $out/Library/Java/JavaVirtualMachines | ||
|
|
||
| bundle=$out/Library/Java/JavaVirtualMachines/${name-prefix}-${lib.versions.major version}.jdk | ||
| mv $sourceRoot $bundle | ||
|
|
||
| # jni.h expects jni_md.h to be in the header search path. | ||
| ln -s $out/Contents/Home/include/darwin/*_md.h $out/Contents/Home/include/ | ||
| ln -s $bundle/Contents/Home/include/darwin/*_md.h $bundle/Contents/Home/include/ | ||
|
|
||
| # Remove some broken manpages. | ||
| # Only for 11 and earlier. | ||
| [ -e "$out/Contents/Home/man/ja" ] && rm -r $out/Contents/Home/man/ja | ||
| [ -e "$bundle/Contents/Home/man/ja" ] && rm -r $bundle/Contents/Home/man/ja | ||
|
|
||
| ln -s $out/Contents/Home/* $out/ | ||
| ln -s $bundle/Contents/Home/* $out/ |
There was a problem hiding this comment.
I can't see anything wrong with any of this. I just don't like it for "I don't like Darwin" reasons. Wacky paths.
wegank
added
12.approvals: 1
Samasaur1
force-pushed
the
push-sxnxmpuoupyt
branch
from
e87ff4c to
46628f9
Compare
github-actions
bot
added
10.rebuild-darwin: 1001-2500
and removed
10.rebuild-darwin: 501-1000
labels
wegank
removed
the
12.approvals: 1
SuperSandro2000
requested changes
Mar 14, 2025
There was a problem hiding this comment.
This PR rebuilds a lot of packages which means we must target staging. Please follow the contributing guide to not potentially ping a lot of people.
TomaSajt
reviewed
Mar 14, 2025
| @@ -67,6 +70,7 @@ let | |||
| passthru = { | |||
| jre = result; | |||
| home = result; | |||
| bundle = "${result}/Library/Java/JavaVirtualMachines/${name-prefix}-${lib.versions.major finalAttrs.version}.jdk"; | |||
There was a problem hiding this comment.
For correctness:
result should be swapped out for finalAttrs.finalPackage.
(Also, the old instances of result should probably get replaced as well.)
If you don't want to do this right now, it's fine. (It won't cause a rebuild if we do it in another PR) (Also, this change would be unrelated to the actual PR, so more reason not to do it now).
There was a problem hiding this comment.
I included it in this PR because I was also the one to introduce the usage of finalAttrs in the first place, but I did separate it out into its own commit. I'm happy to squash it into the previous commit or discard it entirely if you'd prefer
Samasaur1
force-pushed
the
push-sxnxmpuoupyt
branch
from
46628f9 to
fa90dc0
Compare
Samasaur1
force-pushed
the
push-sxnxmpuoupyt
branch
from
fa90dc0 to
be08110
Compare
Samasaur1
force-pushed
the
push-sxnxmpuoupyt
branch
from
be08110 to
fb14d78
Compare
github-actions
bot
added
10.rebuild-linux: 0
|
Thanks! |
|
lets hope for the best 🤞🏼 as we obviously cannot test everything |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The proper way to install a JDK on macOS is to copy or link a "JDK bundle" into
/Library/Java/JavaVirtualMachines(or~/Library/Java/JavaVirtualMachines). Most of the JDKs in nixpkgs already produce these JDKs, but they are not produced at a consistent path relative to the root of the derivation, so it's hard to install them. For example:$out/zulu-$MAJOR_VERSION.jdk$outThis PR standardizes the bundle path at
$out/Library/Java/JavaVirtualMachines/$NAME-$MAJOR_VERSION.jdk(chosen because it feels like adding the JDK toenvironment.systemPackageswith the appropriatepathsToLink). It also sets apassthru.bundleattribute that is the path to this produced bundle.This PR should have no effect on non-Darwin platforms. It is technically a breaking change for anything that expects to find JDK bundles at the previous locations, but I couldn't find any use of this in-tree and this PR should make it easier to find JDK bundles in any case.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.