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

Fix CMake scripts for installation and SYCL backend #40

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

pzehner
Copy link
Collaborator

@pzehner pzehner commented Jan 30, 2024

This PR aims to fix two aspects of the CMake setup:

  1. Installation of the library with cmake --install/make install: this PR uses standard CMake practices to make the project configured with CMAKE_INSTALL_PREFIX=/path/to/install findable by a project configured with CMAKE_PREFIX_PATH=/path/to/install; and
  2. Installation of the library with a SYCL backend: this PR adds missing CMake commands for this backend.

@pzehner pzehner added the enhancement New feature or request label Jan 30, 2024
@pzehner pzehner self-assigned this Jan 30, 2024
@yasahi-hpc
Copy link
Collaborator

@pzehner Maybe we also need to add install_test/bin/install_sycl.sh and add install test in workflow. But these stuffs will eventually be integrated differently in your new CI, so it may not make much sense.

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.

So kokkosFFT is the official name ?

I have a few comments about the build scripts, but not from this pr: you should not pass Kokkos Cmake options except for building Kokkos. When using an external and already installed kokkos, you should check the configuration from the actual install.

@yasahi-hpc
Copy link
Collaborator

yasahi-hpc commented Jan 31, 2024

As for the name, I propose kokkos-fft for docs (including repo name) and KokkosFFT for the rest (source codes, CMake). This may be the common way in Kokkos related libraries.

The install procedure as well as install test for Intel would be updated in new PR for CI by @pzehner ?

@pzehner
Copy link
Collaborator Author

pzehner commented Jan 31, 2024

@yasahi-hpc yes, I didn't add an install_test/bin/install_sycl.sh script as this work is handled differently in #29.

About the naming convention, at least on CMake side, I followed the same approach of Kokkos, for which the repository name is kokkos (lower case), and the CMake name is Kokkos (Pascal case, or at least title case).

@yasahi-hpc
Copy link
Collaborator

@pzehner Sure. If you are fine, I will merge this.

@pzehner
Copy link
Collaborator Author

pzehner commented Jan 31, 2024

I'm OK for merging.

@yasahi-hpc yasahi-hpc merged commit 38115f3 into main Jan 31, 2024
7 checks passed
@yasahi-hpc yasahi-hpc deleted the feature/cmake_sycl branch January 31, 2024 10:24
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.

3 participants