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.

Sign up for GitHub

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

Jump to bottom

matrix-appservice-irc: init at 0.20.1 #95854

Closed
wants to merge 3 commits into from
Closed

matrix-appservice-irc: init at 0.20.1 #95854

wants to merge 3 commits into from

Conversation

JJJollyjim
Copy link
Member

Motivation for this change

Time for attempt number 7 (!) at getting this in!

This PR:

  • Starts with @puffnfresh's matrix-appservice-irc: init at 0.17.1 #89106
  • Updates to the latest version
  • Resolves @mweinelt's review comments
  • Implements an RFC0042-based configuration approach, with build-time checking against upstream's json-schema
  • Adds a comprehensive nixos test, which incidentally greatly increases the nixos test coverage of matrix-synapse

Thanks to all those whose work this is building on. I'd love to be able to use this in 20.09 :)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@JJJollyjim
Copy link
Member Author

also cc @andir

@mweinelt mweinelt mentioned this pull request Aug 20, 2020
Closed
10 tasks
@zowoq
Copy link
Contributor

zowoq commented Aug 22, 2020

@ofborg eval

@JJJollyjim

This comment has been minimized.

Sign in to view
@JJJollyjim

This comment has been minimized.

Sign in to view
@JJJollyjim JJJollyjim force-pushed the matrix-appservice-irc branch 2 times, most recently from 74c332a to e0e5c9a Compare August 22, 2020 06:25
@JJJollyjim
Copy link
Member Author

@ofborg eval

@ofborg ofborg bot requested a review from puffnfresh August 22, 2020 06:48
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/276

@JJJollyjim
Copy link
Member Author

cc @jonringer -- sorry to add to your workload, but I'm really itching to see this in 20.09 -- any chance you could look at merging?

@jonringer
Copy link
Contributor

to be honest, I'm not the biggest fan of these massive node diffs, it's 1000's of lines in the source repo, and it's a significant runtime closure which can't be shared with other node programs. However, I'm not aware of a better paradigm

$ nix path-info -Sh /nix/store/ygcgxc3yssh6p7wp828dz7d52xbikkwi-node_matrix-appservice-irc-0.20.1
/nix/store/ygcgxc3yssh6p7wp828dz7d52xbikkwi-node_matrix-appservice-irc-0.20.1	 740.4M
$ nix path-info -Sh /nix/store/l8b8x54kbx8gadz1wb3ffh89ppwlxav3-node-sources
/nix/store/l8b8x54kbx8gadz1wb3ffh89ppwlxav3-node-sources	 263.9M

jonringer
jonringer reviewed Aug 30, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

at the very least, this should probably be 3 commits:

maintainer
matrix-appservice-irc
nixos/matrix-appservice-irc

and maybe the test as another.

@JJJollyjim
Copy link
Member Author

@jonringer have split 👍

mweinelt
mweinelt requested changes Aug 30, 2020
View reviewed changes
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. In general this looks pretty good, I added remarks to some nits and questions. Loving the test!

I'm also a bit torn by the node madness, but we certainly need to apply a cost-benefit equation for cases like this.

I'm already running the version by @puffnfresh and will test this update asap. If all goes well I think we'll be ready for 20.09.

nixos/tests/matrix-appservice-irc.nix Outdated Show resolved Hide resolved
nixos/tests/matrix-appservice-irc.nix Show resolved Hide resolved
nixos/tests/matrix-appservice-irc.nix Outdated Show resolved Hide resolved
nixos/tests/matrix-appservice-irc.nix
services.matrix-synapse = {
enable = true;
database_type = "sqlite3";
app_service_config_files = [ "/registration.yml" ];
Copy link
Member

Choose a reason for hiding this comment

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

How does the absolute path here work?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, the registration file is copied to this location. Weird choice of location.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder why synapse should even be allowed to read this location. Maybe the synapse home would be a more appropriate location, and then we think about locking down synapse more in a next step.

nixos/tests/matrix-appservice-irc.nix Outdated Show resolved Hide resolved
nixos/tests/matrix-appservice-irc.nix Outdated Show resolved Hide resolved
nixos/tests/matrix-appservice-irc.nix Show resolved Hide resolved
nixos/tests/matrix-appservice-irc.nix Show resolved Hide resolved
nixos/tests/matrix-appservice-irc.nix Show resolved Hide resolved
nixos/tests/matrix-appservice-irc.nix Outdated Show resolved Hide resolved
@mweinelt
Copy link
Member

mweinelt commented Aug 30, 2020 •
edited

The editorconfig test stage is unhappy.

pkgs/servers/matrix-synapse/matrix-appservice-irc/node-composition.nix:
	Wrong line endings or new final newline

I guess we need an exception like

nixpkgs/.editorconfig

Lines 98 to 99 in 5d8dd5c

[pkgs/development/node-packages/node-packages.nix]
insert_final_newline = unset

Maybe @zowoq has some input.

@zowoq
Copy link
Contributor

zowoq commented Aug 30, 2020

I guess we need an exception like

Done: 4b00ca0

puffnfresh and others added 2 commits August 30, 2020 23:53
@JJJollyjim JJJollyjim force-pushed the matrix-appservice-irc branch 2 times, most recently from d78f9ae to 0c70f3e Compare August 30, 2020 12:01
@JJJollyjim
Copy link
Member Author

Okay, rebased on master and nits addressed other than the extraneous newlines, which are required by black -_- -- on the whole I'd still rather keep doCheck on.

mweinelt
mweinelt reviewed Aug 30, 2020
View reviewed changes
nixos/modules/services/misc/matrix-appservice-irc.nix
freeformType = unspecified;

options = {
homeserver = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

This restricts the possible options that are usable below homeserver.

https://github.com/matrix-org/matrix-appservice-irc/blob/develop/config.schema.yml#L33-L50

error: The option `services.matrix-appservice-irc.settings.homeserver.enablePresence' defined in `/etc/nixos/config/matrix/appservice-irc.nix' does not exist.

@ajs124 ajs124 mentioned this pull request Sep 17, 2020
Closed
10 tasks
AluisioASG added a commit to AluisioASG/nixexprs that referenced this pull request Oct 29, 2020
@AluisioASG
matrix-appservice-irc: Add NixOS module
be10c38 
Inspired by, but not a copy of, github:NixOS/nixpkgs#95854.
@redvers redvers requested a review from mweinelt November 8, 2020 21:30
@mweinelt mweinelt removed their request for review November 8, 2020 21:50
piegamesde
piegamesde reviewed Feb 7, 2021
View reviewed changes
nixos/modules/services/misc/matrix-appservice-irc.nix
# Allow synapse access to the registration
if ${getBin pkgs.glibc}/bin/getent group matrix-synapse > /dev/null; then
chgrp matrix-synapse ${registrationFile}
chmod g+r ${registrationFile}
Copy link
Member

Choose a reason for hiding this comment

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

This looks like something that's going to bite the first person working with a home server that is not Synapse.

Copy link
Member

@mweinelt mweinelt Feb 19, 2021 •
edited

Choose a reason for hiding this comment

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

Yep, let's address that when another homeserver implements the appservice protocol.

Edit: the appservice protocol is specified.
https://matrix.org/docs/spec/application_service/latest

piegamesde
piegamesde reviewed Feb 18, 2021
View reviewed changes
nixos/modules/services/misc/matrix-appservice-irc.nix

# Generate registration
if ! [ -f "${registrationFile}" ]; then
${bin} -c ${configFile} -f ${registrationFile} -r -u ${cfg.registrationUrl} -l ${cfg.localpart}
Copy link
Member

Choose a reason for hiding this comment

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

This will be generated once and never be updated, even when the configuration changes.

This was referenced Feb 19, 2021
Closed
Merged
@mweinelt mweinelt closed this Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mweinelt mweinelt mweinelt requested changes

@Ma27 Ma27 Ma27 left review comments

@piegamesde piegamesde piegamesde left review comments

@puffnfresh puffnfresh Awaiting requested review from puffnfresh

@jonringer jonringer Awaiting requested review from jonringer

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@JJJollyjim @zowoq @nixos-discourse @jonringer @mweinelt @puffnfresh @Ma27 @piegamesde @SuperSandro2000