treewide: migrate fetchgit rev = "refs/tags/..." to tag

[pbsds]

This is a automated migration from rev = "refs/tags/..." to tag = ... added in #355973.
I based the script below on #336172 which checks that the derivations are unchanged.
This PR commit-spams to simplify rebasing, squash-merge it if you dislike this.

Done with:

#!/usr/bin/env nix-shell
#!nix-shell --pure -i bash -p ripgrep sd jq git lix delta

export NIXPKGS_ALLOW_UNFREE=1
export NIXPKGS_ALLOW_INSECURE=1
export NIXPKGS_ALLOW_BROKEN=1

git restore .

test -s packages.json || ( set -x;
  time nix-env --extra-experimental-features no-url-literals --option system x86_64-linux -f ./. -qaP --json --meta --show-trace --no-allow-import-from-derivation > packages.json
)

jq <packages.json 'to_entries[] | select(.value.meta.position==null|not) | "\(.key)\t\(.value.meta.position)"' -r |
    sed -e "s#\t$(realpath .)/#\t#" |
    sed -e 's#:\([0-9]*\)$#\t\1#' |
    tr '0123456789' '9876543210' | sort | tr '0123456789' '9876543210' |
    grep . |
    grep -iv haskell |
    grep -iv /top-level/ |
    grep -iv chicken |
    grep -iv build |
    grep -E '/(package|default)\.nix' |
    while read attrpath fname col; do
        grep -qE "(fetchgit|fetchFromGitHub)" "$fname" || continue
        grep -qF 'rev = "refs/tags/' "$fname" || continue

        echo "$attrpath"

        data1a="$(nix eval --impure  --expr 'with import ./. {}; { inherit ('"$attrpath"') meta drvPath; _outPath='"$attrpath"'.outPath; src = { inherit ('"$attrpath"'.src) drvPath; _outPath='"$attrpath"'.src.outPath; }; }' --json)"
        data1b="$(nix eval --impure  --expr 'with import ./. {}; { src = { inherit ('"$attrpath"'.src) rev tag; }; }' --json)"
        test -n "$data1a" || continue
        test -n "$data1b" || continue

        # test if src builds
        #if ! nix-build . -A "$attrpath".src ; then
        #    continue # /shrug
        #fi

        # extract tag
        tag="$(grep -E '\brev = "refs/tags/(.*)";' "$fname" -o | cut -d'"' -f2 | cut -d/ -f3-)"

        test -n "$tag" || continue
        [[ $(wc -l <<<"$tag") -eq 1 ]] || continue

        (set -x
            # convert rev -> tag
            sd '\brev = "refs/tags/(.*)";'  'tag = "$1";'  "$fname"
            sd '\btag = "\$\{(.*)\}";'      'tag = $1;'    "$fname"

            # fix meta changelog (YAGNI)
            sd -F '/releases/tag/${self.src.rev}'        '/releases/tag/'"$tag"  "$fname"
            sd -F '/releases/tag/${src.rev}'             '/releases/tag/'"$tag"  "$fname"
            sd -F '/releases/tag/${finalAttrs.src.rev}'  '/releases/tag/'"$tag"  "$fname"

            # undo https://github.com/NixOS/nixpkgs/pull/338301
            sd -F '/releases/tag/${lib.removePrefix "refs/tags/" src.rev}'             '/releases/tag/'"$tag"  "$fname"
            sd -F '/releases/tag/${lib.removePrefix "refs/tags/" finalAttrs.src.rev}'  '/releases/tag/'"$tag"  "$fname"
            sd -F '/releases/tag/${lib.removePrefix "refs/tags/" self.src.rev}'        '/releases/tag/'"$tag"  "$fname"
        )

        data2a="$(nix eval --impure  --expr 'with import ./. {}; { inherit ('"$attrpath"') meta drvPath; _outPath='"$attrpath"'.outPath; src = { inherit ('"$attrpath"'.src) drvPath; _outPath='"$attrpath"'.src.outPath; }; }' --json)"
        data2b="$(nix eval --impure  --expr 'with import ./. {}; { src = { inherit ('"$attrpath"'.src) rev tag; }; }' --json)"

        if ! test "$data1a" = "$data2a"; then
            >&2 echo "meta or {,src.}{drvPath,outPath} changed"
            git diff | delta --paging=never
            diff -u <(echo $data1a) <(echo $data2a) | delta --paging=never
            (set -x; git restore "$fname")
            continue
        fi

        if test "$data1b" = "$data2b"; then
            >&2 echo "src.tag didn't change"
            diff -u <(echo $data1b) <(echo $data2b) | delta --paging=never
            git diff | delta --paging=never
            (set -x; git restore "$fname")
            continue
        fi

        # test if src build, this check the FOD hash
        # not needed since outPath and drvPath are unchanged
        #if ! nix-build . -A "$attrpath".src --check ; then
        #    >&2 echo "src doesn't build"
        #    git diff | delta --paging=never
        #    (set -x; git restore "$fname")
        #    continue
        #fi

        # stage
        (set -x;
            git add "$fname"
            # git commit -m "$attrpath: migrate fetchgit from rev to tag"
        )
    done

i ran it for a while then terminated it. I want more eyes on the script before we commit to a mega PR. should be 0 rebuilds

Should we address #367739 before going forward with this?

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[GaetanLepage]

In a separate PR/thread, @Atemu highlighted that relying on src.* attributes was inherently a bad idea. We should limit ourselves to using ${version} there.

[pbsds]

This PR only migrates drv.meta.changelog to use src.tag, which is lazily evaluated and thus not a problem for most override cases. If src is overridden to use a rev then the url would be incorrect anyway

[Atemu]

If having the commits separate is easier to handle for you that's fine but I'd prefer one treewide commit in the end and GH struggles quite a lot once you're past a dozen commits or so.

Could you commit the script itself to nixpkgs too for future reference? The code itself will be dead right after this PR because there should be no instances left but parts of this could be re-used for similar refactors. I don't know if there's a collection point for these but if there isn't, there should be one. I think there was an issue about reproducible treewides already.

I've had a look at it generally LGTM apart from depending on src.tag as @GaetanLepage mentioned.
It doesn't look like there's too many instances but the correct/better thing to do is to just depend on ${version} or define a new tag reference to use in both places.

        # test if src build, this check the FOD hash
        # not needed since the drvPath is unchanged
        #if ! nix-build . -A "$attrpath".src --check ; then
        #    >&2 echo "src doesn't build"
        #    git diff | delta --paging=never
        #    (set -x; git restore "$fname")
        #    continue
        #fi

Why is this not needed? Doesn't a FOD only detectably change if name or hash are changed?

If src is overridden to use a rev then the url would be incorrect anyway

It'd be incorrect but at least eval. Missing attr errors are impossible to catch IIRC which makes them super annoying to deal with.

[pbsds]

If having the commits separate is easier to handle for you that's fine but I'd prefer one treewide commit in the end and GH struggles quite a lot once you're past a dozen commits or so.

👍

Could you commit the script itself to nixpkgs too for future reference? [...]

I'm tracking a lot of the migration scripts i've written in #346453 and plan to put this one there as well if it gets merged. I don't consider this script rigorous enough to commit to /maintainers/scripts, since it relies on not commiting changes that don't work rather than ensuring every change attempt is successfull. As such it mainly migrates low-hanging fruit.

I've had a look at it generally LGTM apart from depending on src.tag as @GaetanLepage mentioned. It doesn't look like there's too many instances but the correct/better thing to do is to just depend on ${version} or define a new tag reference to use in both places.

This change is essentially undoing #338301 in favor of src.tag. src.rev would already fail if src is overriden with a local path, something i frequently do when writing patches.

If we want to migrate away from src.* then a lint in https://github.com/nix-community/nixpkgs-lint would make sense. It should IMO be run in CI in a "don't introduce any new lint errors" mode.

Why is this not needed? Doesn't a FOD only detectably change if name or hash are changed?

What i've learned from https://noogle.dev/f/pkgs/invalidateFetcherByDrvHash is that even if the outPath of FODs are fully determined by name and outputHash, the drvPath is still free to change. drvPath is essentially a hash of the build steps and inputs.
I should however also ensure that outPath is unchanged. I'll add that and re-run it for about the same amout of time. 👍

It'd be incorrect but at least eval. Missing attr errors are impossible to catch IIRC which makes them super annoying to deal with.

Would a migration to something like changelog = if finalAttrs.src?tag then ... else null; make sense, or should we go through the about 60+ cases manually in this PR?

[Atemu]

Would a migration to something like changelog = if finalAttrs.src?tag then ... else null; make sense

I don't think so. That strikes me as overly defensive and over the top.

I'd advocate to just use the same value that is put in tag. Either duplicate the code (YAGNI), make a let-binding or introduce a new mkDerivation arg and reference it via finalAttrs. (I personally am not convinced about making finalAttrs bear any significant load.)

[Atemu]

Commit title suggestion:

treewide: migrate fetchgit `rev = "refs/tags/..."` to `tag`

[pbsds]

It struck me while I was out of coverage that the YAGNI approach is scriptable. The script in the OP is updated

[pbsds]

A new concern just struck me: the rev = ...; line usually sits right next to the FOD hashes, which makes this migration likely to cause merge conflicts with open bump PRs and bumps in staging. This could wreak havoc on the periodic master->staging{,-next} merges if we do a complete treewide

[philiptaron]

I looked at all the changes so far; I haven't checked to see if there are no finalAttrs.src that were in the files changed yet.

[philiptaron]

This could wreak havoc on the periodic master->staging{,-next} merges if we do a complete treewide.

No, I don't think so. The change will go from master to staging once, and it'll be obvious to a human what to do.

[Atemu]

I've had a glance at the modified files and it generally LGTM.

Given that CI doesn't complain about any meta issues and that the script verifies that FODs have not have changed in any way, I'd say we're good to go.

To ease backports, could you run the script/do your manual fixups on stable too?
24.11-only would be fine given that 24.05 will be EOL in a few days. I'd like a week of delay though in the off chance we do find anything anything wrong with this which should be a good trade-off w.r.t. breaking backports for the affected packages vs. seeing issues crop up.

I'm going to ask in the staging matrix channel on how they'd like to have this treewide handled as it does have potential for at least a handful of conflicts.

[pbsds]

I added an extra check and ran it over all of nixpkgs overnight resulting in additional 4191 files affected. Should we fragment it over multiple PRs or make this one a mega PR an try to race the merge conflict?

[Atemu]

I personally think that, if it's coordinated, it's best to have one large PR.

If this is fine for the staging folks and CI passes, we can merge immediately as far as I'm concerned.

[GaetanLepage]

I personally think that, if it's coordinated, it's best to have one large PR.

I think this is right too.

If this is fine for the staging folks and CI passes, we can merge immediately as far as I'm concerned.

cc @vcunat @infinisil @K900 would you be fine with a big one-shot PR ?

[pbsds]

This should enable even more migrations using this script

• fetchgit{,hub,lab}: passthru rev based on tag #369506

[vcunat]

To be clear, from perspective of staging* I think it's OK to merge basically anytime into master. (and ideally resolve conflicts in staging* immediately / rerun script or something) Some conflicts will be there surely, as I see this line is directly next to the hash line, so any updates... and that includes open PRs. But none of that sounds bad to me. (I think you got a similar answer on the staging chat immediately, but for the record.)

[pbsds]

I ran the revised script overnight (added to OP), which now also ensures that only one line matches rev = "refs/tags/(.*)"; in the each file.
#369506 resulted in 600 additional migrations.
I then rebased it on master and fixed 41 conflicting files, some of which already got migrated.
Here is the part of the PR I made manually
I then fixed 7 more conflicts.

I'll let this sit here until at least 00:00 UTC+1, fix conflicts, push, wait for eval, and self-merge.
I'll then cherry-pick the change onto staging.

[Atemu]

It really is.

I think a merge to staging-next (and then that into staging) would be the appropriate action, not cherry-pick. Master is merged that way periodically anyways.

[Atemu]

You should perhaps also check whether staging-next/staging contain any new references and fix those up.

[pbsds]

a local merge from master to staging-next was conflict free, should i leave it to the automation?

[Atemu]

Sure, why not. Is the same true for staging?

[pbsds]

No, 25 conflicting files. should i merge from master or cherry-pick? I've yet to learn how conflicts between these branches are usually resolved.

[pbsds]

I've read through the documentation a couple of times, and i think a manual staging-next->staging is the way to go

[Atemu]

Yes, master -> staging-next -> staging.

[pbsds]

huh, perfect timing https://github.com/NixOS/nixpkgs/actions/runs/12605651116

[Atemu]

Have you checked for any remaining instances of refs/tags/ on staging/staging-next that weren't present on master?

This also needs to be backported to 24.11 to make backports apply cleanly. As mentioned previously though, I think we should wait a few days~a week to see whether any potential issues crop up.

[pbsds]

Have you checked for any remaining instances of refs/tags/ on staging/staging-next that weren't present on master?

I assume more will pop up over time, so a couple of repeats would make sense. They will luckily be nowhere near the same scale

This also needs to be backported to 24.11 to make backports apply cleanly. As mentioned previously though, I think we should wait a few days~a week to see whether any potential issues crop up.

👍

[trofi]

0cd04d3 "treewide: migrate fetchgit rev = "refs/tags/..." to tag``" broke at least the diffoscopeeval onmaster as:

$ nix build --no-link -f. diffoscope
error:
       … while evaluating the attribute 'drvPath'
         at /home/slyfox/dev/git/nixpkgs-staging/lib/customisation.nix:416:7:
          415|     // {
          416|       drvPath =
             |       ^
          417|         assert condition;

       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:34:12:
           33|
           34|   strict = derivationStrict drvAttrs;
             |            ^
           35|

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: fetchFromGitHub requires one of either `rev` or `tag` to be provided (not both).

(diffoscope overrides tlsh version with a rev =)

[trofi]

Proposed diffoscope eval fix as:

• diffoscope: fix the eval #370795

[Bot-wxt1221]

@pbsds Could we backport to 24.11 now. It really breaks a lot of backport.

[mweinelt]

This broke a bunch of packages, that were quietly fixed, so I'm not super confident that a backport will be including all those fixes.

[pbsds]

I tried cherrypicking the treewide, but 1000+ conflicts made me instead resort to running the script again, it's going to take a while.

I adapted the script to only migrate the ones that now use tag on master. I also plan on comparing a full nix-env ... --out-path ... json from before and after the treewide to ensure no eval failures are introduced. Why CI doesn't tell us about those i don't know

[mweinelt]

These are the two issues I personally came across.

• 3e01e22
• 8474e7a

[pbsds]

• [release-24.11] treewide: migrate fetchgit rev = "refs/tags/..." to tag #373256