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

zoneminder: 1.32.3 -> 1.34.3 #79488

Merged
merged 4 commits into from Mar 7, 2020
Merged

Conversation

@danielfullmer
Copy link
Contributor

danielfullmer commented Feb 8, 2020

Motivation for this change

Update to latest version. The version currently in nixpkgs has a lot of vulnerabilities.
See: https://github.com/ZoneMinder/zoneminder/releases

A user with an existing database would also need to run zmupdate.pl as the zoneminder user. I've added some missing perl runtime dependencies for zmupdate.pl as well.

I've tested this with my currently running zoneminder instance and it seems to be running correctly after the upgrade.

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.

CC @peterhoeg
Closes: #54630

Copy link
Member

veprbl left a comment

LGTM

@aanderse
Copy link
Contributor

aanderse commented Feb 9, 2020

@danielfullmer can we run zmupdate.pl on every service start so the user doesn't have to? Or do we add notes somewhere mentioning this?

@danielfullmer
Copy link
Contributor Author

danielfullmer commented Feb 9, 2020

@aanderse It looks like we can run zmupdate.pl on startup. I originally thought it might be a problem since the script is interactive by default--but the -nointeractive flag works for our use. The script doesn't do anything if the database is already at the current version.

Just added a commit addressing this. I've tested upgrading from 1.32.3 to 1.34.2 in a VM and it seems to work fine.

@veprbl
Copy link
Member

veprbl commented Feb 9, 2020

I had my reservations about forcing an automatic schema update back when discussed in #54630. However, I'm not using zoneminder on NixOS, so I, personally, don't mind the change.

@veprbl veprbl dismissed their stale review Feb 9, 2020

Dismiss on PR update

@aanderse
Copy link
Contributor

aanderse commented Feb 10, 2020

@danielfullmer sorry my bad. I forgot that automatic database schema updates aren't universally accepted among the team. This is a judgement call that you can make. Keep the schema update or not. I think I'll start a discourse thread to get some feedback on the topic in general.

@danielfullmer
Copy link
Contributor Author

danielfullmer commented Feb 10, 2020

Any commiters please hold off on merging this for the time being. I've found an issue with the UI being unresponsive when configuring cameras that I would need to debug. (or anyone else interested)

With regard to automatic updating--I'm open to either option. If we do go for automatic schema updates I'd love to have someone else test it as well before this gets merged. One nice feature of manually running zmupdate.pl interactively is that it has optional functionality to make a database backup before updating.

I'm also very busy with my thesis at the moment so I'm not sure if I will get to fixing the UI issue in the next few days.

@danielfullmer danielfullmer force-pushed the danielfullmer:zoneminder-1.34.2 branch 2 times, most recently from d088bf5 to 9bd95c7 Feb 22, 2020
@danielfullmer
Copy link
Contributor Author

danielfullmer commented Feb 22, 2020

I've updated this PR with a few changes. It should be ready to merge after review.

Updated to 1.34.3.
Added a fix for improper timezone detection.

As far as automatic schema updates. I think we should do this only if services.zoneminder.database.createLocally is enabled. Zoneminder can be used with multiple instances backed by the same remote database--and we almost surely do not want to do automatic updates in that case. Conditionally updating the schema if it is a local database is also what upstream does in their debian postinstall script.

The UI issue was a result of zoneminder's "cache busting". They create symlinks in /var/cache/zoneminder to the js/css files, where the symlink has the timestamp of the destination file in the filename. This was intended to ensure that newer files would get a different URL. However, in the nix store, the files all have the timestamp "1". So when I upgraded, these symlinks would still point to the old version of zoneminder, and were not replaced. My solution was to patch the symlink filename calculation to replace the timestamp with just the hash of the zoneminder source.

@danielfullmer danielfullmer changed the title zoneminder: 1.32.3 -> 1.34.2 zoneminder: 1.32.3 -> 1.34.3 Feb 22, 2020
@danielfullmer danielfullmer requested a review from veprbl Mar 1, 2020
@veprbl
veprbl approved these changes Mar 4, 2020
Copy link
Member

veprbl left a comment

Looks reasonable, but I can't test this now.

@veprbl
Copy link
Member

veprbl commented Mar 7, 2020

@danielfullmer Please squash the last two commits and edit "zoneminder: 1.32.3 -> zoneminder 1.34.3" to "zoneminder: 1.32.3 -> 1.34.3"

@danielfullmer danielfullmer force-pushed the danielfullmer:zoneminder-1.34.2 branch from 9a2ee8d to bdc7676 Mar 7, 2020
@danielfullmer danielfullmer force-pushed the danielfullmer:zoneminder-1.34.2 branch from bdc7676 to ce34b92 Mar 7, 2020
@danielfullmer
Copy link
Contributor Author

danielfullmer commented Mar 7, 2020

@veprbl Just squashed those commits and fixed the commit title. Thanks!

@veprbl veprbl merged commit 93745d2 into NixOS:master Mar 7, 2020
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
zoneminder on aarch64-linux Success
Details
zoneminder on x86_64-linux Success
Details
veprbl added a commit that referenced this pull request Mar 7, 2020
(cherry picked from commit 2685e45)

cc #79488
veprbl added a commit that referenced this pull request Mar 7, 2020
veprbl added a commit that referenced this pull request Mar 7, 2020
(cherry picked from commit 630de55)

cc #79488
veprbl added a commit that referenced this pull request Mar 7, 2020
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.

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