Closed narekgharibyan closed 7 years ago
And looks I broke coveralls :/
Yes, coverage is an own mode or 'build type' as it is called in cmake.
I wonder if the cmake file already covers everything that scons had. Looks a bit to simple for me. ;-)
E.g. there are also some TPIE flags scons sets.
I will try your PR next week and give feedback. Lets ensure that we get everything right before the switch.
Nice, works for me.
@narekgharibyan anything missing except the doc changes? Any chance to move the CMakeLists.txt into the keyvi subfolder?
@hendrikmuhs Thanks!
remaining things are:
regarding to CMakeLists.txt location: I checked and still it's a bit tricky to import project into CLion, I prefer to keep it in the root, but that's not a mandatory.
@narekgharibyan
This debian packaging you mentioned is outdated, so just remove it, no problem. The real debian packaging stuff is in the keyvi-package repo. It needs adjustment:
https://github.com/cliqz-oss/keyvi-package/blob/master/debian/rules
... but this can be done afterwards IMO.
As for the CMakeLists: I do not understand it completely - still haven't tried CLion: You need it in root to have 1 project for both keyvi and pykeyvi? If the file is located under keyvi, you would not be able to see pykeyvi? Is that ritght?
Actually that's how I work using eclipse/pycharm. I have 2 projects there: a keyvi and a pykeyvi project. As both have there own build system I also see them as 2 projects.
Anyway, as it is just 1 file, I do not insist on putting it into the keyvi subfolder although I believe it belongs there.
@hendrikmuhs exactly!
@hendrikmuhs The compile section in file https://github.com/cliqz-oss/keyvi/blob/master/keyvi/README.md#compile looks to me a bit incorrect, as we are not compiling C++ library, rather we compile 3rd party lib and create standalone executables and unit tests executable. What do you think ?
@narekgharibyan Correct, in the very early days it was a library, but this isn't the case for a long time (not even sure if it ever was the case for 'keyvi' or the predecessor).
Anyway, should be rephrased, good catch.
LGTM!
(can you do a squash merge for less history polluting)
@hendrikmuhs this fixes #182
Just don't know how to be with
deb
packaging, also docs needs to be updated.