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

Improve usability of CMake build as a sub-project #9638

Closed
wants to merge 1 commit into from

Conversation

jbms
Copy link
Contributor

@jbms jbms commented Oct 4, 2022

  • Renames uninstall -> curl_uninstall
  • Ensures all export rules are guarded by CURL_ENABLE_EXPORT_TARGET

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
if(NOT (HAVE_RAND_EGD))
check_symbol_exists(RAND_egd "${CURL_INCLUDES}" HAVE_RAND_EGD)
endif()
if(NOT (DEFINED HAVE_BORINGSSL))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other cmake checks for HAVE_* options use if()/if(NOT) without DEFINED. I think we should not introduce this inconsistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an explanatory comment at the top. I am checking for DEFINED rather than for the boolean value, because someone could define it to OFF, and in that case we still want to skip the symbol detection.

@bagder bagder added the cmake label Oct 4, 2022
- Renames `uninstall` -> `curl_uninstall`
- Ensures all export rules are guarded by CURL_ENABLE_EXPORT_TARGET
Copy link
Contributor

@jzakrzewski jzakrzewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know if it's complete but looks like a step in the right direction.

@bagder bagder closed this in 8698825 Oct 21, 2022
@bagder
Copy link
Member

bagder commented Oct 21, 2022

Thanks!

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

4 participants