-
-
Notifications
You must be signed in to change notification settings - Fork 347
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
Switch from outdated SharpZipLib.Patched to newer SharpZipLib #3329
Switch from outdated SharpZipLib.Patched to newer SharpZipLib #3329
Conversation
This comment has been minimized.
This comment has been minimized.
Ugh, I've paid attention that it doesn't underline it in the PR preview, but apparently it only happens once the PR is created. Let's do some trickery. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎲
Interestingly this inflates and installs successfully for me, even though I had problems with UTF-8 in #3323 😕 |
Me, too. Maybe the Inflator's instance is missing some locale data? |
Now this is interesting. Running
I expected to see the "multiple files" error, but instead got the on from https://github.com/KSP-CKAN/NetKAN/issues/8425, but on Linux 🤯 |
Good chance the container is missing some locale data. I doubt it's included. |
Looks like SharpZipLib changed the default for reading and writing zips away from UTF-8 in version 1.1.0: Setting |
Is there a non-static equivalent? That's a bit ugly in terms of encapsulation. |
There seems to be an overhaul of this in progress, which also mentions going back to support Unicode by default (for creating new zips judging from the discussion, not sure if and how this would affect reading zips): Otherwise I haven't really digged into it yet, I'm currently trying to write a unit test for this in order to prevent this in the future, and also simpler patch testing. |
It looks like the SharpZipLib team is moving in a good direction, but it's less clear when those changes will land. I'm comfortable setting the static bool as a short term workaround, and I've subscribed to their repo's release notifications so we can revisit this. |
Motivation
https://github.com/KSP-CKAN/NetKAN/issues/8425 and KSP-CKAN/NetKAN#8315 show some weird errors when trying to extract certain zips with special characters on Windows systems with CJK locales.
Maybe this is a bug of
SharpZipLib
, and maybe it is already fixed in a newer versions. It's a total stab in the dark, but worth a try. I've asked affected users to try a debug build with an updated version of the package, we should know when we get an answer.In any case, there have been some improvements to that library in the releases since, the usual performance enhancements, and support for BZip2 compression.
We have been using a "Patched" version of this library, which has been deprecated now. Its NuGet page doesn't give any details on what it's patching, but it seems to be related to:
The fix is applied upstream since v1.0.0-rc1:
Changes
Switch to the upstream SharpZipLib, v1.3.1. I haven't seen any breaking changes, and mod installation and inflation still works perfectly fine in my testing.
Add
venv/
folders to.gitignore
- I have one insidebin/
for the Python scripts there.Add
exitstatus
tobin/requirements.txt
. For some reason I didn't catch the missing dependency during the PRs for theckan-merge-pr.py
.(Gonna give this the "Enhancement" flair for the time being and not say it closes the issue https://github.com/KSP-CKAN/NetKAN/issues/8425, since we don't know for sure yet)