Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drivers are unusable when statically linked with CMake #2562

Open
jtanx opened this issue Feb 26, 2025 · 8 comments
Open

Drivers are unusable when statically linked with CMake #2562

jtanx opened this issue Feb 26, 2025 · 8 comments
Labels
Type: bug Something isn't working

Comments

@jtanx
Copy link

jtanx commented Feb 26, 2025

What happened?

Taking the postgres driver as an example:

  • The CMake package adbc_driver_postgresql links to adbc_driver_common and a few other vendored dependencies like nanoarrow. Only adbc_driver_postgresql is installed; the other libraries are missing. This leads to a linking error since the symbols provided in the other libraries are missing.
  • For reasons I do not understand, the driver unnecessarily redefines the entirety of the ADBC driver manager functions (e.g. AdbcDatabaseGetOption). This leads to a duplicate symbol error when you try to link both the ADBC driver manager library and the postgresql driver together.
  • Furthermore, PostgresqlDriverInit is not declared in any installed header (the driver does not install any headers?), which complicates using it with AdbcDriverManagerDatabaseSetInitFunc. This is less egregious as it's not that hard to just declare the function signature manually, but it is still odd that it's not provided.
  • The way vendored dependencies are handled is also problematic if linking statically - no attempt is made to first search for these packages before using the vendored dependencies. This can lead to conflicts if I am also using e.g. the fmt library myself from a third party source.

Stack Trace

No response

How can we reproduce the bug?

  • Use CMake
  • Build statically
  • Install the static libs
  • Try to use the libraries statically from another project

Environment/Setup

C driver version 1.4.0

@jtanx jtanx added the Type: bug Something isn't working label Feb 26, 2025
@lidavidm
Copy link
Member

There's an existing PR to unvendor dependencies: #2546

@lidavidm
Copy link
Member

The redefinition is intentional, but maybe we should provide an option to omit it. We could also optionally install headers (frankly, the expected method of usage is via the driver manager so this is all untrodden ground)

@jtanx
Copy link
Author

jtanx commented Feb 26, 2025

If the expected method of usage is via the driver manager, why is anything redefined at all? Shouldn't only e.g. PostgresqlDriverInit be exposed from a driver?

@paleolimbot
Copy link
Member

Shouldn't only e.g. PostgresqlDriverInit be exposed from a driver?

I have also always wondered this 🙂

@jtanx
Copy link
Author

jtanx commented Feb 26, 2025

Woops, I made a mistake in my original post; to be clear adbc_driver_common is not installed, so it's not just a problem with vendored dependencies.

We could also optionally install headers (frankly, the expected method of usage is via the driver manager so this is all untrodden ground)

Rereading this, I slightly misunderstood. I'm not saying to install the existing headers with everything exposed, I'm saying that there should be a header that exposes PostgresqlDriverInit and nothing else - indeed I don't see why any other symbol from a driver should be exported at all (and I also still think that redefining the functions don't make sense at all).

@lidavidm
Copy link
Member

You can link the postgres driver and use the Adbc* functions.

@jtanx
Copy link
Author

jtanx commented Feb 27, 2025

As in linking just the postgres driver and not the driver manager? Seems like an over optimisation?

@paleolimbot
Copy link
Member

You loose a bit of documentation but it's pretty easy to use a macro to just call the driver callbacks:

https://github.com/OSGeo/gdal/blob/c7006dbfb1c8a3f9030f46642d17bb28b34b0144/ogr/ogrsf_frmts/adbc/ogradbclayer.cpp#L24

At the very least, an opt-out of getting any symbols other than the init function seems helpful (I also happen to think turning them off by default might lead to less accidental problems as people build more ADBC drivers into things).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants