Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding install capability #22

Merged
merged 15 commits into from
Jan 19, 2024
41 changes: 39 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ list(INSERT CMAKE_MODULE_PATH 0 "${CMAKE_SOURCE_DIR}/cmake")
# Options
option(BUILD_EXAMPLES "Build kokkos-fft examples" ON)
option(KokkosFFT_ENABLE_HOST_AND_DEVICE "Enable fft on both host and device" OFF)
option(KokkosFFT_ENABLE_INSTALL_TEST "Enable install test" ON)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think you need this flag. Install test should more likely run by CI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Modified accordingly


find_package(Kokkos CONFIG)
if(NOT kokkos_FOUND)
if(NOT Kokkos_FOUND)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your issue with the install test is here: when building your library if you do not find an external Kokkos you build one and you do not install it.
Install test should be performed with an already available Kokkos installation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true. With the modifications, local install test worked if Kokkos is already installed. If Kokkos is not installed, it would still fail

add_subdirectory(tpls/kokkos)
endif()
cedricchevalier19 marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -22,8 +23,44 @@ if(BUILD_TESTING)
endif()
endif()

# Set directories used for install
include(GNUInstallDirs)
set(INSTALL_INCLUDEDIR ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_INCLUDEDIR})
set(INSTALL_LIBDIR ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR})

add_subdirectory(common)
add_subdirectory(fft)
if(BUILD_EXAMPLES)
add_subdirectory(examples)
endif()
endif()

# Installation
install(
TARGETS common fft
EXPORT KokkosFFT-targets
)

install(
EXPORT KokkosFFT-targets
cedricchevalier19 marked this conversation as resolved.
Show resolved Hide resolved
DESTINATION ${INSTALL_INCLUDEDIR}
)

install(
DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/common/
DESTINATION ${INSTALL_INCLUDEDIR}
)

install(
DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/fft/
DESTINATION ${INSTALL_INCLUDEDIR}
)

configure_package_config_file(cmake/KokkosFFTConfig.cmake.in
${CMAKE_CURRENT_BINARY_DIR}/KokkosFFTConfig.cmake
INSTALL_DESTINATION ${INSTALL_INCLUDEDIR}
cedricchevalier19 marked this conversation as resolved.
Show resolved Hide resolved
)

install(
FILES ${CMAKE_CURRENT_BINARY_DIR}/KokkosFFTConfig.cmake
DESTINATION ${INSTALL_INCLUDEDIR}
)
cedricchevalier19 marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 8 additions & 0 deletions cmake/KokkosFFTConfig.cmake.in
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
@PACKAGE_INIT@

include(CMakeFindDependencyMacro)
find_dependency(Kokkos)

include(${CMAKE_CURRENT_LIST_DIR}/KokkosFFT-targets.cmake)

check_required_components(KokkosFFT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@PACKAGE_INIT@
include(CMakeFindDependencyMacro)
find_dependency(Kokkos)
include(${CMAKE_CURRENT_LIST_DIR}/KokkosFFT-targets.cmake)
check_required_components(KokkosFFT)
@PACKAGE_INIT@
# Set and Check must go first else macro might be redefined by find_dependency ...
set_and_check(KokkosFFT_TARGET_FILE "${PACKAGE_PREFIX_DIR}/@config_install_dir@/@[email protected]")
include(CMakeFindDependencyMacro)
find_dependency(Kokkos)
include(${KokkosFFT_TARGET_FILE})
check_required_components(KokkosFFT)

And define required variable in the CMakeLists.txt before calling configure_package_config_file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason,

set_and_check(KokkosFFT_TARGET_FILE "${PACKAGE_PREFIX_DIR}/@config_install_dir@/@[email protected]")

this line did not work appropriately. @config_install_dir@ and @KokkosFFT_EXPORT_TARGET@ are empty.

Thus, I did like this

set_and_check(KokkosFFT_TARGET_FILE
 "${PACKAGE_PREFIX_DIR}/include/KokkosFFT-targets.cmake")

I also added dependency to fft libraries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to define config_install_dir and KokkosFFT_EXPORT_TARGET variables in the CMakeLists.txt before calling configure_file.

I still think include is not a good idea to install *.cmake files.

Copy link
Collaborator Author

@yasahi-hpc yasahi-hpc Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cedricchevalier19 Firstly, thank you for the comments.

Sure, I will set the configure_install_dir and KokkosFFT_EXPORT_TARGET.
Yes, in the current configuration under <install_path> would look like this

<install_path>/
  |—include/
  |        |--CMakeLists.txt 
  |        |--FindFFTW.cmake
  |        |--KokkosFFT-targets.cmake
  |        |--src
  |        └--unit_test
  └─lib64
      └─cmake
          └─kokkos-fft
                 |--KokkosFFTConfig.cmake 
                 └--KokkosFFTConfigVersion.cmake

Should it be something like this?

<install_path>/
  |—include/    
  |       └-*hpp
  |        
  └─lib64
      └─cmake
          └─kokkos-fft
                 |--FindFFTW.cmake
                 |--KokkosFFT-targets.cmake
                 |--KokkosFFTConfig.cmake 
                 └--KokkosFFTConfigVersion.cmake

I feel that CMakeLists.txt and unit_test under include are unnecessary and headers under src should be directly placed under include

5 changes: 4 additions & 1 deletion common/src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,7 @@ elseif(Kokkos_ENABLE_SERIAL)
endif()

target_compile_features(common INTERFACE cxx_std_17)
target_include_directories(common INTERFACE ${CMAKE_CURRENT_SOURCE_DIR})
target_include_directories(common INTERFACE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
$<INSTALL_INTERFACE:${INSTALL_INCLUDEDIR}>
)
5 changes: 4 additions & 1 deletion fft/src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,8 @@ target_link_libraries(fft
Kokkos::kokkos
)

target_include_directories(fft INTERFACE ${CMAKE_CURRENT_SOURCE_DIR})
target_include_directories(fft INTERFACE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
$<INSTALL_INTERFACE:${INSTALL_INCLUDEDIR}>
)
add_library(Kokkos::fft ALIAS fft)
cedricchevalier19 marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 8 additions & 0 deletions install_test/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
cmake_minimum_required(VERSION 3.21)
project(install-test LANGUAGES CXX)

find_package(KokkosFFT CONFIG REQUIRED)
cedricchevalier19 marked this conversation as resolved.
Show resolved Hide resolved

add_executable(install-test main.cpp)
target_compile_features(install-test PUBLIC cxx_std_17)
target_link_libraries(install-test PUBLIC Kokkos::fft)
44 changes: 44 additions & 0 deletions install_test/main.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#include <Kokkos_Core.hpp>
#include <Kokkos_Complex.hpp>
#include <Kokkos_Random.hpp>
#include <KokkosFFT.hpp>

using execution_space = Kokkos::DefaultExecutionSpace;
template <typename T>
using View1D = Kokkos::View<T*, execution_space>;

int main(int argc, char* argv[]) {
Kokkos::initialize(argc, argv);
{
constexpr int n0 = 128;
const Kokkos::complex<double> I(1.0, 1.0);

// 1D C2C FFT (Forward and Backward)
View1D<Kokkos::complex<double> > xc2c("xc2c", n0);
View1D<Kokkos::complex<double> > xc2c_hat("xc2c_hat", n0);
View1D<Kokkos::complex<double> > xc2c_inv("xc2c_inv", n0);

Kokkos::Random_XorShift64_Pool<> random_pool(12345);
Kokkos::fill_random(xc2c, random_pool, I);

KokkosFFT::fft(execution_space(), xc2c, xc2c_hat);
KokkosFFT::ifft(execution_space(), xc2c_hat, xc2c_inv);

// 1D R2C FFT
View1D<double> xr2c("xr2c", n0);
View1D<Kokkos::complex<double> > xr2c_hat("xr2c_hat", n0 / 2 + 1);
Kokkos::fill_random(xr2c, random_pool, 1);

KokkosFFT::rfft(execution_space(), xr2c, xr2c_hat);

// 1D C2R FFT
View1D<Kokkos::complex<double> > xc2r("xr2c_hat", n0 / 2 + 1);
View1D<double> xc2r_hat("xc2r", n0);
Kokkos::fill_random(xc2r, random_pool, I);

KokkosFFT::irfft(execution_space(), xc2r, xc2r_hat);
}
Kokkos::finalize();

return 0;
}