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

corretto8: init at 8.202.08.2 #55156

Closed
wants to merge 1 commit into from
Closed

Conversation

andrew-d
Copy link
Contributor

@andrew-d andrew-d commented Feb 4, 2019

Motivation for this change

AWS Corretto is a distribution of OpenJDK that comes with additional performance and security patches, and long-term support until at least June 2023.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

As maintainers of the existing OpenJDK 8 package:
r? @edwtjo @NeQuissimus

I'd be interested to hear what you folks think about this "override openjdk8" approach; it saves a ton of code duplication and refactoring compared to writing an entirely new derivation.


# Some minor patches to OpenJDK to get everything building; taken from:
# https://github.com/corretto/corretto-8/issues/23
./corretto.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to fetch rather than add the patch to the repo?

Copy link
Contributor Author

@andrew-d andrew-d Mar 23, 2019

Choose a reason for hiding this comment

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

The actual patch isn't a PR on the main repository, but rather a commit from a fork:
dacut/corretto-8@68c43e7

(EDIT: especially since the patch is so small)

I thought that committing the patch would be more resilient to the fork being deleted or changed, but happy to fetchpatch that diff if you'd rather.

Copy link
Contributor

Choose a reason for hiding this comment

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

By itself this is benign, I guess, but it still adds to the overall amount of unnecessary stuff in the repo. Your call.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Source files are normally mirrored by the nixos org, can't say whether patch files are without checking the script)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll keep it; if it was more than 2 lines, I'd fetch it, but as-is I think it's pretty small.

, bash
, openjdk8

, ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Without args I think it makes sense to remove the rest capture as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, also fair - done! 👍

@@ -7196,6 +7196,11 @@ in
licenseAccepted = config.oraclejdk.accept_license or false;
});

corretto8 = callPackage ../development/compilers/openjdk/corretto.nix {
bootjdk = bootjdk.override { version = "8"; };
inherit (gnome2) GConf gnome_vfs;
Copy link
Contributor

Choose a reason for hiding this comment

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

Eval error here. I guess these used to be captured by args

@aanderse
Copy link
Member

@andrew-d What is the status of this?

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

5 participants