-
Notifications
You must be signed in to change notification settings - Fork 134
feat: adding back our nix modules #454
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
Conversation
moq-relay command
therishidesai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets remove all of the lines that have "Gordy" in it
flake.nix
Outdated
| inputsFrom = [ | ||
| rs.devShells.${system}.default | ||
| js.devShells.${system}.default | ||
| js.devShells.${system}.default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: whitespace
flake.nix
Outdated
| js.devShells.${system}.default | ||
| ]; | ||
| shellHook = '' | ||
| echo "🎯 Using Gordy's Top-Level MoQ Flake (Rust environment only)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove your name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should combine this flake.nix with the top level flake.nix so there is one unified nix flake in the moq repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just the rs/flake.nix or also the js/flake.nix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also - problem here is if i combine this with the top level flake.nix, it gives me problems with the js stuff which isn't in the moq-relay server but is related to the webplayer and front end
rs/nix/modules/moq-relay.nix
Outdated
|
|
||
| serviceConfig = { | ||
| ExecStart = "${pkgs.moq-relay}/bin/moq-relay --bind [::]:${builtins.toString cfg.port}" + | ||
| (if cfg.dev.enable then " --tls-generate ${cfg.dev.tls_url}" else ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these args are up to date with the new moq-relay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated them
rs/nix/packages/moq.nix
Outdated
| @@ -0,0 +1,34 @@ | |||
| { fenix, naersk, flake-utils, ... }: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why naersk? We used to use crate2nix in the original moq. I think we should stick to crate2nix as it allows for better caching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched over to naersk primarily because it doesn't require a separate lockfile (don't want to mandate Nix). I don't know the differences otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that can get updated now, but naersk also works fine. This is a smaller project so shouldn't be an issue with the caching
rs/nix/shell.nix
Outdated
| @@ -0,0 +1,40 @@ | |||
| { self, nixpkgs, flake-utils, fenix, ... }: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the dev shell from the top level nix into this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the dev shells have both rs and js, so i gotta keep it at the top level
therishidesai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me on the nix side of things. In future PRs let's add a way of validating the config file at eval time I case the underlying rust code gets changed. Let's also add a NixOS VM test for the relay in a future PR.
@kixelated thoughts on this PR?
js/hang-demo/src/index.ts
Outdated
|
|
||
| // If query params are provided, use it as the broadcast name. | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
js/hang-demo/src/index.ts
Outdated
| const urlParams = new URLSearchParams(window.location.search); | ||
| const name = urlParams.get("name") ?? "demo/bbb"; | ||
| watch.setAttribute("url", `http://localhost:4443/${name}.hang`); | ||
| const name = urlParams.get("name") ?? "jwt-test/bbb"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
|
|
||
| # Environment variables from moq-rs | ||
| shellHook = '' | ||
| export LIBCLANG_PATH="${pkgs.libclang.lib}/lib" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deals with some C dependencies in Rust code
rs/justfile
Outdated
| fi | ||
|
|
||
| # Generate publisher-only token | ||
| if [ ! -f "dev/bum-publisher.jwt" ]; then \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these?
rs/nix/modules/moq-relay.nix
Outdated
| tomlFormat = pkgs.formats.toml {}; | ||
|
|
||
| # Build the configuration | ||
| configData = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty cool tbh
rs/nix/overlay.nix
Outdated
| moq-token = naersk'.buildPackage { | ||
| pname = "moq-token-cli"; | ||
| src = ../.; | ||
| cargoBuildOptions = opts: opts ++ [ "-p" "moq-token-cli" ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also have these options for the other binaries.
rs/nix/shell.nix
Outdated
| with pkgs; | ||
| mkShell { | ||
| nativeBuildInputs = [ | ||
| go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Moq-Relay Module Fails Due to Incorrect Binary Reference
The moq-relay NixOS module incorrectly references the moq-token binary as moq-token instead of moq-token-cli, causing preStart commands for auth key and cluster token generation to fail. Additionally, the cluster token generation lacks validation that the authentication key has been successfully generated, leading to potential failures if the key generation step fails.
rs/nix/modules/moq-relay.nix#L221-L234
Was this report helpful? Give feedback by reacting with 👍 or 👎
FYI @gordysunsaronic I let Cursor fix these issues, but I don't have a way of validating the nixos fixes. Could you double check everything still runs? |
adding back out nix modules