This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 109561 run on x86_64-darwin 1

Hmm, I can't reproduce the failing test on my machine. Looking into it.

Hmm, I can't reproduce the failing test on my machine. Looking into it.

Keep in mind that I have sandbox enabled. Probably related to that.

Keep in mind that I have sandbox enabled. Probably related to that.

Is darwin even supported by upstream?

I also made a basic monolithic configuration with the default settings autogenerated from the dendrite yaml in accordance with the latest settings RFC

There are two things I'm missing though (though feel free to correct me if I'm wrong :)):

• As shown in an example in rfc42, settings that are not expected to be used in 99% of the deployments should be set with a mkDefault.
• The RFC doesn't prohibit explicitly defined options in the settings-submodule: in some cases I'd prefer to have explicit options set, at least for e.g. network ports.
• I'm not sure if it's a good idea to just use all defaults from the upstream config, especially since it seems (to me at least) to be a reference to all possible configuration options:
  • By default, no kafka is used AFAIU, however it's configured in your defaults.
  • I'm not really sure if we want to hard-code any third-party servers as trusted (as option default it may be fine though since it can be seen in the manual then).
  • This may also apply to some other sections that I've missed during a brief review.
• Have you checked whether it would be sufficient to use DynamicUser in conjunction with StateDirectory from systemd rather than configuring a new user? This is not a requirement, though I'd be curious if that would suffice :)

Is darwin even supported by upstream?

Yes! It's mentioned the installation instructions at least. FWIW it seems to work fine locally (for me).

By default, no kafka is used AFAIU, however it's configured in your defaults.

I'm using Naffka. It's just configured under the kafka section. Do you mean the ports I set up?

As shown in an example in rfc42, settings that are not expected to be used in 99% of the deployments should be set with a mkDefault.
The RFC doesn't prohibit explicitly defined options in the settings-submodule: in some cases I'd prefer to have explicit options set, at least for e.g. network ports.
I'm not sure if it's a good idea to just use all defaults from the upstream config, especially since it seems (to me at least) to be a reference to all possible configuration options:
I'm not really sure if we want to hard-code any third-party servers as trusted (as option default it may be fine though since it can be seen in the manual then).

If I delete the non-pertinent sections of the config from settings, can people still configure these options? (This is my first nixos module).

Which upstream defaults should we keep then? And which one's should we make options? Right now I'm assuming port, and servers should be options, are there any others?

Have you checked whether it would be sufficient to use DynamicUser in conjunction with StateDirectory from systemd rather than configuring a new user? This is not a requirement, though I'd be curious if that would suffice :)

I can try! Do you have a reference I can base this off of? One issue is I'm storing the keyfiles in the user home directory, which needs to persist (and I believe a dynamic user's home directory will be wiped). We probably don't want to store these in the nix store.

The RFC doesn't prohibit explicitly defined options in the settings-submodule: in some cases I'd prefer to have explicit options set, at least for e.g. network ports.

On the contrary, I find modules where all the configuration is dumped into a settings option pretty sad. I think there should be NixOS options for the most frequently used and mandatory settings. The type system and assertions are extremely useful for catching common errors, plus they can also provide a bit of documentation with examples.

On the contrary, I find modules where all the configuration is dumped into a settings option pretty sad.

Sorry to make you sad 😆 I will do the work to fix this, this was just my first pass!

Sorry to make you sad 😆 I will do the work to fix this, this was just my first pass!

Of course, sorry: I didn't mean to offend your work.

No offense taken! I was just prefacing this with some context that I might need more feedback this first time. I read through the source where they read settings, and tested it. This seems to be close to the minimal configuration necessary:

version: 1
global:
  server_name: localhost
  trusted_third_party_id_servers:
  - matrix.org
  - vector.im
  kafka:
    use_naffka: true
client_api:
  external_api:
    listen: http://[::]:8071
federation_api:
  external_api:
    listen: http://[::]:8072
media_api:
  external_api:
    listen: http://[::]:8074
sync_api:
  external_api:
    listen: http://[::]:8073
logging:
- type: file
  level: info
  params:
    path: /home/michael/.cache/log/dendrite

Should I replace the settings with this minimal yamltand make options for server_name and trusted_third_party_id_servers? P

I can add additional args for the ports in the systemd service, and also add these as options (so we'd have a total of 5, including dataDir). These aren't read from the config but from the config file.

httpBindAddr   = flag.String("http-bind-address", ":8008", "The HTTP listening port for the server")
httpsBindAddr  = flag.String("https-bind-address", ":8448", "The HTTPS listening port for the server")

The other options I would probably want to include are 3 for paths to keys (for a total of 8), right now you can just copy the keys over to the directory made for the matrix-dendrite user.

I think I have addressed everyone's concerns, users are now required to generate and specify their own matrix keys and TLS keys (no magic service anymore). I didn't add additional sandboxing but will file an issue after this is merged.

About to test your latest changes.

One question: why does disable_federation default to true ?

The suggested default seems to be false
https://github.com/matrix-org/dendrite/blob/464b908bd0c13854b3f6b9a17467f39e0608dc08/dendrite-config.yaml#L65

Ah, I'm not sure that particularly necessary to have as a default option, so I just went ahead and removed the whole thing (it can still be set by the user ofc)

@infinisil I think I addressed your review.

Nice! Feel free to merge whenever :) (I can't)

Why was this even merged without tests?

Yeah tests would be ideal, but not required imo. Also this PR is already pretty old and has seen a lot of review, the fact that @mjlbach has been updating it throughout all this time is commendable. But I think at some point the potential risk of losing a valuable contributor just because of PR's being reviewed for too long over-weighs the benefits.

I have a WIP branch with tests, but as this is my first time writing tests for a nixos module, I am having a bit of difficulty debugging. I'm happy to open the draft PR today for feedback, and will address additional feedback to continue to improve the module. FWIW I think this is a valuable module as-is as it makes matrix homeserver hosting much more accessible (synapse is a resource hog, and is infeasible to run on a raspberry pi as many do with znc bouncers.

@infinisil I was happy to stick through the review process, and thank you for continuing to provide feedback throughout it as well :) I learned a lot too!

As an update, the tests are blocked by the fact that matrix python sdk which we use for tests hasn't been updated in 2 years and isn't compatible with dendrite. Matrix-nio isn't either, but I submitted an upstream patch which will fix it. matrix-nio/matrix-nio#254

I'm going to go ahead and open the draft PR, pending that fix being merged and the version bumped in nixpkgs.

Edit: nvm, it's merged. Now just to wait for a release.

Yeah tests would be ideal, but not required imo.

They are not required but it seems like in this case it would have been rather easy to copy them from Synapse's.

But I think at some point the potential risk of losing a valuable contributor just because of PR's being reviewed for too long over-weighs the benefits.

100% agreed

FWIW I think this is a valuable module as-is as it makes matrix homeserver hosting much more accessible

For sure! If only dendrite supported push notifications (not for me, but for those who use crippled phones).

They are not required but it seems like in this case it would have been rather easy to copy them from Synapse's.

Yes, sadly this was not the case as the python SDK used by those tests is fairly abandoned. Matrix-nio, which I used for the new test, supports dendrite (as of tonight) and probably should be preferred for testing all matrix-servers going forward.