[DuckDB] Add special case for windows

[KronosTheLate]

Attempting to fix this issue by adding a special case for windows. Tried to make that special case a mixture of the generic build script, and the contents here

[giordano]

Why not using conditionals inside the script? Having different scripts for different platforms is a needless complication.

[KronosTheLate]

I figured that the cases inside the script would have be decided based on the target, which seemed to be defined to this julia array:

julia> using BinaryBuilder; expand_cxxstring_abis(supported_platforms())
┌ Warning: Running Registrator on a Julia version that does not support weak dependencies. Weak dependencies will not be registered.
└ @ RegistryTools C:\Users\dennis.bal\.julia\packages\RegistryTools\tHkoa\src\RegistryTools.jl:18
29-element Vector{Platform}:
 Linux i686 {cxxstring_abi=cxx03, libc=glibc}
 Linux i686 {cxxstring_abi=cxx11, libc=glibc}
 Linux x86_64 {cxxstring_abi=cxx03, libc=glibc}
 Linux x86_64 {cxxstring_abi=cxx11, libc=glibc}
 Linux aarch64 {cxxstring_abi=cxx03, libc=glibc}
 Linux aarch64 {cxxstring_abi=cxx11, libc=glibc}
 Linux armv6l {call_abi=eabihf, cxxstring_abi=cxx03, libc=glibc}
 Linux armv6l {call_abi=eabihf, cxxstring_abi=cxx11, libc=glibc}
 Linux armv7l {call_abi=eabihf, cxxstring_abi=cxx03, libc=glibc}
 Linux armv7l {call_abi=eabihf, cxxstring_abi=cxx11, libc=glibc}
 Linux powerpc64le {cxxstring_abi=cxx03, libc=glibc}
 Linux powerpc64le {cxxstring_abi=cxx11, libc=glibc}
 Linux i686 {cxxstring_abi=cxx03, libc=musl}
 Linux i686 {cxxstring_abi=cxx11, libc=musl}
 Linux x86_64 {cxxstring_abi=cxx03, libc=musl}
 Linux x86_64 {cxxstring_abi=cxx11, libc=musl}
 Linux aarch64 {cxxstring_abi=cxx03, libc=musl}
 Linux aarch64 {cxxstring_abi=cxx11, libc=musl}
 Linux armv6l {call_abi=eabihf, cxxstring_abi=cxx03, libc=musl}
 Linux armv6l {call_abi=eabihf, cxxstring_abi=cxx11, libc=musl}
 Linux armv7l {call_abi=eabihf, cxxstring_abi=cxx03, libc=musl}
 Linux armv7l {call_abi=eabihf, cxxstring_abi=cxx11, libc=musl}
 macOS x86_64
 macOS aarch64
 FreeBSD x86_64
 Windows i686 {cxxstring_abi=cxx03}
 Windows i686 {cxxstring_abi=cxx11}
 Windows x86_64 {cxxstring_abi=cxx03}
 Windows x86_64 {cxxstring_abi=cxx11}

Where there are e.g. multiple names for windows that have to be concidered. Furthermore, it is unclear how those Platform objects are passed to bash.

I figured it was actually extremely neat and tidy to move that logic into julia, using clearly named and easy-to-use function is Sys.iswindows, instead of adding string matching for multiple cases, which to me felt hacky. So we seem to have different intuitions from looking at the same stuff.

That said, you are the desciding vote, and I am happy to implement it whatever way you deem best. I am however possibly not capable of doing it the way you want, so you might have to make the change yourself.

[stemann]

@KronosTheLate The platform being built is available in $target (and in $bb_full_target).

So you just need to do something like:

if [[ "$target" == *-w64-mingw32* ]]; then
  # special handling for Windows
fi

[giordano]

This is also explained in the docs:

• https://docs.binarybuilder.org/stable/build_tips/#Initiating-different-shell-commands-based-on-target
• https://docs.binarybuilder.org/stable/building/#Platforms

[KronosTheLate]

if [[ "$target" == *-w64-mingw32* ]]; then
  # special handling for Windows
fi

Is that better or worse than this snippet from the linked file in the docs?

if [[ "${target}" == *-ming* ]]; then
    # Fast path for Windows: just copy the content of the tarball to the prefix
    cp -r ${WORKSPACE}/srcdir/${target}/mingw${nbits}/* ${prefix}
    exit
fi

In any case, both options are far less understandable to me than what I have done. It is both easy to understand what is done, and have confidence that it covers all windows platforms. I will take any chance I get to do my scripting in Julia instead of bash xD

With that said, would you prefer if I changed it? If yes, which variant?

[stemann]

Yes - please adapt.

I would prefer the first variant (but either is fine).

[giordano]

In any case, both options are far less understandable to me than what I have done.

What you've done is wrong because you'll register the package twice. Also, what you've done has duplicate code, which is error prone and less maintainable. Please use the conditionals inside the script instead.

[KronosTheLate]

The current error from the build is here, and reads

[09:13:37] ../../third_party/mbedtls/libduckdb_mbedtls.a(mbedtls_wrapper.cpp.o): In function `duckdb_mbedtls::MbedTlsWrapper::AESGCMStateMBEDTLS::~AESGCMStateMBEDTLS()':
[09:13:37] mbedtls_wrapper.cpp:(.text+0x4c): undefined reference to `duckdb::EncryptionState::~EncryptionState()'
[09:13:37] ../../third_party/mbedtls/libduckdb_mbedtls.a(mbedtls_wrapper.cpp.o): In function `duckdb_mbedtls::MbedTlsWrapper::AESGCMStateMBEDTLS::~AESGCMStateMBEDTLS()':
[09:13:37] mbedtls_wrapper.cpp:(.text+0x3f4): undefined reference to `duckdb::EncryptionState::~EncryptionState()'
[09:13:37] ../../third_party/mbedtls/libduckdb_mbedtls.a(mbedtls_wrapper.cpp.o): In function `duckdb_mbedtls::MbedTlsWrapper::AESGCMStateMBEDTLS::AESGCMStateMBEDTLS()':
[09:13:37] mbedtls_wrapper.cpp:(.text+0xd80): undefined reference to `duckdb::EncryptionState::EncryptionState()'
[09:13:37] mbedtls_wrapper.cpp:(.text+0xdcc): undefined reference to `duckdb::EncryptionState::~EncryptionState()'
[09:13:37] ../../third_party/mbedtls/libduckdb_mbedtls.a(mbedtls_wrapper.cpp.o):(.data.rel.ro._ZTIN14duckdb_mbedtls14MbedTlsWrapper18AESGCMStateMBEDTLSE[_ZTIN14duckdb_mbedtls14MbedTlsWrapper18AESGCMStateMBEDTLSE]+0x10): undefined reference to `typeinfo for duckdb::EncryptionState'

Since the official build instructions:

python scripts/windows_ci.py
        cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_GENERATOR_PLATFORM=x64 -DENABLE_EXTENSION_AUTOLOADING=1 -DENABLE_EXTENSION_AUTOINSTALL=1 -DDUCKDB_EXTENSION_CONFIGS="${GITHUB_WORKSPACE}/.github/config/bundled_extensions.cmake" -DDISABLE_UNITY=1 -DOVERRIDE_GIT_DESCRIBE="$OVERRIDE_GIT_DESCRIBE"
        cmake --build . --config Release --parallel

must work, something in my merging of the existing cmake command and that one has gone wrong. My version looks like this:

cd $WORKSPACE/srcdir/duckdb/

python scripts/windows_ci.py

cmake -B build \
      -DCMAKE_INSTALL_PREFIX=${prefix} \
      -DCMAKE_TOOLCHAIN_FILE=${CMAKE_TARGET_TOOLCHAIN} \
      -DCMAKE_BUILD_TYPE=Release \
      -DENABLE_SANITIZER=FALSE \
      -DBUILD_EXTENSIONS='autocomplete;icu;parquet;json;fts;tpcds;tpch' \
      -DENABLE_EXTENSION_AUTOLOADING=1 \
      -DENABLE_EXTENSION_AUTOINSTALL=1 \
      -DBUILD_UNITTESTS=FALSE \
      -DBUILD_SHELL=TRUE \
      -DDUCKDB_EXPLICIT_PLATFORM="${target}" \
      -DUCKDB_EXTENSION_CONFIGS="$WORKSPACE/srcdir/duckdb/.github/config/bundled_extensions.cmake" \
      -DDISABLE_UNITY=1
cmake --build build --parallel ${nproc}
cmake --install build

install -Dvm 755 "build/src/libduckdb.${dlext}" -t "${libdir}"

We are already past my paygrade, so help is appreciated.

[stemann]

@KronosTheLate You should (very) likely use the MinGW64 instructions and not the MSVC instructions: https://duckdb.org/docs/dev/building/build_instructions.html#msys2-and-mingw64

BinaryBuilder/Yggdrasil is using GCC, not MSVC.

[KronosTheLate]

I am getting a build error, which appears related to not having the right dependencies. Microsoft copilot suggests fixing this in the workflow:

I need some help :/

[stemann]

You should not copy/paste the DuckDB instructions verbatim.

You should not install stuff with pacman - JLL dependencies is the way to get dependencies.

From the DuckDB documentation you should only take away the cmake args:

cmake -G "Ninja" -DCMAKE_BUILD_TYPE=Release -DBUILD_EXTENSIONS="icu;parquet;json"
cmake --build . --config Release

[giordano]

Why is the build command different at all across platforms? Either of the two should be just fine everywhere.

[KronosTheLate]

Just because the official build instructions for windows use -G "Ninja. The library failed to precompile on windows, so I was just guessing that if I use the windows-specific instructions more closely, then that might fix the issue.

As you can see by the commit-history, I am just spitballing, and have no idea what I am doing. Help appreciated.

[giordano]

Ninja isn't really Windows-specific in any way. Either Ninja or makefile is just fine everywhere.

[giordano]

Why was this removed?

[KronosTheLate]

Whoops - I mistakenly forgot the expand_cxxstring_abis part when undoing the separation between the windows command. Fixed in separate commit.

[KronosTheLate]

It now builds without error, but that was the case before this PR. I am not sure how to test the build other than attempting to install DuckDB on a windows machine after this is merged, which I am happy to test. I am also not sure if merging this will result in DuckDB_jll.jl to use the new binaries, or if that only happens when a new version is tagged. Perhaps one of the maintainers know if this will be effective after merge?

Because the precompilation error occurs for DuckDB_jll.jl, which is built by Yggdrasil, it seems fair enough to make a test that installs the package as part of the build process. That might add a lot of overhead though, so I am not sure that it is overall worth it.