Merge pull request #34 from samansmink/enable-windows

Enable windows builds
duckdb · Jun 20, 2024 · 82d2003 · 82d2003
2 parents 5cfefa5 + 7229c0c
commit 82d2003
Show file tree

Hide file tree

Showing 5 changed files with 619 additions and 33 deletions.
diff --git a/.github/workflows/MainDistributionPipeline.yml b/.github/workflows/MainDistributionPipeline.yml
@@ -18,7 +18,7 @@ jobs:
     with:
       duckdb_version: v1.0.0
       extension_name: delta
-      exclude_archs: 'wasm_mvp;wasm_eh;wasm_threads;windows_amd64_rtools;windows_amd64'
+      exclude_archs: 'wasm_mvp;wasm_eh;wasm_threads;windows_amd64_rtools'
 
   duckdb-stable-deploy:
     name: Deploy extension binaries
@@ -28,5 +28,5 @@ jobs:
     with:
       extension_name: delta
       duckdb_version: v1.0.0
-      exclude_archs: 'wasm_mvp;wasm_eh;wasm_threads;windows_amd64_rtools;windows_amd64'
+      exclude_archs: 'wasm_mvp;wasm_eh;wasm_threads;windows_amd64_rtools'
       deploy_latest: ${{ startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/main' }}
diff --git a/.github/workflows/_extension_distribution.yml b/.github/workflows/_extension_distribution.yml
@@ -317,6 +317,9 @@ jobs:
         with:
           python-version: '3.11'
 
+      - name: Setup Rust
+        uses: dtolnay/rust-toolchain@stable
+
       - uses: r-lib/actions/setup-r@v2
         if: matrix.duckdb_arch == 'windows_amd64_rtools'
         with:
@@ -340,23 +343,18 @@ jobs:
         with:
           vcpkgGitCommitId: ${{ inputs.vcpkg_commit }}
 
-      - name: Fix for MSVC issue
-        shell: bash
-        env:
-          OVERLAY_TRIPLET_SRC:  ${{ github.workspace }}/vcpkg/triplets/community/x64-windows-static-md.cmake
-          OVERLAY_TRIPLET_DST:  ${{ github.workspace }}/overlay_triplets/x64-windows-static-md.cmake
-        run: |
-          mkdir overlay_triplets
-          cp $OVERLAY_TRIPLET_SRC $OVERLAY_TRIPLET_DST
-          echo "set(VCPKG_PLATFORM_TOOLSET_VERSION "14.38")" >> $OVERLAY_TRIPLET_DST
-
       - name: Build & test extension
         env:
           VCPKG_OVERLAY_TRIPLETS: "${{ github.workspace }}/overlay_triplets"
           DUCKDB_PLATFORM: ${{ matrix.duckdb_arch }}
         run: |
           make test_release
 
+      - name: Error log
+        if: always()
+        run: |
+          cat build/release/rust/src/delta_kernel-stamp/delta_kernel-build-*.log
+
       - uses: actions/upload-artifact@v2
         with:
           name: ${{ inputs.extension_name }}-${{ inputs.duckdb_version }}-extension-${{matrix.duckdb_arch}}${{inputs.artifact_postfix}}

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -24,7 +24,7 @@ if(APPLE)
 elseif(UNIX)
     set(PLATFORM_LIBS m c resolv)
 elseif(WIN32)
-    set(PLATFORM_LIBS ws2_32 userenv advapi32)
+    set(PLATFORM_LIBS ntdll ncrypt secur32 ws2_32 userenv bcrypt msvcrt advapi32)
 else()
     message(STATUS "UNKNOWN OS")
 endif()
@@ -52,27 +52,53 @@ if("${OS_NAME}" STREQUAL "linux")
         set(RUST_PLATFORM_TARGET "x86_64-unknown-linux-gnu")
     endif()
 elseif("${OS_NAME}" STREQUAL "osx")
-    # TODO: clean up upstream; we are not correctly setting OS_ARCH for cross compile
     if ("${OSX_BUILD_ARCH}" STREQUAL "arm64")
         set(RUST_PLATFORM_TARGET "aarch64-apple-darwin")
     elseif ("${OSX_BUILD_ARCH}" STREQUAL "x86_64")
         set(RUST_PLATFORM_TARGET "x86_64-apple-darwin")
     elseif ("${OS_ARCH}" STREQUAL "arm64")
         set(RUST_PLATFORM_TARGET "aarch64-apple-darwin")
-    else()
-        set(RUST_PLATFORM_TARGET "x86_64-apple-darwin")
+    endif()
+elseif(WIN32)
+    if (MINGW AND "${OS_ARCH}" STREQUAL "arm64")
+        set(RUST_PLATFORM_TARGET "aarch64-pc-windows-gnu")
+    elseif (MINGW AND "${OS_ARCH}" STREQUAL "amd64")
+        set(RUST_PLATFORM_TARGET "x86_64-pc-windows-gnu")
+    elseif (MSVC AND "${OS_ARCH}" STREQUAL "arm64")
+        set(RUST_PLATFORM_TARGET "aarch64-pc-windows-msvc")
+    elseif (MSVC AND "${OS_ARCH}" STREQUAL "amd64")
+        set(RUST_PLATFORM_TARGET "x86_64-pc-windows-msvc")
     endif()
 endif()
 
+# We currently only support the predefined targets.
+if ("${RUST_PLATFORM_TARGET}" STREQUAL "")
+    message(FATAL_ERROR "Failed to detect the correct platform")
+endif()
+
+set(RUST_PLATFORM_PARAM "--target=${RUST_PLATFORM_TARGET}")
+message(STATUS "Building for rust target: ${RUST_PLATFORM_TARGET}")
+
+# Remove whitespaces before and after to prevent messed up env variables
 string(STRIP "${RUST_ENV_VARS}" RUST_ENV_VARS)
 
 # Having these set will mess up cross compilation to linux arm
 set(RUST_UNSET_ENV_VARS --unset=CC --unset=CXX --unset=LD)
 
+# Define all the relevant delta-kernel-rs paths/names
+set(DELTA_KERNEL_LIBNAME "${CMAKE_STATIC_LIBRARY_PREFIX}delta_kernel_ffi${CMAKE_STATIC_LIBRARY_SUFFIX}")
+set(DELTA_KERNEL_LIBPATH_DEBUG "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/${RUST_PLATFORM_TARGET}/debug/${DELTA_KERNEL_LIBNAME}")
+set(DELTA_KERNEL_LIBPATH_RELEASE "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/${RUST_PLATFORM_TARGET}/release/${DELTA_KERNEL_LIBNAME}")
+set(DELTA_KERNEL_FFI_HEADER_PATH "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/ffi-headers")
+set(DELTA_KERNEL_FFI_HEADER_C "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/ffi-headers/delta_kernel_ffi.h")
+set(DELTA_KERNEL_FFI_HEADER_CXX "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/ffi-headers/delta_kernel_ffi.hpp")
+
 # Add rust_example as a CMake target
 ExternalProject_Add(
         ${KERNEL_NAME}
         GIT_REPOSITORY "https://github.com/nicklan/delta-kernel-rs"
+        # WARNING: the FFI headers are currently pinned due to the C linkage issue of the c++ headers. Currently, when bumping
+        # the kernel version, the produced header in ./src/include/delta_kernel_ffi.hpp should be also bumped, applying the fix
         GIT_TAG 181232a45562ca78be763c2f5fb46b88a2463b5c
         # Prints the env variables passed to the cargo build to the terminal, useful in debugging because passing them
         # through CMake is an error-prone mess
@@ -82,27 +108,28 @@ ExternalProject_Add(
         # Build debug build
         BUILD_COMMAND
             ${CMAKE_COMMAND} -E env ${RUST_UNSET_ENV_VARS} ${RUST_ENV_VARS}
-            cargo build --package delta_kernel_ffi --workspace --all-features --target=${RUST_PLATFORM_TARGET}
+            cargo build --package delta_kernel_ffi --workspace --all-features ${RUST_PLATFORM_PARAM}
         # Build release build
         COMMAND
             ${CMAKE_COMMAND} -E env ${RUST_UNSET_ENV_VARS} ${RUST_ENV_VARS}
-            cargo build --package delta_kernel_ffi --workspace --all-features --release --target=${RUST_PLATFORM_TARGET}
+            cargo build --package delta_kernel_ffi --workspace --all-features --release ${RUST_PLATFORM_PARAM}
         # Build DATs
         COMMAND
             ${CMAKE_COMMAND} -E env ${RUST_UNSET_ENV_VARS} ${RUST_ENV_VARS}
             cargo build --manifest-path=${CMAKE_BINARY_DIR}/rust/src/delta_kernel/acceptance/Cargo.toml
-        BUILD_BYPRODUCTS "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/${RUST_PLATFORM_TARGET}/debug/libdelta_kernel_ffi.a"
-        BUILD_BYPRODUCTS "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/${RUST_PLATFORM_TARGET}/release/libdelta_kernel_ffi.a"
-        BUILD_BYPRODUCTS "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/ffi-headers/delta_kernel_ffi.h"
-        BUILD_BYPRODUCTS "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/ffi-headers/delta_kernel_ffi.hpp"
+        # Define the byproducts, required for building with Ninja
+        BUILD_BYPRODUCTS "${DELTA_KERNEL_LIBPATH_DEBUG}"
+        BUILD_BYPRODUCTS "${DELTA_KERNEL_LIBPATH_RELEASE}"
+        BUILD_BYPRODUCTS "${DELTA_KERNEL_FFI_HEADER_C}"
+        BUILD_BYPRODUCTS "${DELTA_KERNEL_FFI_HEADER_CXX}"
         INSTALL_COMMAND ""
         LOG_BUILD ON)
 
 build_static_extension(${TARGET_NAME} ${EXTENSION_SOURCES})
 build_loadable_extension(${TARGET_NAME} " " ${EXTENSION_SOURCES})
 
-include_directories(${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/ffi-headers)
-include_directories(${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/ffi-headers)
+# TODO: when C linkage issue is resolved, we should switch back to using the generated headers
+#include_directories(${DELTA_KERNEL_FFI_HEADER_PATH})
 
 # Hides annoying linker warnings
 set(CMAKE_OSX_DEPLOYMENT_TARGET 13.3 CACHE STRING "Minimum OS X deployment version" FORCE)
@@ -112,15 +139,15 @@ add_compile_definitions(DEFINE_DEFAULT_ENGINE)
 
 # Link delta-kernal-rs to static lib
 target_link_libraries(${EXTENSION_NAME}
-        debug "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/${RUST_PLATFORM_TARGET}/debug/libdelta_kernel_ffi.a"
-        optimized "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/${RUST_PLATFORM_TARGET}/release/libdelta_kernel_ffi.a"
+        debug ${DELTA_KERNEL_LIBPATH_DEBUG}
+        optimized ${DELTA_KERNEL_LIBPATH_RELEASE}
         ${PLATFORM_LIBS})
 add_dependencies(${EXTENSION_NAME} delta_kernel)
 
 # Link delta-kernal-rs to dynamic lib
 target_link_libraries(${LOADABLE_EXTENSION_NAME}
-        debug "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/${RUST_PLATFORM_TARGET}/debug/libdelta_kernel_ffi.a"
-        optimized "${CMAKE_BINARY_DIR}/rust/src/delta_kernel/target/${RUST_PLATFORM_TARGET}/release/libdelta_kernel_ffi.a"
+        debug ${DELTA_KERNEL_LIBPATH_DEBUG}
+        optimized ${DELTA_KERNEL_LIBPATH_RELEASE}
         ${PLATFORM_LIBS})
 add_dependencies(${LOADABLE_EXTENSION_NAME} delta_kernel)
 

diff --git a/src/delta_utils.cpp b/src/delta_utils.cpp
@@ -170,10 +170,10 @@ vector<bool> KernelUtils::FromDeltaBoolSlice(const struct ffi::KernelBoolSlice s
     return result;
 }
 
-PredicateVisitor::PredicateVisitor(const vector<string> &column_names, optional_ptr<TableFilterSet> filters) : EnginePredicate {
-        .predicate = this,
-        .visitor = (uintptr_t (*)(void*, ffi::KernelExpressionVisitorState*)) &VisitPredicate}
-{
+PredicateVisitor::PredicateVisitor(const vector<string> &column_names, optional_ptr<TableFilterSet> filters) {
+    predicate = this;
+    visitor = (uintptr_t (*)(void*, ffi::KernelExpressionVisitorState*)) &VisitPredicate;
+
     if (filters) {
         for (auto& filter : filters->filters) {
             column_filters[column_names[filter.first]] = filter.second.get();
@@ -191,7 +191,7 @@ static auto GetNextFromCallable(Callable* callable) -> decltype(std::declval<Cal
 template <typename Callable>
 ffi::EngineIterator EngineIteratorFromCallable(Callable& callable) {
     auto* get_next = &GetNextFromCallable<Callable>;
-    return {.data = &callable, .get_next = (const void *(*)(void*)) get_next};
+    return {&callable, (const void *(*)(void*)) get_next};
 };
 
 // Helper function to prevent pushing down filters kernel cant handle