mctools/mcpl

Exporting CMake targets

paulromano opened this issue · 10 comments

In the CMakeLists.txt file, executables/libraries are installed via install(TARGETS ...) but there is no corresponding export installation, install(EXPORT ...), which is considered a CMake best practice. If a hypothetical MCPLConfig.cmake file were installed, this would allow downstream projects (e.g., OpenMC), to use MCPL with:

find_package(MCPL)
...
target_link_libraries(libopenmc MCPL::mcpl)

Rather than having to explicitly add -lmcpl and specify the include directory.

You are absolutely right @paulromano, the MCPL CMake file was written when CMake 3 was still not the norm. I'll see when I get around to fixing this, but will certainly do so.

I am looking at this now, but it might take a few days since I am taking the opportunity to give the CMakeLists.txt a more complete overhaul.

This is now hopefully addressed in the newly minted MCPL v1.4.0 release. Let me know of any issues, it was a rather large update of the CMake code in the end.

I am having a bit of trouble using this. When I try to: find_package(MCPL) i get an error saying that MCPL cannot be found, and I need to specify a MCPL_DIR. If I do so I can compile fine, but it kind of goes against the purpose I think.
Update: I find that if I use the attached patch (which puts the .cmake config files in e.g. /cmake/MCPL instead of /cmake i have no problems. As far as I can tell this is consistent with the search path reported at: https://cmake.org/cmake/help/latest/command/find_package.html#search-procedure for unix.
mcpl_cmake.patch.txt

Thanks for investigating this @ebknudsen, and sorry for the delay.

So yeah, it is not clear to me what is actually best practices here, and most likely it depends on your environment, and things such as cmake prefix path. When building NCrystal (which has a similar CMakeLists.txt as MCPL) for conda I set the NCrystal_CMAKEDIR to be lib/cmake https://github.com/conda-forge/ncrystal-feedstock/blob/main/recipe/build.sh , but of course I also fiddle with the prefix path later on.

I am happy to modify this, I just want to convince myself that the change (which I probably will eventually apply to NCrystal as well for consistency) will not be too breaking for existing usage (not so much a worry for MCPL where the find_package feature is anyway new).

Reading a bit on https://stackoverflow.com/questions/51112173/whats-the-ideal-cmake-installation-directory-structure-for-a-relocatable-multip it seems to support your $LIB/cmake/MCPL proposal, which also looks reasonable to me.

Before making the change I would like to test a few things, so it might take a few days. For my information, is your work on openmc hooks actually held back by this (if so I should look at it a bit more urgently)?

I can see that I have a bit of a mess with this proposed pattern with the way I am handling the Geant4-MCPL interface at the CMake level. Anyway, I will sort it out somehow, as I agree the change is necessary.

@tkittel At present (since I have a patch) I can work on the openmc hook without problem, so no rush. It would need to be fixed before adoption into the openmc tree though (I think).

@ebknudsen the mcpl develop branch now contains a candidate for v1.5.0 which should contain the proposed fix. Can you check that it works for you?

Of necessity it also modifies the way the Geant4 bindings are activated in downstream CMake clients, but that should hopefully not affect you at all.

@tkittel Just checked - it appears to work. Thanks mate!

Thanks Erik, this is now published in release v1.5.0 :-)