Fix DuckDB bundling and add support for DuckDB install on MacOS #11069
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
majetideepak
force-pushed
the
fix-duckdb-build
branch
from
1434a38
to
9ce2d24
Compare
|
@yingsu00 Can you verify if this fixes your issue? |
| @@ -192,6 +189,18 @@ if(${VELOX_BUILD_PYTHON_PACKAGE}) | |||
| set(VELOX_ENABLE_SPARK_FUNCTIONS ON) | |||
| endif() | |||
|
|
|||
| if(${VELOX_ENABLE_DUCKDB}) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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/ccacheThere was a problem hiding this comment.
This will prevent an installed duckdb in INSTALL_PREFIX from being found and always build it from source, so not what we want :)
There was a problem hiding this comment.
Make sense. I never thought about installing duckdb in INSTALL_PREFIX, but we do not install duckdb in macOS setup script, right?
There was a problem hiding this comment.
After this PR duckdb will be installed with the setup-macos :) https://github.com/facebookincubator/velox/pull/11069/files#diff-49efb0acd7ee0e6a2d6f57f2e48b90bd170a41a7c46d2ef757a6c078b543d28cR151d
| @@ -192,6 +189,18 @@ if(${VELOX_BUILD_PYTHON_PACKAGE}) | |||
| set(VELOX_ENABLE_SPARK_FUNCTIONS ON) | |||
| endif() | |||
|
|
|||
| if(${VELOX_ENABLE_DUCKDB}) | |||
There was a problem hiding this comment.
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.
|
Once @yingsu00 confirms this fixes the issue this is |
It works for me locally with latest folly. |
assignUser
added
the
ready-to-merge
|
@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@kagamiori merged this pull request in 08bca2a. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
DuckDB fails to build after the recent support for INSTALL_PREFIX
Resolves #11058