Allow HIGHFIVE_USE_INSTALL_DEPS only at install time

[matz-e]

Remove the "flexibility" of finding dependencies at use time. This
simplifies our CMake code greatly.

[codecov]

Codecov Report

Merging #710 (2847541) into master (7acd5ba) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #710   +/-   ##
=======================================
  Coverage   82.38%   82.38%           
=======================================
  Files          67       67           
  Lines        4503     4503           
=======================================
  Hits         3710     3710           
  Misses        793      793           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[tdegeus]

I've made this suggestion already before, but I still think it is worth to consider. I would propose to have a few targets:

HighFive::include (headers only)
HighFive::HighFive (headers + HDF5)
HighFive::Boost (enable Boost, add relevant deps to target)
HighFive::xtensor (,, xtensor ,,,)
...

(In addition to the current target that remains the same for backward compatibility, but that is constructed by HighFive::HighFive + HighFive::Boost)

That way the user could do e.g.

find_package(HighFive required)
target_link_libraries(mytarget HighFive::HighFive HighFive::xtensor)

or even

find_package(HighFive required)
find_package(hdf5 required)
target_link_libraries(mytarget HighFive::include ...my_custom_hdf5_target)

It would have the same simplification and in addition give a lot of flexibility all while cleaning up the user's CMakeLists.txt

[1uc]

The context of this Draft PR is figuring out an internal build failure. It's not to rewrite or restructure our CMake code.

Reworking our CMake code is needed. I'm not sure we need all the targets you propose. One perk of header-only is, we don't compile anything; and we also don't have what CMake calls "private" dependencies, i.e. if a user needs HighFive with XTensor support, they must have XTensor objects. Hence they've set everything to be able to include and link with XTensor, or Eigen, or Boost, or ...

Hence I hope we could get to a point where:

#include <highfive/highfive.hpp>
#include <highfive/eigen.hpp>

is all that's needed to get Eigen support, i.e. a single "library" supports all optional dependencies simply by #includeing the "addons" that are needed (if and only if they're required).

I like the HighFive::include suggestion especially if it would allow us to turn off any HDF5 detection related code.

However, all of this is a topic for a different PR, maybe the one you made, or a different one.

[1uc]

Let's not do any of this; and save changes for when we're willing to actually fix our CMake.

[matz-e]

As @1uc says… we need a revamp. And this does not solve the core issue that HDF5_LIBRARIES sometimes points at targets, other times at libraries.

@tdegeus I've had a look at your old PR again, and I think I misunderstood some of the concepts in there before. I'm sorry! In general, I think it seems to be a better approach (@1uc helped convince me), one thing I'm wondering: Rather than create all kinds of different targets, would it be worth considering using CMake components?

[tdegeus]

@matz-e Don't worry. But indeed, I felt a bit misunderstood ;).

I have no clear arguments pro/con a 'namespace' with several targets and components.

Personally I feel that targets have greatly improved CMake. It allows to better mix an match and to be much more readable. So the focus should stay there in my opinion. Do I understand well that with components e.g.

find_package(highfive COMPONENTS headers hdf5 xtensor) headers/hdf5/xtensor
target_link_libraries(myexec PRIVATE highfive)
# -> target "highfive" that will link/include the 

find_package(highfive COMPONENTS headers xtensor) 
target_link_libraries(myexec PRIVATE highfive)
# -> target "highfive" that will include the headers/xtensor, but not hdf5

and that the equivalent with namespaces would be

find_package(highfive REQUIRED) 
target_link_libraries(myexec PRIVATE highfive::headers highfive::xtensor)
# -> will include the headers/xtensor, but not hdf5

[matz-e]

I agree on the target part. I mentally tuned out that HighFive still used the older variables 🙈

Regarding the components, it seems that they could be used to make the targets available. As in:

find_package(highfive)  # no components in HighFive

and exposing highfive::xtensor would always have to do a find_dependency(xtensor), but with targets that could be made optional, and kind of avoid all these badly namespaced global variables?

find_package(highfive COMPONENTS hdf5 xtensor)

would basically be equivalent to set USE_XTENSOR=ON USE_BOOST=OFF. I would have to see more about how all this normally is implemented, though.