Refactor cmake modules

The new features in cmake 3.28 allow the cmake modules to be simplified.

.github/workflows/linux-build.yml

@@ -70,9 +70,6 @@ jobs:
         # it doesn't work
         run: git config --global --add safe.directory /__w/velox/velox
 
-      - name: Install cmake
-        run: python3 -m pip install cmake==3.28.3
-
       - name: Install Dependencies
         run: |
           # Allows to install arbitrary cuda-version whithout needing to update

.github/workflows/scheduled.yml

@@ -197,7 +197,6 @@ jobs:
         env:
           EXTRA_CMAKE_FLAGS: "-DVELOX_ENABLE_ARROW=ON -DVELOX_BUILD_PYTHON_PACKAGE=ON ${{ inputs.extraCMakeFlags }}"
         run: |
-          python3 -m pip install cmake==3.28.3
           EXTRA_CMAKE_FLAGS="-DPYTHON_EXECUTABLE=$(which python3) $EXTRA_CMAKE_FLAGS"
           make debug
 

CMake/Findre2.cmake

@@ -11,19 +11,55 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-find_library(re2_lib re2)
-find_path(re2_include re2/re2.h)
-
-include(FindPackageHandleStandardArgs)
-find_package_handle_standard_args(re2 REQUIRED_VARS re2_lib re2_include)
 
+find_package(re2 QUIET CONFIG)
 if(re2_FOUND)
-  set(re2_LIBRARIES ${re2_lib})
-  set(re2_INCLUDE_DIRS ${re2_include})
+  message(STATUS "Found RE2 via CMake.")
+  return()
+endif()
+
+# As per https://github.com/grpc/grpc/issues/25434, idempotence is necessary
+# because CMake fails when another target with the same name already exists.
+if(TARGET re2::re2)
+  message(STATUS "Found RE2 via pkg-config already?")
+  return()
+endif()
+
+find_package(PkgConfig REQUIRED)
+# TODO(junyer): Use the IMPORTED_TARGET option whenever CMake 3.6 (or newer)
+# becomes the minimum required: that will take care of the add_library() and
+# set_property() calls; then we can simply alias PkgConfig::RE2 as re2::re2. For
+# now, we can only set INTERFACE_* properties that existed in CMake 3.5.
+pkg_check_modules(RE2 QUIET re2)
+if(RE2_FOUND)
+  set(re2_FOUND "${RE2_FOUND}")
+  add_library(re2::re2 INTERFACE IMPORTED)
+  if(RE2_INCLUDE_DIRS)
+    set_property(TARGET re2::re2 PROPERTY INTERFACE_INCLUDE_DIRECTORIES
+                                          "${RE2_INCLUDE_DIRS}")
+  endif()
+  if(RE2_CFLAGS_OTHER)
+    # Filter out the -std flag, which is handled by CMAKE_CXX_STANDARD.
+    # TODO(junyer): Use the FILTER option whenever CMake 3.6 (or newer) becomes
+    # the minimum required: that will allow this to be concise.
+    foreach(flag IN LISTS RE2_CFLAGS_OTHER)
+      if("${flag}" MATCHES "^-std=")
+        list(REMOVE_ITEM RE2_CFLAGS_OTHER "${flag}")
+      endif()
+    endforeach()
+    set_property(TARGET re2::re2 PROPERTY INTERFACE_COMPILE_OPTIONS
+                                          "${RE2_CFLAGS_OTHER}")
+  endif()
+  if(RE2_LDFLAGS)
+    set_property(TARGET re2::re2 PROPERTY INTERFACE_LINK_LIBRARIES
+                                          "${RE2_LDFLAGS}")
+  endif()
+  message(STATUS "Found RE2 via pkg-config.")
+  return()
+endif()
 
-  add_library(re2::re2 UNKNOWN IMPORTED)
-  target_include_directories(re2::re2 INTERFACE ${re2_INCLUDE_DIRS})
-  target_link_libraries(re2::re2 INTERFACE ${re2_LIBRARIES})
-  set_target_properties(re2::re2 PROPERTIES IMPORTED_LOCATION
-                                            "${re2_LIBRARIES}")
+if(re2_FIND_REQUIRED)
+  message(FATAL_ERROR "Failed to find RE2.")
+elseif(NOT re2_FIND_QUIETLY)
+  message(WARNING "Failed to find RE2.")
 endif()

CMake/resolve_dependency_modules/absl.cmake

@@ -0,0 +1,36 @@
+# Copyright (c) Facebook, Inc. and its affiliates.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+include_guard(GLOBAL)
+
+set(VELOX_ABSL_BUILD_VERSION 20240116.2)
+set(VELOX_ABSL_BUILD_SHA256_CHECKSUM
+    733726b8c3a6d39a4120d7e45ea8b41a434cdacde401cba500f14236c49b39dc)
+string(CONCAT VELOX_ABSL_SOURCE_URL
+              "https://github.com/abseil/abseil-cpp/archive/refs/tags/"
+              "${VELOX_ABSL_BUILD_VERSION}.tar.gz")
+
+resolve_dependency_url(ABSL)
+
+message(STATUS "Building Abseil from source")
+
+FetchContent_Declare(
+  absl
+  URL ${VELOX_ABSL_SOURCE_URL}
+  URL_HASH ${VELOX_ABSL_BUILD_SHA256_CHECKSUM}
+  OVERRIDE_FIND_PACKAGE EXCLUDE_FROM_ALL SYSTEM)
+
+set(ABSL_BUILD_TESTING OFF)
+set(ABSL_PROPAGATE_CXX_STD ON)
+set(ABSL_ENABLE_INSTALL ON)
+FetchContent_MakeAvailable(absl)

CMake/resolve_dependency_modules/google_cloud_cpp_storage.cmake

@@ -0,0 +1,44 @@
+# Copyright (c) Facebook, Inc. and its affiliates.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+include_guard(GLOBAL)
+
+set_source(absl)
+resolve_dependency(absl 20240116 EXACT)
+
+set_source(gRPC)
+resolve_dependency(gRPC 1.48.1 EXACT)
+
+set(VELOX_GOOGLE_CLOUD_CPP_BUILD_VERSION 2.22.0)
+set(VELOX_GOOGLE_CLOUD_CPP_BUILD_SHA256_CHECKSUM
+    0c68782e57959c82e0c81def805c01460a042c1aae0c2feee905acaa2a2dc9bf)
+string(
+  CONCAT VELOX_GOOGLE_CLOUD_CPP_SOURCE_URL
+         "https://github.com/googleapis/google-cloud-cpp/archive/refs/tags/"
+         "v${VELOX_GOOGLE_CLOUD_CPP_BUILD_VERSION}.tar.gz")
+
+resolve_dependency_url(GOOGLE_CLOUD_CPP)
+
+message(STATUS "Building Google Cloud CPP storage from source")
+
+FetchContent_Declare(
+  google_cloud_cpp
+  URL ${VELOX_GOOGLE_CLOUD_CPP_SOURCE_URL}
+  URL_HASH ${VELOX_GOOGLE_CLOUD_CPP_BUILD_SHA256_CHECKSUM}
+  OVERRIDE_FIND_PACKAGE EXCLUDE_FROM_ALL SYSTEM)
+
+set(GOOGLE_CLOUD_CPP_ENABLE_EXAMPLES OFF)
+set(GOOGLE_CLOUD_CPP_ENABLE
+    "storage"
+    CACHE STRING "The list of libraries to build.")
+FetchContent_MakeAvailable(google_cloud_cpp)

CMake/resolve_dependency_modules/grpc.cmake

@@ -0,0 +1,57 @@
+# Copyright (c) Facebook, Inc. and its affiliates.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+include_guard(GLOBAL)
+
+set(VELOX_GRPC_BUILD_VERSION 1.48.1)
+set(VELOX_GRPC_BUILD_SHA256_CHECKSUM
+    320366665d19027cda87b2368c03939006a37e0388bfd1091c8d2a96fbc93bd8)
+string(CONCAT VELOX_GRPC_SOURCE_URL
+              "https://github.com/grpc/grpc/archive/refs/tags/"
+              "v${VELOX_GRPC_BUILD_VERSION}.tar.gz")
+
+resolve_dependency_url(GRPC)
+
+message(STATUS "Building Google Cloud CPP from source")
+
+FetchContent_Declare(
+  gRPC
+  URL ${VELOX_GRPC_SOURCE_URL}
+  URL_HASH ${VELOX_GRPC_BUILD_SHA256_CHECKSUM}
+  OVERRIDE_FIND_PACKAGE EXCLUDE_FROM_ALL)
+
+set(gRPC_ABSL_PROVIDER
+    "package"
+    CACHE STRING "Provider of absl library")
+set(gRPC_ZLIB_PROVIDER
+    "package"
+    CACHE STRING "Provider of zlib library")
+set(gRPC_CARES_PROVIDER
+    "package"
+    CACHE STRING "Provider of c-ares library")
+set(gRPC_RE2_PROVIDER
+    "package"
+    CACHE STRING "Provider of re2 library")
+set(gRPC_SSL_PROVIDER
+    "package"
+    CACHE STRING "Provider of ssl library")
+set(gRPC_PROTOBUF_PROVIDER
+    "package"
+    CACHE STRING "Provider of protobuf library")
+set(gRPC_INSTALL
+    ON
+    CACHE BOOL "Generate installation target")
+FetchContent_MakeAvailable(gRPC)
+add_library(gRPC::grpc ALIAS grpc)
+add_library(gRPC::grpc++ ALIAS grpc++)
+add_executable(gRPC::grpc_cpp_plugin ALIAS grpc_cpp_plugin)

CMake/resolve_dependency_modules/protobuf.cmake

@@ -30,44 +30,10 @@ message(STATUS "Building Protobuf from source")
 FetchContent_Declare(
   protobuf
   URL ${VELOX_PROTOBUF_SOURCE_URL}
-  URL_HASH ${VELOX_PROTOBUF_BUILD_SHA256_CHECKSUM})
+  URL_HASH ${VELOX_PROTOBUF_BUILD_SHA256_CHECKSUM}
+  OVERRIDE_FIND_PACKAGE EXCLUDE_FROM_ALL SYSTEM)
 
-if(NOT protobuf_POPULATED)
-  # We don't want to build tests.
-  set(protobuf_BUILD_TESTS
-      OFF
-      CACHE BOOL "Disable protobuf tests" FORCE)
-  set(CMAKE_CXX_FLAGS_BKP "${CMAKE_CXX_FLAGS}")
-
-  # Disable warnings that would fail protobuf compilation.
-  string(APPEND CMAKE_CXX_FLAGS " -Wno-missing-field-initializers")
-
-  check_cxx_compiler_flag("-Wstringop-overflow"
-                          COMPILER_HAS_W_STRINGOP_OVERFLOW)
-  if(COMPILER_HAS_W_STRINGOP_OVERFLOW)
-    string(APPEND CMAKE_CXX_FLAGS " -Wno-stringop-overflow")
-  endif()
-
-  check_cxx_compiler_flag("-Winvalid-noreturn" COMPILER_HAS_W_INVALID_NORETURN)
-
-  if(COMPILER_HAS_W_INVALID_NORETURN)
-    string(APPEND CMAKE_CXX_FLAGS " -Wno-invalid-noreturn")
-  else()
-    # Currently reproduced on Ubuntu 22.04 with clang 14
-    string(APPEND CMAKE_CXX_FLAGS " -Wno-error")
-  endif()
-
-  # Fetch the content using previously declared details
-  FetchContent_Populate(protobuf)
-
-  # Set right path to libprotobuf-dev include files.
-  set(Protobuf_INCLUDE_DIRS "${protobuf_SOURCE_DIR}/src/")
-  set(Protobuf_PROTOC_EXECUTABLE "${protobuf_BINARY_DIR}/protoc")
-  if(CMAKE_BUILD_TYPE MATCHES Debug)
-    set(Protobuf_LIBRARIES "${protobuf_BINARY_DIR}/libprotobufd.a")
-  else()
-    set(Protobuf_LIBRARIES "${protobuf_BINARY_DIR}/libprotobuf.a")
-  endif()
-  add_subdirectory(${protobuf_SOURCE_DIR} ${protobuf_BINARY_DIR})
-  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS_BKP}")
-endif()
+set(protobuf_BUILD_TESTS OFF)
+FetchContent_MakeAvailable(protobuf)
+set(Protobuf_INCLUDE_DIRS ${protobuf_SOURCE_DIR}/src)
+set(Protobuf_PROTOC_EXECUTABLE "${protobuf_BINARY_DIR}/protoc")

CMakeLists.txt

@@ -222,15 +222,6 @@ if(VELOX_ENABLE_S3)
   add_definitions(-DVELOX_ENABLE_S3)
 endif()
 
-if(VELOX_ENABLE_GCS)
-  # Set GCS_ROOT_DIR if you have a custom install location of GCS SDK CPP.
-  if(GCSSDK_ROOT_DIR)
-    list(APPEND CMAKE_PREFIX_PATH ${GCSSDK_ROOT_DIR})
-  endif()
-  find_package(google_cloud_cpp_storage REQUIRED)
-  add_definitions(-DVELOX_ENABLE_GCS)
-endif()
-
 if(VELOX_ENABLE_ABFS)
   # Set AZURESDK_ROOT_DIR if you have a custom install location of Azure Storage
   # SDK CPP.
@@ -442,6 +433,18 @@ if(${VELOX_BUILD_PYTHON_PACKAGE})
   add_subdirectory(pyvelox)
 endif()
 
+# DWIO (ORC/DWRF) and Substrait depend on protobuf.
+if(${VELOX_BUILD_MINIMAL_WITH_DWIO}
+   OR ${VELOX_ENABLE_HIVE_CONNECTOR}
+   OR ${VELOX_ENABLE_SUBSTRAIT}
+   OR VELOX_ENABLE_GCS)
+
+  # Locate or build protobuf.
+  set_source(Protobuf)
+  resolve_dependency(Protobuf 3.21.4 EXACT)
+  include_directories(${Protobuf_INCLUDE_DIRS})
+endif()
+
 set_source(simdjson)
 resolve_dependency(simdjson 3.8.0)
 
@@ -464,14 +467,10 @@ else()
   set(FOLLY_BENCHMARK Folly::follybenchmark)
 endif()
 
-# DWIO (ORC/DWRF) and Substrait depend on protobuf.
-if(${VELOX_BUILD_MINIMAL_WITH_DWIO}
-   OR ${VELOX_ENABLE_HIVE_CONNECTOR}
-   OR ${VELOX_ENABLE_SUBSTRAIT})
-  # Locate or build protobuf.
-  set_source(Protobuf)
-  resolve_dependency(Protobuf 3.21 EXACT)
-  include_directories(${Protobuf_INCLUDE_DIRS})
+if(VELOX_ENABLE_GCS)
+  set_source(google_cloud_cpp_storage)
+  resolve_dependency(google_cloud_cpp_storage 2.22.0 EXACT)
+  add_definitions(-DVELOX_ENABLE_GCS)
 endif()
 
 # GCC needs to link a library to enable std::filesystem.

velox/dwio/common/CMakeLists.txt

@@ -62,8 +62,6 @@ add_library(
   Writer.cpp
   WriterFactory.cpp)
 
-target_include_directories(velox_dwio_common PRIVATE ${Protobuf_INCLUDE_DIRS})
-
 target_link_libraries(
   velox_dwio_common
   velox_buffer
@@ -77,4 +75,5 @@ target_link_libraries(
   velox_memory
   Boost::regex
   Folly::folly
-  glog::glog)
+  glog::glog
+  protobuf::libprotobuf)

velox/dwio/common/tests/CMakeLists.txt

@@ -53,7 +53,8 @@ target_link_libraries(
   gtest_main
   gmock
   glog::glog
-  fmt::fmt)
+  fmt::fmt
+  protobuf::libprotobuf)
 
 add_executable(velox_dwio_common_data_buffer_benchmark DataBufferBenchmark.cpp)
 

velox/dwio/dwrf/proto/CMakeLists.txt

@@ -45,5 +45,5 @@ add_library(velox_dwio_dwrf_proto ${PROTO_HDRS} ${PROTO_SRCS})
 # Access generated proto file with.
 #
 # #include "velox/dwio/dwrf/proto/dwrf_proto.pb.h"
-target_link_libraries(velox_dwio_dwrf_proto ${Protobuf_LIBRARIES})
+target_link_libraries(velox_dwio_dwrf_proto protobuf::libprotobuf)
 target_include_directories(velox_dwio_dwrf_proto PUBLIC ${PROJECT_BINARY_DIR})

velox/substrait/CMakeLists.txt

@@ -37,7 +37,7 @@ add_custom_command(
     ${Protobuf_PROTOC_EXECUTABLE} --proto_path ${PROJECT_SOURCE_DIR}/
     --proto_path ${Protobuf_INCLUDE_DIRS} --cpp_out ${PROJECT_SOURCE_DIR}
     ${PROTO_FILES}
-  DEPENDS ${PROTO_DIR}
+  DEPENDS ${PROTO_DIR} ${Protobuf_PROTOC_EXECUTABLE}
   COMMENT "Running PROTO compiler"
   VERBATIM)
 add_custom_target(substrait_proto ALL DEPENDS ${PROTO_OUTPUT_FILES})