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

Broken logic in FindMINIZIP.cmake #405

Closed
hosiet opened this issue Aug 2, 2023 · 3 comments
Closed

Broken logic in FindMINIZIP.cmake #405

hosiet opened this issue Aug 2, 2023 · 3 comments

Comments

@hosiet
Copy link

hosiet commented Aug 2, 2023

We look into what FindMINIZIP.cmake does:

if(MINIZIP_FOUND)
file(STRINGS "${MINIZIP_INCLUDE_DIRS}/zlib.h" MINIZIP_VERSION_CONTENTS REGEX "Version [0-9]+\\.[0-9]+(\\.[0-9]+)?")
string(REGEX REPLACE ".*Version ([0-9]+)\\.[0-9]+" "\\1" MINIZIP_VERSION_MAJOR "${MINIZIP_VERSION_CONTENTS}")
string(REGEX REPLACE ".*Version [0-9]+\\.([0-9]+)" "\\1" MINIZIP_VERSION_MINOR "${MINIZIP_VERSION_CONTENTS}")

When enabling USE_SYSTEM_MINIZIP and system libminizip is found, these lines basically look through system's /usr/include/zlib.h and look for strings like Version [0-9]+\\.[0-9]+(\\.[0-9]+)?.


However, the zlib.h header never had such lines:

-> % cat /usr/include/zlib.h | grep Version
#define zlib_version zlibVersion()
ZEXTERN const char * ZEXPORT zlibVersion OF((void));
/* The application can compare zlibVersion and ZLIB_VERSION for consistency.

This condition actually applies to all recent zlib releases.

I believe the logic in FindMINIZIP.cmake must be broken somewhere. Can you take a look?

@jmcnamara
Copy link
Owner

Thanks. I'll take a look.

jmcnamara added a commit that referenced this issue Aug 3, 2023
Fix Cmake minizip version check that had an incorrect regex and
resulted in the package not being found.

Issue #405
@jmcnamara
Copy link
Owner

There was a "Version/version" typo in the regex that prevented FindMINIZIP.cmake from reporting the library found even if it was. I fixed that and the compilation works on macOS and Ubuntu 22.04. I want to do a bit more digging to see why this didn't turn up in the past since that code has been there since 2017 (maybe cmake behaviour has changed?).

Anyway, if you get a chance can you test main to see if it works on your OS.

@jmcnamara
Copy link
Owner

I think I found the reason this didn't fail before. It was due to another change/fix in #398.

Anyway, the good news is that the CI is passing again so I'm going to close this.

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

No branches or pull requests

2 participants