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

Expose more Central Directory details on ZipFile objects #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Apr 23, 2018

This PR exposes a few more details about the Central Directory on ZipFile objects. These being:

  • centralDirectoryOffset
  • centralDirectorySize
  • endOfCentralDirectoryOffset
  • zip64 (true if End Of Central Directory Record has ZIP64 headers)

It also exposes an attribute zip64 on Entry objects (true if the entry has ZIP64 headers).

These attributes can be useful or interesting, and are required for #69 (comment).

@thejoshwolfe
Copy link
Owner

thejoshwolfe commented Oct 5, 2018

The usecase for this PR seems to be enabling a patch for a deficiency in yauzl. I think the real solution here is to fix yauzl to make the patch and this PR unnecessary, which I am now working on again.

Is there any other usecase for this PR?

@overlookmotel
Copy link
Contributor Author

Hi @thejoshwolfe. Thanks for looking at this.

Yes, my primary motivation was to expose these fields in order to enable my patch for Mac OS Archive Utility ZIP files.

However I also feel that exposing this info could be useful in other cases:

  1. When diagnosing problems with a faulty ZIP file
  2. If you're loading a ZIP file from a remote server (e.g. Google Drive) and want to cache sections of the file which will be required every time you want to extract any file from the ZIP (this is my particular use case).
  3. Finding your way around a ZIP file for anyone who wants to learn about how ZIP files are put together

Finally: why not? It doesn't hurt to expose more info, and it could be useful to someone for a reason neither of us has thought of yet.

As far as yauzl-mac is concerned, I totally agree with you that it'd be better if yauzl supported Mac OS ZIPs internally. However, in the meantime, I am using my yauzl-mac module for my work and it'd be great not to have to have a git dependency. Of course I could publish a fork based on this PR to npm, but I'm loathe to clutter up the npm registry with modules which will (hopefully) be short-lived in their usefulness.

(by the way I don't think there is any "deficiency" in yauzl - the Mac OS ZIP files are totally non-conformant, if not corrupt!)

@overlookmotel
Copy link
Contributor Author

Hi @thejoshwolfe.

I wondered if you'd consider merging this? I know you are working on Mac Archive support, however it's been a year!

That's not intended to be a criticism. I imagine you have many other pressing commitments, and it's not anyone's place to demand "I want feature X" from an open source software author.

However, in the meantime, my yauzl-mac package is the only way I know of to handle Mac ZIPs. yauzl-mac has to depend on a fork of yauzl as a git dependency to get access to these properties of the central directory, which is messy.

So could I ask you to please consider merging this PR? It'd be an enormous help, and I can't see a downside.

I agree that Mac Archive support in yauzl itself is the best solution, and I'd be happy to help in any way I can. But I feel that enabling yauzl-mac in the meantime would be pragmatic given the difficulty of getting support into yauzl swiftly.

If you there's something you don't like in my implementation, please just say so and I'll rework this PR in whatever way you'd like it to make it acceptable. Apologies for my desperate tone!

@thejoshwolfe
Copy link
Owner

it's been a year!

oh no! It's been so long! This Mac corruption problem has been really demotivating. :(

I'm going to take another stab at fixing the root issue, and if I'm not onto something meaningful by the end of the day, I'll merge this PR.

@thejoshwolfe
Copy link
Owner

I feel good about the progress I made in #92 . That's a really important step for trying to solve this issue in the core library.

I'd like to take another weekend to bang on this issue before giving up.

Regarding this PR: It's not completely free to merge this into master and publish a new version to npm. Adding new fields means supporting them in some way, even though probably nobody cares but you. I would need to think about adding them to the docs, and what kind of guarantees the fields have. For example, should the centralDirectorySize be validated to make sure it doesn't go out of bounds? That was an issue I ran into while working on #92 , so maybe it should be validated. But that's extra code that probably nobody cares about.

Anyway, it's these kinds of thoughts that concern me with merging this code. Every bit of public API has an implicit guarantee of stability, and thusfar yauzl has been pretty diehard about maintaining API stability (see the "comment instead of fileComment" note in the readme.). It would be better for yauzl's API to obviate this PR than to have partial support for these fields.

@rossj
Copy link

rossj commented Jan 24, 2020

I would suggest that it's time to merge this PR, if only as a stopgap to help overlookmotel/yauzl-mac until @thejoshwolfe has more time for #92 & #69. The additional properties could be left undocumented, or perhaps it could be explicitly documented that these properties have no future stability guarantee.

@overlookmotel
Copy link
Contributor Author

@thejoshwolfe A long time has passed. I guess you've not had much time to work on yauzl (totally understandable).

Could you please consider merging this PR so there's a working method to unzip these pesky Archive Utility ZIP files?

@overlookmotel overlookmotel force-pushed the export-central-dir-attributes branch from 8de423b to 976c862 Compare April 28, 2023 20:49
@overlookmotel
Copy link
Contributor Author

Well, it's 5 years since I opened this PR!

Have published a fork including the changes in this PR, in order to support yauzl-mac. Here it is, in case useful to anyone else:
https://www.npmjs.com/package/@overlookmotel/yauzl

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.

3 participants