vortex: init at 2.2.0#491344
Conversation
MattSturgeon
left a comment
There was a problem hiding this comment.
Thanks for putting this together — this is an impressively thorough initial packaging effort. I won't review in-detail yet, let's get the high-level approach down first.
As a maintainer of the now-deprecated nexusmods-app package, I've been exploring similar work recently, and it's clear a lot of care went into handling Electron ABI alignment, native Node addons, Yarn workspaces, the NativeAOT FOMOD pipeline, etc.
That said, this derivation currently carries a substantial amount of complexity that may become unnecessary once Nexus-Mods/Vortex#19749 lands upstream.
Vortex will ship a generated-sources.json, which provides:
- Fully resolved artefact URLs
- Cryptographic hashes
- A flattened dependency closure
Given nixpkgs has the same fixed-output / hash-pinned constraints, it would likely be preferable to:
- Consume
generated-sources.jsonvia an update script - Remove
fetchYarnDeps, merged lockfile logic, and lockfile rewriting - Reuse upstream dependency resolution instead of regenerating locally
- Replace custom NuGet dependency handling similarly
This would significantly simplify the derivation and reduce long-term maintenance burden, while improving auditability.
I'd recommend considering a refactor along these lines once Nexus-Mods/Vortex#19749 is merged upstream. Alternatively, while this is still a draft, pinning src to that PR could allow aligning the dependency model early.
Ideally, I'd also like to minimise the required patches. It'd be best if we can land configurable build flags (etc) upstream, as necessary.
Aside: we also need to ensure files are formatted correctly to satisfy CI. This can be done by entering the nixpkgs shell and running treefmt, or alternatively nix-shell --run treefmt from the nixpkgs repo root.
There was a problem hiding this comment.
I'm not convinced a NixOS module is warranted here. The functionality is essentially limited to installing the package, which is not enough justification for a module.
IIUC, URL-handler registration should already be handled by a properly defined desktop entry.
There may be some value in exposing options like dataDir, downloadsDir, or stagingDir, but I'm not sure these belong at the system level. These paths are inherently user-scoped and would likely be better handled via Home Manager or application-level configuration.
There was a problem hiding this comment.
There may be some value in exposing options like...
My reason for including the module. I am open to it only being a home-manager module, but don't see the harm in it also being a NixOS module, the enduser should choose how to store their data. There could be multiple users, that use the per-user subsystem offered by Vortex, this subsystem is made for cases where Vortex is installed at the system level.
There was a problem hiding this comment.
I can definitely appreciate that perspective. My concern is mainly about module scope and long-term cost/benefit.
Modules that do little beyond installing a package are typically viewed as low value vs their maintenance and evaluation overhead.
don't see the harm in it also being a NixOS module
Agreed — it's not about harm, but justification. We generally look for clear system-level behaviour before introducing a NixOS module.
this subsystem is made for cases where Vortex is installed at the system level
I'm not fully convinced that system-level installation alone warrants system-level configuration of runtime/data directories. Vortex still runs per-user even when installed system-wide. If it ran as a shared system service, that would be a stronger case for configuring its directories.
|
|
||
| stripFindArgs = lib.concatStringsSep " -o " (map (p: "-name '${p}'") stripPatterns); | ||
| in | ||
| stdenv.mkDerivation (finalAttrs: { |
There was a problem hiding this comment.
We can use
| stdenv.mkDerivation (finalAttrs: { | |
| stdenvNoCC.mkDerivation (finalAttrs: { |
| desktopItems = [ | ||
| (makeDesktopItem { | ||
| name = "vortex"; | ||
| desktopName = "Vortex"; | ||
| comment = "Mod manager from Nexus Mods"; | ||
| exec = "vortex --download %u"; | ||
| icon = "vortex"; | ||
| startupWMClass = "Vortex"; | ||
| categories = ["Game"]; | ||
| mimeTypes = ["x-scheme-handler/nxm"]; | ||
| }) | ||
| ]; |
There was a problem hiding this comment.
Generally, if upstream provides or builds a sane desktop-entry, I prefer to use that (often with some postInstall patches). This is fine if our desktop entry must be significantly different or upstream doesn't build one (yet).
I'm not convinced it is correct to always exec with --download, though. Shouldn't that only be used when handling x-scheme-handler/nxm?
| license = lib.licenses.gpl3Only; | ||
| maintainers = with lib.maintainers; [ caniko ]; | ||
| mainProgram = "vortex"; | ||
| platforms = ["x86_64-linux"]; |
There was a problem hiding this comment.
We should expand this to more platforms; lib.platforms.linux at a minimum. Ideally, we'd also support lib.platforms.windows, but I appreciate that is hard to test and maintain in a nix package.
There was a problem hiding this comment.
Maybe in separate PRs?
There was a problem hiding this comment.
Let's defer discussion until later in the PR. I'd prefer to optimistically declare lib.platforms.linux at least, and then do minimal testing via nixpkgs-review-gha.
lib.platforms.windows is an idealistic stretch goal, and probably something we're unlikely to actually maintain. That's definitely out-of-scope for the initial PR.
| meta = { | ||
| description = "The elegant, powerful, and open-source mod manager from Nexus Mods"; | ||
| homepage = "https://www.nexusmods.com/about/vortex/"; | ||
| changelog = "https://github.com/Nexus-Mods/Vortex/releases/tag/v${finalAttrs.version}"; |
There was a problem hiding this comment.
This can use finalAttrs.src.tag (instead of v${version}), at least when src uses a tagged version.
| ]; | ||
|
|
||
| meta = { | ||
| description = "The elegant, powerful, and open-source mod manager from Nexus Mods"; |
There was a problem hiding this comment.
As per https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#meta-attributes,
- Not start with the definite or an indefinite article.
"the" is the definite article. We can't replace with "an", as that is the indefinite article.
| description = "The elegant, powerful, and open-source mod manager from Nexus Mods"; | |
| description = "Elegant, powerful, and open-source mod manager from Nexus Mods"; |
We should also:
- Provide factual information.
- Avoid subjective language.
So
| description = "The elegant, powerful, and open-source mod manager from Nexus Mods"; | |
| description = "Open-source mod manager from Nexus Mods"; |
| homepage = "https://www.nexusmods.com/about/vortex/"; | ||
| changelog = "https://github.com/Nexus-Mods/Vortex/releases/tag/v${finalAttrs.version}"; | ||
| license = lib.licenses.gpl3Only; | ||
| maintainers = with lib.maintainers; [ caniko ]; |
There was a problem hiding this comment.
I'd be happy to be listed as a maintainer:
| maintainers = with lib.maintainers; [ caniko ]; | |
| maintainers = with lib.maintainers; [ | |
| caniko | |
| MattSturgeon | |
| ]; |
@l0b0 may also wish to be listed.
There was a problem hiding this comment.
I am all for more brains 🧠
| src = fetchFromGitHub { | ||
| owner = "Nexus-Mods"; | ||
| repo = "Vortex"; | ||
| rev = "v${finalAttrs.version}"; |
There was a problem hiding this comment.
| rev = "v${finalAttrs.version}"; | |
| tag = "v${finalAttrs.version}"; |
We can use rev when targeting a specific commit, but when targeting a tag we should use tag.
|
Hello @MattSturgeon; the pleasure is mine! Thank you for the high-level perspective and for sharing your experience with the Regarding your suggestions:
I'll begin looking into pinning Would you like me to ping you once I've pushed the initial refactor using the |
I'm subbed to the PR, but sounds good. Feel free to reach out on matrix too, if you want to discuss anything synchronously: |
|
I've pushed a significant update addressing the review feedback from @MattSturgeon: ChangesVersion bump: 1.16.3 → 2.2.0Vortex has evolved significantly upstream. The current release is v2.2.0 with a completely different build system. Migration from yarn to pnpmThe upstream project migrated from a yarn workspace with ~50 individual extension lockfiles to a pnpm + Nx monorepo (single Simplified derivation
Remaining workThe nix-build -A vortex 2>&1 | grep 'got:'
\# Copy the hash back into pnpmDeps.hashcc @MattSturgeon for re-review |
|
Before I look at the diff, I'd like to make you aware of some parallel and/or related work:
Points 2 & 3 also have some TODOs/discussion around some potential blockers in the upstream build pipeline. In particular, the flatpak generated-sources has not been ported to their pnpm build system yet, and their build system also has some "impure" and "unlocked" network steps. Also, I don't remember whether we discussed requirements for including Vortex in Nixpkgs, yet. I'd feel a little uncomfortable about having a Nixpkgs package before the upstream Vortex team consider Vortex to be "usable" on Linux. IIUC, the Windows build of Vortex running under wine or proton will be more useful to most users still than the native linux build of Vortex, until they begin adding features like wine/proton integration (like they had in NexusMods.App), xdg-portals integration, etc. Should we consider this a draft until then? Or is the Linux build more feature-complete than I remember? Perhaps it'd make sense to maintain the package separately from Nixpkgs for a while (in a dedicated flake)? Even if we do land this in Nixpkgs early, we should come up with a way to inform users that Vortex's Linux version is considered pre-alpha. Maybe via |
MattSturgeon
left a comment
There was a problem hiding this comment.
Most of the earlier review still stands. Here's some additional feedback on the current diff.
There are still some bigger questions to address before we can consider merging though, even once all feedback is addressed. See #491344 (comment)
| --replace-fail \ | ||
| 'process.exit(0);' \ | ||
| 'process.exit(0);' \ |
| # Wrap with electron | ||
| makeWrapper ${electron}/bin/electron $out/bin/vortex \ | ||
| --add-flags $out/opt/vortex \ | ||
| --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--ozone-platform-hint=auto --enable-features=WaylandWindowDecorations --enable-wayland-ime=true}}" \ | ||
| --set GTK_USE_PORTAL 0 \ | ||
| --set IGNORE_UPDATES yes |
| --add-flags $out/opt/vortex \ | ||
| --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--ozone-platform-hint=auto --enable-features=WaylandWindowDecorations --enable-wayland-ime=true}}" \ |
There was a problem hiding this comment.
Can we prefer to use --add-flag (per flag) to avoid whitespace-splitting footguns?
| npm_config_arch = "x64"; | ||
| npm_config_platform = "linux"; |
There was a problem hiding this comment.
Do these two actually need to be exported env vars, or are normal shell variables fine?
| autoPatchelfHook, | ||
| }: | ||
|
|
||
| stdenv.mkDerivation (finalAttrs: { |
There was a problem hiding this comment.
Upstream have been using clangStdenv. Does this matter?
Do we actually need a C compiler, or can we use stdenvNoCC?
| owner = "Nexus-Mods"; | ||
| repo = "Vortex"; | ||
| rev = "v${finalAttrs.version}"; | ||
| hash = "sha256-1g68wgbqb9ymlispwph9r9zk86jgq81i3zm7j5y848dh73zz0705"; |
There was a problem hiding this comment.
error: invalid SRI hash '1g68wgbqb9ymlispwph9r9zk86jgq81i3zm7j5y848dh73zz0705', length 39 != expected length 32
| src = fetchFromGitHub { | ||
| owner = "Nexus-Mods"; | ||
| repo = "Vortex"; | ||
| rev = "v${finalAttrs.version}"; |
There was a problem hiding this comment.
| rev = "v${finalAttrs.version}"; | |
| tag = "v${finalAttrs.version}"; |
| pnpmDeps = fetchPnpmDeps { | ||
| inherit (finalAttrs) pname version src; | ||
| fetcherVersion = 3; | ||
| hash = ""; |
|
|
||
| pnpmDeps = fetchPnpmDeps { | ||
| inherit (finalAttrs) pname version src; | ||
| fetcherVersion = 3; |
There was a problem hiding this comment.
pnpm 11 benefits from
| fetcherVersion = 3; | |
| fetcherVersion = 4; |
See also #531657 which we may benefit from if merged
| }; | ||
|
|
||
| pnpmDeps = fetchPnpmDeps { | ||
| inherit (finalAttrs) pname version src; |
There was a problem hiding this comment.
- We are supposed to pass
fetchPnpmDepsthepnpmpackage we're using - We can pass it the actual package, instead of having it lookup our package from
pkgs.${pname}
| inherit (finalAttrs) pname version src; | |
| inherit (finalAttrs) pname version src; | |
| inherit pnpm; | |
| pkg = finalAttrs.finalPackage |
As per:
It is recommended to pin pnpm to a major version, due to regular breaking changes in the store format
The latest major version is always available underpkgs.pnpm
We should also add at the top of the file:
let
pnpm = pnpm_11;
inSee: https://nixos.org/manual/nixpkgs/unstable/#javascript-pnpm
|
@MattSturgeon thanks for the review and the pointers. On the strategic questions: #21463 -- still open, no response from Vortex maintainers yet. The upstream pnpm migration PR (#23517) is also still open. Your nix-package branch -- adopted it wholesale. Your duckdb-extensions FOD, font-scanner node-gyp fix, electron-builder patching, and upstream desktop entry are all better than what I had. Transplanted into Linux-readiness -- agreed. PRs in draft, will add The derivation is a single commit on current master, using master's |
| "11" = { | ||
| version = "11.5.1"; | ||
| hash = "sha256-3npcG+2DAYBRg6h5l/4XIM1crvtXvoOFNaS/xKFZaVk="; | ||
| }; |
There was a problem hiding this comment.
pnpm 11 is already on master, but your branch is 80064(!) commits behind
Based on MattSturgeon's derivation from MattSturgeon/Vortex@nix-package. Uses nixpkgs master infrastructure (pnpm_11 v11.9.0, electron_42-bin, pnpmBuildHook, fetchPnpmDeps fetcherVersion 4, nodejs_24 v24.16.0). Key features: - pnpm + Nx monorepo build (pnpmBuildHook + pnpmConfigHook) - duckdb-extensions as separate FOD derivation - font-scanner node-gyp rebuild in buildPhase - Upstream flatpak desktop entry (patched run.sh -> vortex) - electron-builder config patched with Nix-provided Electron - forceLegacyDeploy workaround for pnpm 11 deploy bug (NixOS#5315) - Offline deploy with metadata cache pre-population - __structuredAttrs = true, strictDeps = true - meta: linux only, factual description, MattSturgeon as co-maintainer
|
@MattSturgeon rebased onto current master, dropped all the backported patches. Using master's pnpm_11 (v11.9.0), electron_42-bin, pnpmBuildHook, fetcherVersion 4, nodejs v24.16.0. Single commit, just the package.nix. Added |

Things done
passthru.tests.nixpkgs-reviewon this PR. See [nixpkgs-review usage]../result/bin/.Resolves
#147228