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

fix(streamer): zip64 should work on 32-bit now #49035

Merged
merged 1 commit into from
Nov 9, 2024

Conversation

joshtrichards
Copy link
Member

  • Resolves: #

Summary

This should work again now that DeepDiver1975/PHPZipStreamer#11 is merged. We've already bumped v28-v30 up to >2.0.1 of ZipStreamer (in nextcloud/3rdparty#1673 and nextcloud/3rdparty#1863).

Untested since I no longer have a 32-bit environment set up for testing.

Came up while looking into nextcloud/files_zip#200.

Maybe @bcutter feels like testing this change too for the standard built-in zip mechanism? :)

To test:

  • source needs to consist of >4GB worth of files (or) >65536 files.

TODO

  • ...

Checklist

@susnux susnux added enhancement 3. to review Waiting for reviews labels Oct 31, 2024
@susnux susnux added this to the Nextcloud 31 milestone Oct 31, 2024
@bcutter
Copy link

bcutter commented Nov 2, 2024

Tested with a folder of 5.38 GB in total.

  1. Without the patch: took quite some time (5 minutes on a Pi 4 with external SSD), but worked without an issue. During generation there was a 0 KB "place holder" zip file.
    grafik

  2. Because of 1 I did not try to use it with the patched Streamer.php file.

Please note I have patched the file 20 minutes before but reverted that changes, see nextcloud/files_zip#200 (comment) for details.

@szaimen
Copy link
Contributor

szaimen commented Nov 3, 2024

  1. Pi 4

The Pi 4 supports 64bit OS and php afaik

@bcutter
Copy link

bcutter commented Nov 3, 2024

Sure. Back in the days it did not and a migration takes a lot of time - so it still runs on a 32 bit OS. Great you asked.

@come-nc
Copy link
Contributor

come-nc commented Nov 4, 2024

https://github.com/nextcloud/server/actions/runs/11668606807 I can run 32bit CI on this branch, but it seems there is no test for the streamer.
I’m not sure it’s worth it to add a test with >4GB files, it will eat up a lot of CI resources.

Can someone with a 32bit setup test locally instead?

Or @joshtrichards if you push a test to this branch I can run the CI again and afterwards we remove or comment out the test.

@kesselb kesselb removed their request for review November 6, 2024 10:38
@skjnldsv skjnldsv merged commit 7b4a932 into master Nov 9, 2024
179 checks passed
@skjnldsv skjnldsv deleted the jtr/fix-streamer-zip64 branch November 9, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants