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

jre_minimal: strip libraries #115523

Merged
merged 1 commit into from
Apr 22, 2021
Merged

jre_minimal: strip libraries #115523

merged 1 commit into from
Apr 22, 2021

Conversation

raboof
Copy link
Member

@raboof raboof commented Mar 9, 2021

Motivation for this change

If you're building a bespoke minimal JRE, there is a good chance you
want it to be stripped, so do that by default (but allow overriding this
behavior).

Fixes #115486

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.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Mar 9, 2021
@SuperSandro2000
Copy link
Member

Should we maybe also strip the full variant?

@raboof
Copy link
Member Author

raboof commented Mar 9, 2021

Should we maybe also strip the full variant?

I think we already do. (if you want to check for yourself, the most interesting file to look at is libjvm.so - that is ~200mb without stripping and ~50mb with stripping)

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Mar 9, 2021 •
edited
Loading

199M -> 38M

🎉

I think we already do.

I checked and thats true.

@raboof raboof force-pushed the jre_minimal-strip-library branch from 25149aa to a08cfa1 Compare March 9, 2021 13:56
@Atemu
Copy link
Member

Atemu commented Mar 11, 2021

Wasn't there an automatic stripping phase in stdenv?

@raboof
jre_minimal: strip libraries
d3e9040 
runCommand doesn't invoke the automatic stripping from stdenv,
expanding the derivation like this does.

Fixes NixOS#115486
@raboof raboof force-pushed the jre_minimal-strip-library branch from a08cfa1 to d3e9040 Compare March 11, 2021 13:28
@raboof
Copy link
Member Author

raboof commented Mar 11, 2021

Wasn't there an automatic stripping phase in stdenv?

Thanks for the nudge. I was going to reply binaries aren't fully stripped anyway with that (since #21667), but making this a regular derivation and using stripDebugFlags to make sure everything is stripped makes this derivation a lot neater anyway, so I rewrote it to be like that.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/483

@kubek2k
Copy link
Contributor

kubek2k commented Mar 24, 2021

as an upshot - this also stops dragging gcc into the env as a dependency which is a 🙌

roberth
roberth reviewed Apr 22, 2021
View reviewed changes
pkgs/development/compilers/openjdk/jre.nix
passthru = {
home = "${jre}";
};
} ''
jlink --module-path ${jdk}/lib/openjdk/jmods --add-modules ${lib.concatStringsSep "," modules} --output $out
patchelf --shrink-rpath $out/bin/* $out/lib/jexec $out/lib/jspawnhelper $out/lib/*.so $out/lib/*/*.so
Copy link
Member

Choose a reason for hiding this comment

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

I suppose --shrink-rpath has become redundant?
It would be nice to have a test in passthru. Perhaps just a small package that uses this jre in its tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose --shrink-rpath has become redundant?

Yes - I tried patchelf --shrink-rpath on the resulting binaries and it doesn't produce any change anymore

It would be nice to have a test in passthru. Perhaps just a small package that uses this jre in its tests.

I agree that would be nice, though I'd rather not grow this PR with it..

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I respect that.

@raboof
Copy link
Member Author

raboof commented Apr 22, 2021

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

1 package built:
  • jre_minimal

roberth
roberth approved these changes Apr 22, 2021
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

I couldn't find a package to test this. openapi-generator-cli needs org/xml/sax/InputSource. Jitsi needs java.util.logging. These were jre_headless use cases, not jre_minimal. jre_minimal isn't used by nixpkgs itself atm. It's also a fairly new package.
If this breaks anything, the impact seems to be small.

@roberth roberth merged commit fb45a00 into NixOS:master Apr 22, 2021
@roberth
Copy link
Member

roberth commented Apr 22, 2021

I'm even more interested in having a test case now. I wonder if we should define another jre variation with only a couple of frequently used modules. headless is quite big, but we shouldn't define too many distinct variations, because that creates duplication bloat.

@raboof
Copy link
Member Author

raboof commented Apr 28, 2021

I'm even more interested in having a test case now.

I made a basic start in #121043, feedback welcome.

I wonder if we should define another jre variation with only a couple of frequently used modules. headless is quite big, but we shouldn't define too many distinct variations, because that creates duplication bloat.

I think we should follow the approach that AFAIK upstream recommends and other distro's also follow, which is to use the full JDK for 'general-purpose' systems and only use jlink'ed 'minimal' JRE's for bespoke systems (i.e. not in nixpkgs).

If you create 'variations', before you know it a typical system will include multiple of those 'variations', defeating the original purpose.

@roberth
Copy link
Member

roberth commented Apr 28, 2021

If you create 'variations', before you know it a typical system will include multiple of those 'variations', defeating the original purpose.

Yea, the monolithic modules file is a problem. I wonder if we should just unpack all modules into separate derivations and put only the ones an app needs in its runtime --module-path. That way we do get sharing, because different apps will use outputs of the same derivations.
A similar trick can be done for non-modules files.

@raboof
Copy link
Member Author

raboof commented Apr 28, 2021

I wonder if we should just unpack all modules into separate derivations and put only the ones an app needs in its runtime --module-path. That way we do get sharing, because different apps will use outputs of the same derivations.
A similar trick can be done for non-modules files.

That's an interesting idea - hard to predict if it's be worth the extra complexity though..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth approved these changes

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

jre_minimal package does not strip libjvm.so by default to make package size smaller
6 participants
@raboof @SuperSandro2000 @Atemu @nixos-discourse @kubek2k @roberth