Fails to link on Windows, how is libz supposed to be linked?

KarelPeeters commented 1 year ago

I get the following error when trying to build this crate with the default features:

   Compiling highs-sys v1.6.0 (C:\Documents\Programming\Contrib\highs-sys)
error: linking with `link.exe` failed: exit code: 1120
  |
  = note: "C:\\Program Files\\Microsoft Visual Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.36.32532\\bin\\HostX64\\x64\\link.exe" [...]
  = note: highs.lib(HMpsFF.obj) : error LNK2019: unresolved external symbol deflateEnd referenced in function "public: virtual void * __cdecl zstr::istreambuf::`scalar deleting destructor'(unsigned int)" (??_Gistreambuf@zstr@@UEAAPEAXI@Z)
          highs.lib(HMPSIO.obj) : error LNK2001: unresolved external symbol deflateEnd
          highs.lib(reader.obj) : error LNK2001: unresolved external symbol deflateEnd
          highs.lib(HMpsFF.obj) : error LNK2019: unresolved external symbol inflate referenced in function "public: virtual int __cdecl zstr::istreambuf::underflow(void)" (?underflow@istreambuf@zstr@@UEAAHXZ)
          highs.lib(HMPSIO.obj) : error LNK2001: unresolved external symbol inflate
          highs.lib(reader.obj) : error LNK2001: unresolved external symbol inflate
          highs.lib(HMpsFF.obj) : error LNK2019: unresolved external symbol inflateEnd referenced in function "public: virtual void * __cdecl zstr::istreambuf::`scalar deleting destructor'(unsigned int)" (??_Gistreambuf@zstr@@UEAAPEAXI@Z)
          highs.lib(HMPSIO.obj) : error LNK2001: unresolved external symbol inflateEnd
          highs.lib(reader.obj) : error LNK2001: unresolved external symbol inflateEnd
          highs.lib(HMpsFF.obj) : error LNK2001: unresolved external symbol deflateInit2_
          highs.lib(HMPSIO.obj) : error LNK2001: unresolved external symbol deflateInit2_
          highs.lib(reader.obj) : error LNK2001: unresolved external symbol deflateInit2_
          highs.lib(HMpsFF.obj) : error LNK2019: unresolved external symbol inflateInit2_ referenced in function "public: virtual int __cdecl zstr::istreambuf::underflow(void)" (?underflow@istreambuf@zstr@@UEAAHXZ)
          highs.lib(HMPSIO.obj) : error LNK2001: unresolved external symbol inflateInit2_
          highs.lib(reader.obj) : error LNK2001: unresolved external symbol inflateInit2_
          C:\Documents\Programming\Contrib\highs-sys\target\debug\deps\highs_sys-cfaabb362e14e905.exe : fatal error LNK1120: 5 unresolved externals

error: could not compile `highs-sys` (lib test) due to previous error

This makes sense, HiGHS requires zlib to be linked trough the zstr external dependency, and zlib is not linked by default. Enabling the zlib feature fixes this issue, but now the error is

error: could not compile `highs-sys` (test "test_highs_call") due to previous error
error: linking with `link.exe` failed: exit code: 1181
  |
  = note: "C:\\Program Files\\Microsoft Visual Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.36.32532\\bin\\HostX64\\x64\\link.exe"  [...]
  = note: LINK : fatal error LNK1181: cannot open input file 'z.lib'

Again, this makes sense since nothing in the build script or in the cloned submodule HiGHS includes the file z.lib.

My questions:

Why is zlib a feature that is not turned on by default? Is there ever a scenario in which we don't want to link zlib?
How is the built supposed to be getting zlib on Windows? Should I just install it separately and add it to the path? That seems a bit suspicious. Maybe it would be better to add a dependency on https://github.com/rust-lang/libz-sys, or at least to copy their machanism for finding zlib.

lovasoa commented 1 year ago

I am not a windows expert, but our CI continuously tests windows builds. You can have a look at the setup to replicate it: https://github.com/rust-or/highs-sys/actions/runs/6190880349/workflow

If you want to contribute improvements to our windows builds, this will be very welcome !

Thell commented 7 months ago

@KarelPeeters I remember fighting with that as well. I setup a Windows Environment Variable for ZLIB_ROOT and have the z.lib there.

Edit: I forgot to mention I also have the libz-sys crate as a dependency, although I don't recall right now if that helped resolve my issue. It must have because I left it there. :)

Hopefully that works for you too.

Thell commented 7 months ago

Confirmed that I needed the libz-sys as a dependency (not a build dependency) to successfully build and test...

> git clone --recursive git@github.com:rust-or/highs-sys.git
Cloning into 'highs-sys'...
remote: Enumerating objects: 352, done.
remote: Counting objects: 100% (134/134), done.
remote: Compressing objects: 100% (55/55), done.

Receiving objects: 100% (352/352), 63.15 KiB | 979.00 KiB/s, done.
Resolving deltas: 100% (184/184), done.
Submodule 'HiGHS' (git@github.com:ERGO-Code/HiGHS.git) registered for path 'HiGHS'
Cloning into 'C:/Users/thell/Workspaces/rust/highs-sys/HiGHS'...
remote: Enumerating objects: 102724, done.
remote: Counting objects: 100% (21149/21149), done.
remote: Compressing objects: 100% (2864/2864), done.
remote: Total 102724 (delta 14061), reused 20813 (delta 13874), pack-reused 81575
Receiving objects: 100% (102724/102724), 77.76 MiB | 12.32 MiB/s, done.
Resolving deltas: 100% (72101/72101), done.
Submodule path 'HiGHS': checked out '21da9b90e0dceeb22ef9e35e5ff2c3ab17dc5232'
> cd .\highs-sys\
> cargo add libz-sys
    Updating crates.io index
      Adding libz-sys v1.1.16 to dependencies.
             Features:
             + libc
             + stock-zlib
             - asm
             - cmake
             - static
             - zlib-ng
    Updating crates.io index
> ls Env:\ZLIB_ROOT

Name                           Value
----                           -----
ZLIB_ROOT                      C:\Users\thell\AppData\Local\zlib

> cargo test --features "libz"

running 6 tests
test bindgen_test_layout_HighsCallbackDataIn ... ok
test bindgen_test_layout__Lldiv_t ... ok
test bindgen_test_layout___crt_locale_data_public ... ok
test bindgen_test_layout__Mbstatet ... ok
test bindgen_test_layout_HighsCallbackDataOut ... ok
test bindgen_test_layout___crt_locale_pointers ... ok

test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests\test_highs_call.rs (target\debug\deps\test_highs_call-823cedb92a2624fc.exe)

running 1 test
test highs_call ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests\test_highs_functions.rs (target\debug\deps\test_highs_functions-d106f863787d1d6a.exe)

running 1 test
test highs_functions ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

   Doc-tests highs-sys

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

Thell commented 7 months ago

Also, you may note in the source that the threaded test is protected by a cfg guard

#[cfg(not(target_os = "windows"))] // broken on windows

which is not true, as told by @jajhall in this comment https://github.com/ERGO-Code/HiGHS/issues/1257#issuecomment-1510525116 and confirmed in https://github.com/rust-or/highs-sys/issues/9 threading isn't broke on windows.

With the repo used in the previous post to test the zlib setup, I simply removed the cfg guard and added in the setIntOptionValue for 'threads' and ...

running 2 tests
test highs_functions ... ok
test highs_functions_multithread ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s

lovasoa commented 7 months ago

Great, can you open a PR with the change?

Thell commented 7 months ago

I just tested the libz setup again and confirmed that having the ZLIB_ROOT environment variable is not required with the use of the libz-sys crate as a dependency. It will build zlib and make it available.

Perhaps the portion of the README stating

If desired, libz needs to be installed and made discoverable by setting the ZLIB_ROOT environment variable.

should say something like

If desired, libz needs to be installed and made discoverable by adding a libz-sys dependency or by setting the ZLIB_ROOT environment variable.

lovasoa commented 7 months ago

If desired, libz needs to be installed and made discoverable by adding a libz-sys dependency or by setting the ZLIB_ROOT environment variable.

good ! Can you open a small pr ?

Thell commented 7 months ago

Great, can you open a PR with the change?

For the threads on Windows? I should clarify, I meant threading is not broken on Windows. Setting the threads option to 1 allows the threaded test to run and complete/terminate by using only a single thread but to actually use multiple threads on Windows the pthreads env would need to be setup and that is a pain iirc. Far far easier to use WSL2 in that case.

lovasoa commented 7 months ago

The pr can remove the unneeded test skipping, and update the readme

Thell commented 7 months ago

The pr can remove the unneeded test skipping, and update the readme

Wouldn't setting it to 1 negate the actual multi-threaded test on non-Windows setups though?

If there was a simple setup for pthreads on Windows I suppose that 'parallel' could be made into a feature and the build script could ensure availability. I seem to recall looking into that last year but don't recall the specifics right now.

Either way, making that change (to the tests) isn't something I feel comfortable with without understanding more about the different setups involved but the README and libz pull request was submitted. :)

rust-or / highs-sys

Fails to link on Windows, how is libz supposed to be linked? #21