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: add CURL_ENABLE_SSL option #7822

Closed
wants to merge 3 commits into from

Conversation

nightlark
Copy link
Contributor

@nightlark nightlark commented Oct 6, 2021

I came across a project building libcurl as a subproject that was trying to set SSL_ENABLED to OFF to disable SSL support in libcurl. Other than SSL_ENABLED being set as a local variable in libcurl and basically overriding anything that they might have set in the cache, issues would have also been encountered because CMAKE_USE_OPENSSL defaults to ON.

I think adding an ENABLE_SSL option (maybe renamed to match any curl cmake option naming convention) that the various SSL backends also depend on would make it much easier for projects building libcurl as a subproject to disable ssl without needing to determine which CMAKE_USE_* variables for SSL backends are set, and more obvious what option needs to be turned ON/OFF.

By ENABLE_SSL defaulting to ON, the behavior of existing SSL related options should remain the same and avoid breaking any existing ci setups or build scripts.

Thoughts?

@bagder bagder added the cmake label Oct 6, 2021
CMakeLists.txt Outdated Show resolved Hide resolved
@jzakrzewski
Copy link
Contributor

Please namespace the option. ENABLE_SSL is in my opinion too generic and may cause clashes with other projects. I myself have for example included MongoDB driver and curl in the same project and that alone would cause problems with this change.

@nightlark nightlark changed the title cmake: add ENABLE_SSL option cmake: add CURL_ENABLE_SSL option Oct 7, 2021
@nightlark
Copy link
Contributor Author

Option name is now under the CURL_ namespace.

@jzakrzewski
Copy link
Contributor

No time to test but at least looks sensible ;)

@MarcelRaad
Copy link
Member

The test failures look unrelated.

@MarcelRaad
Copy link
Member

Merged now. Thank you!

@nightlark nightlark deleted the add-enable-ssl-option branch October 16, 2021 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants