Skip to content

Different miniz cpp#18

Closed
psyinf wants to merge 1 commit intonextgeniuspro:masterfrom
psyinf:different_miniz-cpp
Closed

Different miniz cpp#18
psyinf wants to merge 1 commit intonextgeniuspro:masterfrom
psyinf:different_miniz-cpp

Conversation

@psyinf
Copy link
Contributor

@psyinf psyinf commented Jan 14, 2025

The currently used minizip is outdated
Unfortunately the most recent version is still broken. So I created a fix and issued a PR

Current fix is based on my fork however

@nextgeniuspro
Copy link
Owner

Hi @psyinf, what exactly do you have broken in minizip? I don't see any usefull changes in your branch that improves this library. Also, where did you make PR, this repository isn't an origin of minizip. Sorry, your changes doesn't look as something important and cannot be merged

@psyinf
Copy link
Contributor Author

psyinf commented Jan 20, 2025

This PR was meant to use a mini-zip that uses static function definitions while using a more recent version of mini-zip.
Unfortunately mini-zip doesn't seem to be maintained anymore and the nyq/miniz-cpp#3 at least is based on a more recent version. The original repo has a PR for this as well (tfussell/miniz-cpp#11) but it seems unmaintained, so therefore I forked and applied those changes.

As it stands now vfspp cannot be used inside a project that links it through a library as the symbols in the mini-zip are visible multiple times for the linker. This was working before, when vfspp wasn't header-only.
Alternatively the inclusion of miniz-cpp could be optional or drop the header-only approach.

@psyinf psyinf force-pushed the different_miniz-cpp branch from 0e195a5 to b5ac7dd Compare January 20, 2025 09:40
@nextgeniuspro
Copy link
Owner

nextgeniuspro commented Jan 21, 2025

This PR was meant to use a mini-zip that uses static function definitions while using a more recent version of mini-zip. Unfortunately mini-zip doesn't seem to be maintained anymore and the nyq/miniz-cpp#3 at least is based on a more recent version. The original repo has a PR for this as well (tfussell/miniz-cpp#11) but it seems unmaintained, so therefore I forked and applied those changes.

As it stands now vfspp cannot be used inside a project that links it through a library as the symbols in the mini-zip are visible multiple times for the linker. This was working before, when vfspp wasn't header-only. Alternatively the inclusion of miniz-cpp could be optional or drop the header-only approach.

I see, ok. But sorry, I can't change submodule url to proposed repo. Can we do the next, I forked miniz-cpp, you can then create PR to this forked repo with library fixes, and add additional fixes to this PR too. Is that would be ok?

Anyway, thank you very much for help

@nextgeniuspro
Copy link
Owner

Alternatively: Would you accept a PR that allows for disabling the currently used mini-zip? In terms of maintainability https://github.com/richgel999/miniz might be a better alternative here. I'd volunteer to integrate it

Oh, but yes, if you can make PR with it would be greate too. Choose please one of possible options, one above with patching mine forked miniz-cpp or switching to use miniz library instead

@psyinf
Copy link
Contributor Author

psyinf commented Jan 21, 2025

Alternatively: Would you accept a PR that allows for disabling the currently used mini-zip? In terms of maintainability https://github.com/richgel999/miniz might be a better alternative here. I'd volunteer to integrate it

Oh, but yes, if you can make PR with it would be greate too. Choose please one of possible options, one above with patching mine forked miniz-cpp or switching to use miniz library instead

Ok, I can surely do this. basically there are two options:

  1. only change the signatures to static
  2. move to a more recent version of miniz amalgated (like in https://github.com/nyq/miniz-cpp)

I can go with option 1 for now, as this will at least fix the linker problems

@psyinf
Copy link
Contributor Author

psyinf commented Jan 21, 2025

see nextgeniuspro/miniz-cpp#1 for the static function fixes
In order to make vfspp usable at least the #16 must be integrated.

@psyinf psyinf closed this Jan 21, 2025
@psyinf psyinf deleted the different_miniz-cpp branch January 27, 2025 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants