cmake: export into build directory

[dglaeser]

We are using highfive in a downstream repository, either via cmake's fetchcontent or by including it in the build via add_subdirectory. Both approaches worked for us with older versions, but with the current main we run into several issues.

With the changes in the PR we have tested the following scenarios, which failed without these changes:

• including HighFive in the build of a downstream project, linking against that one - in the build directory; without installation - in yet another downstream project
• including HighFive in the build of a downstream project, installing it into some non-standard location, and then consuming the downstream project in yet another downstream project by using find_package with -Ddownstream_ROOT=....

[dglaeser]

@1uc, would you mind having a look? (You appeared at the top of the contributors list ;))

[1uc]

There's a reasonably large set of ways we test that integration of HighFive into other projects works:
https://github.com/highfive-devs/highfive/tree/main/tests/cmake_integration

Clearly, we're missing something. @dglaeser could you please provide a sketch of a minimal CMakeLists.txt that exhibits the issue? If we're to support FetchContent it seems prudent to also check it in future.

[dglaeser]

@1uc thanks a lot for your quick reply! I have compiled a minimal working example:
highfive-integration.zip

See the test.sh script in the top folder. I've put a few comments there that hopefully are explanatory. It allows testing downstream project linkage against the build or the install folder (see the first switch in the script). It actually doesn't use fetchcontent as we faced these issues also when integrating it as a subfolder (which is done in this minimal example)

Let me know if this is helpful.

[dglaeser]

Clearly, we're missing something

As far as I recall when making these changes, the first error I got was when the downstream project creates an export set, it then complained that HighFive wouldn't be in any export set (you should see this error when you use my script with the current main branch of HighFive). At first glance it seems to me that this should be covered in your cmake integration tests, not sure yet why this doesn't seem to pop up on your end. The solution was to have the config file in the build directory upon configuration (the added configure_package_config_file instruction).

The other issues we faced were related to shipping HighFive alongside a downstream project and integrating it yet further downstream. Your integration tests probably only cover direct dependencies?

1. The export into the build dir was necessary to support linkage against a build directory
2. Changing the install destination for the config files was necessary such that -Dupstream_ROOT=... works and automatically finds HighFive as well. Using the GNUInstallDirs provided by cmake is probably a good way to define those paths? But since you had lib/cmake/HighFive hardcoded, you may not want this change?

[1uc]

Thank you, this is very helpful! I think I see what's happening; and while we do test that libraries can be installed and used, we only check this for libraries that themselves use find_package(HighFive) to integrate HighFive. In this case upstream is a library that uses submodules or fetch content to integrate HighFive.

I was never sure if a library could sensibly install HighFive, and I'm still not fully convinced. (Except if upstream itself is a header-only library.) However, this PR looks like changes we should anyway be making. I'd like to implement the tests to detect this before merging this.

Thank you for the nice fix.

[1uc]

