search

ugermann / ssplit-cpp

Approximate reimplementation of the sentence splitter from the Moses toolkit.
Other
4 stars 9 forks source link

Pack dependencies into library when using internal abseil and pcre2 #11

Closed jerinphilip closed 3 years ago

jerinphilip commented 3 years ago

When there's provision from within the library for both abseil and pcre2 (USE_ABSEIL with ProvideAbseil and USE_INTERNAL_PCRE2), aren't the libraries expected to be linked to the static library object being created? The current build code-path doesn't have this?

https://github.com/ugermann/ssplit-cpp/blob/53877846639113d3cbc0eb7dac373453dd5cd0f3/src/CMakeLists.txt#L1-L12

Specifically, the following which exists in browsermt/ssplit-cpp (in some capacity for PCRE2, not abseil) doesn't seem to exist here.

add_library(ssplit STATIC ssplit/ssplit.cpp ssplit/regex.cpp)
target_link_libraries(ssplit ${EXT_LIBS}  ${ABSEIL_LIBS})
ugermann commented 3 years ago

My idea was that the abseillib libraries would be added to the link target for executables.

jerinphilip commented 3 years ago

As a user of the library, I want to be able to link libssplit and not bother about the remaining. It's not an interface you supply through libssplit.a, it's a library with implementation. libssplit.a is hell without the target_link_libraries for usage downstream, where I have to find every single dependency and add them manually.

ugermann commented 3 years ago

I guess it's a bit of a philosophical matter. Abeil users have complained about the same thing to the abseil authors, only to be told that "this would encourage bad practise". (https://github.com/abseil/abseil-cpp/issues/367). I personally have no strong opinion about this. What happens if a reference is defined in several libraries, though (e.g. libsentencepiece and libssplit)? If you then create a third static library by linking both of those, won't you end up with multiple definitions in the same library?

jerinphilip commented 3 years ago

What happens if a reference is defined in several libraries?

I don't know, my CPP/build system expertise is limited. sentencepiece seems to provide packed and doesn't cause much headache when I try to integrate it into marian. Bergamot assumptions and me personally would prefer absl be bundled.

What's the cost though, bigger library archive?

ugermann commented 3 years ago

Less tractability. Assume you have two static libraries that both include an implementations from a third library, but different versions of it. Then something crashes within one of those functions. How are you able to track that down? If I make sure that every implementation exists exactly once, I know where to look / update.

ugermann commented 3 years ago

The dependency on abseil has been removed. PCRE2 should be staticly linked if a static pcre2 library is available; dynamically otherwise.