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

Use libarchive for all decompression (except brotli) #3333

Merged
merged 3 commits into from
Apr 15, 2021

Conversation

yorickvP
Copy link
Contributor

@yorickvP yorickvP commented Jan 27, 2020

Libarchive, introduced in an earlier PR, can be used for arbitrary compression and decompression, as well as tar files. This handles support for (de)compression with gzip, bzip2, lzip, xz, lzma, zstd or compress. The main new feature here is zstd support.

The thing conspicuously missing here is brotli. This is because brotli does not have a magic header, so it cannot be automatically detected. We will still have to work around this, all other tests pass.

Fixes #2255.

Todo
cc:

@tomberek @puckipedia

@tomberek
Copy link
Contributor

Exciting change. How are you building and testing zstd? I presume this will come with some release-common.nix and configure.ac changes.

@yorickvP
Copy link
Contributor Author

libarchive already links statically with zstd (and falls back to zstd command line binary when it's not). No further changes are required. It might be good to test zstd specifically, if we plan on using it in global binary caches.

@tomberek
Copy link
Contributor

Yes, while testing zstd it seems to fallback and then not find the command line binary (I specifically uninstalled it to check this).

@domenkozar
Copy link
Member

@domenkozar
Copy link
Member

@yorickvP what's missing here?

@domenkozar
Copy link
Member

Talking to @yorickvP this needs brolti fallback to the old behavior since there's no magic byte header.

@yorickvP yorickvP marked this pull request as ready for review March 10, 2021 21:47
@yorickvP
Copy link
Contributor Author

Okay, this code now passes all tests.

@domenkozar
Copy link
Member

Once serokell#4 is merged, this PR will have tests and can be reviewed.

@domenkozar
Copy link
Member

domenkozar commented Apr 13, 2021

@edolstra I'd really like this merged, could you review this PR?

@edolstra edolstra merged commit 6fb7582 into NixOS:master Apr 15, 2021
@edolstra
Copy link
Member

Thanks, merged!

@domenkozar
Copy link
Member

Wohooo 🥳

@dtzWill
Copy link
Member

dtzWill commented Sep 1, 2021

I'm looking at my PR for adding compression level option-- which is mentioned as possibly on the TODO, thank you -- and was wondering if you happen to have looked into that at all? I've yet to read through these changes or look into how libarchive works yet so not sure if there were issues there or just wasn't a priority / mess plumbing things or whatever :).

Thanks regardless, have a good one!

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/switch-cache-nixos-org-to-zstd-to-fix-slow-nixos-updates-nix-downloads/23961/1

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/switch-cache-nixos-org-to-zstd-to-fix-slow-nixos-updates-nix-downloads/23961/6

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/switch-cache-nixos-org-to-zstd-to-fix-slow-nixos-updates-nix-downloads/23961/48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support zstd compression for binary caches
6 participants