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: Support for DarwinSSL and mbedTLS + cleanup #1228

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@ligfx
Contributor

ligfx commented Jan 26, 2017

No description provided.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jan 26, 2017

@ligfx, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Lekensteyn, @jzakrzewski and @jgsogo to be potential reviewers.

mention-bot commented Jan 26, 2017

@ligfx, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Lekensteyn, @jzakrzewski and @jgsogo to be potential reviewers.

@jay jay added the cmake label Jan 26, 2017

Show outdated Hide outdated CMake/FindMbedTLS.cmake
Show outdated Hide outdated CMakeLists.txt
@webmaster128

Nice, thanks for pushing macOS+cmake. I'll test this on a Mac soon. The current change does not break my custom OpenSSL configuration on Linux, so 👍

Show outdated Hide outdated CMakeLists.txt
@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Feb 20, 2017

Contributor

When I apply this patch, I get a runtime error when setting CURLOPT_SSL_OPTIONS to CURLSSLOPT_NO_REVOKE. Any idea where this is coming from? It works with cmake build + OpenSSL and buildconf/configure build + DarwnSSL.

Contributor

webmaster128 commented Feb 20, 2017

When I apply this patch, I get a runtime error when setting CURLOPT_SSL_OPTIONS to CURLSSLOPT_NO_REVOKE. Any idea where this is coming from? It works with cmake build + OpenSSL and buildconf/configure build + DarwnSSL.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 20, 2017

Member

I get a runtime error

Can you elaborate on exactly what you get when you do what?

Member

bagder commented Feb 20, 2017

I get a runtime error

Can you elaborate on exactly what you get when you do what?

@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Feb 20, 2017

Contributor

The call of curl_easy_setopt with key CURLOPT_SSL_OPTIONS and value CURLSSLOPT_NO_REVOKE returns the code 48 = CURLE_UNKNOWN_OPTION.

Contributor

webmaster128 commented Feb 20, 2017

The call of curl_easy_setopt with key CURLOPT_SSL_OPTIONS and value CURLSSLOPT_NO_REVOKE returns the code 48 = CURLE_UNKNOWN_OPTION.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 20, 2017

Member

Oh, that would imply that the USE_SSL define isn't defined, looking at url.c just lines above the case CURLOPT_SSL_OPTIONS: line. That would rather imply that you get a build completely without SSL?

Member

bagder commented Feb 20, 2017

Oh, that would imply that the USE_SSL define isn't defined, looking at url.c just lines above the case CURLOPT_SSL_OPTIONS: line. That would rather imply that you get a build completely without SSL?

@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Feb 20, 2017

Contributor

I guess USE_SSL is something internal, that CMakeLists.txt should set, right? What is the difference between USE_SSL and SSL_ENABLED?

I cannot really tell what is included in the build. Since my application code fails when doing the above configuration, even my HTTP tests without SSL fail. Is the any useful debugging that I can perform on the .a file to answer your question?

Contributor

webmaster128 commented Feb 20, 2017

I guess USE_SSL is something internal, that CMakeLists.txt should set, right? What is the difference between USE_SSL and SSL_ENABLED?

I cannot really tell what is included in the build. Since my application code fails when doing the above configuration, even my HTTP tests without SSL fail. Is the any useful debugging that I can perform on the .a file to answer your question?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 20, 2017

Member

It is internal but it depends on one of the TLS enabled variables being set. See https://github.com/curl/curl/blob/master/lib/curl_setup.h#L618

Member

bagder commented Feb 20, 2017

It is internal but it depends on one of the TLS enabled variables being set. See https://github.com/curl/curl/blob/master/lib/curl_setup.h#L618

@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Feb 20, 2017

Contributor

I guess there is a USE_DARWINSSL block missing in my auto-generated curl_config.h.

Contributor

webmaster128 commented Feb 20, 2017

I guess there is a USE_DARWINSSL block missing in my auto-generated curl_config.h.

@webmaster128

While testing this change, I found that there is DarwinSSL block missing in curl_config.cmake, see https://github.com/curl/curl/pull/1270/files

@ligfx

This comment has been minimized.

Show comment
Hide comment
@ligfx

ligfx Feb 21, 2017

Contributor

Thanks so much for testing this out and investigating that issue, @webmaster128. I've cherry-picked your commit from the other PR into this one.

I also added another commit that checks that at most one SSL option is enabled. If multiple options are defined, it raises an error like this:

CMake Error at CMakeLists.txt:307 (message):
  Multiple SSL options specified: CMAKE_USE_DARWINSSL;CMAKE_USE_OPENSSL.
  Please pick at most one and disable the rest.
Contributor

ligfx commented Feb 21, 2017

Thanks so much for testing this out and investigating that issue, @webmaster128. I've cherry-picked your commit from the other PR into this one.

I also added another commit that checks that at most one SSL option is enabled. If multiple options are defined, it raises an error like this:

CMake Error at CMakeLists.txt:307 (message):
  Multiple SSL options specified: CMAKE_USE_DARWINSSL;CMAKE_USE_OPENSSL.
  Please pick at most one and disable the rest.
Show outdated Hide outdated CMakeLists.txt
Show outdated Hide outdated CMakeLists.txt
Show outdated Hide outdated CMakeLists.txt
@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Feb 21, 2017

Contributor

Great! I found some minor other stuff. I'll test again soon

Contributor

webmaster128 commented Feb 21, 2017

Great! I found some minor other stuff. I'll test again soon

Show outdated Hide outdated CMakeLists.txt
Show outdated Hide outdated CMakeLists.txt
Show outdated Hide outdated CMakeLists.txt
Show outdated Hide outdated CMakeLists.txt
@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Feb 21, 2017

Contributor

My runtime issue is gone now (just verified the current patch).

What now happens is the following:

// apply this PR onto current master and commit

$ git clean -xdf

$ cmake -DCMAKE_USE_DARWINSSL=1 -DHTTP_ONLY=1 -Wno-dev
// long healthy output ending with:
-- Enabled features: DarwinSSL IPv6 unix-sockets libz Largefile NTLM
-- Enabled protocols: HTTP HTTPS
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/me/myorg/curlwrapper/curl

$ cmake -DCMAKE_USE_DARWINSSL=1 -DHTTP_ONLY=1 -Wno-dev
CMake Warning at CMakeLists.txt:49 (message):
  the curl cmake build system is poorly maintained.  Be aware


-- curl version=[7.53.0-DEV]
Found *nroff option: -- -man
CMake Error at CMakeLists.txt:307 (message):
  Multiple SSL options specified: CMAKE_USE_DARWINSSL;CMAKE_USE_OPENSSL.
  Please pick at most one and disable the rest.


-- Configuring incomplete, errors occurred!

When you look at the cache, you see, that there are two CMAKE_USE_*SSL variables set:

//enable Apple OS native SSL/TLS
CMAKE_USE_DARWINSSL:BOOL=ON

//Use GSSAPI implementation (right now only Heimdal is supported
// with CMake build)
CMAKE_USE_GSSAPI:BOOL=OFF

//Use libSSH2
CMAKE_USE_LIBSSH2:BOOL=OFF

//Enable mbedTLS for SSL/TLS
CMAKE_USE_MBEDTLS:BOOL=OFF

//Use OpenSSL code. Experimental
CMAKE_USE_OPENSSL:BOOL=ON

This is because CMAKE_USE_OPENSSL is ON by default. It is not caugth in the first run because the option is set after the multiple ssl check is performed.

I think the most sustainable way to fix this is to default CMAKE_USE_OPENSSL to OFF.

Contributor

webmaster128 commented Feb 21, 2017

My runtime issue is gone now (just verified the current patch).

What now happens is the following:

// apply this PR onto current master and commit

$ git clean -xdf

$ cmake -DCMAKE_USE_DARWINSSL=1 -DHTTP_ONLY=1 -Wno-dev
// long healthy output ending with:
-- Enabled features: DarwinSSL IPv6 unix-sockets libz Largefile NTLM
-- Enabled protocols: HTTP HTTPS
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/me/myorg/curlwrapper/curl

$ cmake -DCMAKE_USE_DARWINSSL=1 -DHTTP_ONLY=1 -Wno-dev
CMake Warning at CMakeLists.txt:49 (message):
  the curl cmake build system is poorly maintained.  Be aware


-- curl version=[7.53.0-DEV]
Found *nroff option: -- -man
CMake Error at CMakeLists.txt:307 (message):
  Multiple SSL options specified: CMAKE_USE_DARWINSSL;CMAKE_USE_OPENSSL.
  Please pick at most one and disable the rest.


-- Configuring incomplete, errors occurred!

When you look at the cache, you see, that there are two CMAKE_USE_*SSL variables set:

//enable Apple OS native SSL/TLS
CMAKE_USE_DARWINSSL:BOOL=ON

//Use GSSAPI implementation (right now only Heimdal is supported
// with CMake build)
CMAKE_USE_GSSAPI:BOOL=OFF

//Use libSSH2
CMAKE_USE_LIBSSH2:BOOL=OFF

//Enable mbedTLS for SSL/TLS
CMAKE_USE_MBEDTLS:BOOL=OFF

//Use OpenSSL code. Experimental
CMAKE_USE_OPENSSL:BOOL=ON

This is because CMAKE_USE_OPENSSL is ON by default. It is not caugth in the first run because the option is set after the multiple ssl check is performed.

I think the most sustainable way to fix this is to default CMAKE_USE_OPENSSL to OFF.

@ligfx

This comment has been minimized.

Show comment
Hide comment
@ligfx

ligfx Feb 21, 2017

Contributor

@webmaster128 Thanks for the further testing!

The Autoconf build defaults to enabling OpenSSL. I think enabling some form of SSL by default is a good idea since most users expect curl to be able to make secure connections.

I can move the option definitions to the top of "check for SSL" block, before the multiple SSL check is performed. In some ways that's cleaner than before, and makes it easier to do things like different defaults per-OS (if that's ever wanted). That would look like this:

option(CMAKE_USE_OPENSSL "Use OpenSSL code. Experimental" ON)
option(CMAKE_USE_MBEDTLS "Enable mbedTLS for SSL/TLS" OFF)
if(APPLE)
  option(CMAKE_USE_DARWINSSL "enable Apple OS native SSL/TLS" OFF)
endif()
if(WIN32)
  option(CMAKE_USE_WINSSL "enable Windows native SSL/TLS" OFF)
  cmake_dependent_option(CURL_WINDOWS_SSPI "Use windows libraries to allow NTLM authentication without openssl" ON
    CMAKE_USE_WINSSL OFF)
endif()

collect_true(enabled_ssl_options enabled_ssl_options_count
  CMAKE_USE_WINSSL
  CMAKE_USE_DARWINSSL
  CMAKE_USE_OPENSSL
  CMAKE_USE_MBEDTLS
)
if(enabled_ssl_options_count GREATER 1)
  message(FATAL_ERROR "Multiple SSL options specified: ${enabled_ssl_options}. Please pick at most one and disable the rest.")
endif()

if(CMAKE_USE_WINSSL)
  set(SSL_ENABLED ON)
  set(USE_SCHANNEL ON) # Windows native SSL/TLS support
  set(USE_WINDOWS_SSPI ON) # CMAKE_USE_WINSSL implies CURL_WINDOWS_SSPI
  list(APPEND CURL_LIBS "crypt32")
endif()
if(CURL_WINDOWS_SSPI)
  set(USE_WINDOWS_SSPI ON)
endif()

if(CMAKE_USE_DARWINSSL)
  find_library(COREFOUNDATION_FRAMEWORK "CoreFoundation")

Would that work for you?

Contributor

ligfx commented Feb 21, 2017

@webmaster128 Thanks for the further testing!

The Autoconf build defaults to enabling OpenSSL. I think enabling some form of SSL by default is a good idea since most users expect curl to be able to make secure connections.

I can move the option definitions to the top of "check for SSL" block, before the multiple SSL check is performed. In some ways that's cleaner than before, and makes it easier to do things like different defaults per-OS (if that's ever wanted). That would look like this:

option(CMAKE_USE_OPENSSL "Use OpenSSL code. Experimental" ON)
option(CMAKE_USE_MBEDTLS "Enable mbedTLS for SSL/TLS" OFF)
if(APPLE)
  option(CMAKE_USE_DARWINSSL "enable Apple OS native SSL/TLS" OFF)
endif()
if(WIN32)
  option(CMAKE_USE_WINSSL "enable Windows native SSL/TLS" OFF)
  cmake_dependent_option(CURL_WINDOWS_SSPI "Use windows libraries to allow NTLM authentication without openssl" ON
    CMAKE_USE_WINSSL OFF)
endif()

collect_true(enabled_ssl_options enabled_ssl_options_count
  CMAKE_USE_WINSSL
  CMAKE_USE_DARWINSSL
  CMAKE_USE_OPENSSL
  CMAKE_USE_MBEDTLS
)
if(enabled_ssl_options_count GREATER 1)
  message(FATAL_ERROR "Multiple SSL options specified: ${enabled_ssl_options}. Please pick at most one and disable the rest.")
endif()

if(CMAKE_USE_WINSSL)
  set(SSL_ENABLED ON)
  set(USE_SCHANNEL ON) # Windows native SSL/TLS support
  set(USE_WINDOWS_SSPI ON) # CMAKE_USE_WINSSL implies CURL_WINDOWS_SSPI
  list(APPEND CURL_LIBS "crypt32")
endif()
if(CURL_WINDOWS_SSPI)
  set(USE_WINDOWS_SSPI ON)
endif()

if(CMAKE_USE_DARWINSSL)
  find_library(COREFOUNDATION_FRAMEWORK "CoreFoundation")

Would that work for you?

@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Feb 21, 2017

Contributor

This means everyone using a different TLS provider has to explicitly disable OpenSSL, right? cmake -DCMAKE_USE_DARWINSSL=1 would not work anymore. I think the default should only be set when no other TLS provider is enabled.

Apart from that, the drafted version looks good to me.

Contributor

webmaster128 commented Feb 21, 2017

This means everyone using a different TLS provider has to explicitly disable OpenSSL, right? cmake -DCMAKE_USE_DARWINSSL=1 would not work anymore. I think the default should only be set when no other TLS provider is enabled.

Apart from that, the drafted version looks good to me.

@ligfx

This comment has been minimized.

Show comment
Hide comment
@ligfx

ligfx Feb 21, 2017

Contributor

@webmaster128 Okay, I've updated the PR with that logic.

Contributor

ligfx commented Feb 21, 2017

@webmaster128 Okay, I've updated the PR with that logic.

@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Feb 22, 2017

Contributor

All tests run green now :)

The only question I have left is, if there is an equivalent of --without-ca-bundle --without-ca-path from my old build script. Without this parameters I was having the issue that CA files where searched on a user's machine because they were present on the build machine. Is that in the scope of this PR?

Contributor

webmaster128 commented Feb 22, 2017

All tests run green now :)

The only question I have left is, if there is an equivalent of --without-ca-bundle --without-ca-path from my old build script. Without this parameters I was having the issue that CA files where searched on a user's machine because they were present on the build machine. Is that in the scope of this PR?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 22, 2017

Member

I suggest to take that in a separate PR, as that's rather independent of specific TLS backend.

Member

bagder commented Feb 22, 2017

I suggest to take that in a separate PR, as that's rather independent of specific TLS backend.

@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Feb 22, 2017

Contributor

Okay, so I am very happy with is right now.

Note: This PR changes behavior on Windows, where OpenSSL is now enabled by default and will fail with "OpenSSL not found" when OpenSSL is not found unless OpenSSL is disables explicitly. I guess this is okay, but still different to what we have in CMAKE on master.

Contributor

webmaster128 commented Feb 22, 2017

Okay, so I am very happy with is right now.

Note: This PR changes behavior on Windows, where OpenSSL is now enabled by default and will fail with "OpenSSL not found" when OpenSSL is not found unless OpenSSL is disables explicitly. I guess this is okay, but still different to what we have in CMAKE on master.

@ligfx

This comment has been minimized.

