Copilot commented on code in PR #13102:
URL: https://github.com/apache/gluten/pull/13102#discussion_r4081955756
##########
dev/vcpkg/ports/fbthrift/fix-deps.patch:
##########
@@ -12,27 +12,27 @@ index 2f214f5..b45f528 100644
+ set(LIBGFLAGS_LIBRARY gflags::gflags)
+ find_package(glog CONFIG REQUIRED)
+ set(GLOG_LIBRARIES glog::glog)
- find_package(fizz CONFIG REQUIRED)
- find_package(wangle CONFIG REQUIRED)
+ if (THRIFT_RPC)
+ find_package(fizz CONFIG REQUIRED)
+ find_package(wangle CONFIG REQUIRED)
+ endif ()
find_package(ZLIB REQUIRED)
- find_package(Zstd REQUIRED)
+ find_package(zstd CONFIG REQUIRED)
+ set(ZSTD_LIBRARIES zstd::libzstd)
find_package(Xxhash REQUIRED)
- find_package(mvfst CONFIG REQUIRED)
- # https://cmake.org/cmake/help/v3.9/module/FindThreads.html
+ if (THRIFT_RPC)
+ find_package(mvfst CONFIG REQUIRED)
diff --git a/thrift/cmake/FBThriftConfig.cmake.in
b/thrift/cmake/FBThriftConfig.cmake.in
-index e279485..4dd8bd1 100644
+index 9aee56b..4d3616b 100644
--- a/thrift/cmake/FBThriftConfig.cmake.in
+++ b/thrift/cmake/FBThriftConfig.cmake.in
-@@ -29,9 +29,16 @@ else()
- set_and_check(FBTHRIFT_COMPILER "@PACKAGE_BIN_INSTALL_DIR@/thrift1")
+@@ -45,8 +45,15 @@ if (NOT Xxhash_FOUND)
+ return()
endif()
--find_dependency(Xxhash REQUIRED)
-find_dependency(ZLIB REQUIRED)
--find_package(mvfst CONFIG REQUIRED)
-+find_dependency(xxHash CONFIG)
+-find_package(mvfst CONFIG)
+find_dependency(ZLIB)
+find_dependency(mvfst CONFIG)
+find_dependency(fizz CONFIG)
Review Comment:
In `FBThriftConfig.cmake.in`, RPC-related dependencies (`mvfst`, `fizz`) are
now required unconditionally. However, the main build logic in this same patch
gates `find_package(fizz/wangle/mvfst)` behind `if (THRIFT_RPC)`. This mismatch
can break consumers that use a non-RPC build of fbthrift (they’ll still be
forced to have mvfst/fizz available). Make the config file’s dependency
discovery conditional in the same way (e.g., via a configured `@THRIFT_RPC@`
substitution or an exported feature flag).
##########
dev/vcpkg/ports/fizz/fix-build.patch:
##########
@@ -0,0 +1,120 @@
+diff --git a/fizz/CMakeLists.txt b/fizz/CMakeLists.txt
+index 4685146..e3221d0 100644
+--- a/fizz/CMakeLists.txt
++++ b/fizz/CMakeLists.txt
+@@ -65,26 +65,29 @@ include(FizzOptions)
+ # Glog is only required in the default GLOG backend; XLOG routes through
+ # folly::xlog and DISABLED strips logging entirely.
+ if(FIZZ_LOGGING_BACKEND STREQUAL "GLOG")
+- find_package(Glog REQUIRED)
++ find_package(glog CONFIG REQUIRED)
++ set(GLOG_LIBRARIES glog::glog)
+ add_compile_definitions(GLOG_USE_GLOG_EXPORT)
+ endif()
+ find_package(Threads REQUIRED)
+-find_package(Zstd REQUIRED)
++find_package(zstd CONFIG REQUIRED)
++set(ZSTD_LIBRARY zstd::libzstd)
+ if (UNIX AND NOT APPLE)
+ find_package(Librt)
+ endif()
+
+ include(CheckAtomic)
+
+-find_package(Sodium REQUIRED)
+-set(FIZZ_HAVE_SODIUM ${Sodium_FOUND})
++find_package(unofficial-sodium CONFIG REQUIRED)
++set(FIZZ_HAVE_SODIUM ${unofficial-sodium_FOUND})
+
+ SET(FIZZ_SHINY_DEPENDENCIES "")
+ SET(FIZZ_LINK_LIBRARIES "")
+ SET(FIZZ_INCLUDE_DIRECTORIES "")
+
+-find_package(gflags CONFIG QUIET)
+-if (gflags_FOUND)
++find_package(gflags CONFIG REQUIRED)
++set(GFLAGS_LIBRARIES gflags::gflags)
++if (0)
+ message(STATUS "Found gflags from package config")
+ if (TARGET gflags-shared)
+ list(APPEND FIZZ_SHINY_DEPENDENCIES gflags-shared)
+@@ -95,7 +98,7 @@ if (gflags_FOUND)
+ endif()
+ list(APPEND CMAKE_REQUIRED_LIBRARIES ${GFLAGS_LIBRARIES})
+ list(APPEND CMAKE_REQUIRED_INCLUDES ${GFLAGS_INCLUDE_DIR})
+-else()
++elseif(0)
+ find_package(Gflags REQUIRED MODULE)
+ list(APPEND FIZZ_LINK_LIBRARIES ${LIBGFLAGS_LIBRARY})
+ list(APPEND FIZZ_INCLUDE_DIRECTORIES ${LIBGFLAGS_INCLUDE_DIR})
+@@ -103,12 +106,13 @@ else()
+ list(APPEND CMAKE_REQUIRED_INCLUDES ${LIBGFLAGS_INCLUDE_DIR})
+ endif()
+
++find_package(gflags CONFIG REQUIRED)
+ find_package(ZLIB REQUIRED)
+
+-find_package(Libevent CONFIG QUIET)
+-if(TARGET event)
++find_package(Libevent CONFIG REQUIRED)
Review Comment:
`find_package(Libevent CONFIG REQUIRED)` is likely to fail on case-sensitive
platforms if the exported package config is `libeventConfig.cmake` (common in
vcpkg) rather than `LibeventConfig.cmake`. Since you’re checking
`libevent::core`, switch to `find_package(libevent CONFIG REQUIRED)` (or the
exact package name generated by the port) to avoid Linux build failures.
##########
dev/vcpkg/ports/fizz/fix-build.patch:
##########
@@ -0,0 +1,120 @@
+diff --git a/fizz/CMakeLists.txt b/fizz/CMakeLists.txt
+index 4685146..e3221d0 100644
+--- a/fizz/CMakeLists.txt
++++ b/fizz/CMakeLists.txt
+@@ -65,26 +65,29 @@ include(FizzOptions)
+ # Glog is only required in the default GLOG backend; XLOG routes through
+ # folly::xlog and DISABLED strips logging entirely.
+ if(FIZZ_LOGGING_BACKEND STREQUAL "GLOG")
+- find_package(Glog REQUIRED)
++ find_package(glog CONFIG REQUIRED)
++ set(GLOG_LIBRARIES glog::glog)
+ add_compile_definitions(GLOG_USE_GLOG_EXPORT)
+ endif()
+ find_package(Threads REQUIRED)
+-find_package(Zstd REQUIRED)
++find_package(zstd CONFIG REQUIRED)
++set(ZSTD_LIBRARY zstd::libzstd)
+ if (UNIX AND NOT APPLE)
+ find_package(Librt)
+ endif()
+
+ include(CheckAtomic)
+
+-find_package(Sodium REQUIRED)
+-set(FIZZ_HAVE_SODIUM ${Sodium_FOUND})
++find_package(unofficial-sodium CONFIG REQUIRED)
++set(FIZZ_HAVE_SODIUM ${unofficial-sodium_FOUND})
+
+ SET(FIZZ_SHINY_DEPENDENCIES "")
+ SET(FIZZ_LINK_LIBRARIES "")
+ SET(FIZZ_INCLUDE_DIRECTORIES "")
+
+-find_package(gflags CONFIG QUIET)
+-if (gflags_FOUND)
++find_package(gflags CONFIG REQUIRED)
++set(GFLAGS_LIBRARIES gflags::gflags)
++if (0)
Review Comment:
The patch introduces dead/disabled logic (`if(0)` / `elseif(0)`), plus a
duplicated `find_package(gflags CONFIG REQUIRED)`. This is hard to maintain and
makes future rebases painful. Prefer removing the disabled branch entirely and
keeping a single, straightforward config-based discovery path (or gate legacy
module-based fallback with an actual option instead of `0`).
##########
dev/vcpkg/ports/folly/disable-uninitialized-resize-on-new-stl.patch:
##########
@@ -1,26 +1,26 @@
diff --git a/folly/memory/UninitializedMemoryHacks.h
b/folly/memory/UninitializedMemoryHacks.h
-index daf5eb735..1ac44d6b2 100644
+index 9babda4..6e2f505 100644
--- a/folly/memory/UninitializedMemoryHacks.h
+++ b/folly/memory/UninitializedMemoryHacks.h
-@@ -101,6 +101,9 @@ template <
- typename
std::enable_if<std::is_trivially_destructible<T>::value>::type>
- inline void resizeWithoutInitialization(
+@@ -105,6 +105,9 @@ template <typename T>
+ requires std::is_trivially_destructible_v<T>
+ inline void resizeWithoutInitializationImpl(
std::basic_string<T>& s, std::size_t n) {
+#if defined(_MSVC_STL_UPDATE) && _MSVC_STL_UPDATE >= 202206L
+ s.resize(n);
Review Comment:
This patch wraps the entire function body in an `#if
defined(_MSVC_STL_UPDATE) ...` without an `#else`, which makes
`resizeWithoutInitializationImpl` a no-op on non-MSVC (and on MSVC where
`_MSVC_STL_UPDATE` is not defined / below the threshold). That will break
behavior on Linux/macOS. Fix by adding an `#else` that preserves the original
implementation for non-affected toolchains (or invert the condition and only
special-case the MSVC-new-STL path).
##########
dev/vcpkg/ports/folly/disable-uninitialized-resize-on-new-stl.patch:
##########
@@ -1,26 +1,26 @@
diff --git a/folly/memory/UninitializedMemoryHacks.h
b/folly/memory/UninitializedMemoryHacks.h
-index daf5eb735..1ac44d6b2 100644
+index 9babda4..6e2f505 100644
--- a/folly/memory/UninitializedMemoryHacks.h
+++ b/folly/memory/UninitializedMemoryHacks.h
-@@ -101,6 +101,9 @@ template <
- typename
std::enable_if<std::is_trivially_destructible<T>::value>::type>
- inline void resizeWithoutInitialization(
+@@ -105,6 +105,9 @@ template <typename T>
+ requires std::is_trivially_destructible_v<T>
+ inline void resizeWithoutInitializationImpl(
std::basic_string<T>& s, std::size_t n) {
+#if defined(_MSVC_STL_UPDATE) && _MSVC_STL_UPDATE >= 202206L
+ s.resize(n);
+#else
if (n <= s.size()) {
s.resize(n);
} else {
-@@ -111,6 +114,7 @@ inline void resizeWithoutInitialization(
+@@ -115,6 +118,7 @@ inline void resizeWithoutInitializationImpl(
}
- detail::unsafeStringSetLargerSize(s, n);
+ unsafeStringSetLargerSize(s, n);
}
+#endif // defined(_MSVC_STL_UPDATE) && _MSVC_STL_UPDATE >= 202206L
}
Review Comment:
This patch wraps the entire function body in an `#if
defined(_MSVC_STL_UPDATE) ...` without an `#else`, which makes
`resizeWithoutInitializationImpl` a no-op on non-MSVC (and on MSVC where
`_MSVC_STL_UPDATE` is not defined / below the threshold). That will break
behavior on Linux/macOS. Fix by adding an `#else` that preserves the original
implementation for non-affected toolchains (or invert the condition and only
special-case the MSVC-new-STL path).
##########
dev/vcpkg/ports/fizz/fix-build.patch:
##########
@@ -0,0 +1,120 @@
+diff --git a/fizz/CMakeLists.txt b/fizz/CMakeLists.txt
+index 4685146..e3221d0 100644
+--- a/fizz/CMakeLists.txt
++++ b/fizz/CMakeLists.txt
+@@ -65,26 +65,29 @@ include(FizzOptions)
+ # Glog is only required in the default GLOG backend; XLOG routes through
+ # folly::xlog and DISABLED strips logging entirely.
+ if(FIZZ_LOGGING_BACKEND STREQUAL "GLOG")
+- find_package(Glog REQUIRED)
++ find_package(glog CONFIG REQUIRED)
++ set(GLOG_LIBRARIES glog::glog)
+ add_compile_definitions(GLOG_USE_GLOG_EXPORT)
+ endif()
+ find_package(Threads REQUIRED)
+-find_package(Zstd REQUIRED)
++find_package(zstd CONFIG REQUIRED)
++set(ZSTD_LIBRARY zstd::libzstd)
+ if (UNIX AND NOT APPLE)
+ find_package(Librt)
+ endif()
+
+ include(CheckAtomic)
+
+-find_package(Sodium REQUIRED)
+-set(FIZZ_HAVE_SODIUM ${Sodium_FOUND})
++find_package(unofficial-sodium CONFIG REQUIRED)
++set(FIZZ_HAVE_SODIUM ${unofficial-sodium_FOUND})
+
+ SET(FIZZ_SHINY_DEPENDENCIES "")
+ SET(FIZZ_LINK_LIBRARIES "")
+ SET(FIZZ_INCLUDE_DIRECTORIES "")
+
+-find_package(gflags CONFIG QUIET)
+-if (gflags_FOUND)
++find_package(gflags CONFIG REQUIRED)
++set(GFLAGS_LIBRARIES gflags::gflags)
++if (0)
+ message(STATUS "Found gflags from package config")
+ if (TARGET gflags-shared)
+ list(APPEND FIZZ_SHINY_DEPENDENCIES gflags-shared)
+@@ -95,7 +98,7 @@ if (gflags_FOUND)
+ endif()
+ list(APPEND CMAKE_REQUIRED_LIBRARIES ${GFLAGS_LIBRARIES})
+ list(APPEND CMAKE_REQUIRED_INCLUDES ${GFLAGS_INCLUDE_DIR})
+-else()
++elseif(0)
+ find_package(Gflags REQUIRED MODULE)
+ list(APPEND FIZZ_LINK_LIBRARIES ${LIBGFLAGS_LIBRARY})
+ list(APPEND FIZZ_INCLUDE_DIRECTORIES ${LIBGFLAGS_INCLUDE_DIR})
+@@ -103,12 +106,13 @@ else()
+ list(APPEND CMAKE_REQUIRED_INCLUDES ${LIBGFLAGS_INCLUDE_DIR})
+ endif()
+
++find_package(gflags CONFIG REQUIRED)
Review Comment:
The patch introduces dead/disabled logic (`if(0)` / `elseif(0)`), plus a
duplicated `find_package(gflags CONFIG REQUIRED)`. This is hard to maintain and
makes future rebases painful. Prefer removing the disabled branch entirely and
keeping a single, straightforward config-based discovery path (or gate legacy
module-based fallback with an actual option instead of `0`).
##########
dev/vcpkg/ports/fizz/fix-build.patch:
##########
@@ -0,0 +1,120 @@
+diff --git a/fizz/CMakeLists.txt b/fizz/CMakeLists.txt
+index 4685146..e3221d0 100644
+--- a/fizz/CMakeLists.txt
++++ b/fizz/CMakeLists.txt
+@@ -65,26 +65,29 @@ include(FizzOptions)
+ # Glog is only required in the default GLOG backend; XLOG routes through
+ # folly::xlog and DISABLED strips logging entirely.
+ if(FIZZ_LOGGING_BACKEND STREQUAL "GLOG")
+- find_package(Glog REQUIRED)
++ find_package(glog CONFIG REQUIRED)
++ set(GLOG_LIBRARIES glog::glog)
+ add_compile_definitions(GLOG_USE_GLOG_EXPORT)
+ endif()
+ find_package(Threads REQUIRED)
+-find_package(Zstd REQUIRED)
++find_package(zstd CONFIG REQUIRED)
++set(ZSTD_LIBRARY zstd::libzstd)
+ if (UNIX AND NOT APPLE)
+ find_package(Librt)
+ endif()
+
+ include(CheckAtomic)
+
+-find_package(Sodium REQUIRED)
+-set(FIZZ_HAVE_SODIUM ${Sodium_FOUND})
++find_package(unofficial-sodium CONFIG REQUIRED)
++set(FIZZ_HAVE_SODIUM ${unofficial-sodium_FOUND})
+
+ SET(FIZZ_SHINY_DEPENDENCIES "")
+ SET(FIZZ_LINK_LIBRARIES "")
+ SET(FIZZ_INCLUDE_DIRECTORIES "")
+
+-find_package(gflags CONFIG QUIET)
+-if (gflags_FOUND)
++find_package(gflags CONFIG REQUIRED)
++set(GFLAGS_LIBRARIES gflags::gflags)
++if (0)
+ message(STATUS "Found gflags from package config")
+ if (TARGET gflags-shared)
+ list(APPEND FIZZ_SHINY_DEPENDENCIES gflags-shared)
+@@ -95,7 +98,7 @@ if (gflags_FOUND)
+ endif()
+ list(APPEND CMAKE_REQUIRED_LIBRARIES ${GFLAGS_LIBRARIES})
+ list(APPEND CMAKE_REQUIRED_INCLUDES ${GFLAGS_INCLUDE_DIR})
+-else()
++elseif(0)
+ find_package(Gflags REQUIRED MODULE)
+ list(APPEND FIZZ_LINK_LIBRARIES ${LIBGFLAGS_LIBRARY})
+ list(APPEND FIZZ_INCLUDE_DIRECTORIES ${LIBGFLAGS_INCLUDE_DIR})
+@@ -103,12 +106,13 @@ else()
+ list(APPEND CMAKE_REQUIRED_INCLUDES ${LIBGFLAGS_INCLUDE_DIR})
+ endif()
+
++find_package(gflags CONFIG REQUIRED)
+ find_package(ZLIB REQUIRED)
+
+-find_package(Libevent CONFIG QUIET)
+-if(TARGET event)
++find_package(Libevent CONFIG REQUIRED)
++if(TARGET libevent::core)
+ message(STATUS "Found libevent from package config")
+- list(APPEND FIZZ_SHINY_DEPENDENCIES event)
++ list(APPEND FIZZ_SHINY_DEPENDENCIES libevent::core)
+ else()
+ find_package(Libevent MODULE REQUIRED)
+ list(APPEND FIZZ_LINK_LIBRARIES ${LIBEVENT_LIB})
+@@ -214,10 +218,6 @@ target_include_directories(
+ $<BUILD_INTERFACE:${FIZZ_BASE_DIR}>
+ $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/generated>
+ $<INSTALL_INTERFACE:${INCLUDE_INSTALL_DIR}>
+- ${FOLLY_INCLUDE_DIR}
+- ${OPENSSL_INCLUDE_DIR}
+- ${sodium_INCLUDE_DIR}
+- ${ZSTD_INCLUDE_DIR}
+ PRIVATE
+ ${FIZZ_INCLUDE_DIRECTORIES}
+ )
+@@ -268,7 +268,7 @@ target_link_libraries(fizz
+ Folly::folly_portability_unistd
+ Folly::folly_detail_base64_detail_base64_api
+ ${OPENSSL_LIBRARIES}
+- sodium
++ unofficial-sodium::sodium
+ Threads::Threads
+ ZLIB::ZLIB
+ ${ZSTD_LIBRARY}
+@@ -348,8 +348,7 @@ ENDIF(CMAKE_CROSSCOMPILING)
+ SET(FIZZ_TEST_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX})
+
+ if(BUILD_TESTS)
+- find_package(GMock 1.8.0 MODULE REQUIRED)
+- find_package(GTest 1.8.0 MODULE REQUIRED)
++ find_package(GTest CONFIG REQUIRED)
+ endif()
+
+ add_library(fizz_test_support
+diff --git a/fizz/cmake/fizz-config.cmake.in b/fizz/cmake/fizz-config.cmake.in
+index 07b4d01..3004ad2 100644
+--- a/fizz/cmake/fizz-config.cmake.in
++++ b/fizz/cmake/fizz-config.cmake.in
+@@ -32,9 +32,18 @@ set(FIZZ_LIBRARIES fizz::fizz)
+
+ include(CMakeFindDependencyMacro)
+
+-find_dependency(Sodium)
++find_dependency(unofficial-sodium CONFIG)
+ find_dependency(folly CONFIG)
+ find_dependency(ZLIB)
++find_dependency(Libevent CONFIG)
++find_dependency(fmt CONFIG)
++find_dependency(OpenSSL)
++find_dependency(glog CONFIG)
++find_dependency(double-conversion CONFIG)
++find_dependency(Threads)
++find_dependency(gflags CONFIG)
++find_dependency(zstd CONFIG)
++find_dependency(GTest CONFIG)
+ if(FIZZ_HAVE_OQS)
Review Comment:
`fizz-config.cmake.in` unconditionally adds `find_dependency(GTest CONFIG)`,
which forces consumers of the *library* to have GTest installed even when
they’re not building fizz tests. That’s an unnecessary hard dependency for
downstreams and can break minimal builds. Make the GTest dependency conditional
(e.g., only when installing/using test support targets, or guarded by an
exported build option substituted at configure time).
##########
dev/vcpkg/ports/fizz/fix-build.patch:
##########
@@ -0,0 +1,120 @@
+diff --git a/fizz/CMakeLists.txt b/fizz/CMakeLists.txt
+index 4685146..e3221d0 100644
+--- a/fizz/CMakeLists.txt
++++ b/fizz/CMakeLists.txt
+@@ -65,26 +65,29 @@ include(FizzOptions)
+ # Glog is only required in the default GLOG backend; XLOG routes through
+ # folly::xlog and DISABLED strips logging entirely.
+ if(FIZZ_LOGGING_BACKEND STREQUAL "GLOG")
+- find_package(Glog REQUIRED)
++ find_package(glog CONFIG REQUIRED)
++ set(GLOG_LIBRARIES glog::glog)
+ add_compile_definitions(GLOG_USE_GLOG_EXPORT)
+ endif()
+ find_package(Threads REQUIRED)
+-find_package(Zstd REQUIRED)
++find_package(zstd CONFIG REQUIRED)
++set(ZSTD_LIBRARY zstd::libzstd)
+ if (UNIX AND NOT APPLE)
+ find_package(Librt)
+ endif()
+
+ include(CheckAtomic)
+
+-find_package(Sodium REQUIRED)
+-set(FIZZ_HAVE_SODIUM ${Sodium_FOUND})
++find_package(unofficial-sodium CONFIG REQUIRED)
++set(FIZZ_HAVE_SODIUM ${unofficial-sodium_FOUND})
+
+ SET(FIZZ_SHINY_DEPENDENCIES "")
+ SET(FIZZ_LINK_LIBRARIES "")
+ SET(FIZZ_INCLUDE_DIRECTORIES "")
+
+-find_package(gflags CONFIG QUIET)
+-if (gflags_FOUND)
++find_package(gflags CONFIG REQUIRED)
++set(GFLAGS_LIBRARIES gflags::gflags)
++if (0)
+ message(STATUS "Found gflags from package config")
+ if (TARGET gflags-shared)
+ list(APPEND FIZZ_SHINY_DEPENDENCIES gflags-shared)
+@@ -95,7 +98,7 @@ if (gflags_FOUND)
+ endif()
+ list(APPEND CMAKE_REQUIRED_LIBRARIES ${GFLAGS_LIBRARIES})
+ list(APPEND CMAKE_REQUIRED_INCLUDES ${GFLAGS_INCLUDE_DIR})
+-else()
++elseif(0)
Review Comment:
The patch introduces dead/disabled logic (`if(0)` / `elseif(0)`), plus a
duplicated `find_package(gflags CONFIG REQUIRED)`. This is hard to maintain and
makes future rebases painful. Prefer removing the disabled branch entirely and
keeping a single, straightforward config-based discovery path (or gate legacy
module-based fallback with an actual option instead of `0`).
##########
dev/vcpkg/ports/fizz/fix-build.patch:
##########
@@ -0,0 +1,120 @@
+diff --git a/fizz/CMakeLists.txt b/fizz/CMakeLists.txt
+index 4685146..e3221d0 100644
+--- a/fizz/CMakeLists.txt
++++ b/fizz/CMakeLists.txt
+@@ -65,26 +65,29 @@ include(FizzOptions)
+ # Glog is only required in the default GLOG backend; XLOG routes through
+ # folly::xlog and DISABLED strips logging entirely.
+ if(FIZZ_LOGGING_BACKEND STREQUAL "GLOG")
+- find_package(Glog REQUIRED)
++ find_package(glog CONFIG REQUIRED)
++ set(GLOG_LIBRARIES glog::glog)
+ add_compile_definitions(GLOG_USE_GLOG_EXPORT)
+ endif()
+ find_package(Threads REQUIRED)
+-find_package(Zstd REQUIRED)
++find_package(zstd CONFIG REQUIRED)
++set(ZSTD_LIBRARY zstd::libzstd)
+ if (UNIX AND NOT APPLE)
+ find_package(Librt)
+ endif()
+
+ include(CheckAtomic)
+
+-find_package(Sodium REQUIRED)
+-set(FIZZ_HAVE_SODIUM ${Sodium_FOUND})
++find_package(unofficial-sodium CONFIG REQUIRED)
++set(FIZZ_HAVE_SODIUM ${unofficial-sodium_FOUND})
+
+ SET(FIZZ_SHINY_DEPENDENCIES "")
+ SET(FIZZ_LINK_LIBRARIES "")
+ SET(FIZZ_INCLUDE_DIRECTORIES "")
+
+-find_package(gflags CONFIG QUIET)
+-if (gflags_FOUND)
++find_package(gflags CONFIG REQUIRED)
++set(GFLAGS_LIBRARIES gflags::gflags)
++if (0)
+ message(STATUS "Found gflags from package config")
+ if (TARGET gflags-shared)
+ list(APPEND FIZZ_SHINY_DEPENDENCIES gflags-shared)
+@@ -95,7 +98,7 @@ if (gflags_FOUND)
+ endif()
+ list(APPEND CMAKE_REQUIRED_LIBRARIES ${GFLAGS_LIBRARIES})
+ list(APPEND CMAKE_REQUIRED_INCLUDES ${GFLAGS_INCLUDE_DIR})
+-else()
++elseif(0)
+ find_package(Gflags REQUIRED MODULE)
+ list(APPEND FIZZ_LINK_LIBRARIES ${LIBGFLAGS_LIBRARY})
+ list(APPEND FIZZ_INCLUDE_DIRECTORIES ${LIBGFLAGS_INCLUDE_DIR})
+@@ -103,12 +106,13 @@ else()
+ list(APPEND CMAKE_REQUIRED_INCLUDES ${LIBGFLAGS_INCLUDE_DIR})
+ endif()
+
++find_package(gflags CONFIG REQUIRED)
+ find_package(ZLIB REQUIRED)
+
+-find_package(Libevent CONFIG QUIET)
+-if(TARGET event)
++find_package(Libevent CONFIG REQUIRED)
++if(TARGET libevent::core)
+ message(STATUS "Found libevent from package config")
+- list(APPEND FIZZ_SHINY_DEPENDENCIES event)
++ list(APPEND FIZZ_SHINY_DEPENDENCIES libevent::core)
+ else()
+ find_package(Libevent MODULE REQUIRED)
+ list(APPEND FIZZ_LINK_LIBRARIES ${LIBEVENT_LIB})
+@@ -214,10 +218,6 @@ target_include_directories(
+ $<BUILD_INTERFACE:${FIZZ_BASE_DIR}>
+ $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/generated>
+ $<INSTALL_INTERFACE:${INCLUDE_INSTALL_DIR}>
+- ${FOLLY_INCLUDE_DIR}
+- ${OPENSSL_INCLUDE_DIR}
+- ${sodium_INCLUDE_DIR}
+- ${ZSTD_INCLUDE_DIR}
+ PRIVATE
+ ${FIZZ_INCLUDE_DIRECTORIES}
+ )
+@@ -268,7 +268,7 @@ target_link_libraries(fizz
+ Folly::folly_portability_unistd
+ Folly::folly_detail_base64_detail_base64_api
+ ${OPENSSL_LIBRARIES}
+- sodium
++ unofficial-sodium::sodium
+ Threads::Threads
+ ZLIB::ZLIB
+ ${ZSTD_LIBRARY}
+@@ -348,8 +348,7 @@ ENDIF(CMAKE_CROSSCOMPILING)
+ SET(FIZZ_TEST_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX})
+
+ if(BUILD_TESTS)
+- find_package(GMock 1.8.0 MODULE REQUIRED)
+- find_package(GTest 1.8.0 MODULE REQUIRED)
++ find_package(GTest CONFIG REQUIRED)
+ endif()
+
+ add_library(fizz_test_support
+diff --git a/fizz/cmake/fizz-config.cmake.in b/fizz/cmake/fizz-config.cmake.in
+index 07b4d01..3004ad2 100644
+--- a/fizz/cmake/fizz-config.cmake.in
++++ b/fizz/cmake/fizz-config.cmake.in
+@@ -32,9 +32,18 @@ set(FIZZ_LIBRARIES fizz::fizz)
+
+ include(CMakeFindDependencyMacro)
Review Comment:
`fizz-config.cmake.in` unconditionally adds `find_dependency(GTest CONFIG)`,
which forces consumers of the *library* to have GTest installed even when
they’re not building fizz tests. That’s an unnecessary hard dependency for
downstreams and can break minimal builds. Make the GTest dependency conditional
(e.g., only when installing/using test support targets, or guarded by an
exported build option substituted at configure time).
--
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]
