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

Replace adm-zip with yauzl #35

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

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Feb 21, 2025

All adm-zip versions after 0.5.10 seem to have bugs:

  • 0.5.11 causes tests to fail.
  • 0.5.12 causes tests to hang.
  • 0.5.13 causes extracted files to be empty.

This also avoids extracting anything other than the clangd binary.

This is an alternative to #34.

@HighCommander4
Copy link
Contributor

This also avoids extracting anything other than the clangd binary.

I don't think we can do that; the clangd binary requires the other files in the zip file (dynamically linked library files, and in most configurations built-in header files) to operate correctly.

Regarding yauzl as a choice of library to use: it seems like it has a lower-level API which is more involved to use. What do you think about going back to unzipper? We switched away from it in #25 due to a bug in it, but it looks like that bug has since been resolved. Switching back to it could potentially be as simple as reverting #25.

also cc @fannheyward for any thoughts

@tamird
Copy link
Contributor Author

tamird commented Feb 23, 2025

unzipper's issues and pull request count don't inspire confidence :(

https://github.com/ZJONSSON/node-unzipper#readme

I can change the implementation to extract all files, that's straightforward.

@tamird
Copy link
Contributor Author

tamird commented Feb 23, 2025

@HighCommander4 I've updated this to extract the entire archive, as before.

All adm-zip versions after 0.5.10 seem to have bugs:
- 0.5.11 causes tests to fail.
- 0.5.12 causes tests to hang.
- 0.5.13 causes extracted files to be empty.
@HighCommander4
Copy link
Contributor

I'm hesitant about replacing a use of a library that was basically a single function call, with one that involves this much more complexity, with four levels of nested callbacks and such; it seems like this would require more effort to maintain going forward.

I see that #34 has been updated with a change that makes the test pass -- any reason not to go forward with that?

(Also added @hokein for a second opinion.)

@tamird
Copy link
Contributor Author

tamird commented Feb 23, 2025

The change that was made in #34 makes me even more suspicious. It seems to me that the maintainers of adm-zip are a bit sloppy, and that's enough to make me think we should look for an alternative, and yauzl is the most popular library I could find that also has a reasonable support record.

Having said that, downloading clangd is a relatively small part of this library's functionality, so I don't feel very strongly about this.

If we go with #34 we should document the reason for avoiding upgrading adm-zip, particularly since our tests fail to detect the 0.5.13+ issue.

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