[zlib] Add cmake wrapper #18914

dg0yt · 2021-07-11T20:52:53Z

What does your PR fix?

This PR adds a cmake wrapper for zlib which ensures that ZLIB::ZLIB and ZLIB_LIBRARIES are actually properly configured for release (optimized) and debug variant. It fixes inaccurate passing of captured libraries as reported (and attempted to fix) in [hdf5] Link zlib using the ZLIB::ZLIB target #18905.
Which triplets are supported/not supported? Have you updated the CI baseline?

all, no
Does your PR follow the maintainer guide?

Changes limited to adding the wrapper.
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

yes.

Tested with local cmake project.
Not tested with cmake < 3.15 due to #18898.

CC @Neumann-A who pointed out the issue's with CMake's FindZLIB module in #18905.

Neumann-A · 2021-07-11T22:00:41Z

ports/zlib/vcpkg-cmake-wrapper.cmake

+if(NOT ZLIB_ROOT OR ZLIB_ROOT STREQUAL "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}")
+    set(ZLIB_ROOT "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}")
+    find_path(ZLIB_INCLUDE_DIR NAMES zlib.h PATHS "${ZLIB_ROOT}/include" NO_DEFAULT_PATH)
+    find_library(ZLIB_LIBRARY_RELEASE NAMES zlib  z PATHS "${ZLIB_ROOT}/lib" NO_DEFAULT_PATH)
+    find_library(ZLIB_LIBRARY_DEBUG   NAMES zlibd z PATHS "${ZLIB_ROOT}/debug/lib" NO_DEFAULT_PATH)
+    if(NOT ZLIB_INCLUDE_DIR OR NOT ZLIB_LIBRARY_RELEASE OR (NOT ZLIB_LIBRARY_DEBUG AND EXISTS "${ZLIB_ROOT}/debug/lib"))
+        message("Broken installation of vcpkg port zlib")
+    endif()
+    if(CMAKE_VERSION VERSION_LESS 3.4)
+        find_package(SelectLibraryConfigurations)
+        select_library_configurations(ZLIB)
+        unset(ZLIB_FOUND)
+    endif()
+endif()


Why so complicated? If the wrapper is installed and called you can automatically assume that you are wanting zlib from vcpkg (because otherwise the wrapper would not be installed).

Suggested change

if(NOT ZLIB_ROOT OR ZLIB_ROOT STREQUAL "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}")

set(ZLIB_ROOT "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}")

find_path(ZLIB_INCLUDE_DIR NAMES zlib.h PATHS "${ZLIB_ROOT}/include" NO_DEFAULT_PATH)

find_library(ZLIB_LIBRARY_RELEASE NAMES zlib z PATHS "${ZLIB_ROOT}/lib" NO_DEFAULT_PATH)

find_library(ZLIB_LIBRARY_DEBUG NAMES zlibd z PATHS "${ZLIB_ROOT}/debug/lib" NO_DEFAULT_PATH)

if(NOT ZLIB_INCLUDE_DIR OR NOT ZLIB_LIBRARY_RELEASE OR (NOT ZLIB_LIBRARY_DEBUG AND EXISTS "${ZLIB_ROOT}/debug/lib"))

message("Broken installation of vcpkg port zlib")

endif()

if(CMAKE_VERSION VERSION_LESS 3.4)

find_package(SelectLibraryConfigurations)

select_library_configurations(ZLIB)

unset(ZLIB_FOUND)

endif()

endif()

set(ZLIB_ROOT "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}")

find_path(ZLIB_INCLUDE_DIR NAMES zlib.h PATHS "${ZLIB_ROOT}/include" NO_DEFAULT_PATH)

find_library(ZLIB_LIBRARY_RELEASE NAMES zlib z PATHS "${ZLIB_ROOT}/lib" NO_DEFAULT_PATH)

find_library(ZLIB_LIBRARY_DEBUG NAMES zlibd z PATHS "${ZLIB_ROOT}/debug/lib" NO_DEFAULT_PATH)

the rest should be handled by the find_package call (e.g. select_library_configurations and stuff).

I really tried to minimize what is called complicated now. The purpose of the code is:

Ensure that the libraries are not looked up in another path if not found in vcpkg. You may perhaps rely on zlib.h, but no so much on the libs: Most platforms come with some version of zlib. The library names vary with the platform. The community triplets are not even tested by CI. If the module does its own search, it may find a libary in a different place, so that a configuration failure would go unnoticed.

But do not break release-only triplets.

Ensure that configuration is done correctly even in CMake 3.1 ... 3.3. where the module does not call select_library_configurations.

Of course, things might be more compact with a standardized vcpkg_find_libraries which would encapsulate the vcpkg paths and the checks for existance:

vcpkg_find_libraries(ZLIB HEADER zlib.h LIBRARY_NAMES_RELEASE zlib z LIBRARY_NAMES_DEBUG zlibd z )

(Leaving the select_library_configurations as proposed.)

dg0yt · 2021-07-12T06:39:40Z

ports/zlib/vcpkg-cmake-wrapper.cmake

@@ -0,0 +1,15 @@
+if(NOT ZLIB_ROOT OR ZLIB_ROOT STREQUAL "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}")


Instead of the VCPKG variables, this could use ${CMAKE_CURRENT_LIST_DIR}/../..

... but ${CMAKE_CURRENT_LIST_DIR}/../.. would not work well for comparing with a user-defined VCPKG_ROOT.
The question if a user-defined VCPKG_ROOT is acceptable for a vcpkg build system. For ABI consistency, the user would need an overlay port for zlib, thus turning of the wrapper anyway.

dg0yt · 2021-07-12T07:31:02Z

python-3:x64-osx: Seems unrelated:

install: mkdir /Users/vagrant/Data/packages/python3_x64-osx/Users/vagrant/Data/installed/x64-osx/debug/lib: File exists
make: *** [altbininstall] Error 71
make: *** Waiting for unfinished jobs....

sentry-native:x64-osx: Gotcha, stumbles over cmake's optimized/debug variant configurations for zlib:

FAILED: crashpad_build/handler/crashpad_handler 
: && /Library/Developer/CommandLineTools/usr/bin/c++ -fPIC -g -arch x86_64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX11.1.sdk -Wl,-search_paths_first -Wl,-headerpad_max_install_names  crashpad_build/handler/CMakeFiles/crashpad_handler.dir/main.cc.o -o crashpad_build/handler/crashpad_handler  crashpad_build/client/libcrashpad_client.a  crashpad_build/handler/libcrashpad_handler_lib.a  crashpad_build/minidump/libcrashpad_minidump.a  crashpad_build/snapshot/libcrashpad_snapshot.a  crashpad_build/tools/libcrashpad_tools.a  crashpad_build/util/libcrashpad_util.a  crashpad_build/third_party/mini_chromium/libmini_chromium.a  crashpad_build/client/libcrashpad_client.a  crashpad_build/util/libcrashpad_util.a  -loptimized  /Users/vagrant/Data/installed/x64-osx/lib/libz.a  -ldebug  /Users/vagrant/Data/installed/x64-osx/debug/lib/libz.a  -lbsm  crashpad_build/third_party/mini_chromium/libmini_chromium.a  -framework ApplicationServices  -framework CoreFoundation  -framework Foundation  -framework IOKit  -framework Security && /Users/vagrant/Data/installed/x64-osx/tools/llvm/dsymutil crashpad_build/handler/crashpad_handler && :
ld: library not found for -loptimized
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

Neumann-A · 2021-07-12T07:42:41Z

try adding cmake_minimum_required (VERSION 3.10) unconditionally to the toplevel cmakelists.txt of sentry-native

dg0yt · 2021-07-12T07:58:45Z

Port sentry-native builds crashpad as a git submodule (i.e. it doesn't use (and depend on) the vcpkg port).

Port sentry-native submodule crashpad may use zlib as a submodule (default mode for MSVC!), but otherwise has it is own "processing" of zlib linking options (https://github.com/getsentry/crashpad/blob/5cf3032b2281cf0928acc8bccf69f91ccf26b939/third_party/zlib/CMakeLists.txt).

But port sentry-native doesn't depend on port zlib, so build results are ... a lottery?

cenit · 2021-07-12T08:21:52Z

ports/zlib/vcpkg-cmake-wrapper.cmake

+        message("Broken installation of vcpkg port zlib")
+    endif()
+    if(CMAKE_VERSION VERSION_LESS 3.4)
+        find_package(SelectLibraryConfigurations)


this script is not a module. Why find_package it and not include?
also, why only for cmake version less than 3.4?

sorry for the questions, but it's the first time i see something like this

You are right about find_package. I was just typing too many find_package calls recently ;-)

The version 3.4 thing is because CMake's find_module isn't handling select_library_configurations before CMake 3.4. vcpkg claims to support CMake 3.1 for user projects (but doesn't not notice when it breaks), and zlib seems to popular to have a higher requirement than vcpkg.cmake.

vcpkg claims to support CMake 3.1 for user projects

"claims" let us just assume the quotes and don't care about anything less then 3.7 ;)
Even if a project does
cmake_minimum_required (VERSION 3.1)
if it is configured with a newer version the modules won't be downgraded with it. The reason vcpkg.cmake needs to assume 3.1 is due to policy stuff otherwise not working correctly.

The version 3.4 thing is because CMake's find_module isn't handling select_library_configurations before CMake 3.4.

any sources for that, please?

https://gitlab.kitware.com/cmake/cmake/-/blob/650c630a663b2d0663e6ef6acda2a5fb94e5a621/Modules/FindZLIB.cmake

vs
https://gitlab.kitware.com/cmake/cmake/-/blob/b4a2ada297214119647b26df8abe394cd73ca53a/Modules/FindZLIB.cmake

ah ok sorry it's a thing specific for this module. I was under the impression that it was meant to be true in general in cmake

The reason vcpkg.cmake needs to assume 3.1 is due to policy stuff otherwise not working correctly.

I would be fine with that, but this is not what I take out from the statements by @strega-nil-ms, just 14 days ago: [1] [2]

"we really do support CMake version back to CMake 3.1 in vcpkg.cmake. There are extra features that require more recent versions, but we do support back to CMake 3.1 (or there's a bug.)"

"All we need is that, if a port supports CMake 3.1, and the user uses 3.1 for their build, it works."

Now I could get away with saying the port zlib doesn't support CMake < 3.4. But if CMake 3.1 isn't enough evev for port zlib, which other port would support that version?

(I'm not arguing for staying with cmake 3.1. I just want to know in advance how to prepare my PRs.)

dg0yt · 2021-07-13T05:33:09Z

What is wrong with arm64-windows now? Both zlib:x64-windows (I assume host) and zlib:arm64-windows complain that the "files are already installed in D:/installed/arm64-windows and are in conflict with" the files from the fresh installation (x64) / build (arm64) of the package.
The problem isn't related to, and isn't limited to, zlib.

cenit · 2021-07-13T05:49:09Z

What is wrong with arm64-windows now? Both zlib:x64-windows (I assume host) and zlib:arm64-windows complain that the "files are already installed in D:/installed/arm64-windows and are in conflict with" the files from the fresh installation (x64) / build (arm64) of the package.
The problem isn't related to, and isn't limited to, zlib.

a port that’s installing his own third party dependencies instead of reusing those provided by vcpkg? it happened and might have happened again

Neumann-A · 2021-07-13T06:03:22Z

some CI bug where the /installed/ folder is not correctly cleaned up probably. Also saw it in some of my PRs with qt....

JackBoosY · 2021-07-13T06:10:08Z

What is wrong with arm64-windows now? Both zlib:x64-windows (I assume host) and zlib:arm64-windows complain that the "files are already installed in D:/installed/arm64-windows and are in conflict with" the files from the fresh installation (x64) / build (arm64) of the package.
The problem isn't related to, and isn't limited to, zlib.

Failure to remove_all_inside(D:\buildtrees) due to file D:\buildtrees\tensorflow-cc\.bzl\install\3d1356cc22901f271d47ddd780d95b50\A-server.jar: The process cannot access the file because it is being used by another process.
Creating failure logs output directory C:\a\1\a\failure-logs

We still don't know why this file was used by another process.

dg0yt · 2021-07-13T06:52:44Z

a port that’s installing his own third party dependencies instead of reusing those provided by vcpkg? it happened and might have happened again

Sure, just see sentry-native in this PR.
But I'm not concerned about ports (which can be patched) but about user projects. Here, the user might set ZLIB_ROOT or related variables, and it won't even be passed to the ports installed via the manifest. It is not part of ABI tracking. So maybe we shouldn't just override ZLIB_ROOT but also unset cached variables for ZLIB_INCLUDE_DIR, ZLIB_LIBRARY_RELEASE and ZLIB_LIBRARY_DEBUG. Or verify that the final values are files in the expected directories.

dg0yt · 2021-07-24T20:55:58Z

I don't see support for CMake < 3.4 coming back, so this block could be removed.
But I don't want to change the PR while waiting for reviews.

JackBoosY · 2021-07-26T02:44:36Z

This PR will be reviewed these days.

dg0yt added 3 commits July 11, 2021 22:03

Format manifest

1943ed2

Add cmake wrapper

6375714

x-add-version

06afd99

dg0yt changed the title ~~Zlib wrapper~~ [zlib] Add cmake wrapper Jul 11, 2021

dg0yt mentioned this pull request Jul 11, 2021

[hdf5] Link zlib using the ZLIB::ZLIB target #18905

Closed

Neumann-A reviewed Jul 11, 2021

View reviewed changes

JonLiu1993 assigned JackBoosY Jul 12, 2021

JackBoosY added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist requires:author-response labels Jul 12, 2021

dg0yt commented Jul 12, 2021

View reviewed changes

JackBoosY added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed requires:author-response labels Jul 12, 2021

cenit reviewed Jul 12, 2021

View reviewed changes

dg0yt added 4 commits July 12, 2021 19:30

Always use vcpkg zlib for sentry native

fc193f5

x-add-version

41c3c83

Fix and simplify wrapper

9bae98d

Update git-tree

8eb360b

This was referenced Jul 19, 2021

How do I build DLLs from vcpkg into CPacked installer? #18943

Closed

[libxml2] fix wrapper #19033

Merged

vicroms approved these changes Jul 26, 2021

View reviewed changes

vicroms merged commit 7dbc055 into microsoft:master Jul 26, 2021

dg0yt deleted the zlib-wrapper branch July 29, 2021 05:43

ThomsonTan mentioned this pull request Sep 9, 2021

Enable CMake to search the new package variable <PackageName>_ROOT open-telemetry/opentelemetry-cpp#975

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[zlib] Add cmake wrapper #18914

[zlib] Add cmake wrapper #18914

dg0yt commented Jul 11, 2021

Neumann-A Jul 11, 2021

dg0yt Jul 12, 2021

dg0yt Jul 12, 2021

dg0yt Jul 12, 2021

dg0yt commented Jul 12, 2021

Neumann-A commented Jul 12, 2021

dg0yt commented Jul 12, 2021

cenit Jul 12, 2021

dg0yt Jul 12, 2021

Neumann-A Jul 12, 2021

cenit Jul 12, 2021

Neumann-A Jul 12, 2021

Neumann-A Jul 12, 2021

cenit Jul 12, 2021

dg0yt Jul 12, 2021

dg0yt commented Jul 13, 2021

cenit commented Jul 13, 2021

Neumann-A commented Jul 13, 2021

JackBoosY commented Jul 13, 2021

dg0yt commented Jul 13, 2021

dg0yt commented Jul 24, 2021

JackBoosY commented Jul 26, 2021

		@@ -0,0 +1,15 @@
		if(NOT ZLIB_ROOT OR ZLIB_ROOT STREQUAL "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}")

[zlib] Add cmake wrapper #18914

[zlib] Add cmake wrapper #18914

Conversation

dg0yt commented Jul 11, 2021

What does your PR fix?

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

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dg0yt commented Jul 12, 2021

Neumann-A commented Jul 12, 2021

dg0yt commented Jul 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dg0yt commented Jul 13, 2021

cenit commented Jul 13, 2021

Neumann-A commented Jul 13, 2021

JackBoosY commented Jul 13, 2021

dg0yt commented Jul 13, 2021

dg0yt commented Jul 24, 2021

JackBoosY commented Jul 26, 2021

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?