Skip to content

temurin-bin: init at 22, update sources#314377

Merged
raboof merged 5 commits intoNixOS:stagingfrom
Infinidoge:bump-openjdk-temurin
Jun 10, 2024
Merged

temurin-bin: init at 22, update sources#314377
raboof merged 5 commits intoNixOS:stagingfrom
Infinidoge:bump-openjdk-temurin

Conversation

@Infinidoge
Copy link
Contributor

@Infinidoge Infinidoge commented May 24, 2024

Description of changes

Split from #286267

CCing @thiagokokada @philiptaron as previous reviewers

Adds temurin bin 22, and regenerates the sources file

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label May 24, 2024
@ofborg ofborg bot requested a review from taku0 May 24, 2024 21:47
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 501-1000 This PR causes many rebuilds on Linux and should normally target the staging branches. labels May 24, 2024
@Infinidoge Infinidoge force-pushed the bump-openjdk-temurin branch 2 times, most recently from 74cbc0e to 2b997ef Compare May 26, 2024 08:47
@Infinidoge Infinidoge changed the base branch from master to staging May 26, 2024 08:48
philiptaron and others added 5 commits May 26, 2024 04:48
In Python, a parenthesized expression is a tuple iff there is a comma
between the parentheses. In order to have a 1-tuple, we need one more
comma.
@Infinidoge Infinidoge force-pushed the bump-openjdk-temurin branch from 2b997ef to 7717ee9 Compare May 26, 2024 08:48
@Infinidoge
Copy link
Contributor Author

Retargeted to staging due to the rebuild count

../development/compilers/zulu/22.nix
{
openjdk22-bootstrap = temurin-bin.jdk-21;
openjdk22-bootstrap = temurin-bin.jdk-22;
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal, but I think we should keep the version at 21 here, to align with the recommended version in the OpenJDK build documentation:

... the boot JDK for building JDK major version N should be a JDK of major version N-1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All recent versions of Java build with major version N, so this keeps it consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Do we know why? Seems to make more sense to bootstrap with N-1 tbh...

Copy link
Member

Choose a reason for hiding this comment

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

If the concern is to keep it consistent, then we should use N-1 everywhere. Using the same version introduces a cyclic dependency. Also, it makes sense to follow the recommendation from OpenJDK build docs.

Copy link
Contributor Author

@Infinidoge Infinidoge May 30, 2024

Choose a reason for hiding this comment

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

I'm not sure how it introduces a cyclic dependency, because the binary package has no dependency on Java.

IMO LTS releases should definitely be built with the N version, because otherwise it introduces a dependency on a package that will be marked insecure. Non-LTS, either way works.

I don't really see the recommendation as an extremely compelling reason, because they note in the same paragraph that N is also acceptable, but that's just me.

Copy link
Member

Choose a reason for hiding this comment

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

I hadn't considered the dependency on an EOL JDK. That's a good point.

By cyclic dependency, I mean that Temurin depends on the same OpenJDK source code that the derivation does.

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

bit weird with the bootstrap, but LGTM nonetheless

../development/compilers/zulu/22.nix
{
openjdk22-bootstrap = temurin-bin.jdk-21;
openjdk22-bootstrap = temurin-bin.jdk-22;
Copy link
Member

Choose a reason for hiding this comment

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

Do we know why? Seems to make more sense to bootstrap with N-1 tbh...

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two persons. label May 29, 2024
@raboof raboof merged commit 373b164 into NixOS:staging Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 501-1000 This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants