ma27: Could you elaborate what you meant by

The entire generation of units for workers somehow works around the module-system in a very weird way that I really don't want to maintain.

ma27: Could you elaborate on what you meant by

The entire generation of units for workers somehow works around the module-system in a very weird way that I really don't want to maintain.

In reply to @dandellion:dodsorf.as

ma27: Could you elaborate on what you meant by

The entire generation of units for workers somehow works around the module-system in a very weird way that I really don't want to maintain.

ok I wasn't too precise there. I meant the import of the worker-module which creates these units, sorry.

The module-system consists of functions with certain arguments (defined by _module.args - such as pkgs/lib/config), but all the other stuff should be built with (internal) options. On a destructured attribute-set as argument the attribute names are not lazy, so you depend on at least the bindings already (though the values seem to be unevaluated thunks). However, while evaluating imports of a declaration (synapse.nix in this case) it's possible that too much of config will be unintentionally evaluated at some point causing weird behavior (prime example of a current issue is that specialArgs.nixosModules works whereas config._module.args.nixosModules causes an infinite recursion). Yes, it seems to just evaluate at the moment, but this code working around the design so much seems like a time-bomb to me which will send somebody on a long debugging journey, so I don't want to see that merged (and internals of the module system may change in a subtle way at some point).

additionally, are there that many differences left now? the other PR also did services.matrix-synapse.workers.foobar = { /* workerConfig */ }; (i.e. attrsOf) when I first looked at it which seems to be your main issue. And generally the other PR seemed closer to being mergable (because of dealing with systemd hardening, logger config and using sockets with redis - by default there's no real reason to use a tcp socket here since everything is on the same machine anyways).

fwiw I'm not sure if more opinionated things such as nginx config generation for load-balancing of workers should be in a nixos module (at least as long as we don't have rfc42 for nginx - otherwise I'm not too worried about such an idea) because it's far too easy then to run into a problem that isn't covered by the module and then the config will be very hard to customize from my experience with maintaining nextcloud which actually ships with a full-blown nginx config. I may be wrong though, so if you think you have a good design for that, feel free to file a patch, I'm happy to take a look :)

Thanks for clarifying.
I agree the import with the function is a little stinky.

I was not aware that this PR does attrsOf now, that's great! I'm not going to block this at all then

The only difference left is probably that I use workers.instances and this one does just pure workers like was suggested. This is fine, I can just use autoWorkers or something for part 2

I'll do a more thourough review, but I generally support merging this more complete implementation