treewide: use tags instead of rev, remove unnecessary string interpolation#473846
Merged
philiptaron merged 5 commits intoNixOS:masterfrom Jan 15, 2026
Merged
treewide: use tags instead of rev, remove unnecessary string interpolation#473846philiptaron merged 5 commits intoNixOS:masterfrom
philiptaron merged 5 commits intoNixOS:masterfrom
Conversation
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/6198 |
nixpkgs-ci
bot
added
10.rebuild-linux: 1-10
gaelj
force-pushed
the
replace-rev-with-git-tag
branch
from
c3168d7 to
11270c9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
|
Building So this PR actually fixes this bug, but only to encounter another bug down the line. I believe the resolution of that bug (failed units tests) is therefore not in the scope of this PR. Edit: in fact, a similar problem (although not exactly the same issue) on this package was already reported, on which I have commented with the latest error log. |
gaelj
force-pushed
the
replace-rev-with-git-tag
branch
from
11270c9 to
95db52e
Compare
13 tasks
nixpkgs-ci
bot
added
the
2.status: merge conflict
gaelj
force-pushed
the
replace-rev-with-git-tag
branch
from
95db52e to
b9009b7
Compare
nixpkgs-ci
bot
removed
the
2.status: merge conflict
gaelj
force-pushed
the
replace-rev-with-git-tag
branch
from
b9009b7 to
5597de9
Compare
qweered
approved these changes
Dec 28, 2025
nixpkgs-ci
bot
added
12.approvals: 1
Member
|
I think the automation for building all the drvs with changed srcs should work (probably...). I am running it on this PR -- there should be results posted whenever my computer finishes building all the (apparently 600ish according to my eval?) drvs with changed src or src-like attributes. I think a lot of these sort of attributes are touched indirectly by this change |
Contributor
Author
|
Thanks, @DieracDelta . EDIT: after some more checking, the (AI generated) scripts aren't working properly, trying to fix them On my side I have been checking the generated URLs are identical thanks to a couple of little bash scripts that:
It skipped the following:
EDIT2 Still lots of packages ignored by the check, but I did find one where the URL has been changed (it's using Both URLs are working though, so I don't think it's a problem. EDIT3 At this point I have checked all packages (containing pname) except those below, and did not find any additional URL changes. |
Contributor
Author
|
Regarding the Evaluating the URL gives the same result, whether using rev = "refs/tags/... or tag = "... EDIT: I misunderstood the comment ! They want to be able to reference actual git commits from time to time. |
gaelj
force-pushed
the
replace-rev-with-git-tag
branch
from
5597de9 to
062b8d3
Compare
nixpkgs-ci
bot
added
2.status: merge conflict
13 tasks
gaelj
force-pushed
the
replace-rev-with-git-tag
branch
from
062b8d3 to
b56932f
Compare
Member
|
I suggest keep this as a draft pr and iterate until you think it is ready for review. Also See previous efforts #368177. and ping appropriate people, after you think it's ready for review. |
gaelj
changed the title
nixpkgs-ci
bot
removed
the
2.status: merge conflict
Contributor
Author
|
So I've manually run |
Member
srcbot: Full Evaluation Results for PR #473846Status: 547/624 packages passed, 77 failed (76 pre-existing) Failed Packages (introduced by this PR) - 1 packages
Pre-existing Failures (false positives) - 76 packagesThese packages also fail on the base branch.
547 packages passed
|
Member
|
I think that worked! It's likely that the libfilezilla failure is a red herring since the first and second runs of srcbot also failed to build it. Seems like the mirror is flakey. Also: the link base used by srcbot for the logs is wrong. https://instance-20251227-2125.tail5ca7.ts.net/srcbot-srv/logs/473846_2/ is correct. So maybe this can be merged at this point? This doesn't seem to introduce new build failures errors AFAICT.
I think only some of the packages need issues. Some of them are stale like nzbhydra2 (#478821) since this was run against commit 14e033b. Others are red herrings classified as errors only because of the nondeterministic output: This case shouldn't be flagged as failing I think. And others might have open PRs/issues already like tika. +1 those that remain should have issues opened. |
Contributor
Author
|
@DieracDelta and I both agree that this PR is safe and ready for merge, so I'm de-drafting. The build failures all exist on current master. Pinging all reviewers listed in the 'previous effort' PR mentioned earlier: @philiptaron Thanks to all ! |
Contributor
|
Contributor
Author
|
Yes openLLM is already failing on master and this change actually solves part of its problem. See #376692 |
qweered
approved these changes
Jan 14, 2026
gaelj
force-pushed
the
replace-rev-with-git-tag
branch
from
1c01092 to
ad6fff8
Compare
…, fetchFromBitbucket, fetchFromSourcehut: use `tag =` instead of `rev = refs/tags/...`
gaelj
force-pushed
the
replace-rev-with-git-tag
branch
from
ad6fff8 to
702acfa
Compare
Contributor
|
Looking for a haskell person to look at ee66ab3 maybe @wolfgangwalther |
Contributor
That commit LGTM. |
philiptaron
approved these changes
Jan 15, 2026
Contributor
|
I reviewed these manually. Always scary making changes to FODs because the rebuild count won't tell you if you get it wrong. But I didn't see a single error, so I think this will work. |
Member
|
@philiptaron I'm curious about how this works!
I'm having trouble parsing this -- are you saying the hash generated by eval for the FOD doesn't change but the drv itself might still not build due to the change? |
Contributor
|
Yep. https://fzakaria.com/2025/10/29/nix-derivation-madness for more. You can only validate FOD changes by rebuilding them with |
Member
|
Wow that is crazy! TIL srcbot was supposed to catch this but based on this blogpost it looks like srcbot won't catch FODs that have something within them that changed that isn't in $out. I wonder if there's a way to iterate all FODs and look more deeply at their attributes to catch stuff like this. |
Member
|
Looking back at the code -- srcbot diffs FODs based on drvPath not outPath. I think the drvPath changes even for FODs that changed but don't touch $out. Of course this is not perfect, but I'd claim it's 90%. If the intermediate attribute (that is a FOD) (.src, .fetchedMavenDeps, etc) of a drv is itself not actually a FOD, then srcbot doesn't work right now. It will just blindly rebuild the attribute anyway. And similarly since I hardcode the common FOD names right now, it might also miss FODs that are not these common names. But this hits the common case that happens most of the time cuz most of the time the FODs use standardized names. I think we might actually be able to do something more clever here using |
Member
Just as an example, the outPath is the same but the drvPath is different (thankfully haha) |
Contributor
Author
|
I did check for each fetcher, that deleting the hash and letting nix build propose a new one, did not change the hash. The only exception was with fetchFromSourceHut, as explained in a prior comment, and which was addressed. And, I reviewed the entire diff manually after each rebase. That and @DieracDelta 's script give me good confidence that nothing will get any more broken than it already is. Also we now have a list of failing packages to work on! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Things done
rev = "refs/tags/withtag = "and removed string interpolations on tags where unnecessary.maintainers/scripts/update-octave-packagesaccordingly.I just left
postgre-sqluntouched because there is a comment stating that the use of git revs is on purpose.I didn't try to build all affected packages, but I did build 1 package for each 'type' of git provider.
Changes were split into individual commits for ease of review. Let me know if they should be squashed or on the contrary, split into multiple PRs.
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.