kou commented on code in PR #2416:
URL: https://github.com/apache/orc/pull/2416#discussion_r2386992747
##########
cmake_modules/FindGTestAlt.cmake:
##########
@@ -52,14 +52,14 @@ find_library (GTEST_STATIC_LIB NAMES
${CMAKE_STATIC_LIBRARY_PREFIX}gtest${CMAKE_
PATH_SUFFIXES "lib")
if (GTEST_INCLUDE_DIR AND GMOCK_LIBRARY)
- set (GTEST_FOUND TRUE)
+ set (GTESTAlt_FOUND TRUE)
Review Comment:
How about using `GTestAlt_FOUND` because this files uses
`FindGTestAlt.cmake` not `FindGTESTAlt.cmake`?
##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -155,6 +183,111 @@ ExternalProject_Add (orc-format_ep
TEST_COMMAND ""
)
+# ----------------------------------------------------------------------
+# Protobuf
+#
+# XXX: It must be processed before ZLIB, otherwise ZLIB_LIBRARIES will
interfere with building protobuf.
+
+if (ORC_PACKAGE_KIND STREQUAL "conan")
+ find_package (Protobuf REQUIRED CONFIG)
+ add_library (orc_protobuf INTERFACE)
+ target_link_libraries(orc_protobuf INTERFACE protobuf::protobuf)
+ list (APPEND ORC_SYSTEM_DEPENDENCIES Protobuf)
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:protobuf::protobuf>")
+elseif (ORC_PACKAGE_KIND STREQUAL "vcpkg")
+ find_package(Protobuf CONFIG REQUIRED)
+ add_library (orc_protobuf INTERFACE IMPORTED)
+ target_link_libraries(orc_protobuf INTERFACE protobuf::libprotobuf)
+ list (APPEND ORC_SYSTEM_DEPENDENCIES Protobuf)
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:protobuf::libprotobuf>")
+ set (PROTOBUF_EXECUTABLE protobuf::protoc)
+elseif (NOT "${PROTOBUF_HOME}" STREQUAL "")
+ find_package (ProtobufAlt REQUIRED)
+
+ if (ORC_PREFER_STATIC_PROTOBUF AND PROTOBUF_STATIC_LIB)
+ orc_add_resolved_library (orc_protobuf ${PROTOBUF_STATIC_LIB}
${PROTOBUF_INCLUDE_DIR})
+ else ()
+ orc_add_resolved_library (orc_protobuf ${PROTOBUF_LIBRARY}
${PROTOBUF_INCLUDE_DIR})
+ endif ()
+
+ if (ORC_PREFER_STATIC_PROTOBUF AND PROTOC_STATIC_LIB)
+ orc_add_resolved_library (orc_protoc ${PROTOC_STATIC_LIB}
${PROTOBUF_INCLUDE_DIR})
+ else ()
+ orc_add_resolved_library (orc_protoc ${PROTOC_LIBRARY}
${PROTOBUF_INCLUDE_DIR})
+ endif ()
+
+ list (APPEND ORC_SYSTEM_DEPENDENCIES ProtobufAlt)
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:protobuf::libprotobuf>")
+ orc_provide_find_module (ProtobufAlt)
+else ()
+ prepare_fetchcontent()
Review Comment:
If we call this in top-level, changed variables in this context are visible
after this clause.
How about using `function()` or `block()`?
```cmake
function(build_protobuf)
prepare_fetchcontent()
...
endfunction()
build_protobuf()
```
https://cmake.org/cmake/help/latest/command/block.html
```cmake
block()
prepare_fetchcontent()
...
endblock()
```
##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -155,6 +183,111 @@ ExternalProject_Add (orc-format_ep
TEST_COMMAND ""
)
+# ----------------------------------------------------------------------
+# Protobuf
+#
+# XXX: It must be processed before ZLIB, otherwise ZLIB_LIBRARIES will
interfere with building protobuf.
+
+if (ORC_PACKAGE_KIND STREQUAL "conan")
+ find_package (Protobuf REQUIRED CONFIG)
+ add_library (orc_protobuf INTERFACE)
+ target_link_libraries(orc_protobuf INTERFACE protobuf::protobuf)
+ list (APPEND ORC_SYSTEM_DEPENDENCIES Protobuf)
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:protobuf::protobuf>")
+elseif (ORC_PACKAGE_KIND STREQUAL "vcpkg")
+ find_package(Protobuf CONFIG REQUIRED)
+ add_library (orc_protobuf INTERFACE IMPORTED)
+ target_link_libraries(orc_protobuf INTERFACE protobuf::libprotobuf)
+ list (APPEND ORC_SYSTEM_DEPENDENCIES Protobuf)
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:protobuf::libprotobuf>")
+ set (PROTOBUF_EXECUTABLE protobuf::protoc)
+elseif (NOT "${PROTOBUF_HOME}" STREQUAL "")
+ find_package (ProtobufAlt REQUIRED)
+
+ if (ORC_PREFER_STATIC_PROTOBUF AND PROTOBUF_STATIC_LIB)
+ orc_add_resolved_library (orc_protobuf ${PROTOBUF_STATIC_LIB}
${PROTOBUF_INCLUDE_DIR})
+ else ()
+ orc_add_resolved_library (orc_protobuf ${PROTOBUF_LIBRARY}
${PROTOBUF_INCLUDE_DIR})
+ endif ()
+
+ if (ORC_PREFER_STATIC_PROTOBUF AND PROTOC_STATIC_LIB)
+ orc_add_resolved_library (orc_protoc ${PROTOC_STATIC_LIB}
${PROTOBUF_INCLUDE_DIR})
+ else ()
+ orc_add_resolved_library (orc_protoc ${PROTOC_LIBRARY}
${PROTOBUF_INCLUDE_DIR})
+ endif ()
+
+ list (APPEND ORC_SYSTEM_DEPENDENCIES ProtobufAlt)
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:protobuf::libprotobuf>")
+ orc_provide_find_module (ProtobufAlt)
+else ()
+ prepare_fetchcontent()
+
+ set(protobuf_INSTALL OFF)
+ set(protobuf_BUILD_TESTS OFF)
+ set(protobuf_BUILD_PROTOBUF_BINARIES ON)
+ set(protobuf_BUILD_PROTOC_BINARIES ON)
+ set(protobuf_WITH_ZLIB OFF)
+ set(protobuf_BUILD_SHARED_LIBS OFF)
+
+ # Set compiler flags to suppress warnings before fetching protobuf
+ set(CMAKE_CXX_FLAGS_BACKUP "${CMAKE_CXX_FLAGS}")
+ set(CMAKE_C_FLAGS_BACKUP "${CMAKE_C_FLAGS}")
+ if(CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL
"GNU")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecated-declarations")
+ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-deprecated-declarations")
+ endif()
+
+ fetchcontent_declare(protobuf
+ URL
"https://github.com/google/protobuf/archive/v${PROTOBUF_VERSION}.tar.gz";
+ SOURCE_SUBDIR "cmake"
+ FIND_PACKAGE_ARGS
+ NAMES Protobuf
+ CONFIG
+ )
+ fetchcontent_makeavailable(protobuf)
+
+ # Restore original compiler flags after protobuf configuration
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS_BACKUP}")
+ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS_BACKUP}")
Review Comment:
We can remove this by creating new variables scope by `function()`/`block()`.
##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -155,6 +183,111 @@ ExternalProject_Add (orc-format_ep
TEST_COMMAND ""
)
+# ----------------------------------------------------------------------
+# Protobuf
+#
+# XXX: It must be processed before ZLIB, otherwise ZLIB_LIBRARIES will
interfere with building protobuf.
+
+if (ORC_PACKAGE_KIND STREQUAL "conan")
+ find_package (Protobuf REQUIRED CONFIG)
+ add_library (orc_protobuf INTERFACE)
+ target_link_libraries(orc_protobuf INTERFACE protobuf::protobuf)
+ list (APPEND ORC_SYSTEM_DEPENDENCIES Protobuf)
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:protobuf::protobuf>")
+elseif (ORC_PACKAGE_KIND STREQUAL "vcpkg")
+ find_package(Protobuf CONFIG REQUIRED)
+ add_library (orc_protobuf INTERFACE IMPORTED)
+ target_link_libraries(orc_protobuf INTERFACE protobuf::libprotobuf)
+ list (APPEND ORC_SYSTEM_DEPENDENCIES Protobuf)
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:protobuf::libprotobuf>")
+ set (PROTOBUF_EXECUTABLE protobuf::protoc)
+elseif (NOT "${PROTOBUF_HOME}" STREQUAL "")
+ find_package (ProtobufAlt REQUIRED)
+
+ if (ORC_PREFER_STATIC_PROTOBUF AND PROTOBUF_STATIC_LIB)
+ orc_add_resolved_library (orc_protobuf ${PROTOBUF_STATIC_LIB}
${PROTOBUF_INCLUDE_DIR})
+ else ()
+ orc_add_resolved_library (orc_protobuf ${PROTOBUF_LIBRARY}
${PROTOBUF_INCLUDE_DIR})
+ endif ()
+
+ if (ORC_PREFER_STATIC_PROTOBUF AND PROTOC_STATIC_LIB)
+ orc_add_resolved_library (orc_protoc ${PROTOC_STATIC_LIB}
${PROTOBUF_INCLUDE_DIR})
+ else ()
+ orc_add_resolved_library (orc_protoc ${PROTOC_LIBRARY}
${PROTOBUF_INCLUDE_DIR})
+ endif ()
+
+ list (APPEND ORC_SYSTEM_DEPENDENCIES ProtobufAlt)
+ list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:protobuf::libprotobuf>")
+ orc_provide_find_module (ProtobufAlt)
+else ()
+ prepare_fetchcontent()
+
+ set(protobuf_INSTALL OFF)
+ set(protobuf_BUILD_TESTS OFF)
+ set(protobuf_BUILD_PROTOBUF_BINARIES ON)
+ set(protobuf_BUILD_PROTOC_BINARIES ON)
+ set(protobuf_WITH_ZLIB OFF)
+ set(protobuf_BUILD_SHARED_LIBS OFF)
+
+ # Set compiler flags to suppress warnings before fetching protobuf
+ set(CMAKE_CXX_FLAGS_BACKUP "${CMAKE_CXX_FLAGS}")
+ set(CMAKE_C_FLAGS_BACKUP "${CMAKE_C_FLAGS}")
+ if(CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL
"GNU")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecated-declarations")
+ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-deprecated-declarations")
+ endif()
+
+ fetchcontent_declare(protobuf
+ URL
"https://github.com/google/protobuf/archive/v${PROTOBUF_VERSION}.tar.gz";
+ SOURCE_SUBDIR "cmake"
+ FIND_PACKAGE_ARGS
+ NAMES Protobuf
+ CONFIG
+ )
+ fetchcontent_makeavailable(protobuf)
+
+ # Restore original compiler flags after protobuf configuration
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS_BACKUP}")
+ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS_BACKUP}")
+
+ if(protobuf_SOURCE_DIR)
+ message(STATUS "Using vendored protobuf")
+
+ if(NOT TARGET protobuf::libprotobuf)
+ add_library(protobuf::libprotobuf ALIAS libprotobuf)
+ endif()
+ if(NOT TARGET protobuf::protoc)
+ add_executable(protobuf::protoc ALIAS protoc)
+ endif()
Review Comment:
It seems that we don't need these `if`s:
```suggestion
add_library(protobuf::libprotobuf ALIAS libprotobuf)
add_executable(protobuf::protoc ALIAS protoc)
```
##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -223,48 +383,74 @@ elseif (ORC_PACKAGE_KIND STREQUAL "vcpkg")
list (APPEND ORC_SYSTEM_DEPENDENCIES ZLIB)
list (APPEND ORC_INSTALL_INTERFACE_TARGETS "$<INSTALL_INTERFACE:ZLIB::ZLIB>")
elseif (NOT "${ZLIB_HOME}" STREQUAL "")
- find_package (ZLIB REQUIRED)
+ find_package (ZLIBAlt REQUIRED)
Review Comment:
We may be able to remove `FindZLIBAlt.cmake`. CMake's `FindZLIB` provides
`ZLIB_ROOT` hint: https://cmake.org/cmake/help/latest/module/FindZLIB.html#hints
##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -16,6 +16,7 @@
# under the License.
INCLUDE(ExternalProject)
+INCLUDE(FetchContent)
set(ORC_VENDOR_DEPENDENCIES)
Review Comment:
It seems that we can remove this.
##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -170,41 +303,68 @@ elseif (ORC_PACKAGE_KIND STREQUAL "vcpkg")
list (APPEND ORC_SYSTEM_DEPENDENCIES Snappy)
list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:Snappy::snappy>")
elseif (NOT "${SNAPPY_HOME}" STREQUAL "")
- find_package (Snappy REQUIRED)
+ find_package (SnappyAlt REQUIRED)
if (ORC_PREFER_STATIC_SNAPPY AND SNAPPY_STATIC_LIB)
orc_add_resolved_library (orc_snappy ${SNAPPY_STATIC_LIB}
${SNAPPY_INCLUDE_DIR})
else ()
orc_add_resolved_library (orc_snappy ${SNAPPY_LIBRARY}
${SNAPPY_INCLUDE_DIR})
endif ()
- list (APPEND ORC_SYSTEM_DEPENDENCIES Snappy)
+ list (APPEND ORC_SYSTEM_DEPENDENCIES SnappyAlt)
list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:Snappy::snappy>")
- orc_provide_find_module (Snappy)
+ orc_provide_find_module (SnappyAlt)
else ()
- set(SNAPPY_HOME "${THIRDPARTY_DIR}/snappy_ep-install")
- set(SNAPPY_INCLUDE_DIR "${SNAPPY_HOME}/include")
- set(SNAPPY_STATIC_LIB_NAME
"${CMAKE_STATIC_LIBRARY_PREFIX}snappy${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(SNAPPY_STATIC_LIB "${SNAPPY_HOME}/lib/${SNAPPY_STATIC_LIB_NAME}")
- set(SNAPPY_CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${SNAPPY_HOME}
- -DBUILD_SHARED_LIBS=OFF -DCMAKE_INSTALL_LIBDIR=lib
- -DSNAPPY_BUILD_BENCHMARKS=OFF)
-
- if (BUILD_POSITION_INDEPENDENT_LIB)
- set(SNAPPY_CMAKE_ARGS ${SNAPPY_CMAKE_ARGS}
-DCMAKE_POSITION_INDEPENDENT_CODE=ON)
- endif ()
+ prepare_fetchcontent()
+
+ set(SNAPPY_BUILD_TESTS OFF)
+ set(SNAPPY_BUILD_BENCHMARKS OFF)
+ set(SNAPPY_INSTALL OFF)
- ExternalProject_Add (snappy_ep
+ fetchcontent_declare(Snappy
URL "https://github.com/google/snappy/archive/${SNAPPY_VERSION}.tar.gz";
- CMAKE_ARGS ${SNAPPY_CMAKE_ARGS} -DSNAPPY_BUILD_TESTS=OFF
- ${THIRDPARTY_LOG_OPTIONS}
- BUILD_BYPRODUCTS "${SNAPPY_STATIC_LIB}")
+ FIND_PACKAGE_ARGS
+ NAMES Snappy
+ CONFIG
+ )
+ fetchcontent_makeavailable(Snappy)
+
+ if(snappy_SOURCE_DIR)
+ message(STATUS "Using vendored snappy")
+ if(NOT TARGET Snappy::snappy)
+ add_library(Snappy::snappy INTERFACE IMPORTED)
+ target_link_libraries(Snappy::snappy INTERFACE snappy)
+ target_include_directories(Snappy::snappy INTERFACE ${snappy_SOURCE_DIR}
${snappy_BINARY_DIR})
+ endif()
- orc_add_built_library (snappy_ep orc_snappy ${SNAPPY_STATIC_LIB}
${SNAPPY_INCLUDE_DIR})
+ if(BUILD_POSITION_INDEPENDENT_LIB)
+ set_target_properties(snappy POSITION_INDEPENDENT_CODE ON)
+ endif()
+
+ if(INSTALL_VENDORED_LIBS)
+ set_target_properties(snappy PROPERTIES OUTPUT_NAME
"orc_vendored_snappy")
+
+ install(FILES ${snappy_SOURCE_DIR}/snappy-c.h DESTINATION
"${CMAKE_INSTALL_INCLUDEDIR}")
+ install(FILES ${snappy_SOURCE_DIR}/snappy-sinksource.h DESTINATION
"${CMAKE_INSTALL_INCLUDEDIR}")
+ install(FILES ${snappy_SOURCE_DIR}/snappy.h DESTINATION
"${CMAKE_INSTALL_INCLUDEDIR}")
+ install(FILES ${snappy_BINARY_DIR}/snappy-stubs-public.h DESTINATION
"${CMAKE_INSTALL_INCLUDEDIR}")
Review Comment:
We may not need to install header files.
ORC doesn't use Snappy in public headers, right?
##########
cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -170,41 +303,68 @@ elseif (ORC_PACKAGE_KIND STREQUAL "vcpkg")
list (APPEND ORC_SYSTEM_DEPENDENCIES Snappy)
list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:Snappy::snappy>")
elseif (NOT "${SNAPPY_HOME}" STREQUAL "")
- find_package (Snappy REQUIRED)
+ find_package (SnappyAlt REQUIRED)
if (ORC_PREFER_STATIC_SNAPPY AND SNAPPY_STATIC_LIB)
orc_add_resolved_library (orc_snappy ${SNAPPY_STATIC_LIB}
${SNAPPY_INCLUDE_DIR})
else ()
orc_add_resolved_library (orc_snappy ${SNAPPY_LIBRARY}
${SNAPPY_INCLUDE_DIR})
endif ()
- list (APPEND ORC_SYSTEM_DEPENDENCIES Snappy)
+ list (APPEND ORC_SYSTEM_DEPENDENCIES SnappyAlt)
list (APPEND ORC_INSTALL_INTERFACE_TARGETS
"$<INSTALL_INTERFACE:Snappy::snappy>")
- orc_provide_find_module (Snappy)
+ orc_provide_find_module (SnappyAlt)
else ()
Review Comment:
We may be able to remove `SnappyAlt` entirely by setting `Snappy_ROOT`:
```suggestion
else ()
if (NOT "${SNAPPY_HOME}" STREQUAL "" AND "${Snappy_ROOT}" STREQUAL "")
set(Snappy_ROOT "${SNAPPY_HOME}")
endif()
```
See also: https://cmake.org/cmake/help/latest/variable/PackageName_ROOT.html
--
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]
