-
Notifications
You must be signed in to change notification settings - Fork 173
fix(c): enable linking to static builds #2738
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
Conversation
kou
left a comment
There was a problem hiding this 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?
|
Thanks, I need to get my global .gitignore back on this new machine... |
00d8509 to
148dd20
Compare
There was a problem hiding this 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
|
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 |
5c1f2ca to
854a8ee
Compare
b4624e6 to
5941015
Compare
| # 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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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/?
There was a problem hiding this comment.
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.
c/driver/common/utils.c
Outdated
| static size_t kErrorBufferSize = 1024; | ||
|
|
||
| int AdbcStatusCodeToErrno(AdbcStatusCode code) { | ||
| int PrivateAdbcStatusCodeToErrno(AdbcStatusCode code) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
Ok, I think everything is working here now...more changes than I expected |
|
|
||
| #pragma once | ||
|
|
||
| #ifndef ADBC_DRIVER_BIGQUERY_H |
There was a problem hiding this comment.
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
WillAyd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
kou
left a comment
There was a problem hiding this 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); |
There was a problem hiding this comment.
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:
| AdbcStatusCode BigQueryDriverInit(int version, void* raw_driver, struct AdbcError* error); | |
| AdbcStatusCode AdbcBigQueryDriverInit(int version, void* raw_driver, struct AdbcError* error); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Any more comments here? |
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Fixes #2562.