feat(c): Use C++ visibility support in Meson configuration #2740

WillAyd · 2025-04-23T15:49:16Z

No description provided.

WillAyd · 2025-04-23T15:49:41Z

go/adbc/drivermgr/arrow-adbc/adbc.h

@@ -152,15 +152,19 @@ struct ArrowArrayStream {
 // Storage class macros for Windows
 // Allow overriding/aliasing with application-defined macros
 #if !defined(ADBC_EXPORT)
-#if defined(_WIN32)
+#if defined_WIN32 || defined __CYGWIN__


The updates to this macro are inspired by @paleolimbot work in apache/arrow-nanoarrow#719

c/include/arrow-adbc/adbc.h

WillAyd · 2025-04-24T20:25:07Z

c/include/arrow-adbc/adbc.h

@@ -152,15 +152,19 @@ struct ArrowArrayStream {
 // Storage class macros for Windows
 // Allow overriding/aliasing with application-defined macros
 #if !defined(ADBC_EXPORT)
-#if defined(_WIN32)
+#if (defined(_WIN32) || defined(__CYGWIN__)) && defined(ADBC_BUILD_DLL)


While this is consistent with nanoarrow, I do wonder if requiring two defines (ADBC_EXPORTING and ADBC_BUILD_DLL) is superfluous; the latter prevents the former from having any meaning when applied to a static library, but maybe it would be easier to just ensure ADBC_EXPORTING is never defined for static targets (?)

ADBC_BUILD_DLL is needed because we can't use __declspec(dll{import,export}) entirely for static linking.

FYI: Apache Arrow C++ uses ARROW_STATIC not ADBC_BUILD_DLL: https://github.com/apache/arrow/blob/6e84d990379a0fe20a0a89311aa864f40efded23/cpp/src/arrow/util/visibility.h#L42

It's specified when static linking not shared linking.

I think that specifying additional macro for building static linking (ARROW_STATIC) approach is easier to use than specifying additional macro for building shared library (ADBC_BUILD_DLL). Because we can add additional cflags with .pc by Cflags.private.

Is the idea that downstream targets that link to a static arrow library would also need to define ADBC_STATIC? The current design necessitates that on Windows, but I'm not sure that is correct

lidavidm · 2025-04-27T01:06:34Z

CC @kou for Meson/build changes?

kou

In general, we should use separated XXX_EXPORT macro for each DLL. For example, we should use ADBC_MANAGER_EXPORT for adbc_driver_manager.dll and ADBC_POSTGRESQL_EXPORT for adbc_driver_postgresql.dll. If we use ADBC_EXPORT for all .dlls, we can't mix static adbc_driver_manager.lib and shared adbc_driver_postgresql.dll.

Could you also update .pc? We need to specify ADBC_BUILD_DLL for shared linking, right? (We can use pkgconf --static and Cflags.private for static linking only flags but how to do it for shared linking only flags...?)

WillAyd · 2025-04-27T14:18:43Z

In general, we should use separated XXX_EXPORT macro for each DLL. For example, we should use ADBC_MANAGER_EXPORT for adbc_driver_manager.dll and ADBC_POSTGRESQL_EXPORT for adbc_driver_postgresql.dll. If we use ADBC_EXPORT for all .dlls, we can't mix static adbc_driver_manager.lib and shared adbc_driver_postgresql.dll.

This is a good idea - OK to do as a follow up? I think that will be a larger change

Could you also update .pc? We need to specify ADBC_BUILD_DLL for shared linking, right? (We can use pkgconf --static and Cflags.private for static linking only flags but how to do it for shared linking only flags...?)

I think if we switch over to your idea for defining a macro when statically linking instead of shared linking then that will help simplify this for Unix platforms. I am not sure about Windows platforms - is there a way within pkg-config to specify that certain defines are only valid for static linkage?

kou · 2025-04-28T09:32:20Z

This is a good idea - OK to do as a follow up? I think that will be a larger change

I'm OK with it. Or it may be better that we split this PR to per driver PRs. Each PR will add separated ADBC_XXX_EXPORT.

I think if we switch over to your idea for defining a macro when statically linking instead of shared linking then that will help simplify this for Unix platforms. I am not sure about Windows platforms - is there a way within pkg-config to specify that certain defines are only valid for static linkage?

We can use the same approach (Cflags.private) with pkg-config on Windows (MSYS2).

WillAyd · 2025-04-28T12:45:27Z

We can use the same approach (Cflags.private) with pkg-config on Windows (MSYS2).

FWIW I don't see Cflags.private as a documented field for pkg-config:

https://people.freedesktop.org/~dbn/pkg-config-guide.html

From that I assume it is Libs.private that should be the right place for this?

WillAyd · 2025-04-28T12:58:19Z

c/driver/bigquery/adbc-driver-bigquery.pc.in

@@ -23,3 +23,4 @@ Description: The ADBC BigQuery driver provides an ADBC driver for BigQuery.
 URL: https://github.com/apache/arrow-adbc
 Version: @ADBC_VERSION@
 Libs: -L${libdir} -ladbc_driver_bigquery
+Libs.private: -DADBC_STATIC


I don't think the modules built by Go (bigquery, flightsql, snowflake) would ever be static (?) so perhaps this is superfluous, but I figured worth keeping for consistency with other .pc files

See #2738, they can be built and used as static libraries, but there's caveats as the static library contains the Go runtime

Ah OK great - with how this PR has evolved its probably worth merging that one first and rebasing this on top

kou · 2025-04-28T22:37:35Z

We can use the same approach (Cflags.private) with pkg-config on Windows (MSYS2).

FWIW I don't see Cflags.private as a documented field for pkg-config:

https://people.freedesktop.org/~dbn/pkg-config-guide.html

Oh, I didn't know it. But pkgconf that is a replacement of pkg-config supports Cflags.private: http://pkgconf.org/features.html

So we can use Cflags.private.

From that I assume it is Libs.private that should be the right place for this?

We should not use Libs.private for this because -DADBC_STATIC is needed for compiling phase not linking phase.

WillAyd · 2025-04-29T12:31:46Z

c/driver/framework/CMakeLists.txt

@@ -35,6 +36,7 @@ if(ADBC_BUILD_TESTS)
                base_driver_test.cc
                EXTRA_LINK_LIBS
                adbc_driver_framework)
+  target_compile_definitions(adbc-driver-framework-test PRIVATE ADBC_STATIC)


I don't see the comment I thought I left originally but I wanted to check this was the desired behavior. The way the macro is now, the library itself and anything that links to it must define ADBC_STATIC. The former makes sense, but we don't want the downstream target to have to define that, right? @kou

Hmm. Not right. All downstream targets also need to define ADBC_STATIC. We can automate it by changing target_compile_definitions(adbc_driver_framework PRIVATE ADBC_EXPORTING ADBC_STATIC) to:

target_compile_definitions(adbc_driver_framework PRIVATE ADBC_EXPORTING) target_compile_definitions(adbc_driver_framework PUBLIC ADBC_STATIC)

Ah thanks - makes sense

This works great for targets that are explicitly defined with add_library(<TARGET> STATIC ...) but doesn't seem to work with the add_arrow_lib function. From looking at it, my understanding is that add_arrow_lib can build both SHARED and STATIC libraries, depending on whether ADBC_BUILD_SHARED and ADBC_BUILD_STATIC are set.

Simply doing:

if(ADBC_BUILD_STATIC) target_compile_definitions(<TARGET> PUBLIC ADBC_STATIC) endif()

I think can end up defining ADBC_STATIC for the shared libraries in the case when ADBC_BUILD_STATIC and ADBC_BUILD_SHARED are both enabled.

To solve that, do I need to extend the add_arrow_lib function to accept an argument like STATIC_COMPILER_FLAGS? There is already a SHARED_LINK_FLAGS argument

Ah nevermind...I see the issue causing the failures in CI currently. The adbc_driver_postgresql and adbc_driver_sqlite targets both link against adbc_driver_common and adbc_driver_framework. The common and framework libraries are both explicitly declared as STATIC in the current CMake configuration, but the postgresql and sqlite drivers are not. So by making the ADBC_STATIC define PUBLIC on the common and framework libraries, I think CMake is also forcing the postgresql and sqlite drivers to define that as well, but that then interferes with their ability to properly dllexport symbols

Ah, we should use separated ADBC_XXX_STATIC like ADBC_XXX_EXPORT.

WillAyd · 2025-05-30T11:32:04Z

Haven't forgotten about this - just struggling with the Windows CMake portion due to a lack of a machine for debugging. If anyone has any ideas on what may be missing there I am open to any suggestions!

WillAyd requested review from zeroshade and lidavidm as code owners April 23, 2025 15:49

github-actions bot added this to the ADBC Libraries 18 milestone Apr 23, 2025

WillAyd commented Apr 23, 2025

View reviewed changes

c/include/arrow-adbc/adbc.h Show resolved Hide resolved

WillAyd force-pushed the symbol-visibility branch 13 times, most recently from b760ead to cac0cbe Compare April 24, 2025 20:16

WillAyd commented Apr 24, 2025

View reviewed changes

WillAyd force-pushed the symbol-visibility branch from cac0cbe to 7d99f27 Compare April 25, 2025 12:50

lidavidm approved these changes Apr 27, 2025

View reviewed changes

kou reviewed Apr 27, 2025

View reviewed changes

WillAyd commented Apr 28, 2025

View reviewed changes

WillAyd force-pushed the symbol-visibility branch 2 times, most recently from 1d0f4ef to 2eaf405 Compare April 28, 2025 13:51

WillAyd commented Apr 29, 2025

View reviewed changes

WillAyd force-pushed the symbol-visibility branch 5 times, most recently from 4733642 to 2aaa9af Compare April 30, 2025 12:49

lidavidm modified the milestones: ADBC Libraries 18, ADBC Libraries 19 May 3, 2025

WillAyd force-pushed the symbol-visibility branch from 2aaa9af to b433498 Compare May 22, 2025 21:05

WillAyd added 2 commits May 23, 2025 13:19

feat(c): Use C++ visibility support in Meson configuration

0373903

Meson windows symbol export support

b64bd87

WillAyd force-pushed the symbol-visibility branch from b433498 to b64bd87 Compare May 23, 2025 17:21

feat(c): Use C++ visibility support in Meson configuration #2740

Are you sure you want to change the base?

feat(c): Use C++ visibility support in Meson configuration #2740

Uh oh!

Conversation

WillAyd commented Apr 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Apr 27, 2025

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

WillAyd commented Apr 27, 2025

Uh oh!

kou commented Apr 28, 2025

Uh oh!

WillAyd commented Apr 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou commented Apr 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd commented May 30, 2025

Uh oh!

Uh oh!

WillAyd Apr 30, 2025 •

edited

Loading