Skip to content

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

Closed
SinisterRectus opened this issue Jul 8, 2024 · 6 comments
Closed

miniz update? #286

SinisterRectus opened this issue Jul 8, 2024 · 6 comments

Comments

@SinisterRectus
Copy link
Member

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

/* Calculate the offset and compare to written offset to get size of data offset at front. */
if (pZip->m_pState->m_file_archive_start_ofs == 0)
pZip->m_pState->m_file_archive_start_ofs = cur_file_ofs - cdir_size - cdir_ofs;

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:

build/luvi samples/test.app -o test.bin
Creating new binary: /home/sinister/luvi/test.bin
Copying initial 1454608 bytes from /home/sinister/luvi/build/luvi
Zipping /home/sinister/luvi/samples/test.app
    add/a.lua
    add/b.lua
    add/init.lua
    greetings.txt
    main.lua
    sonnet-133.txt
    utils.lua
Writing zip file
Done building /home/sinister/luvi/test.bin
./test.bin 1 2 3 4
/home/sinister/luvi/src/lua/luvibundle.lua:292: ERROR: /home/sinister/luvi/1 is not a zip file or a folder
stack traceback:
        [C]: in function 'error'
        /home/sinister/luvi/src/lua/luvibundle.lua:292: in function 'makeBundle'
        /home/sinister/luvi/src/lua/luvibundle.lua:308: in function </home/sinister/luvi/src/lua/luvibundle.lua:304>
make: *** [Makefile:230: test] Error 255

The issue is that miniz.new_reader is failing in luvibundle.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:

Reduce memory usage for inflate. This changes struct tinfl_decompressor_tag and therefore requires a major version bump (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:

Miniz 2 is only source back-compatible with miniz 1.x. It breaks binary compatibility because structures changed

@Bilal2453
Copy link
Contributor

The current version of miniz is 3.0.2, which reportedly breaks ABI compatibility:

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.

@Bilal2453
Copy link
Contributor

Bilal2453 commented Jul 8, 2024

@zhaozg Would it also be possible to maintain the miniz mirror over the Luvi organization instead? ie over luvit/miniz similar to how we do it with other mirrors.

@SinisterRectus
Copy link
Member Author

Let's wait to see if we get a positive response on the PR.

@zhaozg
Copy link
Member

zhaozg commented Jul 9, 2024

@zhaozg can you (or someone else) clarify what this change does?

We append real zip content to executable programs, but old miniz can't find the right begin of zip context,
the patch just calculate the right zip begin offset of total file.

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.

@SinisterRectus
Copy link
Member Author

I see. Thank you @zhaozg!

@SinisterRectus
Copy link
Member Author

@luvit luvit locked and limited conversation to collaborators Jul 9, 2024
@SinisterRectus SinisterRectus converted this issue into discussion #289 Jul 9, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

3 participants