search

gazebosim / gz-cmake

A set of CMake modules that are used by the C++-based Gazebo projects.
https://gazebosim.org/libs/cmake
Apache License 2.0
24 stars 30 forks source link

Find one of multiple packages #4

Open osrf-migration opened 6 years ago

osrf-migration commented 6 years ago

Original report (archived issue) by Shane Loretz (Bitbucket: Shane Loretz, GitHub: sloretz).

Proposal

# find ignition-math3, or ignition-math4
# copy all variables prefixed with the found project name into variables prefixed with ${OUTPUT_VAR}
ign_find_package(
    OUTPUT_VAR
        some-math-version
    ONE_OF
        ignition-math4
        ignition-math3
    REQUIRED)

 # ...

target_link_libraries(my_lib ${some-math-version_LIBRARIES})

Developing on the default branch of ignition projects has one minor annoyance that occurs fairly frequently. The major version being bumped on one branch breaks the build of every downstream project. If the upstream project doesn't have a release then downstream must choose between everything-from-source builds being broken, or CI being broken. To get around this some projects have been finding either ignition-math3 or ignition-math4.

osrf-migration commented 6 years ago

Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).

I'm a tad bit concerned that the arguments of ign_find_package are getting a bit bloated (though I have no one to blame but myself for that), but I definitely like this concept, and I think it's worth doing this (or something very similar).

One implementation detail that could be tricky is that ign_find_package(~) currently expects one mandatory argument (the package name) while all the rest of the arguments are optional. We could get around this by removing the mandatory first argument and replacing it with an optional argument like PACKAGE <package_name> where the caller would have to choose whether they use PACKAGE <package_name> or ONE_OF <package_list>. Or instead we could just always require the caller to specify ONE_OF even if they only intend to ask for one package.

An alternative way of doing this would be to keep the mandatory first argument and instead add optional arguments ALTERNATIVES <package_list> and OUTPUT_VAR <chosen_package>. This would be fully backwards compatible with our current signature for ign_find_package(~), so I think this might be my preference.

One thing I'm not clear on is what exactly OUTPUT_VAR should provide. I don't think a version number really makes sense, since we may want to use this in cases where we're choosing between differently named packages. I'm thinking it should return the name of the package, so that if that package's config file is designed according to convention, we could have something like:

ign_find_package(
    ignition-math4
    ALTERNATIVES
        ignition-math3
        ignition-math2
    OUTPUT_VAR
        chosen_ign-math
    REQUIRED)

And then the variable ${${chosen_ign-math}_LIBRARIES} would contain whatever ign-math libraries were found.

osrf-migration commented 6 years ago

Original comment by Shane Loretz (Bitbucket: Shane Loretz, GitHub: sloretz).

looks good to me. If ign_find_package arguments are getting too bloated, would the situation be made better or worse by making this variant a separate cmake macro?

osrf-migration commented 6 years ago

Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).

I think making it part of ign_find_package is definitely the right way to go, because any alternative that I can imagine would just be more confusing and less manageable. But moving forward, we should definitely be conscious of the risk of feature creep on this function and consider splitting its job up in some way if we decide that it's become bloated.