Open wavefunction91 opened 9 months ago
Yes, like I tried to imply, the CMake (like everything else) was quickly hacked together. Some of these will be separate issues
Optim is https://github.com/kthohr/optim. However, since it doesn't seem to work nicely as a header-only library (you need to configure it to get out the header-only version!), I ended up replacing that dependency with a simple conjugate gradient optimizer...
The library will be header-only; the CMake is just for building the tests / sample executables.
This is a common misconception - even header only libraries need a proper build system. Dependency management, flag manipulation (e.g. enforcing a C++ standard, conditional compliation logic), installation logic, etc.
Failure to include one will drastically limit its ability to be integrated downstream. You cannot expect that downstream users will provide this.
Should have been closed by #6
@susilehtola @loriab You should probably add some discovery / subproject tests ala https://github.com/wavefunction91/GauXC/tree/master/tests/cmake. Can't tell you how many times I've accidentally broken install/subproject logic and CI has saved me from merging it into trunk. That can be a separate issue if you prefer, otherwise what @loriab added lgtm for this to close
A few build system comments (because what's there is not subprojectable nor selfcontained). I'm sure most of these were on your list, but just so you have them for when you want to mess with the build system:
CMAKE_<LANG>_FLAGS
(or really anyCMAKE_XYZ
flags, with a few exceptions as long as you put them back). These are global variables, so it break containment and will make debugging later much more difficult. The only place I see you using this right now isOpenMP
, see next comments.TARGETs
over manual including/linking various flags and libraries. The target you want here isOpenMP::OpenMP_CXX
, that will include all compiler flags and libraries required to use OpenMP on your platform and it will allow for contained linkage (e.g.target_link_libraries(my_lib PUBLIC OpenMP::OpenMP_CXX)
will only affect my_lib and libraries that link to it. It will also make packaging much easier)LAPACK
- link_libraries is global, always prefer target_link_libraries andTARGETs
. Unfortunately, BLAS/LAPACK targets weren't added until a very recent CMake. However, I maintain a set of very robust CMake modules to handle BLAS/LAPACK discovery (they're also more robust than the Kitware Find{BLAS,LAPACK}.cmake). The repo is here and an example usage is here. You don't actually need to changefind_package(LAPACK)
, the same semantics apply.TARGETs
:disappointed: , but it could be made to pretty easily. I'll open a ticket with Kitware. Again, prefertarget_linclude_directories
overinclude_directories
as the latter is global.optim
? It should be packaged if its not already. If it's not, let me know, and I might be able to whip something up quickly