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.

Sign up for GitHub

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

Jump to bottom

Add Spack test support for Kokkos #3753

Merged
merged 1 commit into from
Mar 1, 2021
Merged

Add Spack test support for Kokkos #3753

merged 1 commit into from
Mar 1, 2021

Conversation

janciesko
Copy link
Contributor

Adds Spack test support for Kokkos.

  • CMakeLists.txt is used to assemble test files, to generate CmakeLists files for tests, and to generate a run script.
  • Tests are defined in the 'test_list.def' file. The list can be extended to cover other tests.
  • Tests are built and run at "spack test run" time.

@jrmadsen
Copy link
Contributor

Yeah, I don't think this is the best approach here. I think we should generate a CTestTestfile.cmake. But i have some questions related to what the testing should actually cover. We could run our unit-test suite from Spack very easily.

@jrmadsen jrmadsen self-requested a review January 27, 2021 18:17
@jjwilke
Copy link

jjwilke commented Jan 27, 2021

I'm similarly confused. What are we hoping to enable with this PR? You can add a test phase to any Spack build.

@janciesko
Copy link
Contributor Author

janciesko commented Jan 27, 2021 •
edited

We enable spack test run with this PR. The corresponding spack recipe is here - note the changes towards the end. Recipe

@janciesko
Copy link
Contributor Author

Removed run shell script in favor of using Cmake test.

@janciesko janciesko requested a review from dalg24 January 30, 2021 04:13
@dalg24
Copy link
Member

dalg24 commented Feb 5, 2021

@jrmadsen @masterleinad please review

crtrott
crtrott approved these changes Feb 5, 2021
View reviewed changes
Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

Generally I am happy with this. The examples used seem appropriate for a smoke test (i.e. it tests containers and algorithms too.)

jrmadsen
jrmadsen approved these changes Feb 17, 2021
View reviewed changes
Copy link
Contributor

@jrmadsen jrmadsen left a comment

Choose a reason for hiding this comment

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

Looks good to me. Maybe add TIMEOUT, PROCESSORS, etc. but that's your call.

scripts/spack_test/CMakeLists.txt.in Show resolved Hide resolved
jrmadsen
jrmadsen suggested changes Feb 17, 2021
View reviewed changes
Copy link
Contributor

@jrmadsen jrmadsen left a comment

Choose a reason for hiding this comment

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

Remove that `CMAKE_CXX_COMPILER

scripts/spack_test/CMakeLists.txt.in Outdated

set(SRC_NAME_LIST "@SRC_NAME_LIST@")
set(BIN_NAME_LIST "@BIN_NAME_LIST@")
set(CMAKE_CXX_COMPILER ${Kokkos_CXX_COMPILER})
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait. This does absolutely nothing. At this point, the CXX in project already set the C++ compiler, you cannot override it locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved for now on Slack.

scripts/spack_test/CMakeLists.txt.in Outdated

set(SRC_NAME_LIST "@SRC_NAME_LIST@")
set(BIN_NAME_LIST "@BIN_NAME_LIST@")
set(CMAKE_CXX_COMPILER ${Kokkos_CXX_COMPILER})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
set(CMAKE_CXX_COMPILER ${Kokkos_CXX_COMPILER})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved for now on Slack.

@crtrott crtrott merged commit ecae5a3 into kokkos:develop Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masterleinad masterleinad masterleinad left review comments

@jrmadsen jrmadsen jrmadsen approved these changes

@crtrott crtrott crtrott approved these changes

@jjwilke jjwilke Awaiting requested review from jjwilke

@dalg24 dalg24 Awaiting requested review from dalg24

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@janciesko @jrmadsen @jjwilke @dalg24 @masterleinad @crtrott