wgtmac commented on code in PR #236:
URL: https://github.com/apache/iceberg-cpp/pull/236#discussion_r2361920717
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -450,6 +600,9 @@ resolve_croaring_dependency()
resolve_nlohmann_json_dependency()
resolve_spdlog_dependency()
+resolve_curl_dependency()
Review Comment:
Please add an option to enable it, like `ICEBERG_BUILD_REST_CATALOG`.
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -430,6 +430,156 @@ function(resolve_zlib_dependency)
endfunction()
+# ----------------------------------------------------------------------
+# CURL (for cpr)
+
+function(resolve_curl_dependency)
+ prepare_fetchcontent()
+
+ set(BUILD_CURL_EXE
+ OFF
+ CACHE BOOL "" FORCE)
+ set(BUILD_TESTING
+ OFF
+ CACHE BOOL "" FORCE)
+ set(CURL_ENABLE_EXPORT_TARGET
+ OFF
+ CACHE BOOL "" FORCE)
+ set(BUILD_SHARED_LIBS
+ OFF
+ CACHE BOOL "" FORCE)
+ set(CURL_STATICLIB
+ ON
+ CACHE BOOL "" FORCE)
+ set(HTTP_ONLY
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_CA_BUNDLE
+ "auto"
+ CACHE STRING "" FORCE)
+ set(USE_LIBIDN2
+ OFF
+ CACHE BOOL "" FORCE)
+
+ fetchcontent_declare(CURL
+ ${FC_DECLARE_COMMON_OPTIONS}
+ URL https://curl.se/download/curl-8.11.0.tar.gz
+ FIND_PACKAGE_ARGS
+ NAMES
+ CURL
+ CONFIG)
+
+ fetchcontent_makeavailable(CURL)
+
+ if(TARGET OpenSSL::SSL)
Review Comment:
Why do we need this? Is it vendored by curl? If this is a required library,
we need to also support vendored and installed mode, otherwise it may conflict
with user installed openssl.
##########
src/iceberg/CMakeLists.txt:
##########
@@ -133,13 +133,17 @@ if(ICEBERG_BUILD_BUNDLE)
list(APPEND
ICEBERG_BUNDLE_STATIC_BUILD_INTERFACE_LIBS
"$<IF:$<TARGET_EXISTS:iceberg_static>,iceberg_static,iceberg_shared>"
+ "$<IF:$<TARGET_EXISTS:CURL::libcurl>,CURL::libcurl,CURL::libcurl>"
Review Comment:
Can we add a separate library to link with and use
`ICEBERG_BUILD_REST_CATALOG` to control whether to build it? The bundled
library has nothing to do with cpr and curl.
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -430,6 +430,156 @@ function(resolve_zlib_dependency)
endfunction()
+# ----------------------------------------------------------------------
+# CURL (for cpr)
+
+function(resolve_curl_dependency)
+ prepare_fetchcontent()
+
+ set(BUILD_CURL_EXE
+ OFF
+ CACHE BOOL "" FORCE)
+ set(BUILD_TESTING
+ OFF
+ CACHE BOOL "" FORCE)
+ set(CURL_ENABLE_EXPORT_TARGET
+ OFF
+ CACHE BOOL "" FORCE)
+ set(BUILD_SHARED_LIBS
+ OFF
+ CACHE BOOL "" FORCE)
+ set(CURL_STATICLIB
+ ON
+ CACHE BOOL "" FORCE)
+ set(HTTP_ONLY
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_CA_BUNDLE
+ "auto"
+ CACHE STRING "" FORCE)
+ set(USE_LIBIDN2
+ OFF
+ CACHE BOOL "" FORCE)
+
+ fetchcontent_declare(CURL
+ ${FC_DECLARE_COMMON_OPTIONS}
+ URL https://curl.se/download/curl-8.11.0.tar.gz
+ FIND_PACKAGE_ARGS
+ NAMES
+ CURL
+ CONFIG)
+
+ fetchcontent_makeavailable(CURL)
+
+ if(TARGET OpenSSL::SSL)
+ message(STATUS "Adding OpenSSL to the system dependency list.")
+ list(APPEND ICEBERG_SYSTEM_DEPENDENCIES OpenSSL)
+ endif()
+
+ if(curl_SOURCE_DIR)
+ if(NOT TARGET CURL::libcurl)
+ add_library(CURL::libcurl INTERFACE IMPORTED)
+ target_link_libraries(CURL::libcurl INTERFACE libcurl_static)
+ target_include_directories(CURL::libcurl INTERFACE
${curl_BINARY_DIR}/include
+
${curl_SOURCE_DIR}/include)
+ endif()
+
+ set(CURL_VENDORED TRUE)
+ set_target_properties(libcurl_static PROPERTIES OUTPUT_NAME
"iceberg_vendored_curl"
+ POSITION_INDEPENDENT_CODE
ON)
+ add_library(Iceberg::libcurl_static ALIAS libcurl_static)
+ install(TARGETS libcurl_static
+ EXPORT iceberg_targets
+ RUNTIME DESTINATION "${ICEBERG_INSTALL_BINDIR}"
+ ARCHIVE DESTINATION "${ICEBERG_INSTALL_LIBDIR}"
+ LIBRARY DESTINATION "${ICEBERG_INSTALL_LIBDIR}")
+ message(STATUS "Use vendored CURL")
+
+ list(APPEND ICEBERG_SYSTEM_DEPENDENCIES OpenSSL)
Review Comment:
This duplicates with line 476 above.
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -430,6 +430,156 @@ function(resolve_zlib_dependency)
endfunction()
+# ----------------------------------------------------------------------
+# CURL (for cpr)
+
+function(resolve_curl_dependency)
+ prepare_fetchcontent()
+
+ set(BUILD_CURL_EXE
+ OFF
+ CACHE BOOL "" FORCE)
Review Comment:
```suggestion
set(BUILD_CURL_EXE OFF)
```
This is enough. I have optimized this to use CMP0077:
https://github.com/apache/iceberg-cpp/blob/main/cmake_modules/IcebergThirdpartyToolchain.cmake#L59-L61
##########
cmake_modules/IcebergThirdpartyToolchain.cmake:
##########
@@ -430,6 +430,196 @@ function(resolve_zlib_dependency)
endfunction()
+# ----------------------------------------------------------------------
+# CURL (for cpr)
+
+function(resolve_curl_dependency)
+ prepare_fetchcontent()
+
+ set(BUILD_CURL_EXE
+ OFF
+ CACHE BOOL "" FORCE)
+ set(BUILD_TESTING
+ OFF
+ CACHE BOOL "" FORCE)
+ set(CURL_ENABLE_EXPORT_TARGET
+ OFF
+ CACHE BOOL "" FORCE)
+ set(BUILD_SHARED_LIBS
+ OFF
+ CACHE BOOL "" FORCE)
+ set(CURL_STATICLIB
+ ON
+ CACHE BOOL "" FORCE)
+ set(HTTP_ONLY
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_LDAP
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_LDAPS
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_RTSP
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_FTP
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_FILE
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_TELNET
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_DICT
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_TFTP
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_GOPHER
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_POP3
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_IMAP
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_SMTP
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_DISABLE_SMB
+ ON
+ CACHE BOOL "" FORCE)
+ set(CURL_CA_BUNDLE
+ "auto"
+ CACHE STRING "" FORCE)
+ set(USE_LIBIDN2
+ OFF
+ CACHE BOOL "" FORCE)
+
+ fetchcontent_declare(CURL
+ ${FC_DECLARE_COMMON_OPTIONS}
+ GIT_REPOSITORY https://github.com/curl/curl.git
+ GIT_TAG curl-8_11_0
+ FIND_PACKAGE_ARGS
+ NAMES
+ CURL
+ CONFIG)
+
+ fetchcontent_makeavailable(CURL)
+
+ if(TARGET OpenSSL::SSL)
+ message(STATUS "Adding OpenSSL to the system dependency list.")
+ list(APPEND ICEBERG_SYSTEM_DEPENDENCIES OpenSSL)
+ endif()
+
+ if(curl_SOURCE_DIR)
+ if(NOT TARGET CURL::libcurl)
+ add_library(CURL::libcurl INTERFACE IMPORTED)
+ target_link_libraries(CURL::libcurl INTERFACE libcurl_static)
+ target_include_directories(CURL::libcurl INTERFACE
${curl_BINARY_DIR}/include
+
${curl_SOURCE_DIR}/include)
+ endif()
+
+ set(CURL_VENDORED TRUE)
+ set_target_properties(libcurl_static PROPERTIES OUTPUT_NAME
"iceberg_vendored_curl"
+ POSITION_INDEPENDENT_CODE
ON)
+ add_library(Iceberg::libcurl_static ALIAS libcurl_static)
+ install(TARGETS libcurl_static
+ EXPORT iceberg_targets
+ RUNTIME DESTINATION "${ICEBERG_INSTALL_BINDIR}"
+ ARCHIVE DESTINATION "${ICEBERG_INSTALL_LIBDIR}"
+ LIBRARY DESTINATION "${ICEBERG_INSTALL_LIBDIR}")
+ message(STATUS "Use vendored CURL")
+
+ list(APPEND ICEBERG_SYSTEM_DEPENDENCIES OpenSSL)
+ else()
+ set(CURL_VENDORED FALSE)
+ list(APPEND ICEBERG_SYSTEM_DEPENDENCIES CURL)
+ message(STATUS "Use system CURL")
+ endif()
+
+ set(ICEBERG_SYSTEM_DEPENDENCIES
+ ${ICEBERG_SYSTEM_DEPENDENCIES}
+ PARENT_SCOPE)
+ set(CURL_VENDORED
+ ${CURL_VENDORED}
+ PARENT_SCOPE)
+endfunction()
+
+# ----------------------------------------------------------------------
+# cpr (C++ Requests)
+
+function(resolve_cpr_dependency)
+ prepare_fetchcontent()
+
+ set(CPR_BUILD_TESTS
+ OFF
+ CACHE BOOL "" FORCE)
+ set(CPR_BUILD_TESTS_SSL
+ OFF
+ CACHE BOOL "" FORCE)
+ set(CPR_ENABLE_SSL
+ ON
+ CACHE BOOL "" FORCE)
+ set(CPR_USE_SYSTEM_CURL
+ ON
+ CACHE BOOL "" FORCE)
+ set(CPR_CURL_NOSIGNAL
+ ON
+ CACHE BOOL "" FORCE)
+
+ set(CURL_VERSION_STRING
+ "8.11.0"
+ CACHE STRING "" FORCE)
+ set(CURL_LIB
+ "CURL::libcurl"
+ CACHE STRING "" FORCE)
+
+ fetchcontent_declare(cpr
+ ${FC_DECLARE_COMMON_OPTIONS}
+ GIT_REPOSITORY https://github.com/libcpr/cpr.git
Review Comment:
It is more preferable to use the official release tarball than a git tag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
