-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
openjdk18: init at 18+37 #165354
openjdk18: init at 18+37 #165354
Conversation
Error when running
|
ee4b82d
to
f375389
Compare
@ofborg eval |
Looking upstream it seems OpenJ9 builds (which nixpkgs pulls from AdoptOpenJDK) are now provided by Semeru. The missing packages should be all removed now. |
|
Could this be merged before the NixOS 22.05 feature freeze? #167025 |
src = fetchFromGitHub { | ||
owner = "openjdk"; | ||
repo = "jdk${version.feature}u"; | ||
rev = "jdk-${version.feature}+${version.build}"; |
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.
rev = "jdk-${version.feature}+${version.build}"; | |
rev = "jdk-${version}"; |
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.
version
here refers to the attrset above, not the usual derivation.version
attribute:
let
version = {
feature = "18";
build = "36";
};
...
I can rename and/or inline those variables (also used in derivation version, src repo, and a configure flag) but I'm following convention of other openjdk
derivations.
I've been test driving this for a few weeks. It's worked great until the recent glibc upgrade to 2.34 because this is still being built with glibc 2.33 (I've confirmed rebasing fixes this). Can we get this off the ground please? Is there anything else to do to get this into |
jdkVersion = "18.0.0"; | ||
sha256 = "0ch4jp2d4pjvxbmbswvjwf7w2flajrvjg5f16ggiy80y8l0y15cm"; | ||
}; | ||
}."${stdenv.hostPlatform.system}"; |
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.
Missing or throw unsupported system or eval fails on other platforms.
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.
With all respect, it a. doesn't get called for non-Darwin platforms, and b. is the same way that all OpenJDK derivations have done this. I can change it if required, but it seems unnecessary and out of line.
nixpkgs/pkgs/top-level/java-packages.nix
Lines 51 to 56 in 992d767
mkOpenjdk = path-linux: path-darwin: args: | |
if stdenv.isLinux | |
then mkOpenjdkLinuxOnly path-linux args | |
else let | |
openjdk = callPackage path-darwin {}; | |
in openjdk // { headless = openjdk; }; |
https://github.com/NixOS/nixpkgs/blob/a6b757a52bf2370540ffc2c0210e25e79b419a63/pkgs/top-level/java-packages.nix#L187-L193
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.
Thats not great. It should at least eval on any platform otherwise you can't check if your changes on break evaluation for another platform.
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.
Wouldn't you set the system
anyway when testing evaluation for another platform, then stdenv.hostPlatform
would show that?
Anyway I don't think we put too many expectations on the evaluation of packages if they're not checked on hydra / ofborg, and all the ofborg builds on the PR are green, so I think it's ok.
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 don't think we put too many expectations on the evaluation of packages if they're not checked on hydra / ofborg
Especially important to:
- Help reviewers be more effective, since they can rely on ofborg builds and focus on other aspects of the PR.
- Help contributors get faster signal on their PRs, and have a higher confidence that their code is good if the build is green.
Hello friends! This issue seems to be a bit stalled; is there any way to push this through please? Thank you! |
Please? |
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.
Diff LGTM, built and tested adopt-openjdk17 on NixOS x86_64 for another PR ( #183763 ) by cherry-picking a commit from here over there.
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.
LGTM
@SuperSandro2000 are you happy to proceed? |
Do we have an update on this? thanks. |
Will proceed to merge, let me rebase this. |
@GrahamcOfBorg eval |
Now that openjdk19 is out, is there some mechanism to generate an equivalent pull request? Is it just a matter of copy+pasting the 18 files and pointing them at the latest releases + shas from here - https://github.com/adoptium/temurin19-binaries/releases ? Thanks! |
#195638 is the official packaging request for OpenJDK 19. |
this was originally replaced with temurin in c742218, which landed in staging in acf46b0 (NixOS#140364) but it was also added in b6cb656 (in support of openjdk 18 in da40a44), which landed directly on master in 9413ebb (NixOS#165354). those two conflicted when master was merged into staging-next in a5dfac8 (NixOS#191339), and adoptopenjdk 17 was mistakenly kept during the conflict resolution. the net result is that one would get: $ nix-build -A pkgs.adoptopenjdk-hotspot-bin-17 error: Alias adoptopenjdk-hotspot-bin-17 is still in all-packages.nix ... instead of the desired: $ nix-build -A pkgs.adoptopenjdk-hotspot-bin-17 error: AdoptOpenJDK is now Temurin. Use temurin-bin-17
Description of changes
OpenJDK 18 has moved into general availability (GA) and is considered by the OpenJDK developers to be stable.
This PR provides both
openjdk18
andadoptopenjdk-17
used for bootstrapping in line with other OpenJDK versions, but I'm happy to separate this into two PRs if required (or bootstrap with another JDK).The
ignore-LegalNoticeFilePlugin
patch for Linux JDK17 has been renamed to add a JDK18 variant which has a different check (usingOptional#ifEmpty()
rather than!Optional#ifPresent()
).Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes