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

openjdk18: init at 18+37 #165354

Merged
merged 2 commits into from Sep 18, 2022
Merged

openjdk18: init at 18+37 #165354

merged 2 commits into from Sep 18, 2022

Conversation

xeals
Copy link
Contributor

@xeals xeals commented Mar 23, 2022

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 and adoptopenjdk-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 (using Optional#ifEmpty() rather than !Optional#ifPresent()).

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@willcohen
Copy link
Contributor

Error when running nixpkgs-review pr 165354 on darwin:

$ git -c fetch.prune=false fetch --no-tags --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/165354/head:refs/nixpkgs-review/1
remote: Enumerating objects: 1004, done.
remote: Counting objects: 100% (566/566), done.
remote: Compressing objects: 100% (181/181), done.
remote: Total 304 (delta 218), reused 181 (delta 108), pack-reused 0
Receiving objects: 100% (304/304), 81.97 KiB | 1.91 MiB/s, done.
Resolving deltas: 100% (218/218), completed with 82 local objects.
From https://github.com/NixOS/nixpkgs
   b27a002344d..f7ccecfe39c  master                -> refs/nixpkgs-review/0
 + 3f171a9e885...ae8d50bb213 refs/pull/165354/head -> refs/nixpkgs-review/1  (forced update)
$ git worktree add /Users/wcohen/.cache/nixpkgs-review/pr-165354/nixpkgs f7ccecfe39c4ffb339ef479661e9e1945b7bed15
Preparing worktree (detached HEAD f7ccecfe39c)
Updating files: 100% (30073/30073), done.
HEAD is now at f7ccecfe39c Merge pull request #166061 from SuperSandro2000/sshuttle
$ nix-env --option system x86_64-darwin -f /Users/wcohen/.cache/nixpkgs-review/pr-165354/nixpkgs -qaP --xml --out-path --show-trace
$ git merge --no-commit --no-ff ae8d50bb213d092a351a212dd7336fb13eada667
Auto-merging pkgs/top-level/all-packages.nix
Auto-merging pkgs/top-level/java-packages.nix
Automatic merge went well; stopped before committing as requested
$ nix-env --option system x86_64-darwin -f /Users/wcohen/.cache/nixpkgs-review/pr-165354/nixpkgs -qaP --xml --out-path --show-trace --meta
error: attribute 'openj9' missing

       at /Users/wcohen/.cache/nixpkgs-review/pr-165354/nixpkgs/pkgs/development/compilers/adoptopenjdk-bin/jdk17-darwin.nix:10:63:

            9|   jdk-openj9 = import ./jdk-darwin-base.nix { sourcePerArch = sources.openjdk17.mac.jdk.openj9; };
           10|   jre-openj9 = import ./jdk-darwin-base.nix { sourcePerArch = sources.openjdk17.mac.jre.openj9; };
             |                                                               ^
           11| }

       … while evaluating the attribute 'name'

       at /Users/wcohen/.cache/nixpkgs-review/pr-165354/nixpkgs/pkgs/development/compilers/adoptopenjdk-bin/jdk-darwin-base.nix:13:3:

           12|     result = stdenv.mkDerivation {
           13|   name = if sourcePerArch.packageType == "jdk"
             |   ^
           14|     then "adoptopenjdk-${sourcePerArch.vmType}-bin-${sourcePerArch.${cpuName}.version}"
https://github.com/NixOS/nixpkgs/pull/165354 failed to build
$ git worktree prune

@xeals xeals force-pushed the openjdk18 branch 2 times, most recently from ee4b82d to f375389 Compare March 28, 2022 22:56
@willcohen
Copy link
Contributor

@ofborg eval

@xeals
Copy link
Contributor Author

xeals commented Mar 28, 2022

Looking upstream it seems OpenJ9 builds (which nixpkgs pulls from AdoptOpenJDK) are now provided by Semeru. The missing packages should be all removed now.

@willcohen
Copy link
Contributor

$ /nix/store/mygph0cysf2193ygvx9fd7ypydbrgm4k-zulu18.28.13-ca-jdk-18.0.0/bin/java --version
openjdk 18 2022-03-22
OpenJDK Runtime Environment Zulu18.28+13-CA (build 18+37)
OpenJDK 64-Bit Server VM Zulu18.28+13-CA (build 18+37, mixed mode, sharing)

@totoroot
Copy link
Contributor

totoroot commented Apr 5, 2022

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}";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rev = "jdk-${version.feature}+${version.build}";
rev = "jdk-${version}";

Copy link
Contributor Author

@xeals xeals Apr 10, 2022

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.

@xeals xeals changed the title openjdk18: init at 18+36 openjdk18: init at 18+37 Apr 10, 2022
@michielboekhoff
Copy link
Contributor

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 master?

jdkVersion = "18.0.0";
sha256 = "0ch4jp2d4pjvxbmbswvjwf7w2flajrvjg5f16ggiy80y8l0y15cm";
};
}."${stdenv.hostPlatform.system}";
Copy link
Member

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.

Copy link
Contributor Author

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.

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

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor

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:

  1. Help reviewers be more effective, since they can rely on ofborg builds and focus on other aspects of the PR.
  2. Help contributors get faster signal on their PRs, and have a higher confidence that their code is good if the build is green.

@Jonpez2
Copy link

Jonpez2 commented Jun 24, 2022

Hello friends! This issue seems to be a bit stalled; is there any way to push this through please?

Thank you!

@Jonpez2
Copy link

Jonpez2 commented Jul 3, 2022

Please?

Copy link
Member

@fabianhjr fabianhjr left a 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.

Copy link
Contributor

@marsam marsam left a comment

Choose a reason for hiding this comment

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

LGTM

@uri-canva
Copy link
Contributor

@SuperSandro2000 are you happy to proceed?

@filmon-arefayne
Copy link

Do we have an update on this? thanks.

@uri-canva
Copy link
Contributor

Will proceed to merge, let me rebase this.

@marsam
Copy link
Contributor

marsam commented Sep 13, 2022

@GrahamcOfBorg eval

@marsam marsam merged commit 9413ebb into NixOS:master Sep 18, 2022
@xeals xeals deleted the openjdk18 branch September 19, 2022 01:46
@Jonpez2
Copy link

Jonpez2 commented Oct 3, 2022

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 ?
(Sorry if this is the wrong place to ask this!)

Thanks!

@jerith666
Copy link
Contributor

#195638 is the official packaging request for OpenJDK 19.

jerith666 added a commit to jerith666/nixpkgs that referenced this pull request Nov 2, 2022
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
@jerith666 jerith666 mentioned this pull request Nov 2, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet