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

nixos/jitsi-meet: init #92468

Merged
merged 11 commits into from Aug 3, 2020
Merged

nixos/jitsi-meet: init #92468

merged 11 commits into from Aug 3, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jul 6, 2020 •
edited by ghost

Motivation for this change

Please Read: The previous PR exploded with feature requests and nitpicks. The goal of this PR is to provide a minimum viable product that can be merged into nixpkgs. Additional features and improvements can be added as followup PRs later.

A few friends and me are using the Jitsi Meet module by mmilata since a while and I want to get it merged into nixpkgs.
However I don't have the resources to maintain Jitsi Meet in nixpkgs long-term. I am still looking for someone to help me with that.

TODO

  • Update packages
  • Let's Encrypt based shared certificates for prosody and the ji* services
  • Manual section / documentation on how to set up

Nice to have

  • One more extensive test would be good, at least with two clients joining the room

Out of scope for this PR

  • Build packages from source
  • Reliable OCR-based tests for video
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.

@ghost ghost mentioned this pull request Jul 6, 2020
Closed
10 tasks
@Ma27 Ma27 self-requested a review July 6, 2020 20:20
Ma27
Ma27 reviewed Jul 8, 2020
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking care of this!

I just tested it in a local network and it appears to work fine when enabling TLS. I'm planning to self-host a jitsi instance soon, I can probably provide even more feedback (if I get to this before this PR gets merged).

Below are some comments/thoughts I had while reviewing your changes. Since the module works fine overall, I don't consider those mandatory to get this PR merged though.

nixos/modules/services/networking/jicofo.nix Show resolved Hide resolved
nixos/tests/jitsi-meet.nix Show resolved Hide resolved
nixos/modules/services/web-apps/jitsi-meet.nix Show resolved Hide resolved
pkgs/servers/jitsi-videobridge/default.nix Show resolved Hide resolved
nixos/modules/services/networking/jicofo.nix Show resolved Hide resolved
@Ma27
Copy link
Member

Ma27 commented Jul 9, 2020

The change seems fine now, thanks!

I'd prefer to get a bit more feedback from other community members, other than that this patch seems fine.

Revert "nixos/jicofo: use ExecStart instead of script"
1a5f3d1 
This reverts commit d3a26a5.
Using ServiceConfig.ExecStart instead of script lead to the content not
being executed in a shell anymore, which broke the secrets being read
from a file and passed as a command line parameter.
dpausp added a commit to flyingcircusio/fc-nixos that referenced this pull request Jul 28, 2020
@dpausp
Jitsi from PR NixOS/nixpkgs#92468
284c4cb 
builds
@puckipedia
Copy link
Contributor

One more issue I noticed: the videobridge dynamically tries to load a .so, which binds to openssl, which can't be located. This breaks crypto acceleration somewhat..

@sbourdeauducq
Copy link
Contributor

This breaks crypto acceleration somewhat..

Could we merge this and then fix small issues like that later?

@ryantm
Copy link
Member

ryantm commented Jul 31, 2020

I just started using this set of patches on top of master and it works great for me.

Ma27
Ma27 approved these changes Aug 1, 2020
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

@petabyteboy I'm in favor of merging this. Is there anything missing in your opinion or do you agree? :)

@ghost
Copy link
Author

ghost commented Aug 1, 2020

@petabyteboy I'm in favor of merging this. Is there anything missing in your opinion or do you agree? :)

We don't have any maintainers. Otherwise it would be okay if you merged it, but I would want to complete the todos in the description before I would feel comfortable merging it.

@dpausp
Copy link
Contributor

dpausp commented Aug 3, 2020

I'm running some Jitsi instances for my company https://flyingcircus.io and our customers. Last week, I switched our test system from a docker-based setup to the code from this PR with updated package versions and it's working well so far. I'd like to see this PR merged. After that, I can take over as a maintainer and work on additional features.

@ryantm ryantm merged commit 4162c69 into NixOS:master Aug 3, 2020
@ryantm ryantm mentioned this pull request Aug 3, 2020
Closed
dpausp added a commit to flyingcircusio/fc-nixos that referenced this pull request Aug 7, 2020
@dpausp
Jitsi from PR NixOS/nixpkgs#92468
b42d652 
builds
dpausp added a commit to flyingcircusio/fc-nixos that referenced this pull request Aug 7, 2020
@dpausp
Jitsi from PR NixOS/nixpkgs#92468
03a4888 
builds
dpausp added a commit to flyingcircusio/fc-nixos that referenced this pull request Aug 20, 2020
@dpausp
Jitsi from PR NixOS/nixpkgs#92468
9bbdd88 
builds
alyssais pushed a commit to nixcon/nixcon-video-infra that referenced this pull request Aug 25, 2020
@edef1c
nixpkgs: merge jitsi-meet
a3d6a35 
Link: NixOS/nixpkgs#92468
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Ma27 @puckipedia @sbourdeauducq @ryantm @dpausp @mmilata