BlueBrain / HighFive

HighFive - Header-only C++ HDF5 interface
https://bluebrain.github.io/HighFive/
Boost Software License 1.0
687 stars 161 forks source link

Boost_NO_BOOST_CMAKE TRUE #685

Closed aminiussi closed 1 year ago

aminiussi commented 1 year ago

Describe the bug Forcing Boost_NO_BOOST_CMAKE to TRUE might prevent build whe nested as a submodule of a project using different Boost discovery settings.

To Reproduce

This is tricky, as I only get the issue on a HPE Cray system (which is not easy to get). I got the problem by cloning a project using my own fork of HighFive (because of another issue) with a boost-cmake enabled installation of boost.

Expected behavior

In CMake/HighFiveTargetDeps.cmake we find:

    set(Boost_NO_BOOST_CMAKE TRUE)  # Consistency

I am not sure to understand the consistency issue, but why not rely on the user settings, which would prevent conflicting with the possible super project ?

Basicaly, I would expect the HighProject to find the system and serialization library the same way the enclosing project finds them and let me decide if I want to use boost-caml=ke or not.

Stacktrace

CMake Error at /lus/work/CT4/oca7233/alainm/fargOCA/cmake-3.25.0-cce-15.0.0-7fqa/share/cmake-3.25/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Boost (missing: system serialization) (found version
  "1.80.0")
Call Stack (most recent call first):
  /lus/work/CT4/oca7233/alainm/fargOCA/cmake-3.25.0-cce-15.0.0-7fqa/share/cmake-3.25/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  /lus/work/CT4/oca7233/alainm/fargOCA/cmake-3.25.0-cce-15.0.0-7fqa/share/cmake-3.25/Modules/FindBoost.cmake:2376 (find_package_handle_standard_args)
  external/HighFive/CMake/HighFiveTargetDeps.cmake:26 (find_package)
  external/HighFive/CMakeLists.txt:63 (include)

Desktop (please complete the following information):

Additional context

$ CC --version
Cray clang version 15.0.0  (324a8e7de6a18594c06a0ee5d8c0eda2109c6ac6)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /opt/cray/pe/cce/15.0.0/cce-clang/x86_64/share/../bin
1uc commented 1 year ago

It seems to be to "Enhance IBM BlueGene/Q support" back in 25627b08. There's two easy ways we could tackle this:

  1. Simply remove the line, since it's arguably a bug if we hard code it to a specific value.
  2. Set the value to On only when the variable hasn't been set yet.

The advantage of the first approach is that it modernizes the CMake code for HighFive, and arguably makes it less surprising. Downside, it might break existing builds, this should be fixable by adding -DBoost_NO_BOOST_CMAKE=On to the CMake invocation of projects consuming HighFive.

The second approach flips advantages and disadvantages of the first approach. Personally, I like the idea of not risking breaking exisiting setups. We could always later move from the second approach to the first.

matz-e commented 1 year ago

That comment is also woefully inadequate. One reason I could see that historically, there were too many differences between the FindBoost CMake provides and the configuration Boost installed. I'd normally hope that these differences have disappeared during recent years.

I would be in favor of approach (1), as, as a user, I would not expect a library to set these things.

For (2), projects relying on HighFive as a submodule should expect changes when updating, anyways. That would seem to then impact less people.

1uc commented 1 year ago

I think the file this is in also gets installed with HighFive: https://github.com/BlueBrain/HighFive/blob/master/CMake/HighFiveConfig.cmake.in#L64

Therefore, it'll also affect users that install HighFive and then rely on CMake to find HighFive and transitively Boost.

matz-e commented 1 year ago

Which I find in itself offensive. HighFive should not refind its dependencies over and over again... but fixing that CMake mess is something else.

1uc commented 1 year ago

@aminiussi Thank you for pointing out the issue, removing the line (or deactivating it by choosing to allow Boost CMake) removes numerous annoying (and misleading) CMake warnings.