Skip to content
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

Enable shared libraries. #50

Merged
merged 23 commits into from
Feb 4, 2024
Merged

Enable shared libraries. #50

merged 23 commits into from
Feb 4, 2024

Conversation

jchen351
Copy link
Contributor

@jchen351 jchen351 commented Feb 2, 2024

No description provided.

@RyanUnderhill
Copy link
Member

How are the python bindings built with this, they should be statically linked as they don't use the C API.

CMakeLists.txt Outdated
@@ -116,7 +116,7 @@ if(USE_TOKENIZER)
FetchContent_Declare(simdjson URL https://github.com/simdjson/simdjson/archive/refs/tags/v3.6.3.zip URL_HASH SHA1=2b063a2e81f74a5d1cb937fadf3d2fca0f1edb09)
FetchContent_MakeAvailable (simdjson)

add_library (tokenizer ${tokenizer_srcs})
add_library (tokenizer SHARED ${tokenizer_srcs})
Copy link
Member

Choose a reason for hiding this comment

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

The tokenizer library should be part of the onnxruntime-genai library, not a separate shared library.

CMakeLists.txt Outdated
@@ -161,7 +161,7 @@ target_link_directories(python PRIVATE ${CMAKE_SOURCE_DIR}/ort/lib)
target_link_libraries(python PRIVATE onnxruntime-genai ${ONNXRUNTIME_LIB})
set_target_properties(python PROPERTIES OUTPUT_NAME "onnxruntime_genai")
if (WIN32)
SET(WINDOWS_EXPORT_ALL_SYMBOLS ON)
SET(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use a def file? We don't want to export all of the symbols, just the C API

@jchen351 jchen351 merged commit 52dab33 into main Feb 4, 2024
10 checks passed
@natke natke deleted the Cjian/shared_lib branch February 27, 2024 00: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.

2 participants