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

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

Merged
merged 2 commits into from May 17, 2020
Merged

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

merged 2 commits into from May 17, 2020

Conversation

danielfullmer
Copy link
Contributor

@danielfullmer danielfullmer commented Apr 10, 2020 •
edited

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
Merged
10 tasks
@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

@talyz Thanks this fixed the issue for me!

@danielfullmer danielfullmer marked this pull request as ready for review April 11, 2020 17:34
@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
Merged
10 tasks
@aanderse
Copy link
Member

ping

etu
etu reviewed May 17, 2020
View reviewed changes
nixos/modules/services/misc/zoneminder.nix Outdated
@@ -289,11 +285,9 @@ in {
phpfpm = lib.mkIf useNginx {
pools.zoneminder = {
inherit user group;
phpPackage = pkgs.php.withExtensions (e: pkgs.php.enabledExtensions ++ [ e.apcu ]);
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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

@etu
Copy link
Contributor

etu commented May 17, 2020 •
edited

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Reviewers

@talyz talyz talyz left review comments

@etu etu etu left review comments

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@danielfullmer @talyz @aanderse @etu