Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: release-24.11
Choose a base branch
from

Conversation

pbsds
Copy link
Member

@pbsds pbsds commented Jan 12, 2025

This is a redo of #368177 on release-24.11, not because of "correctness" but to alleviate automatic backport conflicts.
The script was adapted (see data1c) to only migrate packages that already use tag on master:

#!/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 --out-path --drv-path --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)"
        data1c="$(nix eval --impure  --expr 'with import ../../master {}; '"$attrpath"'.src.tag != null' --json)"
        test -n "$data1a" || continue
        test -n "$data1b" || continue
        test "$data1c" = "true" || 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


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

I went through the nix-env diff and fixed all eval failures in a separate commit.
The remaining diff to packages.json after my fixups may be found here:

https://gist.github.com/pbsds/465391bdf3edf34ad5cd73cbe4d92260#file-packages-diff

Also included are the nix-diff output and a diff -r of the one changed storepath.

I've looked into various fixups of the original treewide to see if they apply here:

  • 5bd568b is not in release-24.11
  • 8474e7a does have the v prefix on release-24.11 (image)
  • 3e01e22 was fixed in my fixup commit
  • 8defe4c was also fixed in my fixup commit
  • 4784bc2 was also fixed in my fixup commit

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:
  • 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.

@pbsds pbsds changed the title treewide: migrate fetchgit rev = "refs/tags/..." to tag [release-24.11] treewide: migrate fetchgit rev = "refs/tags/..." to tag Jan 12, 2025
@github-actions github-actions bot added 6.topic: python 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: vim 6.topic: ocaml 6.topic: mate The MATE Desktop Environment 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: php labels Jan 12, 2025
@pbsds pbsds force-pushed the migrate-git-tag-1736602973 branch from 1d06ad9 to 04a2e85 Compare January 12, 2025 18:38
@pbsds
Copy link
Member Author

pbsds commented Jan 12, 2025

This is my manual diff when fixing merge conflicts

https://gist.github.com/pbsds/6445e4c28eba8ee710d3c4a5b94bc6ba

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is just codeowners noping out of requesting too many reviews.

I spot-chekced and it LGTM.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jan 12, 2025

Can we stop backporting massive treewide changes? I am not looking forward to all the merge conflicts this will create on my stable fork that I then have to fix.

Also as previously demonstrated this caused a bunch of regressions and I am pretty sure that others will happen on stable and for someone not super deep into nixpkgs they are not easily fixable.

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 12, 2025
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 12, 2025
@Atemu
Copy link
Member

Atemu commented Jan 12, 2025

I am not looking forward to all the merge conflicts this will create on my stable fork that I then have to fix.

I don't think internal code organisation is part of the stable interface. I also don't think we should care about other people's forks to begin with; whether stable channel or not.

Additionally the reason why this is even backported is because this would otherwise be a diff to unstable where it matters most (version info), making backports a pain for everyone because cherry-picks cause conflicts. If this wasn't the case, I wouldn't advocate for backporting this either.

as previously demonstrated this caused a bunch of regressions

All things considered, those were pretty minor, relied on questionable interfaces and were trivial to fix. Finding those is also part of the reason we have the unstable channel. All the ones found so far are also included here AFAICT.

I am pretty sure that others will happen on stable and for someone not super deep into nixpkgs they are not easily fixable.

Those are going to be extremely fringe at this point. I'd like to avoid those too for stable but the question is always: at what cost?

I find the cost of not having automated backports for thousands of packages for the next 5 months too high to pay and would rather risk breaking extremely fringe usages of things that (IMHO) never should have been depended upon.

You could of course convince me by volunteering to do the backports yourself and resolve conflicts manually but I doubt you want that.

@Bot-wxt1221
Copy link
Member

As I see, It prevent backporting bot working properly many times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: mate The MATE Desktop Environment 6.topic: ocaml 6.topic: php 6.topic: python 6.topic: vim 9.needs: community feedback 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants