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

[WillAyd]

No description provided.

[WillAyd]

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

[WillAyd]

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 (?)

[kou]

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.

[WillAyd]

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]

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]

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]

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]

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]

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

[lidavidm]

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

[WillAyd]

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

[kou]

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]

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

[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)

[WillAyd]

Ah thanks - makes sense

[WillAyd]

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

[WillAyd]

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

[kou]

Ah, we should use separated ADBC_XXX_STATIC like ADBC_XXX_EXPORT.

[WillAyd]

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!