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

nixos/zoneminder: fix evaluation with new php refactor #84952

Merged
merged 2 commits into from May 17, 2020

Conversation

@danielfullmer
Copy link
Contributor

danielfullmer commented Apr 10, 2020

Motivation for this change

Zoneminder is currently broken in master, I believe as a result of recently merging #83896. In this PR I've fixed the evaluation and also added a test to hopefully catch regressions in the future.

This PR should be ready to merge after #84993.


Original issue (now fixed thanks to @talyz below):

However, the phpfm service still does not yet work.

The included test fails with:

machine: must succeed: curl --fail http://localhost:8095/
machine # [   15.334307] nginx[778]: 2020/04/10 19:14:14 [error] 779#779: *2 FastCGI sent in stderr: "PHP message: Unable to connect to ZM DB SQLSTATE[HY000] [2002] No such file or directoryPHP message: PHP Fatal error:  Uncaught Error: Call to a member function query() on null in /nix/store/cj1xaxbwisjm3vj3nwnc408b50l7l7m8-zoneminder-1.34.3/share/zoneminder/www/includes/config.php:165
machine # [   15.336875] nginx[778]: Stack trace:
machine # [   15.337622] nginx[778]: curl: (22) The reque#0 /nix/store/cj1xaxbwisjm3vj3nwnc408b50l7l7m8-zoneminder-1.34.3/share/zoneminder/www/includes/config.php(140): loadConfig()
machine # [   15.339833] sted URL returned enginx[778]: #1 /nix/store/cj1xaxbwisjm3vj3nwnc408b50l7l7m8-zoneminder-1.34.3/share/zoneminder/www/index.php(46): require_once('/nix/store/cj1x...')
machine # [   15.342032] nginx[778]: #2 {main}
machine # [   15.342566] nginx[778]:   thrown in /nix/store/cj1xaxbwisjm3vj3nwnc408b50l7l7m8-zoneminder-1.34.3/share/zoneminder/www/includes/config.php on line 165" while reading response header from upstream, client: 127.0.0.1, server: localhost, request: "GET / HTTP/1.1", upstream: "fastcgi://unix:/run/phpfpm/zoneminder.sock:", host: "localhost:8095"rror: 500 Internal Server Error

It appears to fail here:
https://github.com/ZoneMinder/zoneminder/blob/689956bba738de157ebf31f2e9e22f02bc574686/web/includes/database.php#L59
However, as far as I can tell, the DB connection parameters are set correctly.

I'm not very familiar with PHP to properly debug this. I was hoping I could get some feedback from @etu or @talyz if they have any ideas if this is a result of their PR.

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 mentioned this pull request Apr 11, 2020
5 of 10 tasks complete
@talyz
Copy link
Contributor

talyz commented Apr 11, 2020

Thank you for fixing zoneminder and for supplying the test!

The issue here is that the pdo_mysql driver doesn't know where the MySQL unix socket is located. I've opened #84993 to fix that. If you want a quick fix to continue testing before that's merged, you can set the socket location in the config with services.zoneminder.database.host = "localhost:/run/mysqld/mysqld.sock".

@danielfullmer
Copy link
Contributor Author

danielfullmer commented Apr 11, 2020

@talyz Thanks this fixed the issue for me!

@danielfullmer danielfullmer force-pushed the danielfullmer:zoneminder-php-fix branch from ea6b518 to cdb8c32 Apr 11, 2020
@danielfullmer danielfullmer marked this pull request as ready for review Apr 11, 2020
@danielfullmer danielfullmer changed the title zoneminder: fix evaluation with new php refactor nixos/zoneminder: fix evaluation with new php refactor Apr 11, 2020
@danielfullmer danielfullmer mentioned this pull request Apr 11, 2020
5 of 10 tasks complete
@danielfullmer danielfullmer force-pushed the danielfullmer:zoneminder-php-fix branch from cdb8c32 to 9e339cd Apr 11, 2020
@aanderse
Copy link
Contributor

aanderse commented May 17, 2020

ping

@@ -289,11 +285,9 @@ in {
phpfpm = lib.mkIf useNginx {
pools.zoneminder = {
inherit user group;
phpPackage = pkgs.php.withExtensions (e: pkgs.php.enabledExtensions ++ [ e.apcu ]);

This comment has been minimized.

Copy link
@etu

etu May 17, 2020

Contributor

I'm pretty sure that this will fail on current master, we did another change in #85026 😉

Suggested change
phpPackage = pkgs.php.withExtensions (e: pkgs.php.enabledExtensions ++ [ e.apcu ]);
phpPackage = pkgs.php.withExtensions ({ enabled, all }: enabled ++ [ all.apcu ]);

This should work with the updated withExtensions, so a rebase on master and this fix and it should be all fine 🙂

@danielfullmer danielfullmer force-pushed the danielfullmer:zoneminder-php-fix branch from 9e339cd to 2d65971 May 17, 2020
@danielfullmer
Copy link
Contributor Author

danielfullmer commented May 17, 2020

Just rebased against master and briefly tested that it's working.

@etu
Copy link
Contributor

etu commented May 17, 2020

@danielfullmer Now it doesn't work, the tests doesn't build. But it's because of the reason I've mentioned above :-)

$ nix-build -A nixosTests.zoneminder
error: attribute 'enabledExtensions' missing, at /persistent/home/etu/code/nixpkgs/nixos/modules/services/misc/zoneminder.nix:288:52
(use '--show-trace' to show detailed location information)
@danielfullmer danielfullmer force-pushed the danielfullmer:zoneminder-php-fix branch from 2d65971 to fed0777 May 17, 2020
@danielfullmer
Copy link
Contributor Author

danielfullmer commented May 17, 2020

@etu Whoops! I didn't fixup that commit correctly. Just pushed the change.

@etu
Copy link
Contributor

etu commented May 17, 2020

@danielfullmer Now the tests works, that's nice :)

Do you mind applying this diff as well to the commit adding the test?

diff --git a/pkgs/servers/zoneminder/default.nix b/pkgs/servers/zoneminder/default.nix
index 978893d28ff..8dcbe36850a 100644
--- a/pkgs/servers/zoneminder/default.nix
+++ b/pkgs/servers/zoneminder/default.nix
@@ -1,7 +1,7 @@
 { stdenv, lib, fetchFromGitHub, fetchurl, substituteAll, cmake, makeWrapper, pkgconfig
 , curl, ffmpeg, glib, libjpeg, libselinux, libsepol, mp4v2, libmysqlclient, mysql, pcre, perl, perlPackages
 , polkit, utillinuxMinimal, x264, zlib
-, coreutils, procps, psmisc }:
+, coreutils, procps, psmisc, nixosTests }:
 
 # NOTES:
 #
@@ -172,7 +172,10 @@ in stdenv.mkDerivation rec {
     "-DZM_WEB_GROUP=${user}"
   ];
 
-  passthru = { inherit dirName; };
+  passthru = {
+    inherit dirName;
+    tests = nixosTests.zoneminder;
+  };
 
   postInstall = ''
     PERL5LIB="$PERL5LIB''${PERL5LIB:+:}$out/${perl.libPrefix}"

That way one can build the attribute zoneminder.tests to run all the tests for that package 🙂

@danielfullmer danielfullmer force-pushed the danielfullmer:zoneminder-php-fix branch from fed0777 to 4f35b7e May 17, 2020
@danielfullmer
Copy link
Contributor Author

danielfullmer commented May 17, 2020

@etu Done.

@etu etu merged commit 4437c3d into NixOS:master May 17, 2020
@etu
Copy link
Contributor

etu commented May 17, 2020

@danielfullmer Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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