Skip to content

Conversation

songkq
Copy link
Contributor

@songkq songkq commented May 22, 2023

Add ${TOKENIZERS_C_LINK_LIBS} and append ${CMAKE_DL_LIBS} for Linux platform (#1 (comment))

Add ${TOKENIZERS_C_LINK_LIBS} and append ${CMAKE_DL_LIBS} for Linux  platform (mlc-ai#1 (comment))

add_library(tokenizers_c INTERFACE ${TOKENIZERS_RUST_LIB})
target_link_libraries(tokenizers_c INTERFACE ${TOKENIZERS_RUST_LIB})
target_link_libraries(tokenizers_c INTERFACE ${TOKENIZERS_RUST_LIB} ${TOKENIZERS_C_LINK_LIBS})
Copy link
Contributor

Choose a reason for hiding this comment

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

actually we can rename TOKENIZERS_CPP_LINK_LIBS to TOKENIZERS_C_LINK_LIBS as they are all intended to fix the rust lib deps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, TOKENIZERS_CPP_LINK_LIBS can be replaced with TOKENIZERS_C_LINK_LIBS in this case. I think the TOKENIZERS_CPP_LINK_LIBS maybe also kept empty for extension of some other potential cpp extra libraries.

Copy link
Contributor

@tqchen tqchen May 22, 2023

Choose a reason for hiding this comment

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

What I meant is that we should move some of the TOKENIZERS_CPP_LINK_LIBS deps to TOKENIZERS_C_LINK_LIBS as they are related to C lib really

https://github.com/mlc-ai/tokenizers-cpp/blob/main/CMakeLists.txt#L20

and here
https://github.com/mlc-ai/tokenizers-cpp/blob/main/CMakeLists.txt#L37

@tqchen
Copy link
Contributor

tqchen commented May 22, 2023

Thanks @songkq , made some followup comments on moving some of the deps to TOKENIZERS_C_LINK_LIBS then we are good to go

move some of the TOKENIZERS_CPP_LINK_LIBS  deps to TOKENIZERS_C_LINK_LIBS.
@tqchen tqchen merged commit bd24b8a into mlc-ai:main May 22, 2023
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