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.

Sign up for GitHub

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

Jump to bottom

adoptopenjdk-bin, graalvm11-ce, graalvm8-ce, zulu, zulu8: add support for GTK+ Look and Feel #107784

Merged
merged 5 commits into from Feb 18, 2021
Merged

adoptopenjdk-bin, graalvm11-ce, graalvm8-ce, zulu, zulu8: add support for GTK+ Look and Feel #107784

merged 5 commits into from Feb 18, 2021

Conversation

taku0
Copy link
Contributor

@taku0 taku0 commented Dec 28, 2020

Motivation for this change

This adds Swing support and optional GTK+ theme for JDK packages.

See also #106716.

To test the packages, you can use https://github.com/taku0/swing-set-2:

for i in adoptopenjdk-hotspot-bin-11 adoptopenjdk-hotspot-bin-8 graalvm11-ce graalvm8-ce zulu zulu8
do
  nix-build -I nixpkgs=. -o "${i}" -A pkgs."${i}"
done

git clone --depth 1 https://github.com/taku0/swing-set-2
nix-build -I nixpkgs=. -o SwingSet2 ./swing-set-2

adoptopenjdk-hotspot-bin-8/bin/java -jar SwingSet2/SwingSet2.jar
# and other JDKs.

Then choose Look & Feel → GTK Style Look & Feel from the menu (Linux only).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@taku0 taku0 mentioned this pull request Dec 28, 2020
Open
13 tasks
@ofborg ofborg bot requested review from fpletz and hlolli December 28, 2020 08:32
@ofborg ofborg bot requested a review from glittershark December 30, 2020 03:54
@taku0
Copy link
Contributor Author

taku0 commented Dec 30, 2020

While I prefer style that put do and then on their own line as The Single UNIX Specification, many others seem to prefer hanging delimiters:

glittershark
glittershark suggested changes Jan 2, 2021
View reviewed changes
pkgs/development/compilers/adoptopenjdk-bin/jdk-linux-base.nix Outdated
# runtime dependencies
, cups
# runtime dependencies for GTK+ Look and Feel
, enableGTK ? true
Copy link
Member

Choose a reason for hiding this comment

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

I have seen other packages in nixpkgs call these kinds of flags eg gtkSupport - I wonder if there's an official style guide for this?

Copy link
Member

Choose a reason for hiding this comment

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

@taku0 ^

Copy link
Member

Choose a reason for hiding this comment

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

or enableGTKSupport. I don't think there is any style for that.

pkgs/development/compilers/graalvm/community-edition.nix Show resolved Hide resolved
pkgs/development/compilers/graalvm/community-edition.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/graalvm/community-edition.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/graalvm/community-edition.nix Show resolved Hide resolved
taku0
taku0 commented Jan 3, 2021
View reviewed changes
Copy link
Contributor Author

@taku0 taku0 left a comment

Choose a reason for hiding this comment

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

Fixed per review comments.

pkgs/development/compilers/graalvm/community-edition.nix Show resolved Hide resolved
pkgs/development/compilers/graalvm/community-edition.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from glittershark January 3, 2021 05:55
taku0
taku0 commented Jan 10, 2021
View reviewed changes
Copy link
Contributor Author

@taku0 taku0 left a comment

Choose a reason for hiding this comment

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

Fixed per review comments.

@SuperSandro2000
Copy link
Member

/rebase-staging

@github-actions github-actions bot changed the base branch from master to staging January 10, 2021 12:07
This was referenced Jan 23, 2021
Merged
Closed
@glittershark
Copy link
Member

@taku0 mind rebasing this?

@taku0
Copy link
Contributor Author

taku0 commented Jan 26, 2021

Rebased. Should we wait for #105815 to be merged? That will conflict with this PR.

@glittershark
Copy link
Member

Yeah, probably for the best.

@taku0
Copy link
Contributor Author

taku0 commented Feb 7, 2021

#105815 is merged. I have rebased on the staging and it works again. Ready to merge.

@taku0
Copy link
Contributor Author

taku0 commented Feb 8, 2021

Fixed warnings.

@ofborg ofborg bot requested a review from glittershark February 8, 2021 10:20
@taku0
Copy link
Contributor Author

taku0 commented Feb 14, 2021

Fixed conflicts.

@taku0
Copy link
Contributor Author

taku0 commented Feb 17, 2021

Fixed conflicts.

SuperSandro2000
SuperSandro2000 reviewed Feb 18, 2021
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

I am trying to build the packages mentioned in the title tomorrow.

pkgs/development/compilers/graalvm/community-edition.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/adoptopenjdk-bin/jdk-linux-base.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/zulu/8.nix Outdated Show resolved Hide resolved
@taku0
Copy link
Contributor Author

taku0 commented Feb 18, 2021

Applied suggestions.

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 107784 run on x86_64-linux 1

5 packages built:
  • adoptopenjdk-bin
  • graalvm11-ce
  • graalvm8-ce
  • zulu
  • zulu8

The following issues got detected with the above build packages.
Please fix at least the ones listed with your changed packages:

adoptopenjdk-bin:

installPhase should probably contain runHook preInstall and runHook postInstall.

Near pkgs/development/compilers/adoptopenjdk-bin/jdk-linux-base.nix:63:3:

   |
63 |   installPhase = ''
   |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/missing-phase-hooks.md

graalvm11-ce:

installPhase should probably contain runHook preInstall and runHook postInstall.

Near pkgs/development/compilers/graalvm/community-edition.nix:152:9:

    |
152 |         installPhase = {
    |         ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/missing-phase-hooks.md

graalvm8-ce:

installPhase should probably contain runHook preInstall and runHook postInstall.

Near pkgs/development/compilers/graalvm/community-edition.nix:152:9:

    |
152 |         installPhase = {
    |         ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/missing-phase-hooks.md

zulu8:

gpl2 is a deprecated license, check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

Near pkgs/development/compilers/zulu/8.nix:100:5:

    |
100 |     license = licenses.gpl2;
    |     ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/unclear-gpl.md
gcc-unwrapped is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

Near pkgs/development/compilers/zulu/8.nix:51:3:

   |
51 |   buildInputs = lib.optionals stdenv.isLinux [
   |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/build-tools-in-build-inputs.md
installPhase should probably contain runHook preInstall and runHook postInstall.

Near pkgs/development/compilers/zulu/8.nix:69:3:

   |
69 |   installPhase = ''
   |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/missing-phase-hooks.md

zulu:

gpl2 is a deprecated license, check if project uses gpl2Plus or gpl2Only and change meta.license accordingly.

Near pkgs/development/compilers/zulu/default.nix:100:5:

    |
100 |     license = licenses.gpl2;
    |     ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/unclear-gpl.md
gcc-unwrapped is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

Near pkgs/development/compilers/zulu/default.nix:52:3:

   |
52 |   buildInputs = lib.optionals stdenv.isLinux [
   |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/build-tools-in-build-inputs.md
installPhase should probably contain runHook preInstall and runHook postInstall.

Near pkgs/development/compilers/zulu/default.nix:71:3:

   |
71 |   installPhase = ''
   |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/missing-phase-hooks.md

@SuperSandro2000
Copy link
Member

Maybe it is worth to think about output splitting for the future:

adoptopenjdk-bin: Ask if ouput path include (211.2K) could be split with outputs = [ ... "dev" ];
adoptopenjdk-bin: Ask if ouput path lib (227.7M) could be split with outputs = [ ... "lib" ];
graalvm11-ce: Ask if ouput path include (220.4K) could be split with outputs = [ ... "dev" ];
graalvm11-ce: Ask if ouput path lib (645.8M) could be split with outputs = [ ... "lib" ];
graalvm8-ce: Ask if ouput path include (212.7K) could be split with outputs = [ ... "dev" ];
graalvm8-ce: Ask if ouput path lib (109.5M) could be split with outputs = [ ... "lib" ];
zulu: Ask if ouput path include (211.2K) could be split with outputs = [ ... "dev" ];
zulu: Ask if ouput path lib (227.8M) could be split with outputs = [ ... "lib" ];
zulu8: Ask if ouput path include (203.3K) could be split with outputs = [ ... "dev" ];
zulu8: Ask if ouput path lib (37.5M) could be split with outputs = [ ... "lib" ];

@SuperSandro2000 SuperSandro2000 merged commit 05b0e49 into NixOS:staging Feb 18, 2021
@collares collares mentioned this pull request Feb 27, 2021
Closed
@AndrewKvalheim AndrewKvalheim mentioned this pull request Jul 1, 2023
Merged
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@fpletz fpletz Awaiting requested review from fpletz

@hlolli hlolli Awaiting requested review from hlolli

@glittershark glittershark Awaiting requested review from glittershark

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@taku0 @SuperSandro2000 @glittershark