search

kkm000 / openfst

Port of the OpenFST library to Windows
http://www.openfst.org/
Other
69 stars 38 forks source link

Cmake link with --whole-archive #12

Closed kkm000 closed 6 years ago

kkm000 commented 6 years ago

It seems to me that Cmake builds will experience the same static initialization fiasco with the libfst static library as MSBuild builds. Here is a Cmake-related discussion. IDK whether Cmake's magic will handle this on Windows though, so the change would need some testing.

Excerpt:

top_dit/sub_dir2/CMakeLists.txt
add_executable(exe1 main.c)
target_link_libraries(exe1 "-Wl,--whole-archive" L1 "-Wl,--no-whole-archive")
jtrmal commented 6 years ago

I'll have a look. Y.

On Tue, Jun 5, 2018, 04:56 Kirill Katsnelson notifications@github.com wrote:

It seems to me that Cmake builds will experience the same static initialization fiasco with the libfst static library as MSBuild builds https://github.com/kkm000/openfst/commit/a754372f8447db7c9fc56c7ee99671b481041197. Here is a Cmake-related discussion http://cmake.3232098.n2.nabble.com/How-to-hundle-gcc-link-options-like-whole-archive-allow-multiple-definition-in-CMake-td7593563.html. IDK whether Cmake's magic will handle this on Windows though, so the change would need some testing.

Excerpt:

top_dit/sub_dir2/CMakeLists.txt add_executable(exe1 main.c) target_link_libraries(exe1 "-Wl,--whole-archive" L1 "-Wl,--no-whole-archive")

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/12, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisXyFioaymp2luJZHPA_Ky1xIwQEpzks5t5kfYgaJpZM4Uaah- .

kkm000 commented 6 years ago

@jtrmal Yenda, I am closing this as fixed.

I just released 1.6.9.1. There is a little change in layout: test include files have been moved to include/tests. I do not think it would affect CMakefiles. Also, one trivial file is added to support building fstspecial from the same sources as fstconvert (I remember I once thought about adding same, as I was struggling to build it in my pretty rigid MSBuild setting, where the name of an executable is derived from source names). This may or may not affect cmake; if it was able to build them, I do not see why that should change, but you may want to check.

Here are the notable changes:

$ git diff orig/1.6.7.1..orig/1.6.9.1 --name-status | grep -v ^M
A       src/extensions/special/fstspecial.cc
R099    src/test/algo_test.h    src/include/fst/test/algo_test.h
R100    src/test/fst_test.h     src/include/fst/test/fst_test.h
R100    src/test/rand-fst.h     src/include/fst/test/rand-fst.h
R100    src/test/weight-tester.h        src/include/fst/test/weight-tester.h
$ cat src/extensions/special/fstspecial.cc
// [ .. snip comments .. ]
#include "fstconvert-main.cc"   // NOLINT
#include "fstconvert.cc"        // NOLINT

Also, I reverted one #ifndef _MSC_VER that you added in place of their original #if NO_DYNAMIC_LINKING in generic-register.h. My source changes define that symbol in include/fst/config.h, but the problem was the include order in that particular file was such that the config.h file defining the symbol was included later than the #ifdef checking it. They fixed the include order, so I saw no point in keeping the maintenance of the change. Feel free to restore that. I think a better solution, if that would still be a problem, is to define macro symbol NO_DYNAMIC_LINKING to 1 in CMakefiles, conditioned on Windows, but up to you.

jtrmal commented 6 years ago

cool! Makes sense. I will check it out. y.

On Sun, Jul 29, 2018 at 10:17 PM Kirill Katsnelson notifications@github.com wrote:

Closed #12 https://github.com/kkm000/openfst/issues/12.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kkm000/openfst/issues/12#event-1759167271, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisXy3eszNivf_ZE91ad7TDzBZdvtZ-ks5uLmyqgaJpZM4Uaah- .