piwik & piwik service: init at 3.0.4 #26073

florianjacob · 2017-05-24T21:13:08Z

Motivation for this change

Piwik is an open analytics application. Resolves #25266.

Things done

Tested by @Alphor
Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2017-05-24T21:13:13Z

@florianjacob, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @joachifm and @offlinehacker to be potential reviewers.

joachifm · 2017-06-12T23:48:39Z

nixos/modules/misc/ids.nix

@@ -295,6 +295,7 @@
      aria2 = 277;
      clickhouse = 278;
      rslsync = 279;
+      piwik = 280;


Why is a static uid required?

My initial thought was so that the whole dataDir folder /var/lib/piwik can be easily restored from a backup, e.g. after a failure that needed a reinstall or on another machine.

In the meantime I learned that actually only a single file, /var/lib/piwik/config/config.ini.php needs to be backed up. It's probably easy enough to fix uid+gid for a single file manually on restores, so I guess it's not that required anymore. Nothing in the code depends on it, I could simply remove the static assignment again.

This is my first nixpkg package, could you tell me more about in which cases and for what purposes it's considered good pratice to use static ids, and in which it isn't?

The most clear-cut use for static uids is if you need to refer to a user before the user/uid mapping is established. Otherwise, you need to make a judgement about the kind of data the daemon generates, whether you'd transfer the data between hosts, whether ownership can be easily fixed up, what the privacy or security implications would be if the data ended up being owned (however transiently) by another user etc.

Relying on static uids to maintain ownership invariants is fragile imo (it won't help if you transfer data to a non-NixOS host, for example). Better to have code that enforces invariants you care about directly.

Now, I don't mean to say you should overthink this but there should be a reason for doing it :)

Thanks alot for that explanation, that was very helpful. :)

I decided to remove the static ids and added a small chown+chmod script in the systemd unit that is executed as root and enforces the right ownership and permissions on startup instead. I figured that's much more beneficial to what I wanted to achieve, keeping things private while allowing an occasional but easy restore/move.

Sounds good to me. If it turns out that a static uid makes more sense, it can always be added later (the other way 'round is a little harder).

florianjacob · 2017-06-13T10:32:39Z

@joachifm thanks for taking a look! 😄

joachifm

Added some nitpicks for you to consider. None of them blockers.

joachifm · 2017-06-15T20:41:40Z

nixos/modules/services/web-apps/piwik.nix

+      home = dataDir;
+      group  = "${user}";
+    };
+    users.extraGroups = [ { name = "${user}"; } ];


I think users.extraGroups.${user} = {} would work as well.

Much more elegant. 😃

joachifm · 2017-06-15T20:42:22Z

nixos/modules/services/web-apps/piwik.nix

+      requiredBy = [ "${phpExecutionUnit}.service" ];
+      before = [ "${phpExecutionUnit}.service" ];
+      # the update part of the script can only work if the database is already up and running
+      requires = [ "${databaseService}" ];


Note that "${foo}" could be simplified to foo.

joachifm · 2017-06-15T20:42:50Z

nixos/modules/services/web-apps/piwik.nix

+      serviceConfig = {
+        Type = "oneshot";
+        User = user;
+        Group = user;


Group is strictly redundant, it defaults to the primary group of User.

joachifm · 2017-06-15T20:44:08Z

pkgs/servers/web-apps/piwik/default.nix

+    sha512 = "2i0vydr073ynv7wcn078zxhvywdv85c648hympkzicdd746g995878py9006m96iwkmk4q664wn3f8jnfqsl1jd9f26alz1nssizbn9";
+  };
+
+  phases = [ "unpackPhase" "patchPhase" "installPhase" ];


Please consider disabling problematic phases instead of listing phases; this is a known source of bugs.

Could just throw this out completely, did learn that all default phases don't do anything like the buildPhase doesn't do anything if there is no Makefile.

joachifm · 2017-06-15T20:44:39Z

pkgs/servers/web-apps/piwik/default.nix

+
+  phases = [ "unpackPhase" "patchPhase" "installPhase" ];
+
+  buildInputs = [ makeWrapper ];


Can go innativeBuildInputs.

joachifm · 2017-06-15T20:45:30Z

pkgs/servers/web-apps/piwik/default.nix

+
+  # regarding substitutes: looks like this is just a bug / confusion of the directories, and nobod yhas tested this.
+  # PR at https://github.com/piwik/piwik/pull/11661
+  patchPhase = ''


Consider postPatch; at least in general its nice to allow applying arbitrary patches via override. Specifying a custom patch phase prevents that.

joachifm · 2017-06-15T20:47:04Z

nixos/modules/services/web-apps/piwik.nix

+      enable = mkOption {
+        type = types.bool;
+        default = false;
+        example = true;


Nit: examples for booleans are usually skipped, it's self-evident.

joachifm · 2017-06-15T20:48:58Z

nixos/modules/services/web-apps/piwik.nix

+        type = types.bool;
+        default = false;
+        example = true;
+        description = ''


Note that long-form descriptions may not render very well. You can generate the nixos manual to check whether it'll look okay. You can farm out longer documentation fragments to the manual.

You are right, it renders quite bad.

So it would be ok to include a new manual section like II.Configuration > 22. piwik for that longer part? I was struggeling to find a good place for this, but now I see this long description isn't one.

joachifm · 2017-06-15T20:50:48Z

pkgs/servers/web-apps/piwik/default.nix

+  # TODO: Move more unnecessary files from share/, especially using PIWIK_INCLUDE_PATH.
+  #       See https://forum.piwik.org/t/bootstrap-php/5926/10 and
+  #       https://github.com/piwik/piwik/issues/11654#issuecomment-297730843
+  installPhase = ''


Note that this will cause install hooks to be skipped (you can run e..g, the post phase hook manually with runHook postInstall).

I understand. I added runHook call for preInstall / postInstall as first / last commands of my installPhase, mirroring the default implementation I found in pkgs/stdenv/generic/setup.sh. From that file I assume there's no better way to overwrite / have a custom installPhase without having to call the hooks by hand, right?

Pretty much. At some point I think it's possible hooks may become implicit, making this a bit more natural.

joachifm · 2017-06-15T20:52:05Z

pkgs/servers/web-apps/piwik/default.nix

+    description = "A real-time web analytics application";
+    license = licenses.gpl3Plus;
+    homepage = https://piwik.org/;
+    platforms = platforms.all;


If you wish, you could add yourself to the list of maintainers.

I thought you had to be part of the NixOS github organization to be a maintainer. 😅

Not at all. It basically means you care about the package, lets other's know to ping you if there are questions or whatever.

florianjacob · 2017-06-16T20:51:52Z

Added some nitpicks for you to consider. None of them blockers.

I can feel the packaging quality is increasing…✨ 😄

joachifm · 2017-06-16T21:24:09Z

I don't have much more to add, so let me know when you're ready to integrate.

ajarara · 2017-06-17T16:33:16Z

nixos/modules/services/web-apps/doc.xml

@@ -6,7 +6,7 @@

  <title>Piwik</title>
  <para>
-    Piwik is a real-time web analytics application.
+    Piwik is a real-time web analytics web application.


Might want to revert this line :D

It was actually intentional, as it is a web application for web analytics, but I admit it sounds really weird in english, adds nothing much and probably irritates the reader more than clarifies stuff. 😆 Thanks!

Yeah, I could see that.

florianjacob · 2017-06-17T21:12:34Z

@joachifm / @Alphor I've aded a NixOS manual chapter for piwik now, and I think it's good not to try to cram it all into the description. 😄 If you don't have any other suggestions for the manual, I'd be ready to integrate now.

The manual also contains a paragraph on this sendmail issue, I'd remove it later if the issue is resolved, but it seems to be nothing trivial: #26611

@Alphor can I get back on your offer to test other stuff? In case you have some kind of sendmail via nullmailer, postfix or something else, could you hit the „forgot password“ on piwik's login and try to reset it? This should result in an error displayed in the browser, and a journald log error like I posted it in the issue.

ajarara · 2017-06-19T04:02:54Z

@florianjacob
I've never used sendmail/procmail for email. Do I just need an MTA to test this?

florianjacob · 2017-06-19T10:47:31Z

@Alphor yes, an MTA would be enough, it just needs to provide the sendmail command somehow. Does not even need to work and be able to send mails to the outer world, as it'll most likely fail instantly on invoking sendmail. For postfix and nullmailer (did not look at any other MTAs for NixOS so far), just enabling them without any further configuration is enough. (For real operation, at least the fully-qualified domain name needs to be provided if it's not already in your hostname)

joachifm · 2017-06-27T19:51:22Z

Thank you

florianjacob mentioned this pull request May 24, 2017

Add piwik package and service #25266

Closed

florianjacob force-pushed the piwik-package branch 2 times, most recently from 8f86def to d309b61 Compare June 12, 2017 20:17

joachifm reviewed Jun 12, 2017

View reviewed changes

florianjacob force-pushed the piwik-package branch 5 times, most recently from 258b4c1 to 3dd9633 Compare June 15, 2017 17:23

joachifm reviewed Jun 15, 2017

View reviewed changes

florianjacob mentioned this pull request Jun 16, 2017

Can't send mail from php-fpm via sendmail #26611

Closed

ajarara reviewed Jun 17, 2017

View reviewed changes

florianjacob force-pushed the piwik-package branch 3 times, most recently from 6b5fd6d to 711507a Compare June 17, 2017 20:44

piwik & piwik service: init at 3.0.4

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

55844c8

florianjacob force-pushed the piwik-package branch from 711507a to 55844c8 Compare June 19, 2017 14:00

joachifm merged commit 767a8b2 into NixOS:master Jun 27, 2017

florianjacob deleted the piwik-package branch June 27, 2017 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

piwik & piwik service: init at 3.0.4 #26073

piwik & piwik service: init at 3.0.4 #26073

florianjacob commented May 24, 2017

mention-bot commented May 24, 2017

joachifm Jun 12, 2017

florianjacob Jun 13, 2017

joachifm Jun 13, 2017

florianjacob Jun 15, 2017

joachifm Jun 15, 2017

florianjacob commented Jun 13, 2017

joachifm left a comment

joachifm Jun 15, 2017

florianjacob Jun 16, 2017

joachifm Jun 15, 2017

joachifm Jun 15, 2017

joachifm Jun 15, 2017

florianjacob Jun 16, 2017

joachifm Jun 15, 2017

joachifm Jun 15, 2017

joachifm Jun 15, 2017

joachifm Jun 15, 2017

florianjacob Jun 16, 2017

joachifm Jun 15, 2017

florianjacob Jun 16, 2017

joachifm Jun 16, 2017

joachifm Jun 15, 2017

florianjacob Jun 16, 2017

joachifm Jun 16, 2017

florianjacob commented Jun 16, 2017

joachifm commented Jun 16, 2017

ajarara Jun 17, 2017

florianjacob Jun 17, 2017

ajarara Jun 17, 2017

florianjacob commented Jun 17, 2017

ajarara commented Jun 19, 2017

florianjacob commented Jun 19, 2017

joachifm commented Jun 27, 2017


		phases = [ "unpackPhase" "patchPhase" "installPhase" ];

		buildInputs = [ makeWrapper ];

piwik & piwik service: init at 3.0.4 #26073

piwik & piwik service: init at 3.0.4 #26073

Conversation

florianjacob commented May 24, 2017

Motivation for this change

Things done

mention-bot commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florianjacob commented Jun 13, 2017

joachifm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florianjacob commented Jun 16, 2017

joachifm commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florianjacob commented Jun 17, 2017

ajarara commented Jun 19, 2017

florianjacob commented Jun 19, 2017

joachifm commented Jun 27, 2017