Using externally provided third-party libraries

[paulromano]

Along the same lines as #1778, OpenMC should allow using third-party libraries that are not vendored in order to play nicely when it is built as part of a larger software stack. We recently started allowing this for pugixml and fmt, but xtensor, xtl, and gsl-lite are forced to be our own vendored versions.

I'll note that this is not a hypothetical issue -- we've run into problems recently with builds of ENRICO, which may include both OpenMC and Shift, having conflicting copies of pugixml and gsl-lite.

[paulromano]

One possible solution: https://github.com/cpm-cmake/CPM.cmake

Also some chatter on the cmake repo about enabling a seamless integration of FetchContent and find_package (equivalent to what the above CPM.cmake package currently provides).

[paulromano]

Just to reframe this a little bit, there's really two separate parts:

1. Add find_package calls for xtensor and gsl-lite akin to what was done in Do not use pugixml and fmt submodule if already found on system #1757 for pugixml and fmt.
2. Consider replacing our git submodules for dependencies with FetchContent (or something fancier built on top of that like CPM)

[kkiesling]

I have started working on this following the same find_package method in #1757 (see this branch). I am running into an interesting build error with this.

If using CMake with a clean install and no installed packages (so expect to use all the submodules), this works just fine and compiles.

But if I compile a second time that requires updating cmake, the build fails (but cmake is still successful). In this case, the packages are found in the system because they are already built with a previous OpenMC build and it is reported that the module is found. CMake is ultimately successful still. But then in the make process gls fails (see error below), even though it is still using the exact same gls-lite install that was previously used (the warning appears many times and the error actually appears 3 times).

Details:

CMake/make output from a clean install:

-- Did not find pugixml, will use submodule instead
-- Did not find xtensor, will use submodule instead
-- Did not find xtl, will use submodule instead
-- Did not find gsl-lite, will use submodule instead
-- HDF5: Using hdf5 compiler wrapper to determine C configuration
-- OpenMC C++ flags: -fopenmp;-O2
-- OpenMC Linker flags: -fopenmp
-- Submodule update
-- Module support is disabled.
-- Version: 8.0.1
-- Build type: 
-- CXX_STANDARD: 11
-- Required features: cxx_variadic_templates
-- xtl v0.6.13
-- Building xtensor v0.21.3
-- Found xtl v0.6.13
-- Project 'gsl_lite', package 'gsl-lite' version: '0.36.1'
-- Configuring done
-- Generating done
-- Build files have been written to: /home/kkiesling/software/build/openmc
[  3%] Built target fmt
[  4%] Built target faddeeva
[  6%] Built target pugixml
[successful build to 100%]

Force Cmake to run again, without removing install files (can reproduce this by just adding a blank line to the CMakeLists.txt file) and this is the result. Note that all packages are now found as the submodules that were just previously installed. But then there is this narrowing conversion error coming from gls-lite.

-- Found pugixml: /home/kkiesling/software/install/openmc/lib/cmake/pugixml
-- Found xtensor: /home/kkiesling/software/install/openmc/lib/cmake/xtensor (version 0.21.3)
-- Found xtl: /home/kkiesling/software/install/openmc/lib/cmake/xtl (version 0.6.13)
-- Found gsl-lite: /home/kkiesling/software/install/openmc/lib/cmake/gsl-lite (version 0.36.1)
-- HDF5: Using hdf5 compiler wrapper to determine C configuration
-- OpenMC C++ flags: -fopenmp;-O2
-- OpenMC Linker flags: -fopenmp
-- Submodule update
-- Configuring done
-- Generating done
-- Build files have been written to: /home/kkiesling/software/build/openmc
[  2%] Built target faddeeva
Scanning dependencies of target libopenmc
[  3%] Building CXX object CMakeFiles/libopenmc.dir/src/bank.cpp.o
[  4%] Building CXX object CMakeFiles/libopenmc.dir/src/boundary_condition.cpp.o
[  5%] Building CXX object CMakeFiles/libopenmc.dir/src/bremsstrahlung.cpp.o
[  6%] Building CXX object CMakeFiles/libopenmc.dir/src/dagmc.cpp.o
[  7%] Building CXX object CMakeFiles/libopenmc.dir/src/cell.cpp.o
/home/kkiesling/software/opt/openmc/src/cell.cpp: In member function ‘std::vector<openmc::ParentCell> openmc::Cell::find_parent_cells(int32_t, openmc::Particle&) const’:
/home/kkiesling/software/opt/openmc/src/cell.cpp:1355:39: warning: narrowing conversion of ‘(int)coord.openmc::LocalCoord::cell’ from ‘int’ to ‘gsl::index’ {aka ‘long unsigned int’} [-Wnarrowing]
 1355 |     stack.push(coord.universe, {coord.cell, lattice_idx});
      |                                 ~~~~~~^~~~
/home/kkiesling/software/opt/openmc/src/cell.cpp:1355:45: warning: narrowing conversion of ‘lattice_idx’ from ‘int’ to ‘gsl::index’ {aka ‘long unsigned int’} [-Wnarrowing]
 1355 |     stack.push(coord.universe, {coord.cell, lattice_idx});
      |                                             ^~~~~~~~~~~
/home/kkiesling/software/opt/openmc/src/cell.cpp: In member function ‘std::vector<openmc::ParentCell> openmc::Cell::exhaustive_find_parent_cells(int32_t) const’:
/home/kkiesling/software/opt/openmc/src/cell.cpp:1391:73: warning: narrowing conversion of ‘openmc::model::cell_map.std::unordered_map<int, int>::operator[]((& cell)->std::unique_ptr<openmc::Cell>::operator->()->openmc::Cell::id_)’ from ‘std::unordered_map<int, int>::mapped_type’ {aka ‘int’} to ‘gsl::index’ {aka ‘long unsigned int’} [-Wnarrowing]
 1391 |         if (stack.visited(univ_idx, {model::cell_map[cell->id_], C_NONE}))
      |                                                                         ^
/home/kkiesling/software/opt/openmc/src/cell.cpp:1391:73: error: narrowing conversion of ‘-1’ from ‘int’ to ‘gsl::index’ {aka ‘long unsigned int’} [-Wnarrowing]

Two questions:

1. Should we make the cmake use the submodule still, even if the package is found in the openmc install dir?
2. I have no idea what is happening with this error and why it might appear on this second build but not the first?

[paulromano]

If the find_package call succeeds, I don't think we should be using the submodule. Although it's a little weird to find the package in an existing OpenMC installation, I don't see a good way around that (hard to know where the package was picked up from).

I was able to reproduce that error you're seeing, and it looks like we should be using the namespaced name for gsl-lite-v1 in target_link_libraries:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 3d588cb84..38069662a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -446,7 +446,7 @@ endif()
 # target_link_libraries treats any arguments starting with - but not -l as
 # linker flags. Thus, we can pass both linker flags and libraries together.
 target_link_libraries(libopenmc ${ldflags} ${HDF5_LIBRARIES} ${HDF5_HL_LIBRARIES}
-                      pugixml faddeeva xtensor gsl-lite-v1 fmt::fmt)
+                      pugixml faddeeva xtensor gsl::gsl-lite-v1 fmt::fmt)
 
 if(dagmc)
   target_compile_definitions(libopenmc PRIVATE DAGMC)

[kkiesling]

Presumably, the NO_SYSTEM_ENVIRONMENT_PATH option in find_package should be skipping the install that already exists from the previous openmc build, but that does not seem to be the case when I build. I have removed the install path from my PATH and LD_LIBRARY_PATH even. And I have tried all the other NO_* options for find_package to attempt to exclude that path. I am unsure why it continues to be found. There doesn't seem to be any way to exclude a specific path either (then it would be easy to just exclude CMAKE_INSTALL_PREFIX).

[gridley]

This solution is maybe a little dirty, but we could set up all the submodules as forks under github.com/openmc-dev, then just tweak the namespaces of the modules held in OpenMC's forks, for instance using openmc::fmt in order to avoid collisions with the system's version.

There are some unmerged patches I have to use with xtensor anyway for the CUDA version, so maybe this would also carry that benefit going forward: the ability to apply our own tweaks on top of submodules as needed.

[paulromano]

@kkiesling Yeah, I found the behavior you're seeing a little strange too. To reproduce what you were seeing, I explicitly had to add my install directory to CMAKE_PREFIX_PATH. If you haven't already, you may want to try adding -DCMAKE_FIND_DEBUG_MODE=ON so that you can see where it's searching for packages (the order may be indicative of why it's picking up the installed version).

@gridley I understand the motivation to tweak things for GPU porting purposes, but I'd rather not be in the game of managing our own versions of these dependencies. Each time we want to update our own version, there will be potential conflicts to be resolved.

[kkiesling]

my cmake line included -DCMAKE_INSTALL_PREFIX (not CMAKE_PREFIX_PATH, which doesn't appear to be in my CMakeCache.txt even).

[paulromano]

Closed by #2006