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

Always enable Zip64 extension for zipstreamer #5116

Merged
merged 1 commit into from
May 30, 2017

Conversation

McNetic
Copy link
Contributor

@McNetic McNetic commented May 25, 2017

Is there any factual reason to disable zip64 on 32bit machines? ZipStreamer goes to quite some lengths to make sure it works properly on both 32bit and 64bit machines. It does not rely on 8 bit integers for zip64....

Signed-off-by: Nicolai Ehemann <en@enlightened.de>
@mention-bot
Copy link

@McNetic, thanks for your PR! By analyzing the history of the files in this pull request, we identified @VicDeo, @nickvergessen and @MorrisJobke to be potential reviewers.

@LukasReschke LukasReschke self-requested a review May 26, 2017 04:05
@LukasReschke
Copy link
Member

@nickvergessen changed this with owncloud/core#19640, any memories what this was good for? :)

Copy link
Member

@LukasReschke LukasReschke left a comment

Choose a reason for hiding this comment

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

I'd personally say let's try this and if someone complains in the beta phase of 13 we can go back here :-)

@nickvergessen
Copy link
Member

any memories what this was good for? :)

See steps in the PR description?

@ChristophWurst
Copy link
Member

Is this a duplicate of #5112 ?

@McNetic
Copy link
Contributor Author

McNetic commented May 30, 2017

No. #5112 does the exact opposite; it disables Zip64 support (and thus, support for files bigger than 4 GB) completely.

@nickvergessen
Copy link
Member

I don't have 32 bit anywhere, so I can not check if it really solves the issue and whether it works on 32bits. But since it seems to not work now and the change is proposed by the owner of the lib, I'm fine with changing it. :)

@nickvergessen nickvergessen merged commit 1d227d8 into nextcloud:master May 30, 2017
@McNetic
Copy link
Contributor Author

McNetic commented May 30, 2017

Wait; this PR was totally unrelated to #5112 and #2352 (and it will not fix any problems described there). I just wondered (already some months ago) why Zip64 was disabled on 32bit machines at all. I will try on a 32bit machine as soon as possible.

@McNetic
Copy link
Contributor Author

McNetic commented Jun 9, 2017

Meanwhile, I was able to test this on a 32bit machine, and have to admit the Zip64 support in ZipStreamer on 32bit machines is indeed broken. Unfortunately, it is not totally easy to fix the problem, so I would withdraw the PR until I can release a fixed version (I track the problem here: McNetic/PHPZipStreamer#38). Is it possible to revert the merge, or do I have to open a new PR?

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.

None yet

5 participants