szcompressor / SZ

Error-bounded Lossy Data Compressor (for floating-point/integer datasets)
http://szcompressor.org
Other
151 stars 56 forks source link

Better warning about Windows not being supported? #91

Open wingman-jr-addon opened 2 years ago

wingman-jr-addon commented 2 years ago

I recently had a bit of an unfortunate experience. I spent some time cross-compiling SZ for Windows. While I couldn't compile on Windows directly, things went smoothly on Linux using MinGW to target Windows for the core libraries ... or so I thought.

I dug around for a while and finally bumped into this older ticket: https://github.com/szcompressor/SZ/issues/58 This confirmed it won't work on Windows essentially due to sizeof(long) being different on different platforms.

When cross-compiling doesn't work for me, it usually fails much earlier than this, so I was a bit surprised it failed this late. While it would be amazing if SZ could support Window, could a small warning please be added to the front page to let folks know that Windows won't work? Thanks!

wingman-jr-addon commented 2 years ago

Also, exciting note! I had thought to close a Windows-cross-compilation bug for SZ3 that I had opened but the SZ3 team was able to get things running for Windows - see https://github.com/szcompressor/SZ3/issues/5#issuecomment-1094039224

vasole commented 1 year ago

We have been able to get the SZ HDF5 filter working under windows without needing cross-compilation:

https://github.com/silx-kit/hdf5plugin/pull/206

Needed SZ changes are already in current master: https://github.com/szcompressor/SZ/commit/f36afa4ae9da0a8c50da3b9e89bfa89c8908b200

disheng222 commented 1 year ago

Hi Armando, Thanks. I merged the pull request early yesterday. I think the current master has included that change. Feel free to ping me if you still find anything to update. Thanks!

Best, Sheng

On Wed, Nov 23, 2022 at 4:55 AM V. Armando Solé @.***> wrote:

We have been able to get the SZ HDF5 filter working under windows without needing cross-compilation:

silx-kit/hdf5plugin#206 https://github.com/silx-kit/hdf5plugin/pull/206

Needed SZ changes are in current master: f36afa4 https://github.com/szcompressor/SZ/commit/f36afa4ae9da0a8c50da3b9e89bfa89c8908b200

— Reply to this email directly, view it on GitHub https://github.com/szcompressor/SZ/issues/91#issuecomment-1324871735, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACK3KSI5UJDTB6LUPMKT3WTWJXZ3JANCNFSM5SX5UHLA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

VE3NEA commented 1 year ago

Would it be possible to include the building instructions for Windows in the main README.md file? Which compiler to use, what commands to run. The steps described in the Installation section do not work on Windows, in particular, zlib.dll and zstd.dll are built without any exported functions and thus the .lib files are not generated.

disheng222 commented 1 year ago

I am not familiar with the support regarding windows. Waiting for any possible response from other guys.

On Mon, Jun 26, 2023 at 1:04 PM Alex Shovkoplyas @.***> wrote:

Would it be possible to include the building instructions for Windows in the main README.md http:///szcompressor/SZ file? Which compiler to use, what commands to run. The steps described in the Installation section do not work on Windows, in particular, zlib.dll and zstd.dll are built without any exported functions and thus the .lib files are not generated.

— Reply to this email directly, view it on GitHub https://github.com/szcompressor/SZ/issues/91#issuecomment-1607970643, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACK3KSN6MS3WPFMSYD6RFZTXNHFMLANCNFSM5SX5UHLA . You are receiving this because you commented.Message ID: @.***>

vasole commented 1 year ago

@VE3NEA

When using cmake and native microsoft compiler, I only succeed under windows by forcing static compilation of the zlib and the zstd libraries

vasole commented 1 year ago

I detail the changes in PR #109

In any case, there are GCC specific options that need to be handled (first commit of the PR)

robertu94 commented 1 year ago

@vasole and @VE3NEA , our team has limited experience with Windows and don't have an easy way to validate it on our CI/CD resources. With the limited experience if you add -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON to your cmake configuration command, it will configured the compiler to export all of the symbols in the library in the DLL.

We do however fully support WSL, and this works as expected.

vasole commented 1 year ago

In any case, the first commit of my PR is necessary. m.lib is not available when using microsoft compilers because it is already part of the runtime. See for instance:

https://stackoverflow.com/questions/19333898/lnk1181-cannot-open-input-file-m-lib

vasole commented 1 year ago

@robertu94

My interpretation from the message of @disheng222 was that he was asking for help.

The flag Wextra triggers an error and the link to m.lib triggers a second one when using native windows compilers.

It's up to you what you do with that information.

robertu94 commented 1 year ago

@robertu94

My interpretation from the message of @disheng222 was that he was asking for help.

Yes, and we are thankful for your help with Windows!

The flag Wextra triggers an error and the link to m.lib triggers a second one when using native windows compilers.

It's up to you what you do with that information.

Thank you for your PR which addresses these.

My comment here was more 1. Mention how to get the functions exported in the DLL without extensive code changes. 2. Provide an explanation of why we hadn't fixed this earlier.

Thanks again for your help with this!

VE3NEA commented 1 year ago

@vasole 's fork builds fine on Windows with the VS compiler. Both 32-bit and 64-bit versions of SZ.dll are generated. The -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON flag is still needed for the functions in the dll to be exported. This flag forces the export of all symbols (hundreds of them), but that is OK for now.

When SZ_decompress is called, the 64-bit version produces valid results, while the 32-bit one returns garbage. Is there something else that needs to be fixed? Perhaps the size of some types is platform-dependent where it should not be?

robertu94 commented 1 year ago

I'm guessing that there is a calculation where 8 was used instead of sizeof(size_t).

Out of mild curiosity, why do you need 32 bit support?

Robert

VE3NEA commented 1 year ago

@robertu94 The project I am working on is for the hobbyists, the people who sometimes use pretty old hardware for their hobby. It would be nice to support 32-bit systems, if possible.