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

Support mz_* API in Windows shared library #477

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

jimpark
Copy link
Contributor

@jimpark jimpark commented May 8, 2020

No description provided.

@eiszapfen2000
Copy link
Contributor

Would it be possible to go forward with this? Both zip reader and zip writer allow to retrieve the underlying zip handle, but in case of a shared library build on Windows one is then unable to actually do anything with the zip object. Moreover, if one prefers to use zip handles over the zip reader and zip writer, then again the shared library build on Windows is not an option.

@nmoinvaz
Copy link
Member

nmoinvaz commented Dec 1, 2020

Zip object also exposes Stream object, so we would need to export all those functions as well. Why not on Windows just use the command cmake . -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON

@ptc-tgamper
Copy link
Contributor

Just my 2 cents here:
We are using mz_zip directly, instead of the reader and writer, for two reasons:

  1. mz_zip_reader is missing an interface to call mz_zip_get_entry/mz_zip_goto_entry
  2. mz_zip_writer_open(void *handle, void *stream) is missing an additional argument to tell the underlying zip handle if we are either appending or creating. Thus, at least appending to an existing archive is not working, because https://github.com/nmoinvaz/minizip/blob/ee60b3c61443d75f31a2c42e3ac8fd4faaa6d99c/mz_zip.c#L1389 is never reached.

If these two things were improved/fixed, at least for our use case we would be able to get away with using the higher level interfaces of reader and writer, and no additional exports would be needed.

@nmoinvaz
Copy link
Member

nmoinvaz commented Dec 3, 2020

mz_zip_reader and mz_zip_writer are not exported so how does that relate to mz_zip being exported or not.

  1. mz_zip_reader does contain mz_zip_reader_entry_get_info and mz_zip_reader_goto_first_entry and mz_zip_reader_goto_next_entry.
  2. It is possible to use mz_zip_writer_open_file with disk_size=0 and append=1. If you still need a stream version with append then I would be willing to accept a PR with that change.

@ptc-tgamper
Copy link
Contributor

Sorry, I was wrong, it is only the functions in mz_compat.h that are exported.

  1. We are using mz_zip_get_entry to build a lookup table, which allows us later on to quickly extract a random entry from the archive by straight jumping to that entry using mz_zip_goto_entry, without the need to iterate through the archive entries with mz_zip_reader_goto_next_entry.

  2. Yes, we need a stream version. By implementing a separate mz_stream object we were actually able to teach minizip to use all of our internal file apis, thus allowing us to support all our platforms. Would I be allowed to change the signature of mz_zip_writer_open(void *handle, void *stream)?

@nmoinvaz
Copy link
Member

nmoinvaz commented Dec 4, 2020

  1. I don't have a problem with changing the signature.

@tsaizhenling
Copy link

side note, would u help review changes to the vcpkg port to allow export of MZ_* API on windows?
microsoft/vcpkg#18675

@nmoinvaz nmoinvaz changed the base branch from dev to develop April 27, 2022 17:59
@nmoinvaz nmoinvaz removed the 2.0 label Apr 27, 2022
@neheb
Copy link
Contributor

neheb commented Aug 13, 2022

Needs a rebase. I just ran into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distribution Release distribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants