Skip to content

Well, enable ZLIB_FOUND could be able specify by parent CMakeLists.tx… - #917

Closed
lygstate wants to merge 2 commits into
curl:masterfrom
lygstate:master
Closed

Well, enable ZLIB_FOUND could be able specify by parent CMakeLists.tx…#917
lygstate wants to merge 2 commits into
curl:masterfrom
lygstate:master

Conversation

@lygstate

Copy link
Copy Markdown
Contributor

…t directly without the need to find_package, that's would be useful when linkage to static zlib directly.

…t directly without the need to find_package, that's would be useful when linkage to static zlib directly.
@mention-bot

Copy link
Copy Markdown

@lygstate, thanks for your PR! By analyzing the annotation information on this pull request, we identified @billhoffman, @Sukender and @Lekensteyn to be potential reviewers

@snikulov

Copy link
Copy Markdown
Contributor

@lygstate Could you please describe what problem you trying to solve?

@lygstate

Copy link
Copy Markdown
Contributor Author

@snikulov I was trying to include libcurl in another CMake based system.
For example:

  add_subdirectory(zlib)



  set(BUILD_TESTING OFF CACHE BOOL "Disable curl tests")
  set(ZLIB_FOUND ON)
  set(ZLIB_INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/zlib ${CMAKE_CURRENT_BINARY_DIR}/zlib)
  set(ZLIB_LIBRARIES zlibstatic)
  add_subdirectory(curl)
  target_include_directories(libcurl
    PUBLIC
    ${CMAKE_CURRENT_SOURCE_DIR}/curl/include
    ${CURL_BINARY_DIR}/include/curl
  )

So I could link zlib into libcurl as static library. And building zlib directly from souce without need the pre-build binaries

@snikulov

Copy link
Copy Markdown
Contributor

@lygstate I don't think that adding more checks and conditions into curl's CMakeLists.txt for your personal reasons is a good idea, sorry. It is complicated enough already.
In addition, you trying to put more commits within single PR.
It is also bad idea, IMHO.

@jay

jay commented Jul 15, 2016

Copy link
Copy Markdown
Member

What is wrong with the call to check_library_exists_concat?

@snikulov

Copy link
Copy Markdown
Contributor

@jay it's second unrelated to zlib change, which is not described in PR.

@jay

jay commented Jul 16, 2016

Copy link
Copy Markdown
Member

it's second unrelated to zlib change, which is not described in PR.

Possibly but I still want to know why he did it. As far as the zlib change goes I don't know enough about cmake build system to know whether this is a typical thing to want. It seems like he wants to override our settings with his own. Is that typical in cmake?

@lygstate

Copy link
Copy Markdown
Contributor Author

@jay when I trying to compiling under win64 it's failed, that's why I am not calling to check_library_exists_concat directly

@jay

jay commented Jul 18, 2016

Copy link
Copy Markdown
Member

@jay when I trying to compiling under win64 it's failed, that's why I am not calling to check_library_exists_concat directly

How did it fail, can you give us any more information? I can't see why it would fail.

@bagder bagder added the cmake label Jul 21, 2016
@jay jay added the needs-info label Jul 23, 2016
jay pushed a commit that referenced this pull request Jul 28, 2016
The check_library_exists_concat do not check crypt32 library properly.
So include it directly.

Bug: #917
Reported-by: Yonggang Luo

Bug: #935
Reported-by: Alain Danteny
@jay

jay commented Jul 28, 2016

Copy link
Copy Markdown
Member

@lygstate your fix for the crypt32 issue has landed in 2bbed9c, thanks.

@jay jay removed the needs-info label Jul 29, 2016
@bagder

bagder commented Oct 16, 2016

Copy link
Copy Markdown
Member

No response for months. Dead.

@bagder bagder closed this Oct 16, 2016
@lock lock Bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Development

Successfully merging this pull request may close these issues.

5 participants