jmcnamara / libxlsxwriter

A C library for creating Excel XLSX files.
https://libxlsxwriter.github.io
Other
1.48k stars 330 forks source link

Broken logic in FindMINIZIP.cmake #405

Closed hosiet closed 1 year ago

hosiet commented 1 year ago

We look into what FindMINIZIP.cmake does:

https://github.com/jmcnamara/libxlsxwriter/blob/f43e53e018b6f90b06e8b7eadb26c30fbfcf0528/cmake/FindMINIZIP.cmake#L92-L95

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 commented 1 year ago

Thanks. I'll take a look.

jmcnamara commented 1 year ago

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 commented 1 year ago

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.