-
Notifications
You must be signed in to change notification settings - Fork 38
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
[WIP] Fix symbol visibility for user extensions #386
base: main
Are you sure you want to change the base?
Changes from 8 commits
1c617e9
49a7bfa
73e106f
5414d97
571b9d5
512a6fd
a284577
eb659d8
9168324
4d826a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
include(GenerateExportHeader) | ||
|
||
set(CORE_PUBLIC_HEADERS | ||
BasicAllocator.h | ||
Config.h.in | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did you intentionally change from static to shared here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
add_library(csp_core SHARED ${CORE_SOURCE_FILES}) | ||
generate_export_header(csp_core | ||
BASE_NAME cspcore | ||
EXPORT_MACRO_NAME CSPCORE_EXPORT | ||
) | ||
set_target_properties(csp_core PROPERTIES PUBLIC_HEADER "${CORE_PUBLIC_HEADERS}") | ||
|
||
install(TARGETS csp_core | ||
|
@@ -34,3 +40,4 @@ install(TARGETS csp_core | |
LIBRARY DESTINATION lib/ | ||
) | ||
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/cpp/csp/core/Config.h DESTINATION include/csp/core) | ||
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/cspcore_export.h DESTINATION include/csp/core) |
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.
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