Remove ${FOLLY_WITH_DEPENDENCIES} and fix Boost::<component> links (f…

…acebookincubator#4735)

Summary:
This PR removes the variable `FOLLY_WITH_DEPENDENCIES` that was used to  link to folly instead of the more appropriate `Folly::folly` target.

A consequence of this change was the proper linking to Boost targets in the form of `Boost::<component>` instead of `BOOST_LIBRARIES` or `BOOST_<component>_LIBRARIES`, this in turn made some changes to the the folly and boost `resolve_dependency` modules necessary:
- folly and Boost are now built within a subdirectory. This creates a new, smaller scope. Previously they were built in the root scope. This allows us to use directory scoped thing like `add_compile_options` without affecting the main build.
- The FindBoost module and the BoostConfig.cmake create a `Boost::headers` target that contains the include dir with the header only components, there are no single targets for each header library (at least in some installations). Building Boost from source does not create this target but rather creates a target for each library. To align both versions of using Boost the new `FindBoost.cmake` will create the Boost::headers target so we can link to that uniformly if we want to use it.

This is to be merged after facebookincubator#4689 (this contains the commits from that PR which I will drop via rebase once it is merged)

Pull Request resolved: facebookincubator#4735

Reviewed By: kgpai

Differential Revision: D45549132

Pulled By: pedroerp

fbshipit-source-id: 08b2b506014013186c4d2dc3f86e40c8dfe823eb

.gitignore

@@ -54,6 +54,7 @@ projects/*
 # Autotools artifacts
 #==============================================================================#
 config/
+!/velox/**/config
 configure
 config-h.in
 autom4te.cache
@@ -65,6 +66,7 @@ config.log
 config.status
 stamp-h1
 config.h
+!/velox/**/config.h
 m4/libtool.m4
 m4/ltoptions.m4
 m4/ltsugar.m4
@@ -86,6 +88,7 @@ _build/
 *.pdf
 *.swp
 a.out
+CMake/resolve_dependency_module/boost/FindBoost.cmake
 
 #==============================================================================#
 # Kate Swap Files

CMake/resolve_dependency_modules/boost.cmake

@@ -12,54 +12,13 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 include_guard(GLOBAL)
-
-if(DEFINED ENV{VELOX_BOOST_URL})
-  set(BOOST_SOURCE_URL "$ENV{VELOX_BOOST_URL}")
-else()
-  # We need to use boost > 1.70 to build it with CMake 1.81 was the first to be
-  # released as a github release INCLUDING the cmake files (which are not in the
-  # officale releases for some reason)
-  set(VELOX_BOOST_BUILD_VERSION 1.81.0)
-  string(
-    CONCAT BOOST_SOURCE_URL
-           "https://github.com/boostorg/boost/releases/download/"
-           "boost-${VELOX_BOOST_BUILD_VERSION}/"
-           "boost-${VELOX_BOOST_BUILD_VERSION}.tar.gz")
-  set(VELOX_BOOST_BUILD_SHA256_CHECKSUM
-      121da556b718fd7bd700b5f2e734f8004f1cfa78b7d30145471c526ba75a151c)
-endif()
-
-message(STATUS "Building boost from source")
-
-# required for Boost::thread
-if(NOT TARGET Threads::Threads)
-  set(THREADS_PREFER_PTHREAD_FLAG ON)
-  find_package(Threads REQUIRED)
-endif()
-
-FetchContent_Declare(
-  Boost
-  URL ${BOOST_SOURCE_URL}
-  URL_HASH SHA256=${VELOX_BOOST_BUILD_SHA256_CHECKSUM})
-
-set(shared_libs ${BUILD_SHARED_LIBS})
-set(BUILD_SHARED_LIBS ON)
-FetchContent_MakeAvailable(Boost)
-set(BUILD_SHARED_LIBS ${shared_libs})
-
-# Manually construct include dirs. This is only necessary until we switch to
-# properly using targets.
-list_subdirs(boost_INCLUDE_DIRS ${boost_SOURCE_DIR}/libs)
-list(TRANSFORM boost_INCLUDE_DIRS APPEND /include)
-
-# numeric contains subdirs with their own include dir
-list_subdirs(numeric_subdirs ${boost_SOURCE_DIR}/libs/numeric)
-list(TRANSFORM numeric_subdirs APPEND /include)
-include_directories(${boost_INCLUDE_DIRS} ${numeric_subdirs})
+add_subdirectory(${CMAKE_CURRENT_LIST_DIR}/boost)
 
 if(${ICU_SOURCE} STREQUAL "BUNDLED")
+  # ensure ICU is built before Boost
   add_dependencies(boost_regex ICU ICU::i18n)
 endif()
+
 # This prevents system boost from leaking in
 set(Boost_NO_SYSTEM_PATHS ON)
 # We have to keep the FindBoost.cmake in an subfolder to prevent it from

CMake/resolve_dependency_modules/boost/CMakeLists.txt

@@ -0,0 +1,63 @@
+# 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.
+
+project(Boost)
+cmake_minimum_required(VERSION 3.14)
+
+# Suppress all warnings (in this subdir)
+add_compile_options(-w)
+
+# We need to use boost > 1.70 to build it with CMake 1.81 was the first to be
+# released as a github release INCLUDING the cmake files (which are not in the
+# officale releases for some reason)
+set(VELOX_BOOST_BUILD_VERSION 1.81.0)
+string(
+  CONCAT VELOX_BOOST_SOURCE_URL
+         "https://github.com/boostorg/boost/releases/download/"
+         "boost-${VELOX_BOOST_BUILD_VERSION}/"
+         "boost-${VELOX_BOOST_BUILD_VERSION}.tar.gz")
+set(VELOX_BOOST_BUILD_SHA256_CHECKSUM
+    121da556b718fd7bd700b5f2e734f8004f1cfa78b7d30145471c526ba75a151c)
+
+resolve_dependency_url(BOOST)
+message(STATUS "Building boost from source")
+
+# required for Boost::thread
+if(NOT TARGET Threads::Threads)
+  set(THREADS_PREFER_PTHREAD_FLAG ON)
+  find_package(Threads REQUIRED)
+endif()
+
+FetchContent_Declare(
+  Boost
+  URL ${VELOX_BOOST_SOURCE_URL}
+  URL_HASH ${VELOX_BOOST_BUILD_SHA256_CHECKSUM})
+
+# Configure the file before adding the header only libs
+configure_file(${CMAKE_CURRENT_LIST_DIR}/FindBoost.cmake.in
+               ${CMAKE_CURRENT_LIST_DIR}/FindBoost.cmake @ONLY)
+
+set(BOOST_HEADER_ONLY crc circular_buffer multi_index random uuid variant)
+list(APPEND BOOST_INCLUDE_LIBRARIES ${BOOST_HEADER_ONLY})
+
+# The `headers` target is not created by Boost cmake and leads to a warning
+list(REMOVE_ITEM BOOST_INCLUDE_LIBRARIES headers)
+set(BUILD_SHARED_LIBS ON)
+FetchContent_MakeAvailable(Boost)
+
+# To aling with Boost system install we create Boost::headers target
+add_library(boost_headers INTERFACE)
+add_library(Boost::headers ALIAS boost_headers)
+list(TRANSFORM BOOST_HEADER_ONLY PREPEND Boost::)
+target_link_libraries(boost_headers INTERFACE ${BOOST_HEADER_ONLY})

CMake/resolve_dependency_modules/boost/FindBoost.cmake → CMake/resolve_dependency_modules/boost/FindBoost.cmake.in

@@ -13,3 +13,6 @@
 # limitations under the License.
 message(STATUS "Using Boost - Bundled")
 set(Boost_FOUND TRUE)
+set(Boost_LIBRARIES @BOOST_INCLUDE_LIBRARIES@)
+list(TRANSFORM Boost_LIBRARIES PREPEND Boost::)
+message(STATUS "Boost targets: ${Boost_LIBRARIES}")

CMake/resolve_dependency_modules/folly.cmake

@@ -13,48 +13,4 @@
 # limitations under the License.
 include_guard(GLOBAL)
 
-set(VELOX_FOLLY_BUILD_VERSION v2022.11.14.00)
-set(VELOX_FOLLY_BUILD_SHA256_CHECKSUM
-    b249436cb61b6dfd5288093565438d8da642b07ae021191a4042b221bc1bdc0e)
-set(VELOX_FOLLY_SOURCE_URL
-    "https://github.com/facebook/folly/archive/${VELOX_FOLLY_BUILD_VERSION}.tar.gz"
-)
-
-resolve_dependency_url(FOLLY)
-
-message(STATUS "Building Folly from source")
-
-# Suppress warnings when compiling folly.
-set(FOLLY_CXX_FLAGS -w)
-
-if(gflags_SOURCE STREQUAL "BUNDLED")
-  set(glog_patch && git apply
-                 ${CMAKE_CURRENT_LIST_DIR}/folly/folly-gflags-glog.patch)
-endif()
-
-FetchContent_Declare(
-  folly
-  URL ${VELOX_FOLLY_SOURCE_URL}
-  URL_HASH ${VELOX_FOLLY_BUILD_SHA256_CHECKSUM}
-  PATCH_COMMAND git apply ${CMAKE_CURRENT_LIST_DIR}/folly/folly-no-export.patch
-                ${glog_patch})
-
-if(ON_APPLE_M1)
-  # folly will wrongly assume x86_64 if this is not set
-  set(CMAKE_LIBRARY_ARCHITECTURE aarch64)
-endif()
-FetchContent_MakeAvailable(folly)
-
-# Avoid possible errors for known warnings
-target_compile_options(folly PUBLIC ${FOLLY_CXX_FLAGS})
-
-# Folly::folly is not valid for FC but we want to match FindFolly
-add_library(Folly::folly ALIAS folly)
-
-if(${gflags_SOURCE} STREQUAL "BUNDLED")
-  add_dependencies(folly glog gflags)
-endif()
-
-set(FOLLY_BENCHMARK_STATIC_LIB
-    ${folly_BINARY_DIR}/folly/libfollybenchmark${CMAKE_STATIC_LIBRARY_SUFFIX})
-set(FOLLY_LIBRARIES folly)
+add_subdirectory(${CMAKE_CURRENT_LIST_DIR}/folly)

CMake/resolve_dependency_modules/folly/CMakeLists.txt

@@ -0,0 +1,62 @@
+# 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.
+project(Folly)
+cmake_minimum_required(VERSION 3.14)
+
+set(VELOX_FOLLY_BUILD_VERSION v2022.11.14.00)
+set(VELOX_FOLLY_BUILD_SHA256_CHECKSUM
+    b249436cb61b6dfd5288093565438d8da642b07ae021191a4042b221bc1bdc0e)
+set(VELOX_FOLLY_SOURCE_URL
+    "https://github.com/facebook/folly/archive/${VELOX_FOLLY_BUILD_VERSION}.tar.gz"
+)
+
+resolve_dependency_url(FOLLY)
+
+message(STATUS "Building Folly from source")
+
+if(gflags_SOURCE STREQUAL "BUNDLED")
+  set(glog_patch && git apply ${CMAKE_CURRENT_LIST_DIR}/folly-gflags-glog.patch)
+endif()
+
+FetchContent_Declare(
+  folly
+  URL ${VELOX_FOLLY_SOURCE_URL}
+  URL_HASH ${VELOX_FOLLY_BUILD_SHA256_CHECKSUM}
+  PATCH_COMMAND git apply ${CMAKE_CURRENT_LIST_DIR}/folly-no-export.patch
+                ${glog_patch})
+
+if(ON_APPLE_M1)
+  # folly will wrongly assume x86_64 if this is not set
+  set(CMAKE_LIBRARY_ARCHITECTURE aarch64)
+endif()
+# Suppress all warnings
+set(FOLLY_CXX_FLAGS -w)
+FetchContent_MakeAvailable(folly)
+
+# Folly::folly is not valid for FC but we want to match FindFolly
+add_library(Folly::folly ALIAS folly)
+
+# The folly target does not contain any include directories, they are propagated
+# from folly_base. This marks them as system headers which should suppress
+# warnigs generated by them when they are included else where.
+get_target_property(_inc folly_base INTERFACE_INCLUDE_DIRECTORIES)
+target_include_directories(folly_base SYSTEM INTERFACE ${_inc})
+
+if(${gflags_SOURCE} STREQUAL "BUNDLED")
+  add_dependencies(folly glog gflags)
+endif()
+
+set(FOLLY_BENCHMARK_STATIC_LIB
+    ${folly_BINARY_DIR}/folly/libfollybenchmark${CMAKE_STATIC_LIBRARY_SUFFIX}
+    PARENT_SCOPE)

CMake/resolve_dependency_modules/icu.cmake

@@ -38,7 +38,7 @@ set(HOST_ENV_CMAKE
     CC="${CMAKE_C_COMPILER}"
     CXX="${CMAKE_CXX_COMPILER}"
     CFLAGS="${CMAKE_C_FLAGS}"
-    CXXFLAGS="${CMAKE_CXX_FLAGS}"
+    CXXFLAGS="${CMAKE_CXX_FLAGS} -w"
     LDFLAGS="${CMAKE_MODULE_LINKER_FLAGS}")
 set(ICU_DIR ${CMAKE_CURRENT_BINARY_DIR}/_deps/icu)
 set(ICU_INCLUDE_DIRS ${ICU_DIR}/include)
@@ -75,7 +75,7 @@ foreach(component ${icu_components})
   set_target_properties(
     ICU::${component}
     PROPERTIES IMPORTED_LOCATION ${ICU_${component}_LIBRARY}
-               INTERFACE_INCLUDE_DIRECTORIES ${ICU_INCLUDE_DIRS})
+               INTERFACE_SYSTEM_INCLUDE_DIRECTORIES ${ICU_INCLUDE_DIRS})
   target_link_libraries(ICU::ICU INTERFACE ICU::${component})
 endforeach()
 

CMakeLists.txt

@@ -307,24 +307,19 @@ message("FINAL CMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS}")
 
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 
-set_source(Boost)
-set_source(ICU)
-
-if(CMAKE_SYSTEM_NAME MATCHES "Darwin" OR ${Boost_SOURCE} STREQUAL "BUNDLED")
-  if(${ICU_SOURCE} STREQUAL "BUNDLED")
-    resolve_dependency(ICU)
-    include_directories(${ICU_INCLUDE_DIRS})
-    link_directories(${ICU_LIBRARIES})
+if(CMAKE_SYSTEM_NAME MATCHES "Darwin")
+  if(ON_APPLE_M1)
+    set(CMAKE_PREFIX_PATH "/opt/homebrew/opt/icu4c" ${CMAKE_PREFIX_PATH})
   else()
-    if(ON_APPLE_M1)
-      set(CMAKE_PREFIX_PATH "/opt/homebrew/opt/icu4c" ${CMAKE_PREFIX_PATH})
-    else()
-      set(CMAKE_PREFIX_PATH "/usr/local/opt/icu4c" ${CMAKE_PREFIX_PATH})
-    endif()
+    set(CMAKE_PREFIX_PATH "/usr/local/opt/icu4c" ${CMAKE_PREFIX_PATH})
   endif()
 endif()
 
+set_source(ICU)
+resolve_dependency(ICU)
+
 set(BOOST_INCLUDE_LIBRARIES
+    headers
     atomic
     context
     date_time
@@ -334,6 +329,7 @@ set(BOOST_INCLUDE_LIBRARIES
     system
     thread)
 
+set_source(Boost)
 resolve_dependency(Boost 1.66.0 COMPONENTS ${BOOST_INCLUDE_LIBRARIES})
 
 # Range-v3 will be enable when the codegen code actually lands keeping it here
@@ -393,31 +389,12 @@ add_compile_definitions(FOLLY_HAVE_INT128_T=1)
 set_source(folly)
 resolve_dependency(folly)
 
-# If we use BUNDLED boost we need to set this manually AFTER folly is configured
-if(NOT DEFINED Boost_LIBRARIES)
-  set(Boost_LIBRARIES ${BOOST_INCLUDE_LIBRARIES})
-  list(TRANSFORM Boost_LIBRARIES PREPEND Boost::)
-endif()
-
-set(FOLLY_WITH_DEPENDENCIES
-    ${FOLLY_LIBRARIES} ${Boost_LIBRARIES} ${DOUBLE_CONVERSION} ${SNAPPY}
-    ${CMAKE_DL_LIBS} ${FMT})
-
 if(DEFINED FOLLY_BENCHMARK_STATIC_LIB)
   set(FOLLY_BENCHMARK ${FOLLY_BENCHMARK_STATIC_LIB})
 else()
   set(FOLLY_BENCHMARK Folly::follybenchmark)
 endif()
 
-if(NOT ${VELOX_BUILD_MINIMAL})
-  find_package(BZip2 MODULE)
-  if(BZIP2_FOUND)
-    list(APPEND FOLLY_WITH_DEPENDENCIES ${BZIP2_LIBRARIES})
-  endif()
-endif()
-
-include_directories(SYSTEM ${FOLLY_INCLUDE_DIRS})
-
 if(NOT ${VELOX_BUILD_MINIMAL})
   # Locate or build protobuf.
   set_source(Protobuf)

docker-compose.yml

@@ -50,3 +50,22 @@ services:
     volumes:
       - .:/velox:delegated
     command: /bin/bash -c "scl enable gcc-toolset-9 '/velox/scripts/docker-command.sh'"
+
+  python:
+    # Usage:
+    #   docker-compose pull ubuntu-cpp or docker-compose build ubuntu-cpp
+    #   docker-compose run --rm ubuntu-cpp
+    #   or
+    #   docker-compose run -e NUM_THREADS=<NUMBER_OF_THREADS_TO_USE> --rm ubuntu-cpp
+    #   to set the number of threads used during compilation
+    image: ghcr.io/facebookincubator/velox-dev:torcharrow-avx
+    build:
+      context: .
+      dockerfile: scripts/velox-torcharrow-container.dockfile
+    environment:
+      PYTHON_EXECUTABLE: python3.7
+      NUM_THREADS: 8 # default value for NUM_THREADS
+      CCACHE_DIR: "/velox/.ccache"
+    volumes:
+      - .:/velox:delegated
+    command: cd /velox && make python-test

velox/benchmarks/basic/CMakeLists.txt

@@ -20,7 +20,7 @@ set(velox_benchmark_deps
     velox_parse_utils
     velox_parse_expression
     velox_serialization
-    ${FOLLY}
+    Folly::folly
     ${FOLLY_BENCHMARK}
     ${DOUBLE_CONVERSION}
     gflags::gflags