Skip to content

Conversation

@Garionion
Copy link

@Garionion Garionion commented Nov 16, 2025

this is an inital attempt at creating a dev-shell for this. i am unsure if all dependencies are present.
I've included (almost) all gstreamer plugins, so its easier to test pipelines, but i'm also unsure if this is needed.
this will also conflict with gstreamer installed e.g. via homebrew (tested on macos), my short solution to this was to uninstall the version from homebrew

Summary by CodeRabbit

  • Chores
    • Added development environment setup for the Media over QUIC - Gstreamer plugin project, enabling developers to build and run the plugin with all necessary dependencies configured automatically.

@coderabbitai
Copy link

coderabbitai bot commented Nov 16, 2025

Walkthrough

This pull request introduces a new flake.nix file that establishes a Nix flake configuration for a Media over QUIC GStreamer plugin project. The flake declares dependencies on nixpkgs, flake-utils, crane, and rust-overlay. It configures a Rust toolchain with development extensions and sets up crane-based build infrastructure. The configuration defines a development shell with GStreamer-related packages and environment variables, and establishes a build target for the hang-gst package using Cargo with specific build inputs and flags. The flake outputs expose both the development shell and the built package as default.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately describes the main change: adding a Nix flake configuration for packaging and providing a development shell for the project.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide custom instructions to shape the summary (bullet lists, tables, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example:

"Create a concise high-level summary as a bullet-point list. Then include a Markdown table showing lines added and removed by each contributing author."


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
flake.nix (3)

33-42: Tighten GStreamer plugin set and GST_PLUGIN_PATH composition

A few suggestions to make the GStreamer side more predictable and minimal:

  • You currently mix makeSearchPath "lib/gstreamer-1.0" (good for plugin dirs) with makeLibraryPath (general lib dirs) inside GST_PLUGIN_PATH_1_0. For clarity, it’s usually better to keep GST_PLUGIN_PATH_1_0 to actual plugin directories only and rely on LD_LIBRARY_PATH / Nix’s default for lib directories. For example:
  • gstPluginPath = nixpkgs.lib.makeSearchPath "lib/gstreamer-1.0" gstreamerSearch + ":" + nixpkgs.lib.makeLibraryPath gstreamerLib;
  • gstPluginPath = nixpkgs.lib.makeSearchPath "lib/gstreamer-1.0" (gstreamerLib ++ gstreamerSearch);
- If the goal is “include almost all plugins”, consider also adding the plugin packages used in `gstreamerLib` to `shell-deps` for better symmetry and to make it obvious which plugin sets are actually on the system path.
- If you only need a subset of plugins for hang-gst development, you might want to trim `gstreamerSearch` down later to speed up registry scans and reduce conflicts.

These are behavioral tweaks rather than blockers but will make the shell easier to reason about.




Also applies to: 44-55, 61-64

---

`57-70`: **Consider exposing the dev shell as `devShells.default` for better `nix develop` UX**

Right now you expose a single `devShell` attribute per system:

```nix
devShell = pkgs.mkShell { ... };

Modern Nix and flake tooling typically expect devShells.${system}.default. With flake-utils.lib.eachDefaultSystem this usually looks like:

-  devShell = pkgs.mkShell {
+  devShells.default = pkgs.mkShell {
     packages = shell-deps;
     env = { ... };
   };

This makes it clearer for users (nix develop / nix develop .#default) and aligns with common templates; it also avoids edge cases across Nix versions and frontends.


72-83: Double‑check native vs build inputs for the hang-gst package

The hang-gst package wiring via craneLib.buildPackage looks generally good, but the build inputs deserve a quick check:

nativeBuildInputs = with pkgs; [
  pkg-config
  glib
  glib.dev
  gst_all_1.gstreamer.dev
];

For Rust crates linking against C libraries via pkg-config, it’s common to:

  • Put tools like pkg-config in nativeBuildInputs.
  • Put the actual libraries (glib, gst_all_1.gstreamer, etc.) in buildInputs so they end up in the link environment for the target.

Having everything in nativeBuildInputs may still work on some platforms but can be fragile, especially for cross‑compilation or non-Linux targets.

I’d recommend splitting these along tool vs library lines and adding any additional GStreamer dev outputs you actually need (e.g. gst_all_1.gstreamer, gst_all_1.gst-plugins-base.dev) once you’ve run a full build in the flake.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2a70f8a and 68cc025.

⛔ Files ignored due to path filters (1)
  • flake.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • flake.nix (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-06T17:21:39.335Z
Learnt from: CR
Repo: kixelated/hang-gst PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-06T17:21:39.335Z
Learning: Applies to src/lib.rs : In src/lib.rs, register the GStreamer plugin as "hang" and register both elements: hangsink and hangsrc

Applied to files:

  • flake.nix
📚 Learning: 2025-10-06T17:21:39.335Z
Learnt from: CR
Repo: kixelated/hang-gst PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-06T17:21:39.335Z
Learning: Applies to src/sink/mod.rs : Place the HangSink GStreamer element wrapper in src/sink/mod.rs

Applied to files:

  • flake.nix
🔇 Additional comments (3)
flake.nix (3)

1-12: Overall flake structure and inputs look solid

The high-level flake shape (description, inputs, use of flake-utils, crane, and rust-overlay) is coherent and follows common patterns. Using nixos-unstable plus a flake lock for reproducibility should be fine for this project.


18-31: Rust toolchain and crane integration are correctly wired

The pkgs import with rust-overlay, the rust-toolchain override with rust-src and rust-analyzer, and craneLib = (crane.mkLib pkgs).overrideToolchain rust-toolchain are all consistent with typical crane setups. The crateInfo helper around crateNameFromCargoToml is also a reasonable abstraction.


72-86: Good default package selection

Exposing packages.hang-gst and wiring packages.default = self.packages.${system}.hang-gst; is a nice touch; it makes nix build and nix build .#hang-gst both do the intuitive thing.

@kixelated
Copy link
Collaborator

kixelated commented Nov 16, 2025

Oh whoops I forgot to port this over. Here's the commit that removed the gstreamer stuff if you want to compare notes: moq-dev/moq@ad1ccce

Can you add envrc and maybe even switch CI to use nix?

@kixelated
Copy link
Collaborator

And yeah, I think Coderabbit's nitpick comments are right? The library path stuff might be unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants