zlib: add ZIP archive support to zlib (ZipFile,ZipBuffer,ZipEntry)#64339
zlib: add ZIP archive support to zlib (ZipFile,ZipBuffer,ZipEntry)#64339pipobscure wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #64339 +/- ##
==========================================
+ Coverage 90.25% 90.36% +0.11%
==========================================
Files 741 755 +14
Lines 241165 245168 +4003
Branches 45423 46227 +804
==========================================
+ Hits 217673 221558 +3885
- Misses 15069 15168 +99
- Partials 8423 8442 +19
🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
0f2b879 to
4258e94
Compare
Codecov flagged low patch coverage on lib/internal/zip.js and lib/internal/vfs/providers/archive.js in nodejs#64339. Add tests exercising Zip64 extra-field parsing, DOS date/time edge cases, streaming-entry state guards, decodeMemberStream/decodeMemberSync's duplicated error branches, the ZipBuffer/ZipFile iteration protocols, several on-disk ZipFile error paths, and ArchiveFileHandle's direct read/write/stat/ truncate surface plus a handful of provider-level error branches the existing tests didn't reach.
2a56a15 to
27ab6b4
Compare
|
See also #45651 |
Thanks @bakkot !!! I think the time has come for it on the one hand, and on the other I added some „motivation“ links earlier. Here some more detail: Based on this, we can modify the loader to directly load from an archive. If we do that, we get application bundles. pipobscure#5 & pipobscure#6 Which can then in turn be used to easily create application bundles: https://github.com/pipobscure/experimental-sea So the world had changed enough that it‘s worth proposing again. |
f157686 to
233f865
Compare
|
I like this a lot. It's likely better to split this into 2 PRs, one for Zip support and one for VFS-Zip, so the Zip support could theoretically be backportable on its own. |
This comment was marked as outdated.
This comment was marked as outdated.
|
As per @mcollina I split this into two PRs. I have the vfs-provider ready to go as follow up one (as it depends on this being merged) I also made sure that the streaming side of things was actually as clean as I intended, and added a few more tests. |
I've added a bunch more tests and compared to what go/python/info-zip do. (info-zip is cli exercise, so it's not entirely clear what gets tested). I also gave zip.js an once over and concluded that it was too large a file (it originated from separate files that I've had for ages combined into one). So I split it back out so it would be easier to review. And gave that another look. |
mcollina
left a comment
There was a problem hiding this comment.
LGTM
I think it should also emit a usage warning that this API is experimental.
Possibly the Zip should be disposable/asyncdisposable.
|
Question: is there anything outstanding at this point that I should do on this. I see 4 failing CI items, but I don’t have permissions to look at them. If there is something I can/should do please let me know. |
@pipobscure Yes, unfortunately the failures look related to this PR: |
|
Thank you!!! If my first guess substantiates, how do we want to solve that? a) gate Zip support on OpenSSL availability That wasn’t it. |
Add ZIP archive support to the node:zlib module through three classes and a set of helpers: - ZipEntry: a single archive member, with buffered reads (content()), bounded-memory streaming reads (contentIterator()), and create()/createStream() for building members. - ZipFile: random access to an archive backed by a file descriptor, reading members lazily without retaining their content and writing new members in place; opened with open()/openSync(). - ZipBuffer: a zero-copy, in-memory view over an archive already held in a Buffer. createZipArchive() serializes a sequence of entries into an archive byte stream, and setMaxZipContentSize() bounds the default in-memory decompression size. Every operation has both an asynchronous and a synchronous form. Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
|
@richardlau it was the use of random bytes in tests (which failed when crypto was not available to generate them) and to lax of a check for python availability (python3 exists as a stub on Windows) which is used to validate against the python libraries, and the unix-mode check on Windows which Windows simply doesn't have and translates reas-only to 0x666 instead of the expected (unix correct) 0x644. There I did an arch==windows check in the test as the OS simply doesn't support precise unix-modes. |
|
@mcollina I validated the disposable functionality to make sure everything that has resources (ZipFile has an fd) and owns streams (that may have fds) (ZipEntry) disposes them correctly. |
|
Well, I'd love to fix the CI issues, but I can't access the putput so I have no clue how to figure out what's the cause. Please ping (@) me if there is anything I can do. |
Also, there seems to be a conflict: |
Summary
Add ZIP archive support to the node:zlib module through three classes
and a set of helpers:
bounded-memory streaming reads (contentIterator()), and
create()/createStream() for building members.
reading members lazily without retaining their content and writing
new members in place; opened with open()/openSync().
in a Buffer.
createZipArchive() serializes a sequence of entries into an archive
byte stream, and setMaxZipContentSize() bounds the default in-memory
decompression size. Every operation has both an asynchronous and a
synchronous form.
P.S.: my CLA should be on file and I wrote this myself so COO is declared