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
Loading
@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Oct 7, 2021

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.

Loading

@nightlark nightlark force-pushed the add-enable-ssl-option branch from 2ad673c to 3eb97df Oct 7, 2021
@nightlark nightlark changed the title cmake: add ENABLE_SSL option cmake: add CURL_ENABLE_SSL option Oct 7, 2021
@nightlark
Copy link
Contributor Author

@nightlark nightlark commented Oct 8, 2021

Option name is now under the CURL_ namespace.

Loading

@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Oct 11, 2021

No time to test but at least looks sensible ;)

Loading

@bagder bagder requested a review from MarcelRaad Oct 14, 2021
@MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented Oct 14, 2021

The test failures look unrelated.

Loading

@MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented Oct 14, 2021

Merged now. Thank you!

Loading

@nightlark nightlark deleted the add-enable-ssl-option branch Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants