openjdk17: init at 17.0.1+12 #140257
openjdk17: init at 17.0.1+12 #140257
Conversation
ofborg
bot
added
10.rebuild-darwin: 0
pkgs/development/compilers/openjdk/ignore-LegalNoticeFilePlugin.patch
Outdated
Show resolved
Hide resolved
pkgs/development/compilers/openjdk/ignore-LegalNoticeFilePlugin.patch
Outdated
Show resolved
Hide resolved
|
|
||
| /* default JDK */ | ||
|
|
||
| jdk = jdk16; | ||
| jdk-lts = jdk17; |
There was a problem hiding this comment.
I don't think introducing jdk-lts attribute is a good idea. If an application doesn't use internal APIs and doesn't parse its own bytecode then using jdk will work fine. If that's not the case then updating to newest JDK is going to break stuff. If a user wants an LTS, I think having to specify a version makes sense.
There was a problem hiding this comment.
I agree, the existing jdk alias itself seems good enough
|
Result of 15 packages marked as broken and skipped:
352 packages failed to build:
|
Patch went bad, pushed a fix |
|
@Uthar Thanks for working on this! Just wanted to chime in and say that I pulled in this PR in my workstation config, and so far everything works fine with jdk17 as far as I can tell (IDEA, my gradle builds, etc.). Cheers |
|
Looking forward for this package to get merged. 🤞🏼 |
|
Is anything blocking this PR from getting merged? 👀 |
| mkdir -p $out/share | ||
| ln -s $out/lib/openjdk/include $out/include | ||
| ln -s $out/lib/openjdk/man $out/share/man | ||
| ln -s $out/lib/openjdk/lib/src.zip $out/lib/src.zip |
There was a problem hiding this comment.
Why is this required? Seems really random.
There was a problem hiding this comment.
IDEs use the provided src.zip to navigate the java codebase.
#95081
There was a problem hiding this comment.
Does this really need to be /lib/src.zip? This will cause problems when you use multiple versions of openjdk.
There was a problem hiding this comment.
Also this should have a comment with that information.
There was a problem hiding this comment.
According to this, the src.zip file should be at $JAVA_HOME/lib/src.zip. I guess previous nix maintainers mirror the /include and /lib/src.zip in top level so that they can set $JAVA_HOME to $PROFILE/ or /nix/store/xxxx...xxxx-openjdk-xx/ rather than $PROFILE/lib/openjdk/ or /nix/store/xxxx....xxxx-openjdk-xx/lib/openjdk/. This is only a guess.
There was a problem hiding this comment.
I guess the /lib/src.zip symlink can be removed if people set their JAVA_HOME to /nix/store/xxxx....xxxx-openjdk-xx/lib/openjdk correctly.
There was a problem hiding this comment.
If you don't set JAVA_HOME, java determines it correctly already:
$ java -XshowSettings:properties -version 2>&1 > /dev/null | grep 'java.home'
java.home = /nix/store/krhlgdsmmcxqf4szq6z1m1wl2fsarx48-openjdk-17+35/lib/openjdk
I'm already using a version of this PR, and my IDE (idea-ultimate) correctly detected the new jdk installation. I never set JAVA_HOME because it shouldn't be set IMHO ;-)
That said, the src.zip symlink was there for e.g. jdk11:
dr-xr-xr-x - root 1 Jan 1970 openjdk
lrwxrwxrwx 19 root 1 Jan 1970 src.zip -> openjdk/lib/src.zip
/nix/store/lvmz1yy4v20f0c7rg55bhfxwiaqfi2rj-openjdk-11.0.12+7/
To not break workflows that relied on that, I'd keep it like that.
There was a problem hiding this comment.
I would just leave it as it is, because a change here would break some people's setup, and it would IMO be more of an issue than a potential clash with multiple jdks in profile. There doesn't seem to be any bug in the issue tracker related to this, so I would simply not touch it until it becomes a real problem for somebody.
There was a problem hiding this comment.
There doesn't seem to be any bug in the issue tracker related to this
That is a really bad metric. There are enough issues in NixOS which do not have an issue and there are people like me which never create issues for problems and just fix the issue.
it would IMO be more of an issue than a potential clash with multiple jdks in profile.
That would still be an issue if it would be under /lib/openjdk. It would need to be moved to a versioned directory which is also not great. For now I would just merge this PR until we have a better way to fix this.
|
Reverted those two because the build broke |
|
@Uthar The git history is full of useless commits. We don't need 15 commits for this patch. Please merge this pull request into one or two commits using |
|
@Uthar After that, you can do a force push to your own branch. By the way, I like your work. Thank you for this PR. |
|
I agree, it's a lot of commits - squashed and force pushed now |
| mkdir -p $out/share | ||
| ln -s $out/lib/openjdk/include $out/include | ||
| ln -s $out/lib/openjdk/man $out/share/man | ||
| ln -s $out/lib/openjdk/lib/src.zip $out/lib/src.zip |
There was a problem hiding this comment.
The $out/lib/src.zip symlink is indeed weird. The reason why this symlink is created here should be put in a comment above this line. Otherwise future maintainers will be confused.
There was a problem hiding this comment.
Good idea - added in bcf9864
|
Always remember do |
|
Just FYI there's a new release of java 17.0.1: https://github.com/openjdk/jdk17u/tags |
Yes. We should use the |
|
Bumped it to the latest tag |
|
Exciting work, can't wait for this to be merged! I'm already using it in a private repo and seems to work fine, thanks a lot for putting up the effort @Uthar 🤩 |
|
Thanks everyone for help with this package :) |
Motivation for this change
Package OpenJDK 17 LTS #137956
Notify maintainers: @edwtjo @asbachb
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)