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

nixos/matrix-synapse: minor improvements to implement worker-support #140979

Merged
merged 2 commits into from
Oct 9, 2021

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Oct 8, 2021

Motivation for this change

This doesn't contain changes for actual worker-support as I haven't figured out (yet) how to properly implement this in this module.

However, there are two changes that I needed to implement it in my personal Matrix configuration:

  • Expose config file for matrix-synapse as read-only option to make sure that custom worker services can access it.
  • Expose synapse.app.generic_worker as executable in pkgs.matrix-synapse.
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages 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/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 8, 2021
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Oct 8, 2021
'console_scripts': [
- 'homeserver = synapse.app.homeserver:main'
+ 'homeserver = synapse.app.homeserver:main',
+ 'worker = synapse.app.generic_worker:main'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are other workers, too: https://matrix-org.github.io/synapse/v1.43/workers.html#available-worker-applications

We should create executables for each of them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this was just a POC, but you're right, I guess I should add all of them before merging this :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this doesn't seem needed: each of these workers uses generic_worker:start and this function decides what to do based on the name of the worker app.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, everything is just a generic worker under the hood. Cool!

@Ma27 Ma27 requested a review from sumnerevans October 8, 2021 23:25
Copy link
Contributor

@sumnerevans sumnerevans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we should also set enableRedis to true by default in the package.

@Ma27
Copy link
Member Author

Ma27 commented Oct 9, 2021

Actually, we should also set enableRedis to true by default in the package.

Isn't that the case already?

@dali99
Copy link
Member

dali99 commented Oct 9, 2021

Actually, we should also set enableRedis to true by default in the package.

We do this since #140207 😄

@sumnerevans
Copy link
Contributor

Oh, right. Forgot that got merged in :)

@Ma27 Ma27 merged commit cbfe4a4 into NixOS:master Oct 9, 2021
@Ma27 Ma27 deleted the matrix-workers branch October 9, 2021 13:24
From 3089758015c64cc1e6788793c4fe40a0e1783457 Mon Sep 17 00:00:00 2001
From: Maximilian Bosch <maximilian@mbosch.me>
Date: Tue, 5 Oct 2021 22:33:12 +0200
Subject: [PATCH 2/2] Expose generic worker as binary under NixOS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we haven't upstreamed this? That looks like something that wouldn't hurt upstream at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants