ugermann / ssplit-cpp

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

Static libraries is hard coded #18

Open kpu opened 3 years ago

kpu commented 3 years ago

I understand static libraries can be a useful option, but hard coding only static libraries is bad practice.

https://github.com/ugermann/ssplit-cpp/blob/a2fd40595c0303ae7edfb55c152ceb992b922967/CMakeLists.txt#L60

For example, this won't compile on Gentoo by default with the error

CMake Error at CMakeLists.txt:70 (message):
  Cannot find pcre2 libraries.  Terminating.

even though libpcre2 is installed, just the dynamic version.

While it currently blames on @XapaJIaMnu, the history shows this is simply an indentation of code that started with @ugermann

XapaJIaMnu commented 3 years ago

Ugh... So. Archlinux and Gentoo by default ship dynamic libs and if those are not found, static. You can change this behaviour in your ebuild.conf (or however it's called) and in makepkg.conf.

Most distros ship both, so I wouldn't blame @ugermann for not knowing arch and gentoo's behaviour, but you're right we should try to find .a and if not fallback to .so. We certainly want to package linux packages linked against .so

kpu commented 3 years ago

I'm aware that I can add USE="static-libs" for libpcre2 to get a static lib in gentoo. But I shouldn't have to.

ugermann commented 3 years ago

The cmake setup is now checking for both static and dynamic pcre2 libraries on *nix setups. Looking for volunteers to provide a gentoo / arch linux workflow.

kpu commented 3 years ago

And now you've broken OS X dynamic linking because that is .dylib. If you're going to do something sketchy, make it optional not the default.

XapaJIaMnu commented 3 years ago

brew normally installs both so it's not an issue, but we can just add the dylib extension?

kpu commented 3 years ago

https://stackoverflow.com/questions/3762057/cmake-how-to-produce-binaries-as-static-as-possible

IF(WIN32)
    SET(CMAKE_FIND_LIBRARY_SUFFIXES .lib .a ${CMAKE_FIND_LIBRARY_SUFFIXES})
ELSE(WIN32)
    SET(CMAKE_FIND_LIBRARY_SUFFIXES .a ${CMAKE_FIND_LIBRARY_SUFFIXES})
ENDIF(WIN32)

I would also add that non-standard behavior should be an option and only sparingly the default.

IF(PREFER_STATIC)
  IF(WIN32)
      SET(CMAKE_FIND_LIBRARY_SUFFIXES .lib .a ${CMAKE_FIND_LIBRARY_SUFFIXES})
  ELSE(WIN32)
      SET(CMAKE_FIND_LIBRARY_SUFFIXES .a ${CMAKE_FIND_LIBRARY_SUFFIXES})
  ENDIF(WIN32)
ENDIF(PREFER_STATIC)
XapaJIaMnu commented 3 years ago

I think we want prefer static 100% of the time for win32. It's preferable to have static build on mac just because we'd be distributing a. .app and not a brew package (or both). For linux it doesn't matter most of the time, I am not sure if wasm has any preferences.