Skip to content

Conversation

@lidavidm
Copy link
Member

@lidavidm lidavidm commented Apr 23, 2025

  • Correct a typo that was causing CMake to look for the shared library instead of the static library.
  • Fix CMake looking for the static library in the wrong place.
  • Test that linking against the static library works.
  • Test that linking with system-provided fmt/nanoarrow works.
  • Test that linking against multiple static drivers works.
  • Add headers to provide the entrypoint for static drivers.
  • Add build option to disable generation of shared Adbc* functions.
  • Respect this option from Go drivers too.
  • Namespace the functions inside adbc_driver_common (things like SetError appear to be leaking out)
  • Namespace the SQLite driver functions (e.g. StatementReaderUpcastInt64ToDouble)
  • Add static linking documentation.

Fixes #2562.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove *~ files?

@lidavidm
Copy link
Member Author

Thanks, I need to get my global .gitignore back on this new machine...

@lidavidm lidavidm force-pushed the gh-2562 branch 3 times, most recently from 00d8509 to 148dd20 Compare April 23, 2025 14:08
Copy link
Contributor

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. FWIW another way to test this would be to leverage the meson configuration to produce static libraries:

meson setup builddir -Ddefault_library=static ...

That would save you from having to mess around with any configuration files, although I'm guessing the CMake changes are for downstream CMake projects anyway

@lidavidm
Copy link
Member Author

Yup, the CMake fixes are to make it actually work with CMake 😅

This still isn't building FWIW - I need to get adbc_driver_common exported properly

@lidavidm lidavidm force-pushed the gh-2562 branch 4 times, most recently from 5c1f2ca to 854a8ee Compare April 24, 2025 03:59
@lidavidm lidavidm marked this pull request as ready for review April 24, 2025 08:25
@lidavidm lidavidm requested a review from zeroshade as a code owner April 24, 2025 08:25
@github-actions github-actions bot added this to the ADBC Libraries 18 milestone Apr 24, 2025
@lidavidm lidavidm force-pushed the gh-2562 branch 3 times, most recently from b4624e6 to 5941015 Compare April 24, 2025 09:46
# applications can link to it
if(ADBC_BUILD_STATIC)
if(ADBC_WITH_VENDORED_NANOARROW)
message(WARNING "adbc_driver_common is not installed when ADBC_WITH_VENDORED_NANOARROW is ON. To use the static libraries, for now you must provide nanoarrow instead of using the vendored copy"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the problem with installing in this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing that it would install/require the headers and ADBC should probably not be installing headers for nanoarrow/?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I tried to install Nanoarrow but CMake failed because of something about the header not being installable - decided to punt since to start with I don't think we need to support every possible case.

static size_t kErrorBufferSize = 1024;

int AdbcStatusCodeToErrno(AdbcStatusCode code) {
int PrivateAdbcStatusCodeToErrno(AdbcStatusCode code) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit but maybe this would be better to call Internal instead of Private just to keep in sync with the arrow repo? I know upstream there is even some confusion over the different namespace terminology apache/arrow#45808

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I can change it. I saw for Nanoarrow we used the 'Private' prefix which is why I went with that

@lidavidm
Copy link
Member Author

Ok, I think everything is working here now...more changes than I expected

@lidavidm
Copy link
Member Author

@WillAyd @kou any comments here?


#pragma once

#ifndef ADBC_DRIVER_BIGQUERY_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we already have #pragma once can we get rid of the manual include guard? Should help simplify the macro usage

Copy link
Contributor

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

#endif

ADBC_EXPORT
AdbcStatusCode BigQueryDriverInit(int version, void* raw_driver, struct AdbcError* error);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to add Adbc prefix for *DriverInit() functions:

Suggested change
AdbcStatusCode BigQueryDriverInit(int version, void* raw_driver, struct AdbcError* error);
AdbcStatusCode AdbcBigQueryDriverInit(int version, void* raw_driver, struct AdbcError* error);

Copy link
Member Author

@lidavidm lidavidm Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From adbc.h it seems this should be AdbcDriverBigqueryInit. I'll make sure these are consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated all of these. I've retained the old names for backwards compatibility.

@lidavidm lidavidm added this to the ADBC Libraries 19 milestone May 3, 2025
@lidavidm
Copy link
Member Author

lidavidm commented May 9, 2025

Any more comments here?

@lidavidm lidavidm merged commit 19ff899 into apache:main May 19, 2025
99 of 101 checks passed
@lidavidm lidavidm deleted the gh-2562 branch May 19, 2025 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drivers are unusable when statically linked with CMake

4 participants