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

Add support for bzip3 #522

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add support for bzip3 #522

wants to merge 9 commits into from

Conversation

freijon
Copy link

@freijon freijon commented Sep 20, 2023

Closes #398
Signed-off-by: Jonas Frei [email protected]

This PR adds support for the bzip3 format. I tested it to the best of my knowledge and it seemed to work fine. Unlike the other formats, one can not modify the compression level directly. Bzip3 works with block sizes which can be varied. I chose a block size of 5MB which yielded good results with most examples I tested.

⚠️ Things to consider before merging

  • The project is quite young (May 2022)
  • While their name is bzip3, they have no affiliation with the original bz/bz2 and I don't know if they have the permission of the original author of bz/bz2 to use this name. Nevertheless, a lot of popular Linux distros added the library to their repository.
  • The Rust crate bzip3-rs uses the unstable try block and may cause complications at build time

P.S. I'm a Rust noob and did this primarily as a proof of concept. Let me know what you think about this.

@marcospb19
Copy link
Member

marcospb19 commented Sep 20, 2023

Good job!

I have a branch with this implemented as an experiment, sort of, look at main...bzip3, our code looks very similar.

I've been busy so I'm really glad to have your help.

Previous Context

I encountered two blockers:

  1. Licensing doubts addressed in Adding support for bzip3 in Ouch bczhc/bzip3-rs#1.
  2. Ouch tests failing, hopefully 🤞 solved after this PR Add test to check if chained encoders finishes bczhc/bzip3-rs#4, as the maintainer quickly followed up with a fix.

New Context

The Rust crate bzip3-rs uses the unstable try block and may cause complications at build time

Seems like you just found a new blocker:

  1. Our releases are built in stable Rust, I might have to kindly ask bzip3-rs's maintainer to remove the try_blocks used in the build script. (PRs open: Make the build script run in stable Rust - use closure bczhc/bzip3-rs#6 or Make the build script run in stable Rust - use option methods bczhc/bzip3-rs#7).

To temporarly ignore this blocker (while we wait for a bzip3-rs fix release), make the following change in Cargo.toml in order to make CI pass:

- bzip3 = "0.8.1"
+ bzip3 = { git = "https://github.com/marcospb19/bzip3-rs", branch = "make-build-script-run-in-stable-1" }

That's my branch with the fix.

Copy link
Member

@marcospb19 marcospb19 left a comment

Choose a reason for hiding this comment

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

Your code looks better than mine!

It's just missing the test code (it's basically one line)

And another requested change,

src/commands/compress.rs Outdated Show resolved Hide resolved
src/commands/compress.rs Outdated Show resolved Hide resolved
@marcospb19
Copy link
Member

Now the CI is failing to build bzip3-rs because it doesn't have bzip3 installed.

I don't know how to solve this for Ubuntu, MacOS and Windows, do we need to clone the repo and build it from scratch? 😭

Perhaps installing nix as a step and using our nix-shell? cc @figsoda again.

@figsoda
Copy link
Member

figsoda commented Sep 21, 2023

Not sure what's the best way to do it on windows, It's available for brew so macos should be easy to take care of, it is only available on ubuntu 23.04+ so we might have to use brew for linux as well

Cargo.toml Outdated Show resolved Hide resolved
@marcospb19
Copy link
Member

I force-pushed a rebase to solve conflicts, and temporarily disabled some CI targets so that we can focus on the code problem for now.

Copy link
Member

@marcospb19 marcospb19 left a comment

Choose a reason for hiding this comment

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

Almost done, there is just one last problem:

Comment on lines 20 to 22
- target: aarch64-unknown-linux-gnu
os: ubuntu-latest
no-zstd-thin: true
# - target: aarch64-unknown-linux-gnu
# os: ubuntu-latest
# no-zstd-thin: true
Copy link
Member

Choose a reason for hiding this comment

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

CI is passing because I temporarily disabled some targets that were giving build errors.

We need to re-enable these targets, otherwise, we'll end up dropping support for them (should we?).

Someone needs to go and uncomment these targets at.github/workflows/build-and-test.yml, and figure out how to make them build successfully, @freijon can you try doing that?

Copy link
Author

@freijon freijon Oct 23, 2023

Choose a reason for hiding this comment

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

I don't think I have the permission to do this? Or can I edit this from my fork?

EDIT: I uncommented the first arch aarch64-unknown-linux-gnu and the error appears to be:

error: failed to run custom build command for libbzip3-sys v0.3.3+1.3.2
...
This crate only supports libclang 3.5 and later.

Copy link
Member

Choose a reason for hiding this comment

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

I tried solving this one by changing the package version that was being installed by apt, no success tho.

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.

Add support for bzip3
5 participants