Skip to content

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

Closed
doronbehar wants to merge 1 commit intoNixOS:stagingfrom
doronbehar:pkg/file-zip-patch
Closed

file: patch to fix misclassification of some zip files#402318
doronbehar wants to merge 1 commit intoNixOS:stagingfrom
doronbehar:pkg/file-zip-patch

Conversation

@doronbehar
Copy link
Contributor

Description of changes

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)
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

@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 27, 2025
Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

IIRC, I had some zip files not identified as such a while ago. I also tried this patch, but somehow it didn't fix my issue. I will try again tomorrow, but there might be more wrong upstream.

@doronbehar
Copy link
Contributor Author

Thanks for reviewing @wolfgangwalther . If you could confirm that this patch fixes an issue you've experienced before it'd be great. If not, I guess we'll merge this in a day or two.

@wolfgangwalther
Copy link
Contributor

I tested and the zip files that I have are still problematic with this. They are returned as mimetype application/octet-stream. The only way to fix this for me, is to downgrade to 5.45. I also tried with latest master of file/file, but that doesn't change anything. So this is an open issue upstream, which I will try to report (not sure how, because I'm not sure whether I can reproduce it with a fake zip file).

So while this is not an issue with this PR, the question is whether the patch here will be enough - or you'd eventually run into the same problem that I have, @txkyel. Not sure whether downgrading would be an option, either.

@wolfgangwalther
Copy link
Contributor

It seems the problem that I am hitting has been reported upstream already: https://bugs.astron.com/view.php?id=622

TLDR: It depends on how you call file / libmagic, whether zip files are detected correctly.

Simple example on this branch:

This one returns "data":

cat any.zip | file -

while this one works and returns zip file info:

file any.zip

@txkyel you wrote the following in the other PR:

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.

Did you only take this info from the PR or did you test yazi with this patch to file applied? Does it recognize zip files again?

I'm not sure whether we should proceed with this rather incomplete fix at this time.

@txkyel
Copy link
Contributor

txkyel commented Apr 28, 2025

Did you only take this info from the PR or did you test yazi with this patch to file applied? Does it recognize zip files again?

I haven't tested yazi with the patch applied to file. If you know how I could build yazi without having to rebuild all the packages depending on file let me know, otherwise my system is unable to do so. I assume however that this patch will work for yazi since the files are passed to file as an arg.

@txkyel
Copy link
Contributor

txkyel commented Apr 28, 2025

A bit hacky of a work around but I was able to get yazi to use the patched file using YAZI_FILE_ONE (a variable made for windows). I can confirm that this patch will fix zip recognition for yazi.

nixpkgs.overlays = [
  (self: super: {
    file-patched = super.file.overrideAttrs (old: {
      patches = old.patches or [ ] ++ [
        (super.fetchpatch {
          url = "https://raw.githubusercontent.com/NixOS/nixpkgs/ae7f8403e0552fababb2110bea2e436283230282/pkgs/tools/misc/file/PR-571-jschleus-Some-zip-files-are-misclassified-as-data.patch";
          hash = "sha256-RVrL7P0Riu3hiLsVvGoGNsJctDplxaF7t+wW/zezdUg=";
        })
      ];
    });
  })
];
environment.systemPackages = [ pkgs.file-patched ];
environment.sessionVariables = {
  YAZI_FILE_ONE = "${lib.getExe pkgs.file-patched}";
};

@doronbehar
Copy link
Contributor Author

It seems the problem that I am hitting has been reported upstream already: bugs.astron.com/view.php?id=622

Thanks @wolfgangwalther for finding that issue. In light of that, I am marking this PR as a draft, and perhaps in the near future we'll see a fix for the rest of the issue at upstream soon, which will can be added to this PR as well. You see it'll take time for the staging -> staging-next -> master cycle anyway, meaning you'll have to live with your local overlays a long time anyway.

@doronbehar doronbehar marked this pull request as draft April 29, 2025 09:33
@midchildan
Copy link
Member

I wonder if it makes sense to revert file to 5.45. Misclassifying zip files is a major regression, so it might be worth it.

@dtomvan
Copy link
Contributor

dtomvan commented May 14, 2025

I suppose it's better to revert than to try to monkeypatch it out temporarily, with the fix not even being as reliable. A little earlier this thread:

I tested and the zip files that I have are still problematic with this

Especially with the new release coming up, I think it's better to go back to the more predictable 5.45:

$ cat OneDrive_1_3-7-2025.zip | file -
/dev/stdin: data
$ file OneDrive_1_3-7-2025.zip
OneDrive_1_3-7-2025.zip: data
$ file --version
file-5.46
magic file from /nix/store/2lnhqdy9zqbzg525hwab9cwfx9kz1cgh-file-5.46/share/misc/magic
$ nix shell github:nixos/nixpkgs/882842d2a908700540d206baa79efb922ac1c33d#file # https://lazamar.co.uk/nix-versions/?package=file&channel=nixpkgs-unstable
$ file OneDrive_1_3-7-2025.zip
OneDrive_1_3-7-2025.zip: Zip archive data, at least v2.0 to extract, compression method=store
$ cat OneDrive_1_3-7-2025.zip | file -
/dev/stdin: Zip archive data, at least v2.0 to extract, compression method=store
$ gh pr checkout 402318
$ nom-build -E '(import ./. {}).file.override { inherit (import <nixpkgs> {}) stdenv zlib updateAutotoolsGnuConfigScriptsHook; }' # cheat a little bit
$ ./result/bin/file ~/Downloads/OneDrive_1_3-7-2025.zip
/home/tomvd/Downloads/OneDrive_1_3-7-2025.zip: Zip archive data, at least v2.0 to extract, compression method=store
$ cat ~/Downloads/OneDrive_1_3-7-2025.zip | ./result/bin/file -
/dev/stdin: data

@doronbehar
Copy link
Contributor Author

You convinced me @dtomvan :

I'll update this PR accordingly after the above will be merged.

doronbehar added a commit to doronbehar/nixpkgs that referenced this pull request May 16, 2025
This reverts commit 75d2d3c, after
multiple complains best summarized here:

NixOS#402318 (comment)
nixpkgs-ci bot pushed a commit that referenced this pull request May 16, 2025
This reverts commit 75d2d3c, after
multiple complains best summarized here:

#402318 (comment)
(cherry picked from commit 7377f9e)
@doronbehar doronbehar closed this May 17, 2025
@doronbehar doronbehar deleted the pkg/file-zip-patch branch February 10, 2026 08:15
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants