@tsudoko Just a heads up that I've packaged this for Nix

@Ekleog Thanks for the review - it's great to get feedback so fast! I've taken care of most the issues. I also found a few other minor things, like for example using cfg.package instead of pkgs.ankisyncd

I realize now that I should have pushed the fixes regularly, so it's easier to review, and not squashed and force pushed until the end. Sorry about that.

Regarding DynamicUser, this seems like a good idea. If I add DynamicUser = true, and leave the systemd.tmpfiles.rules and the unit's User and Group settings as they are, it works. Is that what you were thinking, or did you intend to also avoid specifying the User and Group?

Without specifying these it seems that the permissions in the systemd.tmpfiles.rules are not properly set. The error is Changing to the requested working directory failed: Permission denied. Indeed, the documentation says Z lines are ignored for "-".

systemd.tmpfiles.rules = [
  "d '${cfg.dataDir}' 0770 - - - -"
  "Z '${cfg.dataDir}' 0770 - - - -"
];

serviceConfig = {
  Type = "simple";
  DynamicUser = true;
  WorkingDirectory = cfg.dataDir;
  ExecStart = "${cfg.package}/bin/ankisyncd";
  Restart = "always";
};

I'm wondering if DynamicUser is safe in this instance, because the data is persistent. It might be okay if the systemd.tmpfiles.rules specify the user and group. Here's what the DynamicUser= documentation has to say about it:

Care should be taken that any processes running as part of a unit for which dynamic users/groups are enabled do not leave files or directories owned by these users/groups around, as a different unit might get the same UID/GID assigned later on, and thus gain access to these files or directories.

Do you have any advice on this? I don't have that much experience with systemd units.

I was thinking of also removing completely User and Group and system.tmpfiles.d settings, and instead use StateDirectory as a way to handle the state. This setup should be supported by systemd, thus avoiding the uid reuse issue :)

As for the force-pushing, do as you feel easiest! Github provides a link on the force-pushed words anyway, that allows seeing the diff from before and after a force push, so it's not a big deal :)

That works and results in a lot less code - great tip! I wasn't familiar with StateDirectory, but yes it looks like it does take care of the uid reuse issue:

If DynamicUser= is used in conjunction with StateDirectory=, CacheDirectory= and LogsDirectory= is slightly altered: the directories are created below /var/lib/private, /var/cache/private and /var/log/private, respectively, which are host directories made inaccessible to unprivileged users, which ensures that access to these directories cannot be gained through dynamic user ID recycling. Symbolic links are created to hide this difference in behaviour. Both from perspective of the host and from inside the unit, the relevant directories hence always appear directly below /var/lib, /var/cache and /var/log.

Let me know how that looks now

It looks great, thank you!