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

@emanuele6 emanuele6 Oct 4, 2022

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

@jbms jbms Oct 4, 2022

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
@jbms jbms force-pushed the cmake-subproject-fix branch from ac18f77 to f05b3b9 Compare Oct 4, 2022
@bagder bagder requested a review from jzakrzewski Oct 7, 2022
Copy link
Contributor

@jzakrzewski jzakrzewski left a comment

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants