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.
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
openjdk17: init at 17.0.1+12 #140257
openjdk17: init at 17.0.1+12 #140257
Changes from all commits
64a379b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why is this required? Seems really random.
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.
IDEs use the provided src.zip to navigate the java codebase.
#95081
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this should have a comment with that information.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the
/lib/src.zip
symlink can be removed if people set theirJAVA_HOME
to/nix/store/xxxx....xxxx-openjdk-xx/lib/openjdk
correctly.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.
If you don't set
JAVA_HOME
, java determines it correctly already: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:
To not break workflows that relied on that, I'd keep it like that.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea - added in bcf9864
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 introducing
jdk-lts
attribute is a good idea. If an application doesn't use internal APIs and doesn't parse its own bytecode then usingjdk
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, the existing
jdk
alias itself seems good enough