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

Get cmake --build --target install working #14

Merged
merged 20 commits into from
Apr 5, 2024
Merged

Get cmake --build --target install working #14

merged 20 commits into from
Apr 5, 2024

Conversation

cwpearson
Copy link
Collaborator

@cwpearson cwpearson commented Mar 28, 2024

  • Install the header files and CMake configuration files.
  • Add a CI check that builds unit tests and perf tests against the install
  • Treat unit_tests and perf_tests as separate CMake projects to facilitate install testing.
  • There are probably many problems with this that I don't know enough about CMake to foresee

original below

  • Install the header files and CMake configuration files.
  • Add a CI check that builds unit tests and perf tests against the install
  • Add a CMake configuration -DKokkosComm_INSTALL_TESTING=ON that uses find_package to find KokkosComm and then builds unit tests and perf tests against that
  • There are probably many problems with this that I don't know enough about CMake to foresee

* Add a CI check for installation
* Add KokosComm_INSTALL_TESTING option to test an install
* Flatten include structure
* Remove some unused testing code in CMake
* Add KokkosComm::KokkosComm target alias
@cwpearson cwpearson marked this pull request as ready for review March 28, 2024 17:55
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.

It's a great idea, but it can be implemented in a simpler manner I think.

You can make the tests an independent project that contains a

if (NOT TARGET KokkosComm::KokkosComm)
  find_package(KokkosComm REQUIRED)
endif()

This way, we do not need special option and we keep the main CMakeLists.txt clean.

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.

I am a little skeptical about using mpicxx as a compiler.
MPI does not require it and it can make things strange mixing with gpu compilers.

However, this change can be done in a separate PR if you prefer to ensure all part of the repository are updated.

@cwpearson
Copy link
Collaborator Author

I am a little skeptical about using mpicxx as a compiler. MPI does not require it and it can make things strange mixing with gpu compilers.

However, this change can be done in a separate PR if you prefer to ensure all part of the repository are updated.

I agree, let's tackle this in another PR. I created an issue: https://github.com/cwpearson/kokkos-comm/issues/17

@cwpearson cwpearson merged commit 141b453 into develop Apr 5, 2024
9 checks passed
@cwpearson cwpearson deleted the install branch April 5, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants