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

cmake: add support for transitive ZLIB target #3123

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@skirk
Contributor

skirk commented Oct 10, 2018

Create transitive CMake targets for ZLIB similarly to OpenSSL. The changes follow the convention as in #3055.

@bagder bagder added the cmake label Oct 10, 2018

@skirk

This comment has been minimized.

Contributor

skirk commented Oct 11, 2018

This still also needs accompanying changes to CMake/curl-config.cmake.in. I will add that to the PR later today.

@skirk

This comment has been minimized.

Contributor

skirk commented Oct 12, 2018

I would like to fix the appveyor build, but I fail to understand what's the issue from the logs. Any ideas?

@bagder

This comment has been minimized.

Member

bagder commented Oct 24, 2018

I think the appveyor failure is just a flaky test and not your fault. I'll retrigger the build.

@bagder bagder requested a review from snikulov Oct 24, 2018

@snikulov

This comment has been minimized.

Member

snikulov commented Oct 26, 2018

@skirk Could you please elaborate more what issue you've tried to solve?
Are any particular troubles without those changes?

@skirk

This comment has been minimized.

Contributor

skirk commented Oct 26, 2018

Hi @snikulov,

In my use case, I want to link ZLIB statically. Currently the default implementation of FindZlib favours
a shared library and that gets hard coded into INTERFACE_LINK_LIBRARIES of generated libcurl-target.cmake which forces shared linkage for the client app too.

The transitive target gives more control to the library user to control whether to link statically or shared without relying on clumsy hacks.

Also in terms of future proofing if usage requirements of ZLIB for some reason change, they will be transitioned with the target to the client app rather than being decomposed to _LIBS and _INCLUDE_DIRS variables which might strip some of those requirements.

@@ -1,9 +1,15 @@
@PACKAGE_INIT@
if("@USE_OPENSSL@")
if("@USE_OPENSSL@" OR "@CURL_ZLIB@" )
include(CMakeFindDependencyMacro)

This comment has been minimized.

@snikulov

snikulov Oct 26, 2018

Member

I believe somebody will add another dependency shortly.
So why not include CMakeFindDependencyMacro unconditionally?

include(CMakeFindDependencyMacro)
if("@USE_OPENSSL@")
  find_dependency(OpenSSL "@OPENSSL_VERSION_MAJOR@")
endif()
if("@USE_ZLIB@")
  find_dependency(ZLIB "@ZLIB_VERSION_MAJOR@")
endif()

find_dependency(OpenSSL "@OPENSSL_VERSION_MAJOR@")
if ("@USE_OPENSSL@")
find_dependency(OpenSSL "@OPENSSL_VERSION_MAJOR@")
endif()

This comment has been minimized.

@snikulov

snikulov Oct 26, 2018

Member

AFAIR, indentation used in curl project is 2 spaces. Could you please fix? Looks like it has lurked from the previous review.

@skirk

This comment has been minimized.

Contributor

skirk commented Oct 27, 2018

The commit above implements the requested changes. I also noticed the HAVE_ZLIB variable wasn't used anywhere so I changed it to USE_ZLIB to be consistent with the USE_OPENSSL.

Also removed the unnecessary quotes from the @variables so the results after configure_file is:
if(ON)
rather than
if("ON")

@snikulov

LGTM

@bagder

This comment has been minimized.

Member

bagder commented Oct 29, 2018

Thanks!

@bagder bagder closed this in e97679a Oct 29, 2018

xquery added a commit to xquery/curl that referenced this pull request Nov 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment