Skip to content

nixos/livekit: init, nixos/lk-jwt-service: init #399627

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

Merged
merged 4 commits into from
Apr 29, 2025
Merged

Conversation

Henry-Hiles
Copy link
Contributor

Things done

Added a module for lk-jwt-service, and livekit, needed for running element-call (already packaged). For a usage example, see here: https://git.henryhiles.com/Henry-Hiles/nixos/src/branch/main/clients/quadraticserver/element-call.nix

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@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 Apr 18, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 18, 2025
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.

I did a first pass to grasp the structure of this and I have some questions on how to best represent the configuration.

Some of the changes apply to both modules, but I only commented on them once.

@Henry-Hiles Henry-Hiles force-pushed the master branch 7 times, most recently from 42cc827 to a066c44 Compare April 18, 2025 17:05
@Henry-Hiles Henry-Hiles requested a review from mweinelt April 18, 2025 17:25
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.

Second pass.

Looks like you created broken references when refactoring the module. Are you using a language server? I will recommend nil just in case.

I'm now pretty confident about the livekit configuration: We pass in both the configuration and a dedicated keys file.

@Henry-Hiles Henry-Hiles force-pushed the master branch 3 times, most recently from cb22c9e to d3f22ba Compare April 18, 2025 20:49
@Henry-Hiles
Copy link
Contributor Author

Second pass.

Looks like you created broken references when refactoring the module. Are you using a language server? I will recommend nil just in case.

I'm now pretty confident about the livekit configuration: We pass in both the configuration and a dedicated keys file.

I am using nil, but it doesn't appear to work that well...

@Henry-Hiles Henry-Hiles requested a review from mweinelt April 18, 2025 20:50
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.

Third pass, a bit smaller because we're slowly getting there.

What I would like next, as previously announced, is a NixOS test. I'm not sure about the scope, whether one for LiveKit itself makes sense or one for the Element Call stack would be more appropriate.

Now we most likely won't place calls on that test, but it should demonstrate the integration of these options with each other and provide some guarantees, that it keeps working.

@Henry-Hiles Henry-Hiles force-pushed the master branch 2 times, most recently from 2153258 to 08caf22 Compare April 18, 2025 22:05
@Henry-Hiles Henry-Hiles requested a review from mweinelt April 18, 2025 22:12
@Henry-Hiles Henry-Hiles force-pushed the master branch 2 times, most recently from c449c3c to 76c5a99 Compare April 19, 2025 01:14
@Henry-Hiles
Copy link
Contributor Author

Third pass, a bit smaller because we're slowly getting there.

What I would like next, as previously announced, is a NixOS test. I'm not sure about the scope, whether one for LiveKit itself makes sense or one for the Element Call stack would be more appropriate.

Now we most likely won't place calls on that test, but it should demonstrate the integration of these options with each other and provide some guarantees, that it keeps working.

Hi, I'd prefer not to bother writing a test, as I have no idea how to do so. Could either you write one or this could be merged and a test could be added later? Thanks.

@Henry-Hiles Henry-Hiles force-pushed the master branch 2 times, most recently from 1c18b28 to 10ecf1f Compare April 19, 2025 03:35
@Henry-Hiles
Copy link
Contributor Author

Henry-Hiles commented Apr 20, 2025

Huh, despite running treefmt, newlines were removed in all-tests.nix

@mweinelt
Copy link
Member

Please don't reformat all-tests.nix and restore the removed blank lines in the packages.

@Henry-Hiles
Copy link
Contributor Author

Please don't reformat all-tests.nix and restore the removed blank lines in the packages.

I will. Do you think these tests are enough, though?

@mweinelt
Copy link
Member

mweinelt commented Apr 21, 2025

They're a start. Ideally we could show some crucial interaction between these services (or between lk-jwt-service and element-call) works.

@Henry-Hiles
Copy link
Contributor Author

They're a start. Ideally we could show some crucial interaction between these services (or between lk-jwt-service and element-call) works.

Right. Is that needed for this PR? I really have no idea how the internals work so this will require a deep dive into livekit documentation / internals for me.

@Henry-Hiles
Copy link
Contributor Author

Okay, I think formatting should be good now.

@Henry-Hiles Henry-Hiles force-pushed the master branch 3 times, most recently from ae14102 to 8569130 Compare April 26, 2025 15:44
@Henry-Hiles Henry-Hiles requested a review from Ma27 April 26, 2025 15:44
@Henry-Hiles
Copy link
Contributor Author

Why did that check fail?

@LordGrimmauld
Copy link
Contributor

Github is rate-limiting its own CI, this happens sometimes. This particular fail shouldn't be an issue.

@Henry-Hiles
Copy link
Contributor Author

Ah, seems to have worked now, thanks.

@mweinelt mweinelt merged commit 7cd0677 into NixOS:master Apr 29, 2025
28 of 31 checks passed
@mweinelt
Copy link
Member

Thank you!

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 (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: tests This PR has tests 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants