update cmake installation to support VCPKG port #156

remz1337 · 2020-07-19T20:03:46Z

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

Martinsos

@remz1337 Hey, thanks for working on this! It is really cool that you are adding/porting Edlib to VCPKG.

So what you did is, you first made installation step optional (although it is still ON by default) and then you enriched it so that additional stuff is created -> version, config and target files. Is that correct?

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.

@SoapZA , sorry to bother you, but would you mind also giving a look here? You are the last person to have been making significant changes to Edlib's CMakeLists.txt and you certainly know much more about it than me, so I would love to hear your opinion on this PR!

My knowledge of CMAKE is pretty basic, and

remz1337 · 2020-07-20T21:12:34Z

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 · 2020-07-21T16:08:01Z

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 · 2020-07-21T16:31:53Z

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

SoapZA · 2020-07-21T16:32:26Z

CMakeLists.txt

+	install(FILES edlib/include/edlib.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
+
+endif()
+### //


you're missing the final trailing line.

SoapZA · 2020-07-21T16:33:38Z

CMakeLists.txt

+### Additional config for VCPKG
+if(EDLIB_ENABLE_INSTALL)
+	include(CMakePackageConfigHelpers)
+	set(EDLIB_CMAKE_DIR "lib/cmake/edlib" CACHE STRING


this is wrong and should be ${CMAKE_INSTALL_LIBDIR}/cmake/edlib

SoapZA · 2020-07-21T16:34:22Z

edlib-config.cmake.in

+@PACKAGE_INIT@
+
+include(${CMAKE_CURRENT_LIST_DIR}/@targets_export_name@.cmake)
+check_required_components(edlib)


same here, please add the trailing missing line

remz1337 · 2020-07-21T20:36:44Z

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 · 2020-07-22T09:57:05Z

@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 · 2020-07-22T10:02:25Z

CMakeLists.txt

-# Create target 'install' for installing libraries.
-install(TARGETS edlib DESTINATION ${CMAKE_INSTALL_LIBDIR})
-install(FILES edlib/include/edlib.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
-
 # configure and install pkg-config file
 configure_file(
    ${CMAKE_CURRENT_SOURCE_DIR}/edlib.pc.in
    ${CMAKE_CURRENT_BINARY_DIR}/edlib-1.pc
    @ONLY)
 install(FILES ${CMAKE_CURRENT_BINARY_DIR}/edlib-1.pc DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig)


don't want to put this in your EDLIB_ENABLE_INSTALL install clause too?

SoapZA · 2020-07-22T10:03:30Z

@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 · 2020-07-22T11:07:58Z

@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 · 2020-07-22T11:09:21Z

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

Martinsos · 2020-07-25T10:46:21Z

@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 · 2020-07-27T19:51:02Z

@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

@remz1337 Approved! Please just add the comment that I mentioned and let me know, I will merge it then.

CMakeLists.txt

…for example vcpkg. Also updated the library version and made the installation optional

Martinsos · 2020-07-31T16:36:06Z

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

SpaceIm · 2020-10-18T13:14:22Z

@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.

Martinsos reviewed Jul 20, 2020

View reviewed changes

SoapZA suggested changes Jul 21, 2020

View reviewed changes

SoapZA reviewed Jul 22, 2020

View reviewed changes

remz1337 closed this Jul 22, 2020

remz1337 reopened this Jul 27, 2020

Martinsos approved these changes Jul 30, 2020

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

These are cmake config files and they are useful for some consumers, …

c19d572

…for example vcpkg. Also updated the library version and made the installation optional

Martinsos merged commit ba784e5 into Martinsos:master Jul 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update cmake installation to support VCPKG port #156

update cmake installation to support VCPKG port #156

remz1337 commented Jul 19, 2020 •

edited

Loading

Martinsos left a comment

remz1337 commented Jul 20, 2020

Martinsos commented Jul 21, 2020

SoapZA commented Jul 21, 2020 •

edited

Loading

SoapZA Jul 21, 2020

SoapZA Jul 21, 2020

SoapZA Jul 21, 2020

remz1337 commented Jul 21, 2020

Martinsos commented Jul 22, 2020

SoapZA Jul 22, 2020

SoapZA commented Jul 22, 2020

Martinsos commented Jul 22, 2020

SoapZA commented Jul 22, 2020

Martinsos commented Jul 25, 2020

remz1337 commented Jul 27, 2020 •

edited

Loading

Martinsos left a comment

Martinsos commented Jul 31, 2020

SpaceIm commented Oct 18, 2020

update cmake installation to support VCPKG port #156

update cmake installation to support VCPKG port #156

Conversation

remz1337 commented Jul 19, 2020 • edited Loading

Martinsos left a comment

Choose a reason for hiding this comment

remz1337 commented Jul 20, 2020

Martinsos commented Jul 21, 2020

SoapZA commented Jul 21, 2020 • edited Loading

SoapZA Jul 21, 2020

Choose a reason for hiding this comment

SoapZA Jul 21, 2020

Choose a reason for hiding this comment

SoapZA Jul 21, 2020

Choose a reason for hiding this comment

remz1337 commented Jul 21, 2020

Martinsos commented Jul 22, 2020

SoapZA Jul 22, 2020

Choose a reason for hiding this comment

SoapZA commented Jul 22, 2020

Martinsos commented Jul 22, 2020

SoapZA commented Jul 22, 2020

Martinsos commented Jul 25, 2020

remz1337 commented Jul 27, 2020 • edited Loading

Martinsos left a comment

Choose a reason for hiding this comment

Martinsos commented Jul 31, 2020

SpaceIm commented Oct 18, 2020

remz1337 commented Jul 19, 2020 •

edited

Loading

SoapZA commented Jul 21, 2020 •

edited

Loading

remz1337 commented Jul 27, 2020 •

edited

Loading