search

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

update cmake installation to support VCPKG port #156

Closed remz1337 closed 3 years ago

remz1337 commented 3 years ago

Hello, I am porting this library to VCPKG but I had to make some changes to the build process. (Here is the link to the VCPKG PR)

For context, with this patch I can install edlib through vcpkg simply by doing vcpkg install edlib

and then I add these 2 lines in my project's CMakeList: find_package(edlib CONFIG REQUIRED) target_link_libraries(main PRIVATE edlib::edlib)

and voila! I can use your library in my project :)

Thanks

remz1337 commented 3 years ago

Yes, that's about it! I'm still new to vcpkg too. I followed the basic setup tutorial on vcpkg to port a new lib, but when I tried with just the default config, I had a bunch of error. So I went around and looked at other existing ports to see what wasn't working came up with this solution.

I'm sure someone with better knowledge of CMake would be able to improve it, but at least it is working for me right now

Thanks for this great library!

Martinsos commented 3 years ago

Yes, that's about it! I'm still new to vcpkg too. I followed the basic setup tutorial on vcpkg to port a new lib, but when I tried with just the default config, I had a bunch of error. So I went around and looked at other existing ports to see what wasn't working came up with this solution.

I'm sure someone with better knowledge of CMake would be able to improve it, but at least it is working for me right now

Thanks for this great library!

@remz1337 ok got it, and thanks again for the effort! The main thing I would like before we merge this PR though, is having a better understanding of the part that you added, and if it is useful for other stuff, not just vcpkg, and also how exactly it fits with the rest of the CMakeLists. Is this something you could shed some light on, or are there some resources you could point me out to? That is also why I invited @SoapZA to possibly give it a look, but I am not sure if he will have time so I would rather we push on with this in the meantime in any case.

SoapZA commented 3 years ago

@Martinsos haven't forgotten about you, I'll take a look now

remz1337 commented 3 years ago

Thanks for the feedback @SoapZA , I just pushed the changes. i've also updated the package version to reflect the latest release (1.2.5 -> 1.2.6)

@Martinsos : I don't fully understand how it works with vcpkg, I've just used the same setup that I saw with other working ports. I've read through the docs of vcpkg but didn't find much explanation. You can have a look here

Martinsos commented 3 years ago

@remz1337 thanks for sharing the link, I will take a look when I get some time! @SoapZA wohoo thanks for helping :)! I am kind of dependent here on you until I ramp up my CMAKE knowledge which is currently not very high on my TODO list unfortunately.

My concern here remains the same - I don't like committing code that at least somebody does not understand on level deeper than "it works". @remz1337 I understand that you don't know the details which is completely fine, but in that case I will have to postpone merging the PR a little bit until I find some time to do the research myself. Unless @SoapZA you have some insights / understanding of changes made?

SoapZA commented 3 years ago

@Martinsos all of the changes are fine. CMake (unfortunately) has promoted a number of anti-patterns, and one of them is their "cmake config files" pattern. The idea being, CMake has its own pseudo-metadata format, akin to pkg-config, but one that only CMake can use (and Meson to some extent). And that's the main reason I don't like them: they only help CMake, and not Autotools, Meson, Makefiles or what have you. That's just my personal take on this idiom.

That said, nothing here breaks pkg-config, edlib or anything for users, hence this PR seems fine to me.

Martinsos commented 3 years ago

@Martinsos all of the changes are fine. CMake (unfortunately) has promoted a number of anti-patterns, and one of them is their "cmake config files" pattern. The idea being, CMake has its own pseudo-metadata format, akin to pkg-config, but one that only CMake can use (and Meson to some extent). And that's the main reason I don't like them: they only help CMake, and not Autotools, Meson, Makefiles or what have you. That's just my personal take on this idiom.

That said, nothing here breaks pkg-config, edlib or anything for users, hence this PR seems fine to me.

Awesome, thanks so much for this explanation! So vcpkg is basically using this CMake pseudo-metadata format, right? Sounds good then, I will let you @remz1337 take care of this last comment from @SoapZA and then we can merge! Also, maybe just add the comment saying these are cmake config files and they are useful for some consumers, for example vcpkg.

SoapZA commented 3 years ago

Yes, and please squash all your commits into one, this is one logical change after all.

Martinsos commented 3 years ago

@remz1337 Hi, what happened here, I see that you closed the issue and there are no commits? Was that a mistake, I hope all is ok? I thought we were very close to integrating it!

remz1337 commented 3 years ago

@Martinsos I had some trouble squashing the commits but not sure why it's closed since my fork is 1 commit ahead. I don't know why this PR isn't seeing my commit.

Edit: weird, looks like commenting on the PR allowed me to reopen it. Should be good now

Martinsos commented 3 years ago

Cool I merged it, thanks @remz1337 and @SoapZA :)!

SpaceIm commented 3 years ago

@remz1337 What I am wondering is -> are these files that are now created somehow specific to VCPKG or are they more general thing? I am getting a feeling that they are not tied just to VCPKG, although it does use them, is that correct? It would be great if you could tell me more about this! Including what is this edlib-config.cmake.in about.

CMake config file is not specific to vcpkg. They are highly recommended to allow consumers to find and consume the lib in CMake projects with find_package. It's good practice to always generate a CMake config file.