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

Include shlwapi.h explicitly on Windows #2407

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Jan 17, 2023

This fixes builds on mingw32 and other compilers.

I suspect this will fix #2396 , #2408

@mkitti
Copy link
Contributor Author

mkitti commented Jan 17, 2023

Looking for StrStrIA in the shlwapi library is not reliable due
to stdcall on mingw32.
@mkitti
Copy link
Contributor Author

mkitti commented Jan 17, 2023

Hmm, this alone may not be sufficient. shlwapi may also needed to be added to LINK_LIB when the header is detected. Testing again.

edit: d300feb is sufficient, I just forgot to apply the patch.

@mkitti
Copy link
Contributor Author

mkitti commented Jan 17, 2023

During configuration this still occurs in the mingw32 environment.

-- Looking for StrStrIA in shlwapi;m;ws2_32;wsock32
-- Looking for StrStrIA in shlwapi;m;ws2_32;wsock32 - not found

This is due to name mangling that occurs in 32-bit windows with stdcall:

$ nm /mingw64/lib/libshlwapi.a  | grep StrStrIA
0000000000000000 I __imp_StrStrIA
0000000000000000 T StrStrIA

$ nm /mingw32/lib/libshlwapi.a  | grep StrStrIA
00000000 I __imp__StrStrIA@8
00000000 T _StrStrIA@8

@hmaarrfk
Copy link

Do you think it cleanly applies to 1.14.0?
If so I'm going to test it in conda-forge/hdf5-feedstock#188 if that is ok.

@hmaarrfk
Copy link

I can confirm that this fixes things for the "windows" compiler, at least in terms of the segfault.

@derobins derobins merged commit 720e04e into HDFGroup:develop Jan 17, 2023
@hmaarrfk
Copy link

didn't you want to fix the mingw32 configuration?

@mkitti
Copy link
Contributor Author

mkitti commented Jan 17, 2023

didn't you want to fix the mingw32 configuration?

This addresses the issues for Windows-based compilation in general actually. Was there something else that needed to be addressed here?

@hmaarrfk
Copy link

No. I'm just glad I got to test the fix before you merged anything.

brtnfld pushed a commit to brtnfld/hdf5 that referenced this pull request May 17, 2023
* Include shlwapi.h explicitly

* Add shlwapi library to LINK_LIB when header is detected

Looking for StrStrIA in the shlwapi library is not reliable due
to stdcall on mingw32.
@emmenlau
Copy link

I think this PR opened a problem, because shlwapi is now a transitive dependency of hdf5, but this is not exposed via the cmake properties. Linking hdf5 on Windows leads to errors that can be resolved by linking shlwapi. But in cmake world, this dependency should be exposed from the cmake target.

So, since this commit, the following is no longer sufficient to link hdf5 for me:

  find_package(HDF5 MODULE REQUIRED COMPONENTS C)

  target_include_directories(libdeps SYSTEM INTERFACE ${HDF5_INCLUDE_DIRS})
  target_link_libraries(libdeps INTERFACE ${HDF5_C_LIBRARIES}) 
  target_compile_definitions(libdeps INTERFACE ${HDF5_DEFINITIONS})

@mkitti
Copy link
Contributor Author

mkitti commented Oct 12, 2023

@emmenlau I think the problem would be easier to track if to created a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants