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

ngtcp2-gnutls: init at 0.7.0 and use in knot-dns #188383

Merged
merged 2 commits into from
Sep 1, 2022

Conversation

vcunat
Copy link
Member

@vcunat vcunat commented Aug 26, 2022

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

vcunat added 2 commits August 26, 2022 11:31
It's not that useful for now, but it only adds about 0.4 MB in closure.
$ kdig @ns1.xdp.cz +quic news.xdp.cz TXT
@vcunat
Copy link
Member Author

vcunat commented Aug 26, 2022

@Izorkin might want to know about the split or comment. Anyway, I don't expect significant usage of the openssl variant before the official openssl is made compatible with QUIC.

@vcunat vcunat mentioned this pull request Aug 26, 2022
13 tasks
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 26, 2022
@Izorkin
Copy link
Contributor

Izorkin commented Aug 26, 2022

Not working this variant?

index 90372cc97a2..8c75eb1b8c5 100644
--- a/pkgs/development/libraries/ngtcp2/default.nix
+++ b/pkgs/development/libraries/ngtcp2/default.nix
@@ -1,10 +1,14 @@
 { lib, stdenv, fetchFromGitHub
 , autoreconfHook, pkg-config, file
-, libev, nghttp3, quictls
+, libev, nghttp3
 , cunit, ncurses
 , withJemalloc ? false, jemalloc
+, withGnutls ? false, gnutls
+, withQuictls ? true, quictls
 }:

+assert !(withGnutls && withQuictls);
+
 stdenv.mkDerivation rec {
   pname = "ngtcp2";
   version = "0.7.0";
@@ -19,9 +23,16 @@ stdenv.mkDerivation rec {
   outputs = [ "out" "dev" "doc" ];

   nativeBuildInputs = [ autoreconfHook pkg-config file ];
-  buildInputs = [ libev nghttp3 quictls ] ++ lib.optional withJemalloc jemalloc;
+
+  buildInputs = [ libev nghttp3 ]
+    ++ lib.optional withQuictls quictls
+    ++ lib.optional withGnutls gnutls
+    ++ lib.optional withJemalloc jemalloc;
+
   checkInputs = [ cunit ncurses ];

+  configureFlags = lib.optional withGnutls "--with-gnutls=yes";
+
   preConfigure = ''
     substituteInPlace ./configure --replace /usr/bin/file ${file}/bin/file
   '';
diff --git a/pkgs/servers/dns/knot-dns/default.nix b/pkgs/servers/dns/knot-dns/default.nix
index 23fefe80e76..fff31f86df3 100644
--- a/pkgs/servers/dns/knot-dns/default.nix
+++ b/pkgs/servers/dns/knot-dns/default.nix
@@ -1,5 +1,5 @@
 { lib, stdenv, fetchurl, pkg-config, gnutls, liburcu, lmdb, libcap_ng, libidn2, libunistring
-, systemd, nettle, libedit, zlib, libiconv, libintl, libmaxminddb, libbpf, nghttp2, libmnl
+, systemd, nettle, libedit, zlib, libiconv, libintl, libmaxminddb, libbpf, ngtcp2, nghttp2, libmnl
 , autoreconfHook, nixosTests, knot-resolver
 }:

@@ -32,6 +32,7 @@ stdenv.mkDerivation rec {
     gnutls liburcu libidn2 libunistring
     nettle libedit
     libiconv lmdb libintl
+    ngtcp2
     nghttp2 # DoH support in kdig
     libmaxminddb # optional for geoip module (it's tiny)
     # without sphinx &al. for developer documentation
diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index bb0430ef58d..6d7576bacef 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -22657,7 +22657,10 @@ with pkgs;

   keycloak = callPackage ../servers/keycloak { };

-  knot-dns = callPackage ../servers/dns/knot-dns { };
+  knot-dns = callPackage ../servers/dns/knot-dns {
+    ngtcp2 = ngtcp2.override { withGnutls = true; withQuictls = false; };
+  };
+
   knot-resolver = callPackage ../servers/dns/knot-resolver {
     systemd = systemdMinimal; # in closure already anyway
   };

@vcunat
Copy link
Member Author

vcunat commented Aug 26, 2022

Yes, it's certainly possible to merge this way into one nix expression. But I might also need ngtcp2 version kept in lock-step with knot-dns. Until nghtcp2 development stabilizes. So the overal code sharing opportunities don't seem significant.

@Izorkin
Copy link
Contributor

Izorkin commented Aug 26, 2022

Then it will be easier to have two variants.
Knot-dns can't work with quictls?

@vcunat
Copy link
Member Author

vcunat commented Aug 27, 2022

No, it only uses gnutls for crypto. (including parts that are not related to TLS or QUIC)

@bobby285271 bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Aug 27, 2022
Why split from ./default.nix?

ngtcp2 libs contain helpers to plug into various crypto libs (gnutls, patched openssl, ...).
Building multiple of them while keeping closures separable would be relatively complicated.
Copy link
Member

Choose a reason for hiding this comment

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

Would it be a problem to compile openssl and gnutls into one package?

Copy link
Member Author

@vcunat vcunat Aug 28, 2022

Choose a reason for hiding this comment

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

You would need to put each of these into a separate output (and leaving the base lib in a third one) – and then patch paths in pkg-config and libtool files. That is, if the closure blowup should be avoided.

Copy link
Member

Choose a reason for hiding this comment

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

"blowup" according to the comment it is not even 1MB, or am I missing something? Are the new deps bigger?

Copy link
Member Author

@vcunat vcunat Sep 2, 2022

Choose a reason for hiding this comment

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

No, that's a different one. The openssl fork takes a few megabytes and gnutls+deps also (and each additional crypto lib that you'd add). Packages always want just one but they'd get all of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

The potential blowup in this comment is for the situation that got merged. That is you maybe need (through different packages) ngtcp2 with both openssl and gnutls, in which case you get the core twice. And the core is 0.3--0.4 MB.

@vcunat vcunat merged commit 2da64a8 into NixOS:master Sep 1, 2022
@vcunat vcunat deleted the p/knot-dns_quic branch September 1, 2022 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants