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
base: master
from

Conversation

Projects
None yet
4 participants
@ruslo
Contributor

ruslo commented Jul 17, 2018

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

@jzakrzewski

This comment has been minimized.

Show comment
Hide comment
@jzakrzewski

jzakrzewski Jul 18, 2018

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.

Contributor

jzakrzewski commented Jul 18, 2018

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

This comment has been minimized.

Show comment
Hide comment
@ruslo

ruslo Jul 18, 2018

Contributor

because it inverts the default we had

option added, by default shared library will be build.

Contributor

ruslo commented Jul 18, 2018

because it inverts the default we had

option added, by default shared library will be build.

@snikulov

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov Jul 19, 2018

Member

@ruslo Appveyour should also be updated not to use CURL_STATICLIB

Member

snikulov commented Jul 19, 2018

@ruslo Appveyour should also be updated not to use CURL_STATICLIB

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

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

This comment has been minimized.

Show comment
Hide comment
@ruslo

ruslo Jul 19, 2018

Contributor

Appveyour should also be updated not to use CURL_STATICLIB

Done

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jul 20, 2018

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@ruslo

ruslo Jul 21, 2018

Contributor

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)
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov Jul 23, 2018

Member

LGTM 👍

Member

snikulov commented Jul 23, 2018

LGTM 👍

@snikulov snikulov self-requested a review Jul 23, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 8, 2018

Member

Thanks!

Member

bagder commented Aug 8, 2018

Thanks!

@bagder bagder closed this in c892795 Aug 8, 2018

@ruslo ruslo deleted the ruslo:pr.build.shared.libs branch Aug 8, 2018

xquery added a commit to xquery/curl that referenced this pull request Aug 9, 2018

CMake: Respect BUILD_SHARED_LIBS
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 #2755

falconindy added a commit to falconindy/curl that referenced this pull request Sep 10, 2018

CMake: Respect BUILD_SHARED_LIBS
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 #2755
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment