fix(c): Generate versioned DLLs and import LIBs when building with MSVC #2858

IIFE · 2025-05-25T21:36:44Z

Hi.

This is to address building binaries and LIBs on Windows. The PR is mainly for consideration purposes, as it's my first PR against this repo so not necessarily a complete one. Following are the changes I needed to make:

Disable ASAN/UBSAN builds configs as they require GCC/Clang, and on Windows we're building with MSVC.
An environment variable to vcpkg root is expected so that third party packages (sqlite, postgres etc) can be installed and found via vcpkg.
Disable some warnings for testing code that are safe to ignore for MSVC builds.
Ensure the generated binaries have file level details (Details tab on the file properties in Windows) for the version. This is required by Windows MSIs that may be used to install the adbc DLLs in order to detect if the DLL being installed is a newer or older version.
Generate import LIB file for drivers built with go build, which includes flight SQL and Snowflake drivers. This allows the testing projects referencing those DLLs to built on Windows.
The GoUtils.cmake is not automatically copying the driver DLLs to the location of the test executables, causing the tests for those drivers to fail. Added a post build command to ensure the DLLs are copied.
Fix some type casting mismatch (casting between size_t and int).
Fix warnings where a variable is hiding a previous one with the same name. Appended 2 to the second variable name.

What I haven't done is update the GitHub actions to verify a complete build on a Windows machine as I'm not too familiar with the CI/CD pipeline in this repo.

Hopefully this is a good start to get everything building successfully and Windows.

Closes #2846

lidavidm · 2025-05-25T23:54:50Z

Thanks for figuring this out. Overall this looks reasonable. If you don't mind I can attempt to push a Windows pipeline to this PR so we can test it.

c/cmake_modules/AdbcDefines.cmake

c/cmake_modules/BuildUtils.cmake

c/cmake_modules/version.rc.in

kou · 2025-05-26T00:28:32Z

c/cmake_modules/version.rc.in

+FILEFLAGS 0x0L
+FILEOS VOS__WINDOWS32
+FILETYPE VFT_DLL
+FILESUBTYPE VFT2_UNKNOWN


Do we need to specify this with VFT_DLL?

It's the main file type value that makes sense for the produced files (DLLs). The rest are unrelated to the DLL file type

c/driver/flightsql/CMakeLists.txt

c/cmake_modules/BuildUtils.cmake

c/cmake_modules/GoUtils.cmake

Co-authored-by: Sutou Kouhei <[email protected]>

…o window-build

Co-authored-by: Sutou Kouhei <[email protected]>

IIFE · 2025-05-26T01:22:34Z

Thanks for figuring this out. Overall this looks reasonable. If you don't mind I can attempt to push a Windows pipeline to this PR so we can test it.

Yes please @lidavidm that would be helpful.

…o window-build

lidavidm · 2025-05-27T07:48:30Z

Got it - I'll try to get to it soon

IIFE · 2025-05-27T08:31:05Z

pre-commit issues addressed

Kakolukia91 · 2025-05-27T13:47:17Z

Hmm not sure why the pre-commit is still failing. Is it because I've run mine on a Windows machine so it's not capturing everything the CI does (which runs on Ubuntu from the looks of it)? 🤔

lidavidm · 2025-05-27T13:50:47Z

How are you running it?

IIFE · 2025-05-27T18:05:19Z

Using this command:
pre-commit run --show-diff-on-failure --color=always --all-files

lidavidm · 2025-05-29T08:26:43Z

I don't see a problem. I guess it could be Windows.

lidavidm · 2025-05-30T05:46:50Z

It looks like tests might not fully build:

C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: validation/Debug/libadbc_validation_util.a(adbc_validation_util.cc.obj): in function `adbc_validation::StatementGetOption[abi:cxx11](AdbcStatement*, std::basic_string_view<char, std::char_traits<char> >, AdbcError*)':
D:/a/arrow-adbc/arrow-adbc/c/validation/adbc_validation_util.cc:50: undefined reference to `__imp_AdbcStatementGetOption'

I'm OK disabling tests for time time being if we can just get DLLs to build properly (we could perhaps just verify that DLLs and implibs exist)

lidavidm · 2025-05-30T06:16:38Z

Also I see gendef being invoked, but it's not installed by CMake - that probably also needs to be tweaked

lidavidm · 2025-05-30T06:17:51Z

Oh wait - is the dll.a sufficient? (That's being installed.) Does the .def file need to be installed?

IIFE · 2025-05-30T12:02:49Z

Also I see gendef being invoked, but it's not installed by CMake - that probably also needs to be tweaked

Yeah that's needed to generate the DEF file, which is then used to create the lib import

It looks like tests might not fully build:

C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: validation/Debug/libadbc_validation_util.a(adbc_validation_util.cc.obj): in function `adbc_validation::StatementGetOption[abi:cxx11](AdbcStatement*, std::basic_string_view<char, std::char_traits<char> >, AdbcError*)':
D:/a/arrow-adbc/arrow-adbc/c/validation/adbc_validation_util.cc:50: undefined reference to `__imp_AdbcStatementGetOption'

I'm OK disabling tests for time time being if we can just get DLLs to build properly (we could perhaps just verify that DLLs and implibs exist)

Hmm interesting, I didn't encounter this. Suggests the DEF/Lib is not getting generated?

Oh wait - is the dll.a sufficient? (That's being installed.) Does the .def file need to be installed?

Unfortunately the DLL by itself is not sufficient, as on Windows the LIB import file is needed in order to link at build time. So without the LIB file the test projects don't build.

lidavidm · 2025-05-30T12:34:00Z

Yeah, I ask because it seems the .lib isn't there

- Installing: D:/a/arrow-adbc/arrow-adbc/local/lib/pkgconfig/adbc-driver-sqlite.pc
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/include/arrow-adbc/driver/snowflake.h
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/bin/libadbc_driver_snowflake.dll
-- Up-to-date: D:/a/arrow-adbc/arrow-adbc/local/bin/libadbc_driver_snowflake.dll
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/lib/libadbc_driver_snowflake.dll.a
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/lib/cmake/AdbcDriverSnowflake/AdbcDriverSnowflakeConfig.cmake
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/lib/cmake/AdbcDriverSnowflake/AdbcDriverSnowflakeConfigVersion.cmake
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/lib/pkgconfig/adbc-driver-snowflake.pc

lidavidm · 2025-05-30T12:34:20Z

Hmm interesting, I didn't encounter this. Suggests the DEF/Lib is not getting generated?

I'm guessing it just doesn't know where to look; either that or the steps aren't getting sequenced right

IIFE · 2025-05-30T13:12:59Z

Yeah, I ask because it seems the .lib isn't there

- Installing: D:/a/arrow-adbc/arrow-adbc/local/lib/pkgconfig/adbc-driver-sqlite.pc
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/include/arrow-adbc/driver/snowflake.h
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/bin/libadbc_driver_snowflake.dll
-- Up-to-date: D:/a/arrow-adbc/arrow-adbc/local/bin/libadbc_driver_snowflake.dll
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/lib/libadbc_driver_snowflake.dll.a
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/lib/cmake/AdbcDriverSnowflake/AdbcDriverSnowflakeConfig.cmake
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/lib/cmake/AdbcDriverSnowflake/AdbcDriverSnowflakeConfigVersion.cmake
-- Installing: D:/a/arrow-adbc/arrow-adbc/local/lib/pkgconfig/adbc-driver-snowflake.pc

Are the 2 required msys64 tools (gendef and dlltool) installed? Is this warning getting generated?

Import library generation tools not found. You may need to link directly to the DLL or use delay-loading.

lidavidm · 2025-05-30T13:17:29Z

I don't see that in the log: https://github.com/apache/arrow-adbc/actions/runs/15340229681/job/43164905567?pr=2858

IIFE · 2025-05-30T19:02:34Z

I don't see that in the log: https://github.com/apache/arrow-adbc/actions/runs/15340229681/job/43164905567?pr=2858

The issue is down to the commands in GitHub action not simulating Visual Studio. Pushed these changes to the GitHub flow:

Use Visual Studio generator so that the required vcxproj files are created
Build using msbuild against ALL_BUILD.vcxproj
Install using msbuild against INSTALL.vcxproj

Tested the above from PowerShell locally and works as expected. Let's see if the GitHub action passes now. This is also with tests enabled.

c/version.rc.in

lidavidm · 2025-06-02T02:19:44Z

c/driver/flightsql/CMakeLists.txt

@@ -63,6 +63,18 @@ if(ADBC_BUILD_TESTS)
                adbc_driver_common
                adbc_validation
                ${TEST_LINK_LIBS})
+
+  if(ADBC_TEST_LINKAGE STREQUAL "shared")
+    # The go build is not copying the DLL to the test location, causing the test to fail on Windows.


Hmm, how is this handled for the C/C++ builds? I wouldn't think CMake copies the DLL over...

It seems by default cmake copies the dll when it is referenced as a library by another cmake project (the test one in this case). However, because the flightsql and snowflake ones are generated manually via GoUtils, they are not getting copied to the test bin folder.

lidavidm · 2025-06-02T02:25:06Z

c/cmake_modules/GoUtils.cmake

-                               "${LIBOUT_SHARED}.${ADBC_SO_VERSION}.0.h"
-                       COMMENT "Building Go Shared lib ${GO_LIBNAME}"
-                       COMMAND_EXPAND_LISTS)
+    if(WIN32)


We're now fully duplicating the build paths...if Windows doesn't want the version suffix, can it just rename the file afterwards instead?

Yes ideally the Windows files don't need the version in the file name, as that makes copying them during build events in Visual Studio difficult. Where is the duplication happening? The main change was to remove ${ADBC_FULL_SO_VERSION} and ${ADBC_SO_VERSION} suffixes.

The build commands are effectively the same, though. If it's just a matter of filename, can we either (1) introduce a variable to hold the filename, or (2) just rename it after build?

lidavidm · 2025-06-02T02:25:48Z

Also ping @zeroshade

…o window-build

lidavidm · 2025-06-04T02:07:56Z

I have another concern in that we seem to be installing gmock/gtest, but that should be unrelated to this PR

         -- Install configuration: "Release"
         -- Installing: D:/a/arrow-adbc/arrow-adbc/local/include
         -- Installing: D:/a/arrow-adbc/arrow-adbc/local/include/gmock
         -- Installing: D:/a/arrow-adbc/arrow-adbc/local/include/gmock/gmock-actions.h
         -- Installing: D:/a/arrow-adbc/arrow-adbc/local/include/gmock/gmock-cardinalities.h

Generate versioned DLLs and import LIBs when building with MSVC

a01b836

IIFE requested a review from lidavidm as a code owner May 25, 2025 21:36

github-actions bot added this to the ADBC Libraries 19 milestone May 25, 2025

IIFE changed the title ~~Generate versioned DLLs and import LIBs when building with MSVC~~ fix(c): Generate versioned DLLs and import LIBs when building with MSVC May 25, 2025

kou reviewed May 26, 2025

View reviewed changes

IIFE and others added 6 commits May 26, 2025 02:04

Update c/cmake_modules/AdbcDefines.cmake

4a2394a

Co-authored-by: Sutou Kouhei <[email protected]>

Sory warning list

3ae75c3

Merge branch 'window-build' of https://github.com/IIFE/arrow-adbc int…

b8ad1f2

…o window-build

Move version rc to c folder

c2254ee

Add Software Foundation to company name

cc41040

Update c/driver/flightsql/CMakeLists.txt

f87b1d4

Co-authored-by: Sutou Kouhei <[email protected]>

IIFE added 4 commits May 26, 2025 21:58

Include licence comment

563cfd4

Merge branch 'window-build' of https://github.com/IIFE/arrow-adbc int…

cf87023

…o window-build

Use defines for hard-coded values

fa7e0f6

Use WIN32 instead of MSVC for windows specific conditions

9f73ec3

Fix pre-commit

2a5a1eb

Add vcpkg pipeline

08f96db

lidavidm requested a review from zeroshade as a code owner May 30, 2025 05:16

lidavidm added 4 commits May 30, 2025 14:18

try again

d0ebe48

try again

112599a

try again

4cefd77

vcpkg install

12f66b7

don't build tests

aab31a1

Use msbuild to build and install

81e9ea5

Fix pre-commit

10c3776

lidavidm reviewed Jun 2, 2025

View reviewed changes

IIFE added 2 commits June 2, 2025 10:04

Put licence before include

81b6db7

Merge branch 'window-build' of https://github.com/IIFE/arrow-adbc int…

622a066

…o window-build

fix(c): Generate versioned DLLs and import LIBs when building with MSVC #2858

Are you sure you want to change the base?

fix(c): Generate versioned DLLs and import LIBs when building with MSVC #2858

Uh oh!

Conversation

IIFE commented May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidavidm commented May 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kou May 26, 2025

Choose a reason for hiding this comment

Uh oh!

IIFE May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IIFE commented May 26, 2025

Uh oh!

lidavidm commented May 27, 2025

Uh oh!

IIFE commented May 27, 2025

Uh oh!

Kakolukia91 commented May 27, 2025

Uh oh!

lidavidm commented May 27, 2025

Uh oh!

IIFE commented May 27, 2025

Uh oh!

lidavidm commented May 29, 2025

Uh oh!

lidavidm commented May 30, 2025

Uh oh!

lidavidm commented May 30, 2025

Uh oh!

lidavidm commented May 30, 2025

Uh oh!

IIFE commented May 30, 2025

Uh oh!

lidavidm commented May 30, 2025

Uh oh!

lidavidm commented May 30, 2025

Uh oh!

IIFE commented May 30, 2025

Uh oh!

lidavidm commented May 30, 2025

Uh oh!

IIFE commented May 30, 2025

Uh oh!

Uh oh!

lidavidm Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

IIFE Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

lidavidm Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

IIFE Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

lidavidm Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Jun 2, 2025

Uh oh!

lidavidm commented Jun 4, 2025

Uh oh!

Uh oh!

IIFE commented May 25, 2025 •

edited

Loading