miniz maintenance #289
Replies: 8 comments 3 replies
-
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. |
Beta Was this translation helpful? Give feedback.
-
@zhaozg Would it also be possible to maintain the miniz mirror over the Luvi organization instead? ie over |
Beta Was this translation helpful? Give feedback.
-
Let's wait to see if we get a positive response on the PR. |
Beta Was this translation helpful? Give feedback.
-
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. |
Beta Was this translation helpful? Give feedback.
-
I see. Thank you @zhaozg! |
Beta Was this translation helpful? Give feedback.
-
Related pull requests: |
Beta Was this translation helpful? Give feedback.
-
I moved the previous "issue" to a "discussion". @Bilal2453, @Duckwhale, and I talked briefly in Discord about moving our miniz bindings into their own repository/project. Among the non-luv custom bindings we have in luvi (miniz, env, winsvc, snapshot), miniz may benefit the most from being independent. It's only one file, so it should be easy to maintain. I could find no such package on luarocks, so it may be useful to people outside of luvi. The biggest disadvantage would probably be simply that it is an extra project to support. Any thoughts on this while we wait for the upstream change? |
Beta Was this translation helpful? Give feedback.
-
I guess I kinda forgot to mention my intention for suggesting that to begin with, I wanted to write documentation for lminiz as a part of my typing efforts of Luvit (over in luvit-meta) but I didn't really know where would that be, there is no luvi wiki (nor am I sure if lminiz would go there if one existed), maybe in the luvi readme? but that feels like it wouldn't get noticed. lminiz is a core part of luvi and how luvi works and I think it deserves a bit more attention, I noticed a fair bit of interest from average users who wanted to read and write zips but couldn't read any C. There is one problem behind my motivation: it applies to most if not all Luvi modules, as I would probably want to also document/type the rest of modules such as winsvc etc, although I admit I saw no interest from average users for those it is mostly just ZIP files. |
Beta Was this translation helpful? Give feedback.
-
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:
Beta Was this translation helpful? Give feedback.
All reactions