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: publicly export and document the zip64 threshold constants #79

Merged
merged 1 commit into from
Jul 20, 2024

Conversation

cosmicexplorer
Copy link
Contributor

See zip-rs/zip-old#402. Closes #60.

@cosmicexplorer
Copy link
Contributor Author

It looks like the added test is failing:

failures:

---- src/spec.rs - spec::ZIP64_BYTES_THR (line 24) stdout ----
Test executable failed (exit status: 101).

stderr:
thread 'main' panicked at src/spec.rs:18:2:
assertion failed: zip.start_file("one.dat", options).is_err()

I'll take a look into why this behavior changed since zip-rs#402 and try to bisect the failure.

@Pr0methean Pr0methean added the Please fix failing tests Tests are failing with this change; please fix them. label May 2, 2024
@cosmicexplorer
Copy link
Contributor Author

This may be able to use the work from #67.

src/spec.rs Show resolved Hide resolved
src/spec.rs Outdated Show resolved Hide resolved
@Pr0methean Pr0methean added Please address review comments Some review comments are still open. and removed Please fix failing tests Tests are failing with this change; please fix them. labels May 4, 2024
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.

Are you still planning to work on this PR, or have you folded it into one of the bigger ones?

src/spec.rs Outdated Show resolved Hide resolved
@cosmicexplorer cosmicexplorer force-pushed the oldpr402 branch 2 times, most recently from 391bd07 to 7ef505a Compare June 13, 2024 21:06
@cosmicexplorer
Copy link
Contributor Author

@Pr0methean this should be ready to review -- I hadn't gotten the hang of the nifty new behavior to rewind any file entries upon write or other errors before, but the test is working now. Fantastic functionality, so incredibly useful.

@cosmicexplorer cosmicexplorer force-pushed the oldpr402 branch 2 times, most recently from 71aad56 to 9ba04c3 Compare June 13, 2024 21:09
@Pr0methean Pr0methean added waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. and removed Please address review comments Some review comments are still open. labels Jun 13, 2024
@Pr0methean
Copy link
Member

@cosmicexplorer Sorry if the merge for this PR is delayed. I've been experimenting with new ways to generate and improve the fuzz corpus, and have found more panic-causing bugs tonight than I would've expected in a year -- and that's before cross-pollinating the corpora from different restarts!

@Pr0methean
Copy link
Member

Of course, if you're able to fix any of these bugs yourself in separate PRs, I'll merge them very gratefully.

@cosmicexplorer
Copy link
Contributor Author

Rebased and fixed conflicts; sorry for dropping this one! really want to get a new version of the parallel extraction PR #72 going (since that one is much more straightforward/less error-prone than the rest of #193, and I believe also much more requested by users), but will absolutely be trying to keep an eye out for bugfixes of that sort. I really like reading the code you've added to this crate and fixing bugs is a joy. Please especially feel free to ping me in the future if there's anything in particular you might like an extra hand with.

src/spec.rs Outdated Show resolved Hide resolved
@cosmicexplorer
Copy link
Contributor Author

CI is failing on a separate change, I'll push a quick fix for that now in a separate PR.

@cosmicexplorer
Copy link
Contributor Author

Ping @Pr0methean!

Pr0methean
Pr0methean previously approved these changes Jul 15, 2024
@Pr0methean Pr0methean added this pull request to the merge queue Jul 15, 2024
@Pr0methean Pr0methean removed this pull request from the merge queue due to a manual request Jul 15, 2024
@Pr0methean
Copy link
Member

This has a merge conflict, which I'll try to resolve once the queue is empty.

@Pr0methean Pr0methean added the merge conflict This PR has a merge conflict with a PR that was in the merge queue when this label was applied. label Jul 16, 2024
@cosmicexplorer
Copy link
Contributor Author

The merge conflict mysteriously disappeared!

@Pr0methean Pr0methean added this pull request to the merge queue Jul 17, 2024
@Pr0methean Pr0methean removed the merge conflict This PR has a merge conflict with a PR that was in the merge queue when this label was applied. label Jul 17, 2024
@Pr0methean Pr0methean removed this pull request from the merge queue due to a manual request Jul 17, 2024
@Pr0methean Pr0methean added this pull request to the merge queue Jul 17, 2024
@Pr0methean Pr0methean removed this pull request from the merge queue due to a manual request Jul 17, 2024
@Pr0methean Pr0methean added this pull request to the merge queue Jul 17, 2024
@Pr0methean Pr0methean removed this pull request from the merge queue due to a manual request Jul 17, 2024
@Pr0methean
Copy link
Member

It also mysteriously reappeared - apparently the conflict was with another PR that got merged today but had been queued before.

@Pr0methean
Copy link
Member

The cause of the conflict is that spec.rs no longer imports core::mem::align_of.

@Pr0methean Pr0methean added Please fix rebase conflicts Please rebase this PR against master and fix the conflicts. and removed waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. labels Jul 18, 2024
@cosmicexplorer
Copy link
Contributor Author

Fixed!

@Pr0methean Pr0methean added waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. and removed Please fix rebase conflicts Please rebase this PR against master and fix the conflicts. labels Jul 19, 2024
@Pr0methean Pr0methean added this pull request to the merge queue Jul 20, 2024
Merged via the queue into zip-rs:master with commit a7c1230 Jul 20, 2024
38 checks passed
@Pr0methean Pr0methean removed the waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. label Jul 20, 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