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

Jump to bottom

openssl: split the (mostly empty) runtime dependencies of static builds into a separate output #87879

Closed
wants to merge 1 commit into from
Closed

openssl: split the (mostly empty) runtime dependencies of static builds into a separate output #87879

wants to merge 1 commit into from

Conversation

monacoremo
Copy link

Motivation for this change

This closure size of all packages that statically link openssl is unnecessarily large, as they get a runtime dependency on the static openssl build.

This is because the paths to several mostly empty directories and files in --openssldir and ENGINESDIR that are being baked into the libcrypto.a file:

$ strings /nix/store/qlw2d1jwjijm1g70f0p4cbwqf9ccmw78-openssl-1.1.1d/lib/libcrypto.a | grep /nix/store
/nix/store/qlw2d1jwjijm1g70f0p4cbwqf9ccmw78-openssl-1.1.1d/etc/ssl/ct_log_list.cnf
OPENSSLDIR: "/nix/store/qlw2d1jwjijm1g70f0p4cbwqf9ccmw78-openssl-1.1.1d/etc/ssl"
ENGINESDIR: "/nix/store/qlw2d1jwjijm1g70f0p4cbwqf9ccmw78-openssl-1.1.1d/lib/engines-1.1"
/nix/store/qlw2d1jwjijm1g70f0p4cbwqf9ccmw78-openssl-1.1.1d/lib/engines-1.1
/nix/store/qlw2d1jwjijm1g70f0p4cbwqf9ccmw78-openssl-1.1.1d/etc/ssl/private
/nix/store/qlw2d1jwjijm1g70f0p4cbwqf9ccmw78-openssl-1.1.1d/etc/ssl
/nix/store/qlw2d1jwjijm1g70f0p4cbwqf9ccmw78-openssl-1.1.1d/etc/ssl/certs

This is an attempt to reduce the runtime dependencies of packages that statically link openssl by:

  • Moving the '--openssldir' to a separate output.

This is relatively simple and clean, but unfortunately not enough, as the ENGINESDIR, which is also being baked into the static library and it cannot be configured (hardcoded to be under libdir by openssl). So we also need to:

  • Patch the ENGINESDIR to be in the separate output instead of libdir on static builds.

This is a bit messy, but I could't figure out a cleaner solution so far... The substitution is only done on static builds, as the dynamic *.so files in ENGINESDIR contain references to $out and cannot be moved to a separate output.

I would be very grateful for any hints on how to improve this PR!

In my use case, this reduces the closure size of e.g. https://github.com/PostgREST/postgrest by over 60%, from 13mb compressed to about 5mb.

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 (added comments)
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from peti May 15, 2020 10:33
@monacoremo monacoremo mentioned this pull request May 15, 2020
Merged
@monacoremo
Copy link
Author

@ajs124 I've tested it with 1.0.2, the version check was not needed and I removed it.

I also renamed the new output from openssldir to etc, as it also contains the ENGINESDIR and reflects the content better, I think.

@FRidh FRidh added this to WIP in Staging via automation Jun 13, 2020
@FRidh FRidh moved this from WIP to Needs review in Staging Jun 13, 2020
@FRidh FRidh changed the base branch from master to staging June 13, 2020 09:07
@monacoremo
Copy link
Author

@ajs124 @peti I believe this PR should be a no-op for non-static builds and all tests should pass, is there a way to re-trigger the test suite?

Do you have any comments on this PR? Happy to work on improving it!

As described above, we are successfully using this patch in PostgREST, it significantly reduces the closure size of the static build from >13mb to 4mb.

@ajs124
Copy link
Member

ajs124 commented Oct 7, 2020

This isn't a no-op for non-static builds, because of the changes in postInstall and postFixup.

It's probably still not a bad idea, though.

@ajs124 ajs124 requested review from mweinelt and vcunat October 7, 2020 15:19
SuperSandro2000
SuperSandro2000 approved these changes Nov 27, 2020
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 like the idea

Staging automation moved this from Needs review to Ready Nov 27, 2020
@mweinelt mweinelt removed their request for review November 28, 2020 00:01
@monacoremo monacoremo mentioned this pull request Dec 22, 2020
Merged
@FRidh FRidh added this to the 21.03 milestone Jan 4, 2021
@monacoremo monacoremo force-pushed the monacoremo-openssl-fix-static-deps branch from 5bcea10 to bbc1a9d Compare April 8, 2021 15:59
@monacoremo
Copy link
Author

I rebased on staging to resolve the merge conflict (rebased on master first by mistake, sorry for the automated review requests :-| ) and tested that pkgs.openssl and pkgsMusl,openssl evaluate and build on NixOS/x64.

@monacoremo monacoremo force-pushed the monacoremo-openssl-fix-static-deps branch from b7f1472 to f4fc277 Compare July 15, 2021 07:54
@monacoremo
Copy link
Author

Rebased on latest staging

@stale
Copy link

stale bot commented Apr 25, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 25, 2022
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 4, 2022
@robx
Copy link
Contributor

robx commented Jul 15, 2022

This patch continues to be maintained in PostgREST, a version against current nixpkgs is at https://github.com/PostgREST/postgrest/blob/692b23abbd7c88f2aaf7b3fac18bd52a0eb09d76/nix/patches/nixpkgs-openssl-split-runtime-dependencies-of-static-builds.patch. I'm a bit curious whether it's realistic to get that applied to nixpkgs?

@veprbl
Copy link
Member

veprbl commented Jul 15, 2022

Quite realistic. But this needs a rebase.

@robx robx mentioned this pull request Jul 22, 2022
Merged
13 tasks
@robx
Copy link
Contributor

robx commented Jul 22, 2022

Quite realistic. But this needs a rebase.

I created a rebased PR at #182444, since @monacoremo isn't around.

@fpletz
Copy link
Member

fpletz commented Jul 26, 2022

As #182444 was merged we can close this. Thanks!

@fpletz fpletz closed this Jul 26, 2022
Staging automation moved this from Ready to Done Jul 26, 2022
@robx robx mentioned this pull request Aug 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajs124 ajs124 ajs124 left review comments

@peti peti Awaiting requested review from peti

@vcunat vcunat Awaiting requested review from vcunat

@edolstra edolstra Awaiting requested review from edolstra

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: closure-size 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 12.approvals: 1
Projects
Staging
  
Done
Milestone
21.05
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@monacoremo @ajs124 @robx @veprbl @fpletz @SuperSandro2000 @vcunat @FRidh @r-burns @Artturin