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
Merged

Adding install capability #22

merged 15 commits into from
Jan 19, 2024

Conversation

yasahi-hpc
Copy link
Collaborator

@yasahi-hpc yasahi-hpc commented Jan 9, 2024

In this PR, I am trying to add install capability for KokkosFFT.
The test code (install_test/main.cpp) using the KokkosFFT as installed library fails since Kokkos not found is raised in KokkosFFTConfig.cmake.
Once succeeded, I need to add a GitHub action for this test.

Installation of KokkosFFT for SKX

mkdir build && cd build
cmake -DBUILD_TESTING=OFF -DCMAKE_CXX_COMPILER=icpx -DCMAKE_BUILD_TYPE=RelWithDebInfo -DKokkos_ENABLE_OPENMP=ON -DKokkos_ARCH_SKX=ON -DCMAKE_INSTALL_PREFIX=${KokkosFFT_Install_dir}/install ..
make install

Build testing of test code (configure fails)

export KokkosFFT_DIR=${KokkosFFT_Install_dir}/install/include
cd ..
mkdir -p install_test/build && cd install_test/build
cmake -DCMAKE_CXX_COMPILER=icpx -DCMAKE_BUILD_TYPE=Release -DKokkos_ENABLE_OPENMP=ON -DKokkos_ARCH_SKX=ON ..

@yasahi-hpc yasahi-hpc requested review from jbigot and cedricchevalier19 and removed request for jbigot and cedricchevalier19 January 9, 2024 12:25
@yasahi-hpc
Copy link
Collaborator Author

I guess I made some mistakes, but Kokkos_FOUND is set False when installed. Thus, KokkosFFT_Found also becomes False. I would appreciate it if some of you could comment on this PR. @jbigot @cedricchevalier19

Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

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

Some minor issues to solve. I will be happy to look again at these aspects.

CMakeLists.txt Outdated

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

CMakeLists.txt Outdated
@@ -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

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Comment on lines 1 to 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

CMakeLists.txt Outdated Show resolved Hide resolved
fft/src/CMakeLists.txt Outdated Show resolved Hide resolved
install_test/CMakeLists.txt Outdated Show resolved Hide resolved
@yasahi-hpc
Copy link
Collaborator Author

@cedricchevalier19 Thanks a lot for your comment! I will give it a try to fix the issues.

@yasahi-hpc
Copy link
Collaborator Author

Some minor issues to solve. I will be happy to look again at these aspects.

@cedricchevalier19 Hello, re-reviews are highly appreciated. If it is OK, I will implement CI for installation

@yasahi-hpc
Copy link
Collaborator Author

@cedricchevalier19 Firstly, I would like to add CIs. After that, I will update CMake based on your comments.

@yasahi-hpc yasahi-hpc changed the title [WIP] Adding install capability Adding install capability Jan 19, 2024
@yasahi-hpc
Copy link
Collaborator Author

@cedricchevalier19 Since most of issues are fixed, I will merge this PR. Any comments or suggestions are still welcome. Thank you again for your help.

@yasahi-hpc yasahi-hpc merged commit 231a82e into main Jan 19, 2024
5 checks passed
@yasahi-hpc yasahi-hpc deleted the cmake-install branch January 19, 2024 18:34
@yasahi-hpc yasahi-hpc mentioned this pull request Jan 19, 2024
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants