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

CMake: Respect BUILD_SHARED_LIBS #2755

Closed
wants to merge 1 commit into from
Closed

CMake: Respect BUILD_SHARED_LIBS #2755

wants to merge 1 commit into from

Conversation

ruslo
Copy link
Contributor

@ruslo ruslo commented Jul 17, 2018

Use standard CMake variable BUILD_SHARED_LIBS instead of introducing
custom option CURL_STATICLIB.

@jzakrzewski
Copy link
Contributor

I am not sure about this (even though I actually prefer using standard ways of doing things) because it inverts the default we had. I feel that

  • most of people want dynamic libs
  • building and using static libs is more involved

But the CMake documentation says: "This variable is often added to projects as an OPTION so that each user of a project can decide if they want to build the project using shared or static libraries."
If it would be an OPTION, we could probably have it defaulted to ON.

@ruslo
Copy link
Contributor Author

ruslo commented Jul 18, 2018

because it inverts the default we had

option added, by default shared library will be build.

@snikulov
Copy link
Member

@ruslo Appveyour should also be updated not to use CURL_STATICLIB

Use standard CMake variable BUILD_SHARED_LIBS instead of introducing
custom option CURL_STATICLIB.

Use '-DBUILD_SHARED_LIBS=%SHARED%' in appveyor.yml.
@ruslo
Copy link
Contributor Author

ruslo commented Jul 19, 2018

Appveyour should also be updated not to use CURL_STATICLIB

Done

@bagder bagder added the cmake label Jul 20, 2018
@bagder
Copy link
Member

bagder commented Jul 20, 2018

CURL_STATICLIB is not really "introduced" by this code. The define is used and known by the public curl/curl.h header. See

#ifdef CURL_STATICLIB

@ruslo
Copy link
Contributor Author

ruslo commented Jul 21, 2018

CURL_STATICLIB is not really "introduced" by this code. The define is used and known by the public curl/curl.h header. See

@bagder C++ part is not affected by this change, CURL_SHATICLIB still defined by curl_config.h header, see configure_file step:

if(BUILD_SHARED_LIBS)
  set(CURL_STATICLIB NO)
else()
  set(CURL_STATICLIB YES)
endif()

# Use:
# * CURL_STATICLIB
 configure_file(curl_config.h.cmake
   ${CMAKE_CURRENT_BINARY_DIR}/curl_config.h)

@snikulov
Copy link
Member

LGTM 👍

@snikulov snikulov self-requested a review July 23, 2018 18:29
@bagder
Copy link
Member

bagder commented Aug 8, 2018

Thanks!

@bagder bagder closed this in c892795 Aug 8, 2018
@ruslo ruslo deleted the pr.build.shared.libs branch August 8, 2018 10:10
xquery pushed a commit to xquery/curl that referenced this pull request Aug 9, 2018
Use standard CMake variable BUILD_SHARED_LIBS instead of introducing
custom option CURL_STATICLIB.

Use '-DBUILD_SHARED_LIBS=%SHARED%' in appveyor.yml.

Reviewed-by: Sergei Nikulov
Closes curl#2755
falconindy pushed a commit to falconindy/curl that referenced this pull request Sep 10, 2018
Use standard CMake variable BUILD_SHARED_LIBS instead of introducing
custom option CURL_STATICLIB.

Use '-DBUILD_SHARED_LIBS=%SHARED%' in appveyor.yml.

Reviewed-by: Sergei Nikulov
Closes curl#2755
@lock lock bot locked as resolved and limited conversation to collaborators Nov 6, 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.

None yet

4 participants