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

[pulsar-client-cpp] Fix build failure when CMAKE_BUILD_TYPE is not specified #36147

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Jan 12, 2024

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

This patch mainly fixes the error that when CMAKE_BUILD_TYPE is not specified with a single-configuration generator, the build will fail because the imported location cannot be found.

For example, in <cmake-build-dir>/CMakeFiles/<target.dir>/build.cmake, there could be a line like:

<target>: unofficial::pulsar::pulsar-NOTFOUND

This patch fixes the error above by making unofficial::pulsar::pulsar links debug libraries by default.

In addition, fix the error that Snappy cannot be found on Linux and the logic to find libraries of the pulsar library.

@BewareMyPower
Copy link
Contributor Author

I verified the patch with the following triplets in GitHub actions, see BewareMyPower#2.

  • x64-windows
  • x64-windows-static
  • x64-linux
  • x64-linux-dynamic
  • x64-osx
  • x64-osx-dynamic

And the arm64-osx triplet is verified locally on my m1 laptop.

@BewareMyPower BewareMyPower marked this pull request as draft January 12, 2024 13:05
@BewareMyPower
Copy link
Contributor Author

For Unix Makefiles, when I ran cmake -B build and then ran cd build && make VERBOSE=1, I found the following static libraries were linked:

  • vcpkg_installed/arm64-osx/lib/libpulsar.a
  • vcpkg_installed/arm64-osx/debug/lib/libprotobufd.a
  • vcpkg_installed/arm64-osx/debug/lib/libcurl-d.a
  • vcpkg_installed/arm64-osx/debug/lib/libssl.a
  • vcpkg_installed/arm64-osx/debug/lib/libcrypto.a
  • vcpkg_installed/arm64-osx/lib/libz.a
  • vcpkg_installed/arm64-osx/debug/lib/libzstd.a
  • vcpkg_installed/arm64-osx/debug/lib/libsnappy.a

So I think the debug version of libpulsar.a should be linked with the default config. What confused me is that lib/libz.a but not debug/lib/libz.a is linked .

@BewareMyPower
Copy link
Contributor Author

Just found FindZLIB.cmake sets ZLIB_LIBRARY via select_library_configurations, which chooses the release library by default.

    if( ${basename}_LIBRARY_DEBUG AND ${basename}_LIBRARY_RELEASE AND
           NOT ${basename}_LIBRARY_DEBUG STREQUAL ${basename}_LIBRARY_RELEASE AND
           ( _isMultiConfig OR CMAKE_BUILD_TYPE ) )
        # ...
    elseif( ${basename}_LIBRARY_RELEASE )
        set( ${basename}_LIBRARY ${${basename}_LIBRARY_RELEASE} )
    elseif( ${basename}_LIBRARY_DEBUG )
        set( ${basename}_LIBRARY ${${basename}_LIBRARY_DEBUG} )
    else()
        set( ${basename}_LIBRARY "${basename}_LIBRARY-NOTFOUND")
    endif()

Also, after learning FindZLIB.cmake, it seems that target pattern contains no '%' was not caused by the IMPORTED_LOCATION not set. I will recheck the issue.

@BewareMyPower
Copy link
Contributor Author

@Neumann-A I updated the PR description and the cmake code, PTAL again. Now with Makefile as the generator, all debug libraries will be linked when CMAKE_BUILD_TYPE is not specified. See the verbose output on my local env:

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -g -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk -Wl,-search_paths_first -Wl,-headerpad_max_install_names CMakeFiles/HelloWorld.dir/helloworld.cpp.o -o HelloWorld vcpkg_installed/arm64-osx/debug/lib/libpulsar.a vcpkg_installed/arm64-osx/debug/lib/libprotobufd.a vcpkg_installed/arm64-osx/debug/lib/libcurl-d.a vcpkg_installed/arm64-osx/debug/lib/libssl.a vcpkg_installed/arm64-osx/debug/lib/libcrypto.a vcpkg_installed/arm64-osx/debug/lib/libz.a -framework SystemConfiguration -framework CoreFoundation -framework CoreServices vcpkg_installed/arm64-osx/debug/lib/libzstd.a vcpkg_installed/arm64-osx/debug/lib/libsnappy.a

@BewareMyPower BewareMyPower marked this pull request as ready for review January 13, 2024 16:46
@JonLiu1993 JonLiu1993 self-assigned this Jan 15, 2024
@JonLiu1993 JonLiu1993 added the category:port-bug The issue is with a library, which is something the port should already support label Jan 15, 2024
@JonLiu1993
Copy link
Member

Tested usage successfully by pulsar-client-cpp:x64-windows:

pulsar-client-cpp provides CMake targets:

  find_package(unofficial-pulsar CONFIG REQUIRED)
  target_link_libraries(main PRIVATE unofficial::pulsar::pulsar)

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Jan 16, 2024
@JavierMatosD JavierMatosD merged commit 38d1652 into microsoft:master Jan 17, 2024
15 checks passed
@BewareMyPower BewareMyPower deleted the fix-pulsar-find-library branch January 19, 2024 09:25
Osyotr pushed a commit to Osyotr/vcpkg that referenced this pull request Jan 23, 2024
…ecified (microsoft#36147)

* [pulsar-client-cpp] Fix build failure when CMAKE_BUILD_TYPE is not specified

* Force finding debug libraries by default
TomKatom pushed a commit to TomKatom/vcpkg that referenced this pull request Feb 23, 2024
…ecified (microsoft#36147)

* [pulsar-client-cpp] Fix build failure when CMAKE_BUILD_TYPE is not specified

* Force finding debug libraries by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

4 participants