don't use builtins.fetchGit for cargo git dependencies#1308
don't use builtins.fetchGit for cargo git dependencies#1308sysedwinistrator wants to merge 4 commits intosodiboo:mainfrom
Conversation
|
I am aware of this and want to fix it; indeed i should be specifying fixed hashes. But simply adding the hashes is not a complete solution because i also have automations to update the input sources in this repo automatically. The existing workflows are only based around updating flake inputs; and this is the entire reason why i use builtin fetchGit (it's simply easier). To use fixed hashes, the workflows need to update those hashes, and it needs to be specified for all four packages ( So, if you feel like doing so, and, shudders, want to deal with GitHub Actions, be my guest and update this PR. If you don't feel like doing so, you can close this PR, and i pinky promise that i will eventually get around to it. |
|
I'm trying out https://github.com/Mic92/nix-update for this. I got ahead of myself and removed the lockfile import that triggers IFD ( |
6c0c702 to
d9bc96d
Compare
|
I modified the workflow to run nix-update before updating flake.lock. It's a bit rudimentary, as it's using the update-flake-lock step after it to commit the changes. Also, the job is still named Here's a run of the workflow in my fork: https://github.com/sysedwinistrator/niri-flake/actions/runs/17805089719/job/50614809720. It successfully created a PR that updates both flake.lock and niri-unstable: https://github.com/sysedwinistrator/niri-flake/pull/1/files. The rest of the |
|
As far as i can tell, the |
.github/workflows/update.yml
Outdated
| nix run nixpkgs#nix-update -- --flake --version=stable --override-file=pkgs/niri/stable/default.nix niri-stable | ||
| nix run nixpkgs#nix-update -- --flake --version=branch --override-file=pkgs/niri/unstable/default.nix niri-unstable | ||
| nix run nixpkgs#nix-update -- --flake --version=stable --override-file=pkgs/xwayland-satellite/stable/default.nix xwayland-satellite-stable | ||
| nix run nixpkgs#nix-update -- --flake --version=branch --override-file=pkgs/xwayland-satellite/unstable/default.nix xwayland-satellite-unstable |
There was a problem hiding this comment.
nix-update should not be automatically updating the stable packages here
or at least, not for niri. maybe for xwayland-satellite? haven't thought too much about this.
but, generally, i ensure that the settings module always exhaustively supports niri-stable. it must therefore only be manually updated.
it would be nice if the xwayland-satellite package was automatically updated but not automatically merged? so i get a nixpkgs-like workflow where the bot pings me "hey, this can be updated pretty please". niri could behave like this too. but this should not be in the automatically-merged workflow.
ultimately, that's for me to figure out how exactly i want to deal with it. since i'm the one who'll be manually merging the stable updates. so for now, just remove the lines updating the stable packages.
| nix run nixpkgs#nix-update -- --flake --version=stable --override-file=pkgs/niri/stable/default.nix niri-stable | |
| nix run nixpkgs#nix-update -- --flake --version=branch --override-file=pkgs/niri/unstable/default.nix niri-unstable | |
| nix run nixpkgs#nix-update -- --flake --version=stable --override-file=pkgs/xwayland-satellite/stable/default.nix xwayland-satellite-stable | |
| nix run nixpkgs#nix-update -- --flake --version=branch --override-file=pkgs/xwayland-satellite/unstable/default.nix xwayland-satellite-unstable | |
| nix run nixpkgs#nix-update -- --flake --version=branch --override-file=pkgs/niri/unstable/default.nix niri-unstable | |
| nix run nixpkgs#nix-update -- --flake --version=branch --override-file=pkgs/xwayland-satellite/unstable/default.nix xwayland-satellite-unstable |
There was a problem hiding this comment.
When redoing my PR, I decided to only do unstable updates in CI for now. AFAIK, the stable packages don't get any bugfix releases in between stable versions anyway.
For convenience, I added the nix-update commands for updating the stable packages to the justfile.
pkgs/niri/unstable/default.nix
Outdated
| version = "25.08-unstable-2025-09-14"; | ||
| versionString = "${version} (commit ${src.rev})"; |
There was a problem hiding this comment.
not loving this referring to the previous stable release. i'd argue that's even wrong. when i read "{version number}-unstable" i usually think that means "oh a prerelease of {version number}"; but that's not what it means here, so i' ve previously just omitted any version number to avoid confusion.
i also kept the git revision in the package version string because that means it shows up on version diffs in a system rebuild.
so, like, can we teach nix-update to use the format i prefer?
i also know unstable-2025-09-14 can show up wonky on some version diffs, thinking "unstable" is part of the package name (i mean, it sorta is?) and then saying that "oh you've removed niri-unstable version 2025-08-01-xxxxxx and installed niri version 25.08". i guess this is why it's common to have a (stale) version number prefix. it shows up nicer on diffs as being the same package. but it can also be solved by having the version be 0-unstable-2025-09-14-xxxxxx to ensure it has a digit.
but that kind of weird diff is mainly a concern given the previous approach of comparing the hash against known releases. as this PR currently stands, the unstable package is always called "unstable", even if it coincides with a stable tag. so, maybe then the above "wonky diff" behaviour is even desirable. since it will only show up when switching to/from the unstable package, and not also when the unstable coincides with a tagged release.
There was a problem hiding this comment.
The <last stable release> prefix is currently hardcoded in nix-update: https://github.com/Mic92/nix-update/blob/3c884a778ece6d45b702e323c790691a58a70b76/nix_update/version/github.py#L199. It should be easy enough to create a PR to have an option to not do that. Adding the rev might also be possible, but I'm not sure whether the maintainers want to add a templating engine for the version there.
At the end, I think the entire thing should be possible with a bit of good old sed magic after running nix-update.
There was a problem hiding this comment.
gitrev=$(sed -nE 's/\s+rev = "(.*)"\;/\1/p' default.nix | cut -c1-7)
sed -i -E 's/version = ".+-(unstable-[0-9]{4}-[0-9]{2}-[0-9]{2})";/version = "\1-'$gitrev'";/g' default.nixThere was a problem hiding this comment.
Getting back on this, does it actually matter what the derivation's version attribute is? We could still remove the previous stable version from the versionString used by the build to avoid confusion (especially for upstream maintainers) when people run sth like niri --version.
There was a problem hiding this comment.
Fixed this. the version strings passed to the build are now the exact same as in main.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
299a95b to
84315c1
Compare
|
Hi, sorry for the long silence, but I want to look into this again. One thing I noticed and fixed is that in the previous implementation, the packages were no longer overridable (at least not without a hack of overriding |
- Remove usage of builtins.fetchGit by using cargoHash instead of cargoLock (nix-update can update cargoHash automatically). - Refactor packages to conventional (nixpkgs-like) file structure compatible with nix-update. - Get src using fixed-output-derivation (fetchFromGitHub) instead of source-only flake inputs. This is a requirement for nix-update. - Set unstable versions in format recognized by nix-update. - Remove logic for fetching and looking up versions by their Git rev. - Use callPackage to a generic.nix in a way that keeps the final derivation's arguments overridable (they won't be discoverable via <pkg>.override.__functionArgs, though).
84315c1 to
6ebfdc3
Compare
Summary
The goal of this PR is to remove the usage of
builtins.fetchGitduring evaluation (see below for the reasons).To achieve this, it:
cargoLockwithcargoHashcargoHashneeds to be updated each time the upstream lockfile changesfetchFromGitHubinstead of flake inputs assrccallPackageindependently of the rest of this flake (was on @sodiboo's wishlist)Background
Context
Yesterday, when I was making changes to my config,
home-manager switchgot stuck because https://gitlab.freedesktop.org/pipewire/pipewire-rs was unreachable (apparently it was being (D)DoS'ed yesterday).This struck me as odd, because I had already built the niri-unstable pkg (and any other packages used by my configuration, I only changed some keybindings). Usually the source repo only gets fetched when building a package.
It turns out this is happening because the flake allows
buildRustPackageto usebuiltins.fetchGit, which fetches during evaluation.I also saw some issues in this repo related to this: #990 #923.
Please note that https://gitlab.freedesktop.org is run by volunteers, doesn't have infinite resources, and has had outages from (D)DoS more than once.
So it would be both in the interest of the users of this flake (by being able to use it even when https://gitlab.freedesktop.org is down) and the operators of https://gitlab.freedesktop.org (by not being subjected to unnecessary git pulls) to switch to nixpkgs fetchers.
The downside of this is that the output hashes for all git dependencies have to be specified.
They will also need to be updated if upstream updates or adds git dependencies. In that case, the update automation this repo uses might break.
Explanation (original commit message, outdated)
Unlike nixpkgs fetchers (such as pkgs.fetchgit or pkgs.fetchFromGitLab), which are fixed-output derivations and therefore do the actual fetching only when they're being built; builtins.fetchGit fetches when the expression is being evaluated (regardless of whether the package for which it provides the source is even being built). Also, because it doesn't evaluate to a derivation that can be built, its result cannot be pushed to a binary cache.
This means that the git repos using builtins.fetchGit need to be fetched each time a package using them is being evaluated (although there's some level of local caching), even when the package itself has already been built.
In contrast, when using nixpkgs fetchers for src, the src attribute is basically just another derivation the package depends on, and will be built (or substituted!) when building the package.
It's for these reasons that builtin fetchers such as builtins.fetchGit aren't being used in nixpkgs.
Footnotes
nix-update could also be used to only update the cargoHash. ↩