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

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

Open
wants to merge 4 commits into
base: staging
from

Conversation

@monacoremo
Copy link

monacoremo commented May 15, 2020

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.
@@ -34,9 +34,20 @@ let
substituteInPlace crypto/async/arch/async_posix.h \
--replace '!defined(__ANDROID__) && !defined(__OpenBSD__)' \
'!defined(__ANDROID__) && !defined(__OpenBSD__) && 0'
'' + optionalString (versionAtLeast version "1.1.1" && static)

This comment has been minimized.

Copy link
@ajs124

ajs124 May 15, 2020

Member

Why does this depend on 1.1.1?

This comment has been minimized.

Copy link
@monacoremo

monacoremo May 15, 2020

Author

It should probably also be good for 1.0.2. I'll test that and remove the check if it works!

@monacoremo
Copy link
Author

monacoremo commented May 18, 2020

@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 Jun 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Needs review
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.