Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix DuckDB bundling and add support for DuckDB install on MacOS #11069

Closed
wants to merge 1 commit into from
Closed

Fix DuckDB bundling and add support for DuckDB install on MacOS #11069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,6 @@ endif()
if(DEFINED ENV{INSTALL_PREFIX})
message(STATUS "Dependency install directory set to: $ENV{INSTALL_PREFIX}")
list(APPEND CMAKE_PREFIX_PATH "$ENV{INSTALL_PREFIX}")
# Allow installed package headers to be picked up before brew/system package
# headers
include_directories(BEFORE "$ENV{INSTALL_PREFIX}/include")
endif()

list(PREPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/CMake"
Expand Down Expand Up @@ -192,6 +189,18 @@ if(${VELOX_BUILD_PYTHON_PACKAGE})
set(VELOX_ENABLE_SPARK_FUNCTIONS ON)
endif()

if(${VELOX_ENABLE_DUCKDB})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to happen after line 48? Is it possible to unify the two if(DEFINED ENV{INSTALL_PREFIX}) blocks? If not, will you please add a comment? Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include_directories modifies a global property which is applied to targets at the time of creation. So moving it to after duckdb is resolved keeps the changes away from it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yingsu00 Yes, your proposal that moving CMAKE_PREFIX_PATH setup along w/ include_directoryies works.

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -45,14 +45,6 @@ if(DEFINED ENV{CONDA_PREFIX})
   endif()
 endif()

-if(DEFINED ENV{INSTALL_PREFIX})
-  message(STATUS "Dependency install directory set to: $ENV{INSTALL_PREFIX}")
-  list(APPEND CMAKE_PREFIX_PATH "$ENV{INSTALL_PREFIX}")
-  # Allow installed package headers to be picked up before brew/system package
-  # headers
-  include_directories(BEFORE "$ENV{INSTALL_PREFIX}/include")
-endif()
-
 list(PREPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/CMake"
      "${PROJECT_SOURCE_DIR}/CMake/third-party")

@@ -192,6 +184,21 @@ if(${VELOX_BUILD_PYTHON_PACKAGE})
   set(VELOX_ENABLE_SPARK_FUNCTIONS ON)
 endif()

+if(${VELOX_ENABLE_DUCKDB})
+  set_source(DuckDB)
+  resolve_dependency(DuckDB)
+endif()
+
+if(DEFINED ENV{INSTALL_PREFIX})
+  message(STATUS "Dependency install directory set to: $ENV{INSTALL_PREFIX}")
+  list(APPEND CMAKE_PREFIX_PATH "$ENV{INSTALL_PREFIX}")
+
+  # Allow installed package headers to be picked up before brew/system package
+  # headers
+  include_directories(BEFORE "$ENV{INSTALL_PREFIX}/include")
+endif()

And the output log:

-- Setting DuckDB source to AUTO
-- Building DuckDB from source
-- Using BUNDLED DuckDB
-- Dependency install directory set to: /velox_dependency_install
-- Found OpenSSL: /opt/homebrew/Cellar/openssl@3/3.3.2/lib/libcrypto.dylib (found version "3.3.2")
-- Using ccache: /opt/homebrew/bin/ccache

Copy link
Collaborator

@assignUser assignUser Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will prevent an installed duckdb in INSTALL_PREFIX from being found and always build it from source, so not what we want :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense. I never thought about installing duckdb in INSTALL_PREFIX, but we do not install duckdb in macOS setup script, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this PR duckdb will be installed with the setup-macos :) https://github.com/facebookincubator/velox/pull/11069/files#diff-49efb0acd7ee0e6a2d6f57f2e48b90bd170a41a7c46d2ef757a6c078b543d28cR151d

set_source(DuckDB)
resolve_dependency(DuckDB)
endif()

if(DEFINED ENV{INSTALL_PREFIX})
# Allow installed package headers to be picked up before brew/system package
# headers. We set this after DuckDB bundling since DuckDB uses its own
# dependencies.
include_directories(BEFORE "$ENV{INSTALL_PREFIX}/include")
endif()

# We look for OpenSSL here to cache the result enforce the version across our
# dependencies.
find_package(OpenSSL REQUIRED)
Expand Down Expand Up @@ -413,11 +422,6 @@ else()
endif()
resolve_dependency(glog)

if(${VELOX_ENABLE_DUCKDB})
set_source(DuckDB)
resolve_dependency(DuckDB)
endif()

set_source(fmt)
resolve_dependency(fmt 9.0.0)

Expand Down
10 changes: 10 additions & 0 deletions scripts/setup-macos.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ PYTHON_VENV=${PYHTON_VENV:-"${SCRIPTDIR}/../.venv"}
export OS_CXXFLAGS=" -isystem $(brew --prefix)/include "
NPROC=$(getconf _NPROCESSORS_ONLN)

BUILD_DUCKDB="${BUILD_DUCKDB:-true}"
DEPENDENCY_DIR=${DEPENDENCY_DIR:-$(pwd)}
MACOS_VELOX_DEPS="bison flex gflags glog googletest icu4c libevent libsodium lz4 lzo openssl protobuf@21 snappy xz zstd"
MACOS_BUILD_DEPS="ninja cmake"
Expand Down Expand Up @@ -147,6 +148,14 @@ function install_fast_float {
cmake_install_dir fast_float
}

function install_duckdb {
if $BUILD_DUCKDB ; then
echo 'Building DuckDB'
wget_and_untar https://github.com/duckdb/duckdb/archive/refs/tags/v0.8.1.tar.gz duckdb
cmake_install_dir duckdb -DBUILD_UNITTESTS=OFF -DENABLE_SANITIZER=OFF -DENABLE_UBSAN=OFF -DBUILD_SHELL=OFF -DEXPORT_DLL_SYMBOLS=OFF -DCMAKE_BUILD_TYPE=Release
fi
}

function install_velox_deps {
run_and_time install_velox_deps_from_brew
run_and_time install_ranges_v3
Expand All @@ -159,6 +168,7 @@ function install_velox_deps {
run_and_time install_wangle
run_and_time install_mvfst
run_and_time install_fbthrift
run_and_time install_duckdb
}

(return 2> /dev/null) && return # If script was sourced, don't run commands.
Expand Down
Loading