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

[szip, hdf5] Fix mingw import lib names, control linkage #17941

Merged
merged 20 commits into from
Jun 9, 2021

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented May 14, 2021

  • What does your PR fix?

    This PR:

    • restores standard mingw import lib names (...dll.a), resolving the "Could not find proper second linker member" error which was the reason for setting VCPKG_POLICY_SKIP_ARCHITECTURE_CHECK for szip in the mingw triplets,
    • disables building the static libs for triplets with dynamic linkage, following the "Choose either static or shared binaries" vcpkg guide, and properly modifies the provided cmake config files.

    It also changes the version format from version-string "2.1.1-6" to version "2.1.1" port-version 7.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all, no

  • Does your PR follow the maintainer guide?

    yes


Even with this PR, the static builds on Windows have an unusual lib prefix (libszip.lib for MSVC, liblibszip.a for mingw). It could be changed now, when the static lib is no longer build in additon to the dynamic lib.

Triplet release-static debug-static release-dynamic debug-dynamic
windows libszip.lib libszip_D.lib szip.lib * szip_D.lib *
mingw before liblibszip.a liblibszip_D.a szip.lib * szip_D.lib *
mingw after liblibszip.a liblibszip_D.a libszip.dll.a libszip_D.dll.a
linux libszip.a libszip_debug.a libszip.so * libszip_debug.so *
osx libszip.a libszip_debug.a libszip.dylib * libszip_debug.dylib *

*) Before this PR: static lib was also built

@JackBoosY JackBoosY added category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. category:port-bug The issue is with a library, which is something the port should already support labels May 15, 2021
@JackBoosY
Copy link
Contributor

When building hdf5:x86-windows:

[359/445] cmd.exe /C "cd . && D:\downloads\tools\cmake-3.20.2-windows\cmake-3.20.2-windows-i386\bin\cmake.exe -E vs_link_dll --intdir=src\CMakeFiles\hdf5-shared.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100190~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100190~1.0\x64\mt.exe --manifests  -- C:\PROGRA~2\MICROS~1\2019\ENTERP~1\VC\Tools\MSVC\1428~1.299\bin\Hostx64\x86\link.exe  @CMakeFiles\hdf5-shared.rsp  /out:bin\hdf5_D.dll /implib:bin\hdf5_D.lib /pdb:bin\hdf5_D.pdb /dll /version:200.0 /machine:X86 /nologo    /debug /INCREMENTAL  && cd ."
FAILED: bin/hdf5_D.dll bin/hdf5_D.lib 
cmd.exe /C "cd . && D:\downloads\tools\cmake-3.20.2-windows\cmake-3.20.2-windows-i386\bin\cmake.exe -E vs_link_dll --intdir=src\CMakeFiles\hdf5-shared.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100190~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100190~1.0\x64\mt.exe --manifests  -- C:\PROGRA~2\MICROS~1\2019\ENTERP~1\VC\Tools\MSVC\1428~1.299\bin\Hostx64\x86\link.exe  @CMakeFiles\hdf5-shared.rsp  /out:bin\hdf5_D.dll /implib:bin\hdf5_D.lib /pdb:bin\hdf5_D.pdb /dll /version:200.0 /machine:X86 /nologo    /debug /INCREMENTAL  && cd ."
LINK Pass 1: command "C:\PROGRA~2\MICROS~1\2019\ENTERP~1\VC\Tools\MSVC\1428~1.299\bin\Hostx64\x86\link.exe @CMakeFiles\hdf5-shared.rsp /out:bin\hdf5_D.dll /implib:bin\hdf5_D.lib /pdb:bin\hdf5_D.pdb /dll /version:200.0 /machine:X86 /nologo /debug /INCREMENTAL /MANIFEST /MANIFESTFILE:src\CMakeFiles\hdf5-shared.dir/intermediate.manifest src\CMakeFiles\hdf5-shared.dir/manifest.res" failed (exit code 1120) with the following output:
   Creating library bin\hdf5_D.lib and object bin\hdf5_D.exp
H5Z.c.obj : error LNK2019: unresolved external symbol _SZ_encoder_enabled referenced in function _H5Z__init_package
H5Zszip.c.obj : error LNK2019: unresolved external symbol _SZ_BufftoBuffCompress referenced in function _H5Z_can_apply_szip
H5Zszip.c.obj : error LNK2019: unresolved external symbol _SZ_BufftoBuffDecompress referenced in function _H5Z_can_apply_szip

@dg0yt
Copy link
Contributor Author

dg0yt commented May 15, 2021

When building hdf5:x86-windows:

I will take a look. I even did builds with hdf5 but this didn't show up.

Basically, for triplets with dynamic linkage,

  • the static lib is no longer available,
  • the default component in cmake config is "shared" now,
  • there is a preprocessor macro which must be used when accessing the headers (It is in the exported cmake config.)
  • hdf5's own FindSZIP might be not as outdated as the hdf5 portfile indicates.

I will push the hdf5 changes on top of this PR. I need to fix mingw and pc files.
For purpose of fixing hdf5 mingw (also the import library problem) and pc files, I want to fix the "unusual lib prefix" of the static libs (for mingw at least) - this simplifies fixing the pc files.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 15, 2021

  • there is a preprocessor macro which must be used when accessing the headers (It is in the exported cmake config.)

SZ_BUILT_AS_DYNAMIC_LIB is not used when compiling hdf5...
I will modify the szip headers for dynamic linkage. This covers all type of uses.

@dg0yt dg0yt marked this pull request as draft May 15, 2021 07:12
@dg0yt
Copy link
Contributor Author

dg0yt commented May 15, 2021

Triplet release-static debug-static release-dynamic debug-dynamic
mingw with this PR libszip.a libszip_debug.a libszip.dll.a libszip_debug.dll.a
(= same namespec as linux, osx)

This is now extended to hdf5, and fixes hdf5 build failures with mingw (but no hdf5 cross-builds).

  • The next hdf5 release will allow mingw lib names per option.
  • hdf5 has a somewhat strange way of linking "components". It collects "static" and "shared" libs (zlib, szip) but links only to the static ones. That's why the first CI runs failed after changing szip to single linkage.
  • For passing dependencies of hdf5 via pkg-config, I decided to substitute the imported linker flags by corresponding pkg-config module names (d55c227). With this approach, all library name and flag fixup is local within the exporting package (zlib, szip, openmpi), and the importing packages doesn't need difficult pattern matching for names or for optimized vs. debug.
    I added a pc-file to szip for this purpose.
  • For hdf5, the mingw namespec is almost as on linux/osx, only the debug postfix is "_D" as for windows.

@dg0yt dg0yt changed the title [szip] Fix mingw import lib names, control linkage [szip, hdf5] Fix mingw import lib names, control linkage May 15, 2021
@dg0yt dg0yt marked this pull request as ready for review May 15, 2021 19:32
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label May 18, 2021
Copy link
Contributor

@longnguyen2004 longnguyen2004 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this PR! I always felt my approach was wrong, since mingw import libs aren't supposed to have the extension .lib, so I'm very thankful that a proper fix is coming.

ports/szip/portfile.cmake Outdated Show resolved Hide resolved
ports/szip/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label May 19, 2021
@JackBoosY
Copy link
Contributor

@longnguyen2004 Do you have any suggestions?

@longnguyen2004
Copy link
Contributor

LGTM

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label May 21, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented May 25, 2021

"-lszip-static" still seems to slip into the linux hdf5 pc files. I need to verify this again.

@dg0yt dg0yt marked this pull request as draft May 25, 2021 08:16
@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label May 25, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented May 25, 2021

Turned out that the repeated find_package(SZIP...) (first failing, then successful) accumulates libs. I updated the patch to handle it gracefully.

@dg0yt dg0yt marked this pull request as ready for review May 25, 2021 17:33
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label May 26, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Jun 7, 2021

Ping.

@strega-nil-ms
Copy link
Contributor

LGTM, thanks @dg0yt :)

@strega-nil-ms strega-nil-ms merged commit c867e68 into microsoft:master Jun 9, 2021
@dg0yt dg0yt deleted the szip branch June 17, 2021 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants