Martinsos / edlib

Lightweight, super fast C/C++ (& Python) library for sequence alignment using edit (Levenshtein) distance.
http://martinsos.github.io/edlib
MIT License
493 stars 162 forks source link

Meson ci #165

Closed Martinsos closed 3 years ago

Martinsos commented 3 years ago

@cdunn2001 and @SoapZA , it would be great if you could lend me a little bit of help with getting this PR integrated! I pushed as far as I could on my own and would love to get your comments on what could be done better + how to fix some things.

What is not working at the moment:

  1. I can't get meson working on appveyor, windows. I followed their suggestion from https://mesonbuild.com/Continuous-Integration.html#appveyor-for-windows, but I am getting error

    Found ninja-1.10.0.git.kitware.jobserver-1 at C:\python37-x64\Scripts\ninja.EXE
    meson compile -C build_dir
    ninja: Entering directory `build_dir'
    [1/8] Compiling C object hello-world.exe.p/apps_hello-world_helloWorld.c.obj
    [2/8] Compiling C++ object runTests.exe.p/test_runTests.cpp.obj
    [3/8] Compiling C++ object edlib.dll.p/edlib_src_edlib.cpp.obj
    [4/8] Linking static target libedlib.a
    [5/8] Linking target edlib.dll
    [6/8] Generating symbol file edlib.dll.p/edlib.dll.symbols
    [7/8] Linking target hello-world.exe
    FAILED: hello-world.exe 
    "link"  /MACHINE:x64 /OUT:hello-world.exe hello-world.exe.p/apps_hello-world_helloWorld.c.obj "/nologo" "/release" "/nologo" "/OPT:REF" "edlib.lib" "/SUBSYSTEM:CONSOLE" "kernel32.lib" "user32.lib" "gdi32.lib" "winspool.lib" "shell32.lib" "ole32.lib" "oleaut32.lib" "uuid.lib" "comdlg32.lib" "advapi32.lib"
    LINK : fatal error LNK1181: cannot open input file 'edlib.lib'
    [8/8] Linking target runTests.exe
    FAILED: runTests.exe 

    I found this QA in their FAQ: https://mesonbuild.com/FAQ.html . Solution seems to be adding certain statements in the C/C++ code, but I don't feel like I will be able to do this properly, with understanding, unless I invest some decent time into understanding the whole context of it, which I can't do at the moment due to other obligations. Btw. notice that I added option in meson.build to build both static and shared library -> I am not sure how that influences the whole thing. Also, I wonder how is it that this worked out of the box in Cmake but does not in Meson. I read somewhere it is because CMake uses some kind of opinionated solution for this and Meson authors did not want to make such opinionated approach. Anyway, if you could help me figure this out, that would be tremendous help!

  2. Since I installed python3 for meson, I can't get edlib python binding to compile -> there seems to be some mismatch between python2 and python3, as if a part of binding is using python3 but expecting python2. I can solve this in some later PR, this is not urgent, but if you have some immediate idea on what is the problem, I am all ears. Why haven't I personally solved it yet? I don't really use python much normally, so my knowledge of the whole versioning and packaging is lacking. Don't bother with this if it is not your thing, I just thought I would mention it, in case you have some ideas. The thing with Appveyor is more pressing. -> This actually just got solved with another PR I just merged, that introduced building of python wheels and since it split travis build into stages, it also took care of conflicting python versions.

cdunn2001 commented 3 years ago

I try not to deal with MSVC myself. Some options:

  1. You can use something like this:

    // Export macros for DLL visibility
    #if defined(JSON_DLL_BUILD)
    # if defined(_MSC_VER) || defined(__MINGW32__)
    #  define EDLIB_API __declspec(dllexport)
    # else
    #  define EDLIB_API
    # endif // if defined(_MSC_VER)
    #endif

    Then prepend EDLIB_API to class and function declarations that should be exposed in the DLL. Pretty annoying.

  2. Another option is /OPT:NOREF, to disable the removal of unused symbols in general. You have to modify meson.build for this.

  3. A third option is the /INCLUDE directive to force the MSVC linker to include a symbol. A common approach is to create a single unused function that references all objects exported for your plugins. Then you only need a single /INCLUDE directive in meson.build for that function.

(2) is not terrible. There must be a way to add that via Meson. But instead, I would try:

  1. Similar to (3), add a function that uses your entire public API. Declare only that function with __declspec(dllexport). But don't bother with the EDLIB_API macro or with /INCLUDE. Instead, wrap that entire function in #if defined _MSC_VER || defined(__MINGW32__). Then all the annoyance is in one place, and Meson doesn't even need to know.
cdunn2001 commented 3 years ago

You have 4 builds, 2 builds for each CI system. I suggest going into travis-ci.org, find the settings for this project, and disable "build on push". You really only need "build for pull-request". I think Appveyor has a similar settings.

cdunn2001 commented 3 years ago

https://mesonbuild.com/Continuous-Integration.html#appveyor-for-windows

Martinsos commented 3 years ago

You have 4 builds, 2 builds for each CI system. I suggest going into travis-ci.org, find the settings for this project, and disable "build on push". You really only need "build for pull-request". I think Appveyor has a similar settings.

Woah you are right, I fixed it! I added

branches:
  only:
    - master

to both travis and appveyor, this will build only for PRs and for pushes to master.

Martinsos commented 3 years ago

https://mesonbuild.com/Continuous-Integration.html#appveyor-for-windows

I used this as a template for writing my appveyor config, you will see that they are extremely similar. But it does not mention anything about this error or does anything to take care of it.

Martinsos commented 3 years ago

I try not to deal with MSVC myself. Some options:

1. You can use something like this:
// Export macros for DLL visibility
#if defined(JSON_DLL_BUILD)
# if defined(_MSC_VER) || defined(__MINGW32__)
#  define EDLIB_API __declspec(dllexport)
# else
#  define EDLIB_API
# endif // if defined(_MSC_VER)
#endif

Then prepend EDLIB_API to class and function declarations that should be exposed in the DLL. Pretty annoying.

1. Another option is `/OPT:NOREF`, to disable the removal of unused symbols in general. You have to modify `meson.build` for this.

2. A third option is the `/INCLUDE` directive to force the MSVC linker to include a symbol. A common approach is to create a single unused function that references all objects exported for your plugins. Then you only need a single /INCLUDE directive in `meson.build` for that function.

(2) is not terrible. There must be a way to add that via Meson. But instead, I would try:

1. Similar to (3), add a function that uses your entire public API. Declare _only_ that function with `__declspec(dllexport)`. But don't bother with the `EDLIB_API` macro or with `/INCLUDE`. Instead, wrap that entire function in `#if defined _MSC_VER || defined(__MINGW32__)`. Then all the annoyance is in one place, and Meson doesn't even need to know.

Thanks a lot, this gives me a lot to work by! I will then try approaches (2) and (4) -> probably start with (2) because it sounds like it might be quick to get working + there are no changes in the code. (4) sounds reasonable, although it sounds hackish to add a function that is not really needed, only for smth like this, right?

Martinsos commented 3 years ago

I try not to deal with MSVC myself. Some options:

1. You can use something like this:
// Export macros for DLL visibility
#if defined(JSON_DLL_BUILD)
# if defined(_MSC_VER) || defined(__MINGW32__)
#  define EDLIB_API __declspec(dllexport)
# else
#  define EDLIB_API
# endif // if defined(_MSC_VER)
#endif

Then prepend EDLIB_API to class and function declarations that should be exposed in the DLL. Pretty annoying.

1. Another option is `/OPT:NOREF`, to disable the removal of unused symbols in general. You have to modify `meson.build` for this.

2. A third option is the `/INCLUDE` directive to force the MSVC linker to include a symbol. A common approach is to create a single unused function that references all objects exported for your plugins. Then you only need a single /INCLUDE directive in `meson.build` for that function.

(2) is not terrible. There must be a way to add that via Meson. But instead, I would try:

1. Similar to (3), add a function that uses your entire public API. Declare _only_ that function with `__declspec(dllexport)`. But don't bother with the `EDLIB_API` macro or with `/INCLUDE`. Instead, wrap that entire function in `#if defined _MSC_VER || defined(__MINGW32__)`. Then all the annoyance is in one place, and Meson doesn't even need to know.

@cdunn2001 , thanks again for advice, I got it working after too many hours of trying!

I first went with /OPT:NOREF, and I managed to set it as an option for msvc linker. However, that did not help, and from further reading I did, I read that /OPT:NOREF is not enough, and that it actually does not help:

  • When using /OPT:REF linker removes unreferenced code and data from object files it decides to add to the executable. That is the optimization, and you can turn it off by using /OPT:NOREF
  • When searching libraries, linker picks up only object files that contain explicitly referenced code or data -- so it will not pick up some variable with global constructor if there is no reference from your code (or from other object file, or from object file included in some library if linker decides to include that object file). That is NOT the optimization, linker always works this way, and that behavior is NOT controlled by /OPT:NOREF.

from https://social.msdn.microsoft.com/Forums/vstudio/en-US/2aa2e1b7-6677-4986-99cc-62f463c94ef3/linkexe-bug-optnoref-option-doesnt-work?forum=vclanguage .

I then considered adding function that uses the whole API, but found that somewhat dirty/hacky -> I would have to make up arbitrary logic/code in that function to call/use everything, and I could easily forget smth, and somebody could forget to add it -> it just didn't feel great. So I went with option (1) -> that seems to be the currently recommended approach online + it is, although verbose, very explicit. I had to slightly modify the macro you wrote, to add empty EDLIB_API when not building DLL, but otherwise it was all good, and it works now, Meson on Windows :)!