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

php.extensions.mysqlnd: Fix compression support #89266

Merged
merged 2 commits into from Jun 3, 2020
Merged

php.extensions.mysqlnd: Fix compression support #89266

merged 2 commits into from Jun 3, 2020

Conversation

talyz
Copy link
Contributor

@talyz talyz commented May 31, 2020

Motivation for this change

HAVE_ZLIB has to be defined in mysqlnd.h for compression support to be turned on, but the configure script doesn't actually define it even when zlib is available.

Fixes #89226.

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.

@talyz talyz requested review from aanderse, Izorkin and a team May 31, 2020 20:48
@jtojnar
Copy link
Contributor

jtojnar commented May 31, 2020 •
edited

Possibly relevant: php/php-src@ee4295b

@Izorkin
Copy link
Contributor

Izorkin commented Jun 1, 2020

I disabled the zlib extension, now in mysqlnd Compression supported does not change to Compression - not supported
I think there should be a dynamic check of the zlib extension.

@talyz
Copy link
Contributor Author

talyz commented Jun 1, 2020

@jtojnar Yes, that seems relevant - it just didn't go all the way, likely because it's not needed when building the extension as a part of PHP...

@Izorkin Well, the mysqlnd extension doesn't use the zlib extension, but links to zlib on its own, so that's expected behavior.

@jtojnar
Copy link
Contributor

jtojnar commented Jun 1, 2020 •
edited

@talyz Not sure what you mean by going all the way, PKG_CHECK_MODULES should set HAVE_ZLIB variable.

@talyz
Copy link
Contributor Author

talyz commented Jun 2, 2020

@jtojnar Oh, maybe it should. That's not how they set it in the zlib module (they use AC_DEFINE(HAVE_ZLIB,1,[ ]) there), so I figured that's what they would have done here too; or just remove HAVE_ZLIB, since it's explicitly checked for anyway :)

@jtojnar
Copy link
Contributor

jtojnar commented Jun 2, 2020

Yeah, that seems to work: php/php-src#5655

