ros2 / orocos_kdl_vendor

Apache License 2.0
2 stars 6 forks source link

Use FetchContent for python_orocos_kdl #2

Closed sloretz closed 2 years ago

sloretz commented 2 years ago

This adds on to #1, but implements the python_orocos_kdl_vendor package with FetchContent instead of ExternalProject.

FetchContent downloads content at configure time so that it becomes possible to add_subdirectory() it. This means the fetched project's targets become part of the current project instead of being a completely separate project in the build directory. This has a few advantages but importantly for python_orocos_kdl_vendor it means it's now possible to determine where the PyKDL module is installed.

A side benefit is forwarding CMake arguments is no longer necessary since the targets are now part of the current project. That removes a bit of the ANDROID_* boilerplate.

nuclearsandwich commented 2 years ago

FetchContent is new to me. I'll have to read the docs. Two questions I have about this approach which I may find the answers to when I read the docs or not.

  1. Does using FetchContent rather than ExternalProject still produce a separate cmake project? We haven't had "seamless" vendor projects where a package at the CMake level has no knowledge of the vendor package but removing a vendor package layer is as straightforward as substituting the vendor package dependency for the upstream platform dependency and then deleting find_package(*_vendor) from the CMakeLists.txt. I'd like to maintain a uniform usage mode for all vendor packages and I'd very much dislike if find_package(*_) replaced the upstream find_package(*) call.
  2. Would you consider FetchContent the preferred way to implement vendor packages going forward or are there other limitations which make it less than a strict improvement over ExternalProject?
sloretz commented 2 years ago

Does using FetchContent rather than ExternalProject still produce a separate cmake project?

Not in the same way. It pulls targets into the current CMake project via add_subdirectory(), but since the CMakeLists.txt inside that subdirectory calls project(...) right away it does set the variables like ${PROJECT_NAME} in there.

I'd like to maintain a uniform usage mode for all vendor packages and I'd very much dislike if findpackage(*) replaced the upstream find_package(*) call.

I'd like to test this locally when I get a moment, but here's what I think would happen. I predict that find_package(foobar_vendor) would not replace find_package(foobar) because the package in the subdirectory is still creating and installing foobarConfig.cmake etc. I would expect find_package(foobar) could be used without calling find_package(foobar_vendor) at all so long as the package had a package.xml dependency on foobar_vendor to make colcon make it available in an isolated workspace.

Would you consider FetchContent the preferred way to implement vendor packages going forward or are there other limitations which make it less than a strict improvement over ExternalProject?

I don't have an opinion yet. I like that it removes the need to forward CMake arguments. I've heard packages with a lot of FetchContent stuff can experience long configure times because CMake will check if the content is up to date, but I haven't experienced that first hand. I got the idea to apply it here because I've been reading about CMake superbuilds lately (something I'd like to do a show-and-tell on when I get a moment).

nuclearsandwich commented 2 years ago

I'd like to test this locally when I get a moment, but here's what I think would happen. I predict that find_package(foobar_vendor) would not replace find_package(foobar) because the package in the subdirectory is still creating and installing foobarConfig.cmake etc.

I don't have any experience with multiple projects within the same CMake context (that happening for ROS 1 was all before my time here) but so long as your predictions hold, I don't see any issue with using FetchContent here and it resolves an explicit configure vs build time challenge with this vendor package.

I would expect find_package(foobar) could be used without calling find_package(foobar_vendor) at all so long as the package had a package.xml dependency on foobar_vendor to make colcon make it available in an isolated workspace.

That's interesting. I think that some of the vendor packages provide CMake hooks or hints for finding their vendored packages but it could be that there are some situations where calling the vendor package in CMake is entirely spurious. Again, my preference is for all of them to interact the exact same way regardless of implementation. But that's quite promising as a way of increasing the seamlessness of vendored dependencies. Getting it out of CMakeLists.txt and consigning it just to package.xml would be very nice indeed.

sloretz commented 2 years ago

@jacobperron since this PR is targeting your branch, may I merge it?