voxtype,voxtype-{vulkan,onnx}: init at 0.6.2#491124
voxtype,voxtype-{vulkan,onnx}: init at 0.6.2#491124DuskyElf wants to merge 1 commit intoNixOS:masterfrom
Conversation
|
@delafthi can you please review it? I'm new to packaging |
There was a problem hiding this comment.
Pull request overview
This PR adds three package variants of voxtype, a voice-to-text application with push-to-talk support for Wayland compositors. The package supports CPU-only and Vulkan-accelerated modes through different build features.
Changes:
- Added voxtype package with configurable acceleration support (CPU or Vulkan)
- Added voxtype-cpu and voxtype-vulkan variants in all-packages.nix
- Referenced ollama package structure for handling acceleration variants
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkgs/by-name/vo/voxtype/package.nix | Main package definition with acceleration parameter support for CPU and Vulkan modes |
| pkgs/top-level/all-packages.nix | Adds explicit variant definitions for voxtype-cpu and voxtype-vulkan |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
|
How come it builds fine locally but not on github actions via nixpkgs-review-gha ? |
I had a quick look and at first glance it seems good. I'll review it tomorrow. |
I'm not sure but does vulkan stuff depend on hardware (gpus)? That might not be available on the action runners. Probably not - I see you have all the vulkan stuff. I'll have a look tomorrow. |
I believe it shouldn't depend at build time ... |
Vulkan has two parts, this links against the interface, the runtime is loaded dynamically. So this is fine. By the way, 0.6.0 is out. |
|
I'm not sure if this should include wtype and wl-clipboard, as they are only useful in some DM, the tool itself has many other backends, and they are all taken from PATH. IMHO it's better to leave that to the final user. |
delafthi
left a comment
There was a problem hiding this comment.
About the issue with CI/nixpkgs-review-gha. The build fails when compiling whisper-cpp. I had a look at the package declaration in nixpkgs and there is this:
nixpkgs/pkgs/by-name/wh/whisper-cpp/package.nix
Lines 140 to 144 in a82ccc3
Maybe we're hitting that on GHA runners.
pkgs/by-name/vo/voxtype/package.nix
Outdated
| vulkan-loader, | ||
|
|
||
| # one of `[ null false "vulkan" ]` | ||
| acceleration ? null, |
There was a problem hiding this comment.
I think I would rather go with:
| acceleration ? null, | |
| vulkanSupport ? false, | |
| <packages for vulkanSupport> | |
| rocmSupport ? false, | |
| <packages for rocmSupport> | |
| cudaSupport ? false, | |
| <packages for cudaSupport> |
(This is just a suggestion - just add the acceleration you want to support.)
This seems to be a common pattern ( see https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/development/python-modules/torch/source/default.nix or https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/by-name/wh/whisper-cpp/package.nix)
There was a problem hiding this comment.
I saw that acceleration pattern in ollama here -
nixpkgs/pkgs/by-name/ol/ollama/package.nix
Lines 38 to 39 in 36e6063
pkgs/by-name/vo/voxtype/package.nix
Outdated
| assert builtins.elem acceleration [ | ||
| null | ||
| false | ||
| "vulkan" | ||
| ]; |
There was a problem hiding this comment.
Change the asserts accordingly. I'm not sure if only one of the acc-backend features can be enabled. Maybe all of them can be enabled - then you could just remove the assert.
pkgs/top-level/all-packages.nix
Outdated
| voxtype-cpu = callPackage ../by-name/vo/voxtype/package.nix { acceleration = false; }; | ||
| voxtype-vulkan = callPackage ../by-name/vo/voxtype/package.nix { acceleration = "vulkan"; }; |
There was a problem hiding this comment.
Also the naming <package>With<feature> seems to be common. I would change to that.
| voxtype-cpu = callPackage ../by-name/vo/voxtype/package.nix { acceleration = false; }; | |
| voxtype-vulkan = callPackage ../by-name/vo/voxtype/package.nix { acceleration = "vulkan"; }; | |
| voxtypeWithVulkan = callPackage ../by-name/vo/voxtype/package.nix { vulkanSupport = true; }; |
Though whisper-cpp uses the dash version, ollama as well 🙈
pkgs/by-name/vo/voxtype/package.nix
Outdated
| "Voice-to-text with push-to-talk for Wayland compositors" | ||
| + lib.optionalString vulkanEnabled ", using Vulkan for generic GPU acceleration"; |
There was a problem hiding this comment.
I would just have a single description without the optional part.
pkgs/by-name/vo/voxtype/package.nix
Outdated
| cmake | ||
| clang | ||
| pkg-config | ||
| makeWrapper |
There was a problem hiding this comment.
Please use makeBinaryWrapper.
https://nixos.org/manual/nixpkgs/stable/#fun-makeWrapper
Using the makeBinaryWrapper implementation is usually preferred, as it creates a tiny compiled wrapper executable, that ...
There was a problem hiding this comment.
It uses wrapProgram and not makeWapper, and it needs those extra --prefix argument
There was a problem hiding this comment.
makeBinaryWrapper also provides wrapProgram. Just include makeBinaryWrapper in nativeBuildInputs - you don't have to change anything in your postInstall hook.
I agree, this seems unflexible. A possible solution would be to have a waylandPackages option as argument which defaults to the packages suggested by voxtype, but allows the user to override them using |
I think this is the best option. |
What possible fixes are there for us? |
|
0.6.0 added a lot of new binaries, and you can see this open PR by the creator to bring those to the upstream flake.nix |
Doing the same here should work. |
I think to avoid blowing up the amount of packages/builds keeping only no gpu / vulkan packages and having the other as optional attributes. That will limit the package to be whisper only (unless recompiled) but if someone wanna try they can always override. |
alright, that makes sense |
Can you please clarify -- doing what? --, I'm unable to understand |
Edit: A look into the CMakeLists.txt documentation of whisper-cpp indicates that the previously mentioned CMake options are not applicable to this issue. |
|
@DuskyElf Maybe that will help: https://codeberg.org/tazz4843/whisper-rs#troubleshooting
|
|
Hi! 👋 I am the upstream maintainer of voxtype, and I feel like I might be creating more problems than I'm solving for NixOS users. I know very little about packaging and publishing for NixOS. Would y'all be open to a discussion on a strategy going forward for me to not cause you a lot of work when new releases are published? By way of example, I've opened a PR to add @DuskyElf as a code owner on the NixOS packaging files in the voxtype repo (peteonrails/voxtype#226). Would it be useful for you to have collaborator access to the repo so you can help keep the in-repo flake and the nixpkgs package in sync without needing a review from me? A couple of notes on the PR:
Happy to help with anything you need for the nixpkgs packaging, and happy to figure out how best to work together. |
|
Hi @peteonrails, Thanks for reaching out, I’m happy to collaborate. I’m also new to nixpkgs packaging. A discussion sounds great such that we keep both the flake.nix (of the upstream) and nixpkgs package (here) in sync with the releases. I use Voxtype daily with my Wayland compositor and it’s been really helpful. I haven’t tried the newer features (like meeting mode) yet, but I’ll test them soon. Welcome to nixpkgs, and thanks again for the great project! |
|
@peteonrails I don’t think you’re making things difficult for the packagers - it’s perfectly normal for a fast-moving package to undergo frequent changes that require more effort to maintain downstream. |
|
@DuskyElf have a look at #492111 I created a derivation based on the existing one at peteonrails/voxtype. Feel free to take wathever you need into this PR. I'll close it when this has been merged. I just tested the build and not the binary. Would be nice to also support cuda/rocm and darwin platforms in the future. |
|
|
https://voxtype.io : Voice-to-text with push-to-talk, integrates nicely with Wayland compositors
repo: https://github.com/peteonrails/voxtype
Added 3 packages
CUDA and ROCm packages could be added later (I don't have a way to test them yet).
References used
The current release of the upstream doesn't support MacOS
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.