-
Notifications
You must be signed in to change notification settings - Fork 63
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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
miniz update? #286
Comments
Personally, I think we shouldn't care about dependencies ABI compatibility as long as it doesn't break the luvi interface, it would hold us from progressing quite a lot. For example, we wouldn't be using OpenSSL 3 and PCRE2 etc right now. ABI compatibility, especially for merely dependencies that we can fix internally, shouldn't be a goal in my opinion. |
@zhaozg Would it also be possible to maintain the miniz mirror over the Luvi organization instead? ie over |
Let's wait to see if we get a positive response on the PR. |
We append real zip content to executable programs, but old miniz can't find the right begin of zip context, Now then office version support the feature, please see richgel999/miniz@43bc679. We can make deps/miniz submodle of https://github.com/richgel999/miniz do some test, and replace my hack version. Please wait my PR to do this. |
I see. Thank you @zhaozg! |
Related pull requests: |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
Luvi currently uses a modified copy of miniz 2.1.0. This copy was added to luvi with commit 0806985 on May 1, 2019 from zhaozg/miniz.
The code that makes this copy unique is:
luvi/deps/miniz.c
Lines 3638 to 3640 in 0806985
There was no pull request associated with commit 0806985, so the purpose and origin of this code is unclear to me.
There is an open upstream pull request at richgel999/miniz#126 on May 1, 2019, but I cannot find the code's addition to zhaozg/miniz until July 29, 2023 with commit zhaozg/miniz@0cda2f5. It looks like there were a few force-pushes, so maybe the history was wiped?
Removing this code causes an error in luvi as seen during
make test
:The issue is that
miniz.new_reader
is failing inluvibundle.makeBundle
so apparently the code is doing something to detect the bundled zip?@zhaozg can you (or someone else) clarify what this change does?
@Bilal2453 commented on #200 (comment) that he would like to see miniz updated before we release luvi 2.15.0. The current version of miniz is 3.0.2, which reportedly breaks ABI compatibility:
I was able to compile luvi on at least one platform using miniz 3.0.2 + our custom change. I am concerned that upgrading to 3.0.2 would break compatibility in luvi, but note that commit 0806985 already introduced a major version bump that was also said to be ABI incompatible:
The text was updated successfully, but these errors were encountered: