openzim / libzim

Reference implementation of the ZIM specification
https://download.openzim.org/release/libzim/
GNU General Public License v2.0
164 stars 50 forks source link

Windows public api declaration seems broken. #808

Closed mgautierfr closed 1 year ago

mgautierfr commented 1 year ago

The last change made in https://github.com/openzim/libzim/pull/796 seems broken. The PR #807 add some ddl export of forgotten symbol.

But it seems it brokes something on the compilation of kiwix-tools. Now we have a lot (if not all) of libkiwix symbols not found. I don't know why.

@xiaoyifang can you try to compile kiwix-tools on windows ? Or do a cross-compilation to win32 if you are on linux (kiwix-build kiwix-tools --target-platform win32_dyn) ?

xiaoyifang commented 1 year ago

https://github.com/openzim/libzim/blob/d87ff0d0d80d251ce976d34bac96ffc53c04bfe4/include/zim/zim.h#L38

should || defined __CYGWIN__ be removed ,if it can work before?

(kiwix-build kiwix-tools --target-platform win32_dyn) ?

how dows kiwix-build get the latest libzim source_code?

mgautierfr commented 1 year ago

Setting LIBZIM_API only if we compile with Ms Compiler seems to work (at least on Cygwin). @xiaoyifang can you test #807 on Windows to be sure it doesn't brake on Windows ?

xiaoyifang commented 1 year ago

Ok ,I'll let you know when I tested

xiaoyifang commented 1 year ago

https://github.com/xiaoyifang/vcpkg/blob/3880056be47c74f1ebe709c8671062ad18ef2292/ports/libzim/dllexport.diff

This is how vcpkg community handled this.

image

xiaoyifang commented 1 year ago

if defined(_MSC_VER) && defined(LIBZIM_EXPORT_DLL)

This can work on windows.

But I would recommend changes like this

https://github.com/microsoft/vcpkg/blob/3880056be47c74f1ebe709c8671062ad18ef2292/ports/libzim/dllexport.diff#L33-L36

such as


+if host_machine.system() == 'windows' and get_option('default_library') == 'shared'
+  public_conf.set('LIBZIM_EXPORT_DLL', true)
+endif
+
kelson42 commented 1 year ago

@mgautierfr Can we foreseen to fix this today:

mgautierfr commented 1 year ago

@xiaoyifang I will merge #807 anyway as it fix our current problem.

I'm not sure about your solution, as it will set decl(export) on gcc cross-compilation to windows and we don't want that.

kelson42 commented 1 year ago

@mgautierfr What's up here? We need to release 8.2.1 ASAP.

mgautierfr commented 1 year ago

This is fixed by #807