@dglaeser I replaced the use of GNUInstallDirs with a variable (similar to what nlohmann/json) does. The default is what it would have been with GNUInstallDirs, i.e. share/HighFive/cmake. It removes possibility of confusion about GNU specific code in a cross-platform project; and it's one less thing one needs to read to understand what's happening (e.g. it's not clear to me that the variable ${..._DATAROOTDIR} could be changed without the CMake code ending up in a location where CMake wont look for it).

I noticed that all the breakage happens because I wasn't aware the CMake can use build directories (and not only install directories) as the source for dependencies of downstream projects. Is there a (particularly) convincing reason to do so? I'm wondering if I've been missing out of something.

[1uc]

@dglaeser to me it looks mergable, thank you for your work! Please let me know if you want to test the small change to make sure it's compatible with your code.

[matz-e]

Small nitpick: we originally had lib/cmake/HighFive...

I personally feel that swapping the order will make the share directory a bit more manageable.

Suggested change

	set(HIGHFIVE_CMAKE_INSTALL_DIR "share/HighFive/cmake" CACHE STRING
	set(HIGHFIVE_CMAKE_INSTALL_DIR "share/cmake/HighFive" CACHE STRING

[1uc]

If I look at say /usr/share, mine looks something like this:

$ ls -1 /usr/share
aclocal
alsa
autoconf
avahi
awk

... and indeed somewhere in there I find:

$ ls -1 /usr/share/cmake/
nlohmann_json
TBB

(and a small number of others). While,

$ ls -1 /usr/share/*/cmake/
/usr/share/eigen3/cmake:
Eigen3Config.cmake
...

and maybe one or two more. Neither seems to be popular.

On this system the overwhelming majority of libraries put their CMake stuff in /lib/cmake/<name> (like we did before). Maybe we just revert back to the old location, but keep the variable, then everyone can put it where they like.

[matz-e]

Come to think of it, lib seems more appropriate - that is also where pkgconfig canonically seems to expect its library configuration files. The share/cmake would also host the CMake internals (at least on my system)

[dglaeser]

Thanks a lot for the comments! I just checked again where cmake looks for config files in calls to find_package in config mode, and it seems that it looks in a relatively large number of places: https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure

So I guess lib/cmake/<name> should be fine as well as all the other options that came up as far as I can see. Sorry for making this change too hasty, didn't seem necessary.

The share/cmake would also host the CMake internals (at least on my system)

Yes that's why I originally used share/<name>/cmake (although share came from CMAKE_INSTALL_DATAROOTDIR)

[dglaeser]

I noticed that all the breakage happens because I wasn't aware the CMake can use build directories (and not only install directories) as the source for dependencies of downstream projects. Is there a (particularly) convincing reason to do so? I'm wondering if I've been missing out of something.

So the issue with the build directory came up in a use case of one of our users: dglaeser/gridformat#299

They have a modular code framework with several core modules and a whole ecoystem of optional modules, each of which is defined in a separate repository. But all of these must be configured together so that (for development) code edits in any of the modules would retrigger any affected target to be rebuilt. Their use case required us to add the possibility to link against the build directory (i.e. export the targets into the build dir), we also didn't consider that originally.

But since it only involved one additional export and had no negative side effects on our originally considered use cases, we just added it. I also saw that in HighFive-2.10.1 such export was still done: https://github.com/highfive-devs/highfive/blob/v2.10.1/CMake/HighFiveTargetExport.cmake#L46.

On our end, after dglaeser/gridformat#299 was reported, we discussed whether we should stop including HighFive as a submodule in our project and require users to install it themselves, or try to make it compatible (which is what we attempted here :)). But I agree that this may be a bit of an exotic use case, so I'd also understand if you didn't want to incorporate these changes.

I noticed that all the breakage happens because I wasn't aware the CMake can use build directories

Yes you're right. The first issue I mention in my comment above (#27 (comment)) also would not appear if the upstream module did not export the targets into the build directory. I think what I said in the comment is actually wrong, configure_package_config_file wouldn't be the solution here, the targets need to be exported into the build dir.

In fact, I think both branch, the PR and the description are misnomers. fetchcontent also seems to work fine if one doesn't export targets that link against HighFive into the build dir. Let me know if I should push a new branch + PR (in case you want to keep these changes)

[dglaeser]

@dglaeser to me it looks mergable, thank you for your work! Please let me know if you want to test the small change to make sure it's compatible with your code.

I tested it, still works fine, thanks!

[1uc]

@dglaeser thank you for the detailed explanations; and I'm happy you decided to report the issue with a fix. The usecase you describe makes sense and I see no reason to prevent it. The removal of export was likely motivated by not understanding what it did; and a strong desire to remove all cryptic, pointless stuff from the CMake code. Now we know precisely why it's needed.

I think what I said in the comment is actually wrong, configure_package_config_file wouldn't be the solution here, the targets need to be exported into the build dir.

I looked at this, and agree, in our case the point of calling configure_package_config_file is only for it to copy the file to the build directory. However, this feels like a principled way of getting the file to appear there; so I like the idea.

The PR is fine like this, because it'll get squash merged and I'll be asked to edit the commit message anyway.

(Github seems to be wonky, right now and isn't picking up the 6th commit. So we'll let it sit a little until that sorts itself out.)

[dglaeser]

@1uc, ok great! Thanks a lot for your help on this!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud