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

Better warning about Windows not being supported? #91

Open
wingman-jr-addon opened this issue Apr 7, 2022 · 14 comments
Open

Better warning about Windows not being supported? #91

wingman-jr-addon opened this issue Apr 7, 2022 · 14 comments

Comments

@wingman-jr-addon
Copy link

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.

  • Cross-compilation worked with a few warnings
  • Libraries were able to be linked
  • APIs could be called
  • Compression and decompression calls succeeded with no error code ... but then returned garbage

I dug around for a while and finally bumped into this older ticket: #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
Copy link
Author

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 szcompressor/SZ3#5 (comment)

@vasole
Copy link
Contributor

vasole commented Nov 23, 2022

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

silx-kit/hdf5plugin#206

Needed SZ changes are already in current master: f36afa4

@disheng222
Copy link
Collaborator

disheng222 commented Nov 23, 2022 via email

@VE3NEA
Copy link

VE3NEA commented Jun 26, 2023

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
Copy link
Collaborator

disheng222 commented Jun 27, 2023 via email

@vasole
Copy link
Contributor

vasole commented Jun 28, 2023

@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
Copy link
Contributor

vasole commented Jun 28, 2023

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
Copy link
Collaborator

robertu94 commented Jun 28, 2023

@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
Copy link
Contributor

vasole commented Jun 28, 2023

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
Copy link
Contributor

vasole commented Jun 28, 2023

@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
Copy link
Collaborator

@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
Copy link

VE3NEA commented Jun 30, 2023

@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
Copy link
Collaborator

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
Copy link

VE3NEA commented Jul 1, 2023

@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.

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

5 participants