mac: fix explicit symbol exports #2389
Conversation
|
Well, my solution seems to break windows. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2389 +/- ##
=======================================
Coverage 91.04% 91.04%
=======================================
Files 1015 1015
Lines 36014 36012 -2
=======================================
- Hits 32789 32788 -1
+ Misses 3225 3224 -1
☔ View full report in Codecov by Sentry. |
|
After some thought, the best solution is probably to link the python library against |
src/include/common/api.h
Outdated
| @@ -13,9 +13,6 @@ | |||
| #define KUZU_HELPER_DEPRECATED __attribute__((__deprecated__)) | |||
| #endif | |||
|
|
|||
| #ifdef KUZU_STATIC_DEFINE | |||
| #define KUZU_API | |||
There was a problem hiding this comment.
Removing this is, I believe, what's causing the windows build failure. The embedded shell is linking against the static library, but kuzu_shared_EXPORTS is not defined when building the shell, so it's using the import symbols but can't find those symbols in the static library.
And without disabling it for the static library, I would think that gets built with export symbols, which doesn't really seem right.
Otherwise this PR looks good to me.
There was a problem hiding this comment.
Yeah that makes sense. I'm primarily wondering how best to avoid the Mac warning and the Windows failure.
|
Actually I'll just ship this without fixing the warning for now and defer this to post release. |
Riolku
force-pushed
the
apple-symbol-fixes
branch
from
f6c3f78
to
1141310
Compare
Exception information necessary for properly catching exceptions, seems to not be exported on Mac unless we export the exception, which seems to be different from Linux & Windows. In additional, Mac issues a warning when building the Python module (which itself builds as a shared library). The issue is that the python module links with our static library, and the static library does not export symbols (since it doesn't do anything for static libraries). However, this mismatch in visibility causes a warning. Technically, we shouldn't build a shared library with a static library since the static library isn't position-independent code, but we don't really have other options unless we embed all of the object files? We should probably link the python library against `libkuzu.so` and ship it alongside the python bindings. However, this will be delayed until after the release. Ref #2136.
Riolku
force-pushed
the
apple-symbol-fixes
branch
from
1141310
to
2b3ff24
Compare
Exception information necessary for properly catching exceptions, seems to not be exported on Mac unless we export the exception, which seems to be different from Linux & Windows.
In additional, Mac issues a warning when building the Python module (which itself builds as a shared library). The issue is that the python module links with our static library, and the static library does not export symbols (since it doesn't do anything for static libraries).
However, this mismatch in visibility causes a warning. Technically, we shouldn't build a shared library with a static library since the static library isn't position-independent code, but we don't really have other options unless we embed all of the object files?
My current solution is to keep the visibility annotations in the static library, which seems to work fine and avoids the warning. However, we should probably find some other solution. Embedding all of the object files isn't super appealing from a design perspective but seems the easiest. It'd be great to embed the shared library.. but that would require some changes to how we package for python, and I haven't looked into it.
Ref #2136.