search

dftlibs / xcfun

XCFun: A library of exchange-correlation functionals with arbitrary-order derivatives
https://dftlibs.org/xcfun/
Mozilla Public License 2.0
58 stars 32 forks source link

Refactor Cmake #59

Closed robertodr closed 6 years ago

robertodr commented 6 years ago

This is ready for review. I have mostly moved stuff around, trying to localize the scope of CMake variables as much as possible. I haven't written any documentation. I believe that can be fixed at a later stage since the number of files changed is already quite large. Once this is reviewed and approved, I can squash some of the commits to provide a more concise history. Merging will fix #34. This is a list of what I have done:

Possibly BREAKING change

Instead of compiling the Fortran module into a library and shipping the (compiler-dependent) .mod file, the Fortran source file is always configured and copied into the binary tree (install rules are also present and the file is always installed alongside the headers). The host program that needs the Fortran bindings will have to compile this file.

Installation folder layout

/home/roberto/Software/xcfun/
├── include
│   └── XCFun
│       ├── config.hpp
│       ├── densvars.hpp
│       ├── functional.hpp
│       ├── specmath.hpp
│       ├── XCFunExport.h
│       ├── xcfun.F90
│       ├── xcfun.h
│       └── xcint.hpp
├── lib64
│   ├── libxcfun.a
│   ├── libxcfun.so -> libxcfun.so.2
│   └── libxcfun.so.2
└── share
    └── cmake
        └── XCFun
            ├── XCFunConfig.cmake
            ├── XCFunConfigVersion.cmake
            ├── XCFunTargets-shared.cmake
            ├── XCFunTargets-shared-debug.cmake
            ├── XCFunTargets-static.cmake
            └── XCFunTargets-static-debug.cmake

Use of the CMake config module

This idiotic C++ example can be configured doing:

cmake -H. -Bbuild -DXCFun_DIR=<install_prefix>/share/cmake/XCFun

and CMake will find XCFun on the system.

cmake_minimum_required(VERSION 3.6 FATAL_ERROR)

project(UseXCFun LANGUAGES CXX)

set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_EXTENSIONS OFF)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

include(GNUInstallDirs)

set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR})
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR})
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_BINDIR})

find_package(XCFun CONFIG REQUIRED)
get_property(_loc TARGET XCFun::xcfun PROPERTY LOCATION)
message(STATUS "Found XCFun: ${_loc} (found version ${XCFun_VERSION})")

add_executable(use_xcfun use_xcfun.cpp)
target_link_libraries(use_xcfun
  PUBLIC
  XCFun::xcfun
  )

set_target_properties(use_xcfun
  PROPERTIES
    MACOSX_RPATH ON
    SKIP_BUILD_RPATH OFF
    BUILD_WITH_INSTALL_RPATH OFF
    INSTALL_RPATH "$ORIGIN/../${CMAKE_INSTALL_LIBDIR}"
    INSTALL_RPATH_USE_LINK_PATH ON
  )

with the source file use_xcfun.cpp:

#include <cstdlib>
#include <iostream>

#ifdef USING_XCFun
#include "XCFun/xcfun.h"
#endif

int main() {
#ifdef USING_XCFun
  std::cout << "We have XCFun" << std::endl;
  std::cout << xcfun_splash() << std::endl;
#endif
  return EXIT_SUCCESS;
}

I assume the same works for Fortran, but I haven't tried.

bast commented 6 years ago

Awesome work! Thank you!

bast commented 6 years ago

I browsed through the changes - looks very good. Looking forward to having this in.

codecov-io commented 6 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@885b23a). Click here to learn what that means. The diff coverage is 50.81%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #59   +/-   ##
=========================================
  Coverage          ?   42.29%           
=========================================
  Files             ?       78           
  Lines             ?     1785           
  Branches          ?        0           
=========================================
  Hits              ?      755           
  Misses            ?     1030           
  Partials          ?        0
Impacted Files Coverage Δ
src/functionals/b97c.hpp 0% <ø> (ø)
src/functionals/b97xc.hpp 0% <ø> (ø)
src/functionals/pbeintx.cpp 0% <0%> (ø)
src/functionals/tw.cpp 0% <0%> (ø)
src/functionals/pbex.hpp 70% <0%> (ø)
src/functionals/pw86x.cpp 0% <0%> (ø)
src/functionals/revtpssx_eps.hpp 0% <0%> (ø)
src/functionals/pbesolx.cpp 0% <0%> (ø)
src/functionals/optx.cpp 0% <0%> (ø)
src/functionals/m06x2c.cpp 0% <0%> (ø)
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 885b23a...45e716b. Read the comment docs.

robertodr commented 6 years ago

The issue with the option command wrappers is an Autocmake issue and will be addressed separately.

bast commented 6 years ago

Is this ready to go from your perspective Roberto?

bast commented 6 years ago

Because it looks good and ready from my perspective. Big gold star for all the work!

robertodr commented 6 years ago

Yes, it is ready. But it's quite large and I'd like to have another set of eyes poking around.

bast commented 6 years ago

OK but I am sure it's better than before and since it is gigantic, not easy to review but I will scroll through things. I prefer smaller PRs :-)

robertodr commented 6 years ago

Yeah the PR snowballed. But I couldn't see a way of chunkyfying it better...

bast commented 6 years ago

It's awesome work. Once Travis is happy, I will merge :-) Thank you!