diff --git a/pkgs/top-level/php-packages.nix b/pkgs/top-level/php-packages.nix
index 82c68c2127d..63243266702 100644
--- a/pkgs/top-level/php-packages.nix
+++ b/pkgs/top-level/php-packages.nix
@@ -1,4 +1,4 @@
-{ stdenv, lib, pkgs, fetchgit, php, autoconf, pkgconfig, re2c
+{ stdenv, lib, pkgs, fetchgit, fetchpatch, php, autoconf, pkgconfig, re2c
 , gettext, bzip2, curl, libxml2, openssl, gmp, icu, oniguruma, libsodium
 , html-tidy, libzip, zlib, pcre, pcre2, libxslt, aspell, openldap, cyrus_sasl
 , uwimap, pam, libiconv, enchant1, libXpm, gd, libwebp, libjpeg, libpng
@@ -983,14 +983,14 @@ in
         # The configure script doesn't correctly add library link
         # flags, so we add them to the variable used by the Makefile
         # when linking.
-        MYSQLND_SHARED_LIBADD = "-lssl -lcrypto -lz";
+        MYSQLND_SHARED_LIBADD = "-lssl -lcrypto";
         # The configure script builds a config.h which is never
         # included. Let's include it in the main header file
         # included by all .c-files.
         patches = [
           (pkgs.writeText "mysqlnd_config.patch" ''
-            --- a/mysqlnd.h
-            +++ b/mysqlnd.h
+            --- a/ext/mysqlnd/mysqlnd.h
+            +++ b/ext/mysqlnd/mysqlnd.h
             @@ -1,3 +1,6 @@
             +#ifdef HAVE_CONFIG_H
             +#include "config.h"
@@ -999,7 +999,20 @@ in
                +----------------------------------------------------------------------+
                | Copyright (c) The PHP Group                                          |
           '')
+        ] ++ lib.optionals (!lib.versionOlder php.version "7.4") [
+          # Fix compression support
+          # https://github.com/php/php-src/pull/5655
+          (fetchpatch {
+            url = "https://github.com/php/php-src/commit/fd852e5896c506a501642b8cd6c5620bfa0cbd36.patch";
+            sha256 = "xhmEAzulTmcfXSIYIFHWHnq3s4zw4nKhwXJgRNMQ3Rk=";
+          })
         ];
+
+        # mkExtension sets sourceRoot to the extension directory
+        # so we need this in order to avoid fiddling with patches.
+        prePatch = ''pushd ../..'';
+        postPatch = ''popd'';
+
         postPhpize = lib.optionalString (lib.versionOlder php.version "7.4") ''
           substituteInPlace configure --replace '$OPENSSL_LIBDIR' '${openssl}/lib' \
                                       --replace '$OPENSSL_INCDIR' '${openssl.dev}/include'
$ nix shell -f . 'php' -L -c php -r 'phpinfo();' | grep Compression
Compression => supported

talyz added 2 commits June 2, 2020 15:37
@talyz
php.extensions: Apply patches from the php source root by default
5db5b25
@talyz
php.extensions.mysqlnd: Fix compression support
2e0d4a8 
HAVE_ZLIB has to be defined in mysqlnd.h for compression support to be
turned on, but the configure script doesn't actually define it even
when zlib is available.
@talyz
Copy link
Contributor Author

talyz commented Jun 2, 2020

@jtojnar I changed the patching behavior, so that mkExtension always patches from the PHP source root, like you did in your patch. I kept my patch, though, since it works for all versions of PHP, not only 7.4. I also feel like removing HAVE_ZLIB might be the way to go for upstream as well, since it's probably just a leftover from before php/php-src@ee4295b - if you specify that you want compression support enabled, the extension won't build without zlib, since the PKG_CHECK_MODULES check will fail...

@Izorkin
Copy link
Contributor

Izorkin commented Jun 2, 2020

Found in build log:

checking whether to enable mysqlnd... yes, shared
checking whether to disable compressed protocol support in mysqlnd... yes

By default, compression is disabled?

@Izorkin
Copy link
Contributor

Izorkin commented Jun 2, 2020

Checking with configureFlags = [ "--disable-mysqlnd-compression-support" ];

checking whether to enable mysqlnd... yes, shared
checking whether to disable compressed protocol support in mysqlnd... no

Default enable compression support.

@jtojnar
Copy link
Contributor

jtojnar commented Jun 3, 2020

I kept my patch, though, since it works for all versions of PHP, not only 7.4. I also feel like removing HAVE_ZLIB might be the way to go for upstream as well

Upstream agrees, could you try to PR your patch then?

@talyz talyz mentioned this pull request Jun 3, 2020
Closed
@talyz
Copy link
Contributor Author

talyz commented Jun 3, 2020

@Izorkin Yeah, I was confused by that too. Seems like it's just the configure output that's being flipped, though.

@jtojnar Done! :) php/php-src#5658

@talyz talyz merged commit b084cf2 into NixOS:master Jun 3, 2020
@talyz talyz deleted the mysqlnd-fix-compression branch June 3, 2020 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar approved these changes

@Izorkin Izorkin Izorkin approved these changes

@aanderse aanderse Awaiting requested review from aanderse aanderse is a code owner

@etu etu Awaiting requested review from etu

@Ma27 Ma27 Awaiting requested review from Ma27 Ma27 is a code owner

@javaguirre javaguirre Awaiting requested review from javaguirre

@offlinehacker offlinehacker Awaiting requested review from offlinehacker

@globin globin Awaiting requested review from globin globin is a code owner

@CMCDragonkai CMCDragonkai Awaiting requested review from CMCDragonkai

Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

phpExtensions.mysqlnd: compression not supported
3 participants
@talyz @jtojnar @Izorkin