[WIP] Fix symbol visibility for user extensions #386
Conversation
… struct cpp example Signed-off-by: Tim Paine <[email protected]>
…cess them Signed-off-by: Adam Glustein <[email protected]>
…bol-visibility-for-user-extensions
Signed-off-by: Adam Glustein <[email protected]>
…-visibility-for-user-extensions
… well Signed-off-by: Adam Glustein <[email protected]>
AdamGlustein
requested review from
robambalu,
svatasoiu,
alexddobkin,
ptomecek and
timkpaine
as code owners
|
Attempting new approach with https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html |
| @@ -29,6 +35,10 @@ if(CSP_BUILD_PARQUET_ADAPTER) | |||
| ${VENDORED_PYARROW_ROOT}/arrow/python/csv.cc | |||
| ${VENDORED_PYARROW_ROOT}/arrow/python/filesystem.cc) | |||
| add_library(parquetadapterimpl SHARED parquetadapterimpl.cpp ${ARROW_PYTHON_SRCS}) | |||
| generate_export_header(parquetadapterimpl | |||
There was a problem hiding this comment.
you can use PREFIX_NAME CSP here instead of BASE_NAME + EXPORT_MACRO_NAME
Signed-off-by: Adam Glustein <[email protected]>
AdamGlustein
force-pushed
the
fix-symbol-visibility-for-user-extensions
branch
from
6862748 to
a284577
Compare
| @@ -25,7 +27,11 @@ set(CORE_SOURCE_FILES | |||
|
|
|||
| configure_file(Config.h.in ${CMAKE_CURRENT_BINARY_DIR}/cpp/csp/core/Config.h) | |||
|
|
|||
| add_library(csp_core STATIC ${CORE_SOURCE_FILES}) | |||
There was a problem hiding this comment.
did you intentionally change from static to shared here?
There was a problem hiding this comment.
yeah, the issue was the test binaries link against csp_core directly. as-is, the static build of csp_core is built without cspimpl export macro, so the classes are marked as dllimport which results in failed imports.
Adam and I discussed a few solutions, one of which is just making csp_core shared and creating its own export macro. another is to continue to build csp_core statically (with its own export macro, cmake's export macro for static libs is a no-op, or none at all), and have users link to that directly instead of cspimpl to get at classes like DateTime. basically need to have exactly one dllexport of each class in any .dll in one way or another if it is ever marked dllimport
There was a problem hiding this comment.
Yeah, as Sorin explained this is so csp_core / csp_engine can individually export their symbols for tests (or any C++ binary that links them in without cspimpl).
I had tried to export most of these symbols in cspimpl but that approach did not work for the above reason.
Signed-off-by: Adam Glustein <[email protected]>
cpp/csp/core/BasicAllocator.h
Outdated
| @@ -15,7 +15,7 @@ namespace csp | |||
| { | |||
|
|
|||
| // Extremely basic non-thread safe fixed-size allocator | |||
| class BasicAllocator | |||
| class CSPCORE_EXPORT BasicAllocator | |||
There was a problem hiding this comment.
seems like most of these classes under core/ are header-only and shouldn't be exported since on windows this means anyone linking against these would link as dllimport; but there's no concrete dllexport for the header-only classes in csp_core.dll
Signed-off-by: Adam Glustein <[email protected]>
AdamGlustein
force-pushed
the
fix-symbol-visibility-for-user-extensions
branch
4 times, most recently
from
7e1f337 to
7649004
Compare
Signed-off-by: Adam Glustein <[email protected]>
AdamGlustein
force-pushed
the
fix-symbol-visibility-for-user-extensions
branch
from
7649004 to
4d826a6
Compare
timkpaine
added
type: bug
No description provided.