Fix duckdb build failure

[yingsu00]

Improve dependency install and build (#10920) installs all dependencies in
deps-install subfolder, and set the include directory to it in front of
other include directories. This caused duckdb build failed with "error:
use of undeclared identifier 'FMT_SNPRINTF'". This commit removes the
BEFORE keyword to fix the problem.

Fixes #11058

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	8210260
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66f0fe61868a65000866e185

[zuyu]

Please also remove the comment regarding the before ordering above.

Just curious, since we already have CMAKE_PREFIX_PATH to include $INSTALL_PREFIX, do we need include_directories("$ENV{INSTALL_PREFIX}/include") at all?

cc @assignUser @majetideepak

[yingsu00]

Please also remove the comment regarding the before ordering above.

Just curious, since we already have CMAKE_PREFIX_PATH to include $INSTALL_PREFIX, do we need include_directories("$ENV{INSTALL_PREFIX}/include") at all?

cc @assignUser @majetideepak

Good catch. I actually think include_directories("$ENV{INSTALL_PREFIX}/include") is not needed. I will try it on a new repo tomorrow.

[majetideepak]

@zuyu, @yingsu00 as described in the comment, we want INSTALL_PREFIX headers to appear before brew headers in the include order. So we need this.
The assumption here was that users either installed dependencies from Bundling or used this script. Mixing both is not a supported case.
But I see that on MacOS, we always bundle DuckDB. And likely DuckDB prefers its own fmt version.
We can bundle DuckDB before this line and fix the issue.

[zuyu]

Building DuckDB before the include statement works. Thank you for the explanation, @majetideepak!

index 8b0d3a67c..f31f91b69 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -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"
@@ -192,6 +189,18 @@ 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})
+  # Allow installed package headers to be picked up before brew/system package
+  # headers
+  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)
@@ -413,11 +423,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)

[majetideepak]

I have a PR here with the proper fix
#11069

[yingsu00]

Closing for
#11069