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

Update CMakeLists.txt for tests #12

Closed
wants to merge 11 commits into from

Conversation

cedricchevalier19
Copy link
Member

  • MPI is an explicit dependency for tests
  • Use CMake MPI variables to launch tests

@cwpearson
Copy link
Collaborator

Maybe we should get https://github.com/cwpearson/kokkos-comm/pull/14 merged and see how it impacts this one

@cedricchevalier19
Copy link
Member Author

@cwpearson, I will update it after #14 is merged.

perf_tests/CMakeLists.txt Outdated Show resolved Hide resolved
@cedricchevalier19 cedricchevalier19 force-pushed the cmake branch 2 times, most recently from a678909 to ba9a691 Compare April 9, 2024 15:51
@cedricchevalier19 cedricchevalier19 marked this pull request as ready for review April 9, 2024 16:14
@cedricchevalier19 cedricchevalier19 self-assigned this Apr 10, 2024
@cedricchevalier19 cedricchevalier19 added the enhancement New feature or request label Apr 10, 2024
@cedricchevalier19 cedricchevalier19 marked this pull request as draft April 10, 2024 16:37
@cedricchevalier19
Copy link
Member Author

Waiting for #26

@cedricchevalier19 cedricchevalier19 force-pushed the cmake branch 2 times, most recently from 8adc37b to c840c1b Compare April 11, 2024 13:21
@cedricchevalier19 cedricchevalier19 marked this pull request as ready for review April 11, 2024 13:51
perf_tests/CMakeLists.txt Outdated Show resolved Hide resolved
unit_tests/CMakeLists.txt Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
cmake/KokkosComm_configuration.hpp.in Outdated Show resolved Hide resolved
cwpearson
cwpearson previously approved these changes Apr 12, 2024
.editorconfig Outdated Show resolved Hide resolved
@@ -7,13 +7,16 @@ enable_testing()
# find Kokkos Comm to do a standalone build
if (NOT TARGET KokkosComm::KokkosComm)
find_package(KokkosComm REQUIRED)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete these comments

Copy link
Member Author

Choose a reason for hiding this comment

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

If the comments are the ones of the following lines, sorry but I do not agree.
(please consider that I am not a native English speaker).

To me, not using find_package(MPI) is already wrong practice as we should explicitly require what we are directly using. Relying on transitive dependencies is evil (I've spent my share of time dealing with this nightmare when updating software packages at Spack level). Most of the time tests are also examples for end-user, so I want to give them a correct template.
Despite this, for these tests, I have agreed to not do find_package(MPI) in order to test that the find_dependency(MPI) is correctly set.

By the way, I should have added find_package(Kokkos)too.

If test code does not reference anymore any MPI functions directly, then I'll happily remove everything.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How would you feel about adding a comment to the top of the file saying something like "These tests require Kokkos and MPI, but we're not using find_package(...) here because we want to make sure KokkosComm's find_dependency is working"?

@@ -7,13 +7,16 @@ enable_testing()
# find Kokkos Comm to do a standalone build
if (NOT TARGET KokkosComm::KokkosComm)
find_package(KokkosComm REQUIRED)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove comments

@cedricchevalier19
Copy link
Member Author

Have to rebase this branch.

Pass flags using an empty target rather than using a CMake function that is easy to misuse.

The main difference is now configuration choices are permanent: it is not possible to use mdspan if configuration has not asked for it.
Ensure that MPIEXEC macro are available, if when compiled as standalone.
Set KokkosComm definitions in this generated header file

KokkosComm Version informations are also located there.
Applying Carl's suggestion.
@cedricchevalier19
Copy link
Member Author

Superseded by #122 .

dssgabriel added a commit to dssgabriel/kokkos-comm that referenced this pull request Sep 19, 2024
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.

4 participants