Show comment
Hide comment
@ligfx

ligfx Feb 22, 2017

Contributor

I didn't realize I changed that behavior. I've changed the PR so it acts the same way as before (doesn't default to OpenSSL on Windows).

Contributor

ligfx commented Feb 22, 2017

I didn't realize I changed that behavior. I've changed the PR so it acts the same way as before (doesn't default to OpenSSL on Windows).

@Lekensteyn

Looks mostly good, but add REQUIRED to find_package

Show outdated Hide outdated CMakeLists.txt
Show outdated Hide outdated CMakeLists.txt
Show outdated Hide outdated CMakeLists.txt
@Lekensteyn

This comment has been minimized.

Show comment
Hide comment
@Lekensteyn

Lekensteyn Feb 25, 2017

Member

Hmm strange, why is CI failing here: https://travis-ci.org/curl/curl/jobs/205374812#L3906

Maybe it is an intermittent error. Can you try to update the INSTALL.cmake file with the updated state for SSL library support and re-trigger CI?

Member

Lekensteyn commented Feb 25, 2017

Hmm strange, why is CI failing here: https://travis-ci.org/curl/curl/jobs/205374812#L3906

Maybe it is an intermittent error. Can you try to update the INSTALL.cmake file with the updated state for SSL library support and re-trigger CI?

@ligfx

This comment has been minimized.

Show comment
Hide comment
@ligfx

ligfx Feb 26, 2017

Contributor

@Lekensteyn Yeah it must be, none of the files touched in this PR are involved in that build process. I've updated the INSTALL.cmake file and re-pushed.

Contributor

ligfx commented Feb 26, 2017

@Lekensteyn Yeah it must be, none of the files touched in this PR are involved in that build process. I've updated the INSTALL.cmake file and re-pushed.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 26, 2017

Member

Hmm strange, why is CI failing here: https://travis-ci.org/curl/curl/jobs/205374812#L3906

From what I understand pipelining there is a timing component in what order the data is received so occasionally they fail.

Member

jay commented Feb 26, 2017

Hmm strange, why is CI failing here: https://travis-ci.org/curl/curl/jobs/205374812#L3906

From what I understand pipelining there is a timing component in what order the data is received so occasionally they fail.

@Lekensteyn

LGTM, still works on Arch Linux with OpenSSL.

@Lekensteyn

This comment has been minimized.

Show comment
Hide comment
@Lekensteyn

Lekensteyn Feb 26, 2017

Member

@jay When merging these patches, should only the last commit message to include "Closes ..."? (Is something else needed?)

Member

Lekensteyn commented Feb 26, 2017

@jay When merging these patches, should only the last commit message to include "Closes ..."? (Is something else needed?)

@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Feb 26, 2017

Contributor

Btw., is CMAKE_* really a naming convention that should be used for the public curl configuration API? As far as I can see, CMAKE_* variables usually come from the CMAKE system (CMAKE_SYSTEM_NAME, CMAKE_C_FLAGS and so on).

The name CMAKE_USE_OPENSSL was introduced in 2009 probably just to avoid a conflict with USE_OPENSSL.

Maybe those should better be prefixed with CURL_*

Contributor

webmaster128 commented Feb 26, 2017

Btw., is CMAKE_* really a naming convention that should be used for the public curl configuration API? As far as I can see, CMAKE_* variables usually come from the CMAKE system (CMAKE_SYSTEM_NAME, CMAKE_C_FLAGS and so on).

The name CMAKE_USE_OPENSSL was introduced in 2009 probably just to avoid a conflict with USE_OPENSSL.

Maybe those should better be prefixed with CURL_*

@ligfx

This comment has been minimized.

Show comment
Hide comment
@ligfx

ligfx Feb 27, 2017

Contributor

@webmaster128 I totally agree. That's a good focus for another PR.

Contributor

ligfx commented Feb 27, 2017

@webmaster128 I totally agree. That's a good focus for another PR.

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Feb 27, 2017

Member

When merging these patches, should only the last commit message to include "Closes ..."? (Is something else needed?)

Typically if it were me I would squash all commits or ask the reporter to do it. I see this is a few things in one PR and there are some distinct changes it may be useful leave separate, however this is more work.

'Closes' directive is never needed as a matter of protocol, it could be closed manually. What is important though is that we have some kind of a reference Closes/Fixes/Bug/Ref:, for example

Ref: https://github.com/curl/curl/pull/1228

Ideally something like that would near the end of each of these commit messages, so git blame nobody has to play detective. Off the top of my head I can't easily think of an automated way to append to all the commit messages at once.

Right now there is:

CMake: Add support for DarwinSSL
CMake: Reorganized SSL support, separated WinSSL and SSPI
CMake: Add mbedTLS support
CMake: at most one SSL library may be set
Add missing DarwinSSL block to curl_config.cmake
CMake: update INSTALL about new SSL lib support

At the very least the commit messages should be modified to meet the style format. That would involve changing the subjects to imperative present tense, adding missing area, ref, etc:

CMake: Add support for DarwinSSL
CMake: Reorganize SSL support, separate WinSSL and SSPI
CMake: Add mbedTLS support
CMake: Set at most one SSL library
CMake: Add missing DarwinSSL block to curl_config.cmake
CMake: update INSTALL about new SSL lib support

Then after rebase make sure to confirm that you're the committer on all of them, and the authors are left intact.

If it were me I'd lose at least these squashable 2:

Pick Add missing DarwinSSL block to curl_config.cmake into CMake: Add support for DarwinSSL and add a Assisted-by: Simon Warta line to that commit message.

Squash CMake: update INSTALL about new SSL lib support into CMake: at most one SSL library may be set.

CMake: Add support for DarwinSSL
CMake: Reorganize SSL support, separate WinSSL and SSPI
CMake: Add mbedTLS support
CMake: Set at most one SSL library
Member

jay commented Feb 27, 2017

When merging these patches, should only the last commit message to include "Closes ..."? (Is something else needed?)

Typically if it were me I would squash all commits or ask the reporter to do it. I see this is a few things in one PR and there are some distinct changes it may be useful leave separate, however this is more work.

'Closes' directive is never needed as a matter of protocol, it could be closed manually. What is important though is that we have some kind of a reference Closes/Fixes/Bug/Ref:, for example

Ref: https://github.com/curl/curl/pull/1228

Ideally something like that would near the end of each of these commit messages, so git blame nobody has to play detective. Off the top of my head I can't easily think of an automated way to append to all the commit messages at once.

Right now there is:

CMake: Add support for DarwinSSL
CMake: Reorganized SSL support, separated WinSSL and SSPI
CMake: Add mbedTLS support
CMake: at most one SSL library may be set
Add missing DarwinSSL block to curl_config.cmake
CMake: update INSTALL about new SSL lib support

At the very least the commit messages should be modified to meet the style format. That would involve changing the subjects to imperative present tense, adding missing area, ref, etc:

CMake: Add support for DarwinSSL
CMake: Reorganize SSL support, separate WinSSL and SSPI
CMake: Add mbedTLS support
CMake: Set at most one SSL library
CMake: Add missing DarwinSSL block to curl_config.cmake
CMake: update INSTALL about new SSL lib support

Then after rebase make sure to confirm that you're the committer on all of them, and the authors are left intact.

If it were me I'd lose at least these squashable 2:

Pick Add missing DarwinSSL block to curl_config.cmake into CMake: Add support for DarwinSSL and add a Assisted-by: Simon Warta line to that commit message.

Squash CMake: update INSTALL about new SSL lib support into CMake: at most one SSL library may be set.

CMake: Add support for DarwinSSL
CMake: Reorganize SSL support, separate WinSSL and SSPI
CMake: Add mbedTLS support
CMake: Set at most one SSL library
@webmaster128

This comment has been minimized.

Show comment
Hide comment
@webmaster128

webmaster128 Feb 27, 2017

Contributor

I totally agree. That's a good focus for another PR.

I can understand that you want this (excellent) piece of work to be done, but my concern is that this introduces two new public variables in a naming format that is at least confusing. As soon as this is in, we need to think about compatibility and I don't think it is worth carrying around compatibility code for a decade just because CMAKE_USE_DARWINSSL and CMAKE_USE_MBEDTLS have been used for one or two weeks, maybe including a release.

Contributor

webmaster128 commented Feb 27, 2017

I totally agree. That's a good focus for another PR.

I can understand that you want this (excellent) piece of work to be done, but my concern is that this introduces two new public variables in a naming format that is at least confusing. As soon as this is in, we need to think about compatibility and I don't think it is worth carrying around compatibility code for a decade just because CMAKE_USE_DARWINSSL and CMAKE_USE_MBEDTLS have been used for one or two weeks, maybe including a release.

@jzakrzewski

This comment has been minimized.

Show comment
Hide comment
@jzakrzewski

jzakrzewski Feb 27, 2017

Contributor

I think we should do this once and good. The names for the options used in command line should be short and simple - without any prefix. After all the user is building curl and he/she and we know that so why should we force them to type those prefixes?

So I'd say - make a separate PR, cleanup all names, break the compatibility once but do it the right way.

Contributor

jzakrzewski commented Feb 27, 2017

I think we should do this once and good. The names for the options used in command line should be short and simple - without any prefix. After all the user is building curl and he/she and we know that so why should we force them to type those prefixes?

So I'd say - make a separate PR, cleanup all names, break the compatibility once but do it the right way.

@Lekensteyn

This comment has been minimized.

Show comment
Hide comment
@Lekensteyn

Lekensteyn Feb 28, 2017

Member

@ligfx Can you squash the commits according to the suggestions from @jay?

@webmaster128 @jzakrzewski There are more inconsistent names (CMAKE_USE_GSSAPI, CMAKE_USE_LIBSSH2), feel free to propose a patch tot rename them (and for backwards compat, the default for the renamed options could use the former).

Maybe it would be even better to introduce a separate CMakeOptions.txt to keep all options in one place. See for example https://github.com/nghttp2/nghttp2/blob/master/CMakeOptions.txt

Member

Lekensteyn commented Feb 28, 2017

@ligfx Can you squash the commits according to the suggestions from @jay?

@webmaster128 @jzakrzewski There are more inconsistent names (CMAKE_USE_GSSAPI, CMAKE_USE_LIBSSH2), feel free to propose a patch tot rename them (and for backwards compat, the default for the renamed options could use the former).

Maybe it would be even better to introduce a separate CMakeOptions.txt to keep all options in one place. See for example https://github.com/nghttp2/nghttp2/blob/master/CMakeOptions.txt

ligfx added some commits Jan 26, 2017

@ligfx

This comment has been minimized.

Show comment
Hide comment
@ligfx

ligfx Feb 28, 2017

Contributor
Contributor

ligfx commented Feb 28, 2017

Lekensteyn added a commit that referenced this pull request Mar 5, 2017

CMake: Reorganize SSL support, separate WinSSL and SSPI
This is closer to how configure.ac does it

Ref: #1228

Lekensteyn added a commit that referenced this pull request Mar 5, 2017

CMake: Add DarwinSSL support
Assisted-by: Simon Warta <simon@kullo.net>
Ref: #1228

Lekensteyn added a commit that referenced this pull request Mar 5, 2017

Lekensteyn added a commit that referenced this pull request Mar 5, 2017

@Lekensteyn

This comment has been minimized.

Show comment
Hide comment
@Lekensteyn

Lekensteyn Mar 5, 2017

Member

Merged via:
f85ff14 CMake: Reorganize SSL support, separate WinSSL and SSPI
eb19e89 CMake: Add DarwinSSL support
c6a9746 CMake: Add mbedTLS support
813263d CMake: Set at most one SSL library

Thanks for your contribution @ligfx!

Member

Lekensteyn commented Mar 5, 2017

Merged via:
f85ff14 CMake: Reorganize SSL support, separate WinSSL and SSPI
eb19e89 CMake: Add DarwinSSL support
c6a9746 CMake: Add mbedTLS support
813263d CMake: Set at most one SSL library

Thanks for your contribution @ligfx!

@Lekensteyn Lekensteyn closed this Mar 5, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 24, 2018

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