Consider switching from the legacy Find*.cmake modules to install(TARGETS ... EXPORT ...)

[SomeoneSerge]

Instead of writing custom Find*.cmake modules

https://github.com/mosra/magnum-integration/blob/f01593fc94556bff23a848ac71187c56e034b6d9/modules/CMakeLists.txt#L26-L30

...consider using install(TARGETS ... EXPORT) so that CMake would generate a safer module for find_package.

For context, I'm observing some confusing issues with the Find-module when trying to build habitat-sim (https://github.com/SomeoneSerge/dust3r.nix/blob/35377e113d4b03e4c625e270554d4d1b1344419a/nix/magnum-integration/package.nix):

...
magnum-integration> -- Install configuration: "Release"
magnum-integration> -- Installing: /nix/store/7d4q02xsprmlsghak8769zj4prb9l5qq-magnum-integration-unstable-2024-03-07/share/cmake/MagnumIntegration/FindMagnumIntegration.cmake
magnum-integration> -- Installing: /nix/store/7d4q02xsprmlsghak8769zj4prb9l5qq-magnum-integration-unstable-2024-03-07/share/cmake/MagnumIntegration/MagnumIntegrationConfig.cmake
magnum-integration> -- Installing: /nix/store/7d4q02xsprmlsghak8769zj4prb9l5qq-magnum-integration-unstable-2024-03-07/include/Magnum/versionIntegration.h
magnum-integration> -- Installing: /nix/store/7d4q02xsprmlsghak8769zj4prb9l5qq-magnum-integration-unstable-2024-03-07/include/Magnum/EigenIntegration/DynamicMatrixIntegration.h
magnum-integration> -- Installing: /nix/store/7d4q02xsprmlsghak8769zj4prb9l5qq-magnum-integration-unstable-2024-03-07/include/Magnum/EigenIntegration/GeometryIntegration.h
magnum-integration> -- Installing: /nix/store/7d4q02xsprmlsghak8769zj4prb9l5qq-magnum-integration-unstable-2024-03-07/include/Magnum/EigenIntegration/Integration.h
...
habitat-sim> CMake Error at /nix/store/paxnwg89pdx4car5fj84mmiprg952459-cmake-3.28.2/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
habitat-sim>   Could NOT find MagnumIntegration (missing: Eigen)
habitat-sim>
habitat-sim>       Reason given by package: Eigen is not built by default. Make sure you enabled MAGNUM_WITH_EIGEN when building Magnum Integration.
...

Thanks!

[mosra]

Hi, sorry for a rather radical opinion, but I'd like to keep it like this.

The reason is that with install(EXPORT) the part of the buildsystem meant for users gets way too tied to the buildsystem used internally by the library. From my past experience with basically every CMake-based library that does this, except for the most simple projects it regularly happens that the generated and installed *Config.cmake files are broken in some way. Having some non-existent library as a dependency, hardcoded absolute file paths, CMake just exiting because the config file has a syntax error, etc. etc. While the dependencies themselves build fine almost always, the CMake configs are almost always horribly broken, and I have to do horrible things to work around those bugs. Just that experience alone led me to believe the CMake config file system is way too prone to accidental errors and even though I have 15 years of CMake experience myself, I still don't think I'm prepared to deal with that.

The way it is done for now is more work for me to keep the Find module in sync with the library, yes, but from the user perspective it makes the process of using the library decoupled from any buildsystem problems the library itself might have. Not to mention certain areas of the library rely on various workarounds to paper over too limited or historically missing CMake functionality, where a particular hack usually works only internally when building the library or inside the Find module but rarely both.

The error message you have seems to be unrelated to this, tho. The headers are there, so does CMake know that it should look for installed Magnum Integration in /nix/store/7d4q02xsprmlsghak8769zj4prb9l5qq-magnum-integration-unstable-2024-03-07? Apart from that, Habitat should be able to build with external dependencies if you pass -DUSE_SYSTEM_MAGNUM=ON etc. to it, like with any other CMake option, no need to patch the sources for it. I'm using those options for local development myself.

[SomeoneSerge]

The headers are there, so does CMake know that it should look for installed Magnum Integration in /nix/store/...-magnum-integration-unstable-2024-03-07

Thanks, I had just fixed that by explicitly hard-coding MAGNUMPLUGINS_INCLUDE_DIR&c: https://github.com/SomeoneSerge/dust3r.nix/commit/47d9eea6fb9c12d6a8881699453033293c2fef3f#diff-4303c42389dc973e38a9870b696fdc2cae9c9401c60ab66848297490b8a324faR104-R106. This normally is not required for find_package to work when using vcpkg/Nixpkgs/etc, because the modules' locations are injected via CMAKE_PREFIX_PATH. I suppose you could make this work with your FindMagnum*.cmake approach by extending the hints: https://github.com/mosra/magnum-integration/blob/f01593fc94556bff23a848ac71187c56e034b6d9/modules/FindMagnumIntegration.cmake#L93-L94

From my past experience with basically every CMake-based library that does this, except for the most simple projects it regularly happens that the generated and installed *Config.cmake files are broken in some way ... led me to believe the CMake config file system is way too prone to accidental errors ... even though I have 15 years of CMake experience

I feel your pain, and this definitely used to be the case... The situation today, I think, is that "simple CMake" works, and does so comparatively smoothly. The "complex cmake" with any custom logic still creates friction. As an example, and I do not mean this as a criticism, your custom Find*.cmake is "more complex" than "no (hand-written) module", and it does fail to "automatically" discover other pieces of magnum when used with vcpkg/Nixpkgs. At the same time, the simpler/naive install(TARGETS ... EXPORT ...) would have passed this particular test.

... gets way too tied to the buildsystem ...

I think this also has changed, here's why:

0. One can install both .pc and .cmake "exports", and non-CMake downstream projects can always rely on pkg-config.
1. CMake exports are being successfully consumed by other projects, e.g. by Meson.
2. The interface that you use to consume different pieces of magnum/corrade in CMake does not change: it's still just a find_package declaration

Habitat should be able to build with external dependencies if you pass ... no need to patch the sources for it.

I was being lazy. They were too many for the moment, I chose to sed them all to ON 😅

[SomeoneSerge]

Maybe one argument for making find_package and CMAKE_PREFIX_PATHS sufficient (whether by updating FindMagnum*.cmake or by letting CMake generate a Magnum*Config.cmake) is that with the current approach we observe downstream projects vendor copies of FindMagnum*.cmake: https://github.com/SomeoneSerge/dust3r.nix/commit/47d9eea6fb9c12d6a8881699453033293c2fef3f#diff-4303c42389dc973e38a9870b696fdc2cae9c9401c60ab66848297490b8a324faR57

[mosra]

Thanks for the extra info!

This normally is not required for find_package to work when using vcpkg/Nixpkgs/etc, because the modules' locations are injected via CMAKE_PREFIX_PATH. I suppose you could make this work with your FindMagnum*.cmake approach by extending the hints:

Interesting, I would also think CMAKE_PREFIX_PATH would be enough. It was always enough for me. Does it mean that FindMagnumIntegration.cmake gets the magnum-integration install location in CMAKE_PREFIX_PATH correctly but just fails to use it, for some reason? Sounds strange.

I tried to reproduce locally, by uninstalling my system-wide installation in /usr, creating a new project with find_package(MagnumIntegration REQUIRED Eigen), pointing CMAKE_PREFIX_PATH to a location where magnum-integration was instead, and it found everything. Is there anything else I would need to do to get closer to what the .nix packages do?

downstream projects vendor copies of FindMagnum*.cmake

Yes. This isn't strictly needed but I was recommending projects to do that because then if the dependency isn't present anywhere in the system, CMake would simply say "hey, Foo was not found" instead of printing a scary long wall of text about how some FooConfig.cmake file needs to be put somewhere. The latter was a very common pain point for users new to CMake, because they didn't recognize that message as telling them to install the dependency but rather as something being wrong with the library they're trying to build.

[SomeoneSerge]

Interesting, I would also think CMAKE_PREFIX_PATH would be enough. It was always enough for me. Does it mean that FindMagnumIntegration.cmake gets the magnum-integration install location in CMAKE_PREFIX_PATH correctly but just fails to use it, for some reason? Sounds strange.

The FindMagnumIntegration.cmake does get loaded correctly, but the find_path fails when magnum, magnum-bindings and magnum-integration (the paths being matched, e.g. Integration.h) are installed into separate non-overlapping prefixes.

(the easiest reproducer I could offer would be removing the hard-coded -Definitions in the repo I linked, but that relies on nix...)

[mosra]

Huh. I tried further, having Corrade, Magnum and Magnum Integration in non-default paths (with nothing in /usr), and cmake .. -DCMAKE_PREFIX_PATH="~/Code/magnum-integration/package/archlinux/pkg/magnum-integration/usr/;~/Code/magnum/package/archlinux/pkg/magnum/usr/;~/Code/corrade/package/archlinux/pkg/corrade/usr" led to

-- Found Corrade: /home/mosra/Code/corrade/package/archlinux/pkg/corrade/usr/include  found components: Containers rc Utility
-- Found Magnum: /home/mosra/Code/magnum/package/archlinux/pkg/magnum/usr/include
-- Found MagnumIntegration: /home/mosra/Code/magnum-integration/package/archlinux/pkg/magnum-integration/usr/include  found components: Eigen

Not sure what could be different here, worked with both the Find modules bundled in the project and taken from the prefix path. CMake 3.28. Can you upload a CMakeCache.txt of the failed configuration somewhere? I'll try to see what were the actual failed variables. What happens if you add

message(STATUS "${CMAKE_PREFIX_PATH}")

at the top of the FindMagnumIntegration.cmake, if it's easy to do? Does it show the actual paths nix should be passing? If not, then something else the way might be clearing those :thinking:

[SomeoneSerge]

Hi, sorry I got a bit caught up and couldn't reply in time.

message(STATUS "${CMAKE_PREFIX_PATH}")

Is empty even when I explicitly pass -DCMAKE_PREFIX_PATH=$CMAKE_PREFIX_PATH on the CLI.

CMakeCache.txt

Sure, here are the logs and the cache for a configuration where I remove all of the MAGNUM.*INCLUDE_DIR variables: https://gist.github.com/SomeoneSerge/2bf89737c1a2520130bafc3af097f955

-- Found MagnumIntegration: /home/mosra/Code/magnum-integration/package/archlinux/pkg/magnum-integration/usr/include found components: Eigen

Interesting. Could you also publish your logs? I'm wondering how is the find_path resolved... By the way I noticed that I can remove -DMAGNUMBINDINGS_INCLUDE_DIR without the build failing.

[mosra]

Thanks for the info, that helped me to finally figure out what was going on. (And sorry for a delayed reply as well.)

That it worked in my case was due to my own stupidity where I put the path to the integration install dir earlier than the path to install dir of magnum, so it did the right thing, completely by accident. After looking into your output and the cache file I realized it could be order dependent, and when I fixed the order in CMAKE_PREFIX_PATH, I finally managed to reproduce. (That CMAKE_PREFIX_PATH was seemingly empty in your case, while being correctly recorded in CMakeCache.txt, is something else, and probably unrelated. Not sure I want to dig any further, heh.)

After making the MAGNUMINTEGRATION_INCLUDE_DIR look for a name unique to this repository instead of just happily picking MAGNUM_INCLUDE_DIR, it finally did the right thing. That fix is in d0aab18311083a35a2372d71c3760d652d2e4849, and a corresponding change also in mosra/magnum-plugins@7dfbea3ed2e24273fde71ccb7cf26b8a48c99cd6, mosra/magnum-extras@aec121de66ac35e233002d4640f59d330ddd4e76 and mosra/magnum-bindings@a775640b027b1b541bd682300c1fe69d7f816a61.

All those commits are in master branches now as I'm pretty sure it works, but in any case, can you confirm that it fixes the problem for you as well? Thank you!

[SomeoneSerge]

Hi! Yes, the fix works.

1. I tested I can now remove the manual -DMAGNUM_*INCLUDE_DIR definitions: https://github.com/SomeoneSerge/dust3r.nix/commit/2c5ed64144bc95ef504de21083a13c03092c40fb#diff-4303c42389dc973e38a9870b696fdc2cae9c9401c60ab66848297490b8a324faL119-L121
2. Of course this only works if I first remove the vendored Find*.cmake modules in habitat-sim which now diverge from upstream: https://github.com/SomeoneSerge/dust3r.nix/commit/2c5ed64144bc95ef504de21083a13c03092c40fb#diff-4303c42389dc973e38a9870b696fdc2cae9c9401c60ab66848297490b8a324faR71.

To conclude, the cause or the "symptom" which prompted this github issue has been addressed and it can be closed as resolved. It can be argued that it's not coincidental that the fix required updating both the three upstream repositories and the downstream project (habitat-sim), that without the vendored modules there'd be less "state" to maintain, and that this is the cost of vendoring vs the cost of trusting cmake. Either way, thank you for your project and for being responsive! Cheers

[mosra]

Thanks!

Of course this only works if I first remove the vendored

Yep. I'm going to update them in the next regular dependency update PR if that's fine. So far there wasn't too many critically important changes since I last did it in https://github.com/facebookresearch/habitat-sim/pull/2322 so it might take a while.

it's not coincidental

Definitely! :) I have something to think about, but for now this was the quickest way to fix the root cause, and learning + implementing proper CMake config files would take a lot more time that I unfortunately currently don't have (which is also a reason there hasn't been a tagged release since 2020, etc.).