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

feat: Support XZ decompression #199

Merged
merged 6 commits into from
Jul 5, 2024
Merged

feat: Support XZ decompression #199

merged 6 commits into from
Jul 5, 2024

Conversation

yujincheng08
Copy link
Contributor

No description provided.

@yujincheng08 yujincheng08 force-pushed the xz branch 2 times, most recently from 426d31f to 000ea13 Compare June 21, 2024 16:50
@yujincheng08 yujincheng08 changed the title Support XZ decompression feat: Support XZ decompression Jun 21, 2024
@yujincheng08 yujincheng08 force-pushed the xz branch 6 times, most recently from 599ddd1 to 6286be5 Compare June 21, 2024 17:23
Copy link
Member

@Pr0methean Pr0methean left a comment

Choose a reason for hiding this comment

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

Mostly looks good.

src/read/xz.rs Outdated Show resolved Hide resolved
src/read/xz.rs Outdated Show resolved Hide resolved
src/read/xz.rs Outdated Show resolved Hide resolved
src/read/xz.rs Outdated Show resolved Hide resolved
src/read/xz.rs Outdated Show resolved Hide resolved
@yujincheng08 yujincheng08 force-pushed the xz branch 3 times, most recently from 97b3d8b to fe0b7c0 Compare June 21, 2024 19:10
@yujincheng08
Copy link
Contributor Author

All done

@yujincheng08 yujincheng08 force-pushed the xz branch 8 times, most recently from b658406 to e571a82 Compare June 21, 2024 19:54
@Pr0methean Pr0methean enabled auto-merge June 21, 2024 20:47
Pr0methean
Pr0methean previously approved these changes Jun 21, 2024
@Pr0methean Pr0methean added the waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. label Jun 21, 2024
auto-merge was automatically disabled June 22, 2024 02:43

Head branch was pushed to by a user without write access

@Pr0methean Pr0methean enabled auto-merge June 22, 2024 04:17
@Pr0methean
Copy link
Member

Thanks for your contribution! I'm hoping to merge and release it within the next 24 hours.

@yujincheng08
Copy link
Contributor Author

CI failed but it seems to come from mainline.

@Pr0methean Pr0methean added this pull request to the merge queue Jun 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 23, 2024
@Pr0methean Pr0methean added this pull request to the merge queue Jun 23, 2024
@yujincheng08
Copy link
Contributor Author

@Pr0methean Hi, I found lzma-rust which supports partial decompression and BCJ, and most importantly, it supports compression. Maybe you are interested in using it instead?

@yujincheng08
Copy link
Contributor Author

I have an ugly implementation to use lzma-rust here: https://github.com/yujincheng08/zip2/tree/lzma-rust

But it's not panic-free so I am not opening a PR. Maybe you could build upon it.

@Pr0methean
Copy link
Member

Opened an issue to consider lzma-rust. I'd still like to merge this change in its current form.

@yujincheng08
Copy link
Contributor Author

Thanks. It is reasonable to use lzma-rs for now and later migrates to lzma-rust for both lzma and xz (and add compression support).

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 23, 2024
@Pr0methean
Copy link
Member

Update: It may take another day or two to merge this, since I first need to generate new fuzz corpora with smaller inputs so the fuzz tests can run faster, and the scripts I'm using to do so may still need debugging.

@yujincheng08
Copy link
Contributor Author

yujincheng08 commented Jun 23, 2024

I dont think the repo settings are correct here.

According to the github documentation:

The merge queue provides the same benefits as the Require branches to be up to date before merging branch protection, but does not require a pull request author to update their pull request branch and wait for status checks to finish before trying to merge.

If merge queue is set, we dont need to enable Require branches to be up to date before merging. But both are enabled. Thus merge cannot be done once someone push to the target branch.

Also the timeout is not correctly set while a time-consuming fuzz test is checked, and the checks nearly exceeds the time limit of Github Action (6h). If merge queue is set, some status check can be moved to merge queue while PR check can only contain neccessary ones (like format and compilation without unit test).

I think we could disable Require branches to be up to date before merging, disable unit test (at least diable fuzz test) for PR, set merge queue timeout to a longer time.

@Pr0methean
Copy link
Member

Pr0methean commented Jun 23, 2024

Actually Require branches to be up to date before merging isn't enabled for this repo -- I've only been rebasing this PR when there turned out to have been test failures from a bug that was already in master. As for a longer merge-queue timeout, GitHub unfortunately only supports a maximum of 6 hours. What I'm trying to do by generating a new fuzz corpus is to ensure that the fuzz test runs fast enough that it usually hits the iteration limit before the time limit.

@Pr0methean
Copy link
Member

As for the redundancy of the checks, that's necessary to detect the scenario where two PRs in flight at the same time are compatible with master, but not with each other.

@Pr0methean Pr0methean added this pull request to the merge queue Jul 5, 2024
Merged via the queue into zip-rs:master with commit d45bdcc Jul 5, 2024
38 checks passed
@yujincheng08 yujincheng08 deleted the xz branch July 6, 2024 00:42
@Pr0methean Pr0methean removed the waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. label Jul 6, 2024
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.

2 participants