Skip to content

file: patch to fix misclassification of some zip files#402115

Merged
doronbehar merged 1 commit intoNixOS:masterfrom
txkyel:fix-file-zip-misclassification
Apr 27, 2025
Merged

file: patch to fix misclassification of some zip files#402115
doronbehar merged 1 commit intoNixOS:masterfrom
txkyel:fix-file-zip-misclassification

Conversation

@txkyel
Copy link
Contributor

@txkyel txkyel commented Apr 26, 2025

Apply upstream patch to fix misclassification of some zip archives as data.

Upstream commit for fix: file/file@60b2032.

Upstream bug report: https://bugs.astron.com/view.php?id=571.

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.

@NixOSInfra NixOSInfra added the 12.first-time contribution This PR is the author's first one; please be gentle! label Apr 26, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels Apr 26, 2025
@nix-owners nix-owners bot requested a review from doronbehar April 26, 2025 20:16
@doronbehar
Copy link
Contributor

Hmm I wonder if non critical patches like this should be included in Nixpkgs. Is this needed for a specific package in Nixpkgs that depends directly upon file? If not, I tend to advise you to applying it in your private OS configuration..

self: super: {
  file-patched = super.file.overrideAttrs(old: {
    patches = old.patches or [] ++ [
      (super.fetchpatch {
        url = "...";
        hash = "...";
      })
    ];
  });
}

And naturally use file-patched in your systemPackages.. Note that this allows you to also use fetchpatch.

@dtomvan
Copy link
Contributor

dtomvan commented Apr 27, 2025

Won't this just end up included in the next release of file? If so, why patch it right now for nix? Granted, releases don't come often with this package.

To be fair, both debian and arch include this patch.

But then again, alpine and fedora don't...

@txkyel
Copy link
Contributor Author

txkyel commented Apr 27, 2025

Is this needed for a specific package in Nixpkgs that depends directly upon file?

This is required for yazi which depends on file to classify zip files for extraction or previews. I had found this upstream ticket mentioning applying a patch to file or to downgrade to 5.45 sxyazi/yazi#2536 (comment). If there's a method to apply the patch for file or downgrade to 5.45 without having to rebuild a bunch of its dependencies I can apply it to my private configuration.

Won't this just end up included in the next release of file? If so, why patch it right now for nix? Granted, releases don't come often with this package.

I'm hoping to patch it precisely because releases don't come often.

The work arounds I've found for yazi so far are inelegant or require me to recompile a bunch of peripheral packages which my PC cannot seem to handle. I was hoping to apply this patch sicne the upstream ticket mentioned arch included this patch, but I can understand the concern of permitting non critical patches and will close this PR if need be.

@doronbehar
Copy link
Contributor

Here's an overlay you can start using right now:

self: super: {
  yazi = super.yazi.override {
    yazi-unwrapped = super.yazi-unwrapped.override {
      file = super.file.overrideAttrs(old: {
        patches = old.patches or [] ++ [
          (super.fetchpatch {
            url = "...";
            hash = "...";
          })
        ];
      });
    };
  };
}

Untested, but should work. TBH Now that I read that upstream comment (sxyazi/yazi#2536 (comment)) mentioning Arch Linux has included this patch, I consider maybe we should merge this PR never the less. Also, trying to weight this myself, the bug report seems pretty severe, although it is classified as a 'minor' bug. Maybe @txkyel you could nudge upstream to release a new version? By writing a comment in that issue.

@doronbehar
Copy link
Contributor

On a 2nd thought, I see that upstream has fixed that issue a long time ago, and hasn't released a new version, so I doubt whether such a comment will push him to actually make a release. Let's merge this in the meantime.

@doronbehar doronbehar merged commit 0be1fa1 into NixOS:master Apr 27, 2025
41 of 44 checks passed
@txkyel txkyel deleted the fix-file-zip-misclassification branch April 27, 2025 14:24
@dtomvan
Copy link
Contributor

dtomvan commented Apr 27, 2025

Uhm, I think this should've gone into staging... this rebuilds almost 75K packages

@dtomvan dtomvan mentioned this pull request Apr 27, 2025
13 tasks
@txkyel
Copy link
Contributor Author

txkyel commented Apr 27, 2025 via email

@doronbehar
Copy link
Contributor

doronbehar commented Apr 27, 2025 via email

@doronbehar
Copy link
Contributor

doronbehar commented Apr 27, 2025

A correct PR:

BTW, the following Bash script excerpt was written by a LLM for me, and it helps me avoid creating PRs targeting the wrong branches. It works by inferring the nixpkgs branch from which the commits of the 'feature branch' descended:

feature_branch="$(git rev-parse --abbrev-ref HEAD)"
# Arbitrarily large number
_best_distance=999999999

# Check a list of possible parent branches
for parent in \
    $(git br | grep release | sort | head -1) \
    $(git br | grep staging | sort | head -1) \
    $(git br | grep staging-next | sort | head -1) \
    staging-next \
    staging \
    master; do
    if git show-ref --verify --quiet "refs/heads/$parent"; then
        ancestor=$(git merge-base "$feature_branch" "$parent")
        distance=$(git rev-list --count "$ancestor..$feature_branch")

        if [ "$distance" -lt "$_best_distance" ]; then
            _best_distance=$distance
            best_parent=$parent
        fi
    fi
done

if [ -n "$best_parent" ] && [ "$_best_distance" -lt 1000 ]; then
    echo "Best guess for parent branch: $best_parent"
else
    # TODO: Maybe don't exit if --base was used in "$@"?
    echo "No suitable parent branch found."
    exit 2
fi

I use it with:

gh pr create --base "$best_parent"

To make sure I use the right base.

@dtomvan
Copy link
Contributor

dtomvan commented Apr 27, 2025

@doronbehar thank you for addressing it quickly. Everybody makes mistakes, but I'm afraid all calls to nixpkgs-review pr came to a halt for a second 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants