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

Add installation testing option #3034

Merged
merged 2 commits into from
May 15, 2020
Merged

Conversation

jjwilke
Copy link

@jjwilke jjwilke commented May 13, 2020

Allows easier testing Jenkins of Kokkos installations.

  • Sets all tests to link to alias Kokkos::kokkos
  • Calls find_package(Kokkos) instead of building Kokkos when option is set

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.

I tested this with a CUDA build.

-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
-DCMAKE_CXX_COMPILER=$WORKSPACE/bin/nvcc_wrapper \
-DCMAKE_CXX_FLAGS=-Werror \
Copy link
Author

Choose a reason for hiding this comment

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

I guess this is okay (it works). We could have also done -DKokkos_ROOT=...

Copy link
Contributor

Choose a reason for hiding this comment

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

The enabled tests depend on the backends enabled here, don't they? Hence, these should match.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

I think I'm fine with this once the CI configuration is fixed.
It might also be a good idea to hide this some more from the user by making it an internal variable.

CMakeLists.txt Outdated
@@ -147,6 +147,10 @@ INCLUDE(${KOKKOS_SRC_PATH}/cmake/kokkos_tribits.cmake)
# Check the environment and set certain variables
# to allow platform-specific checks
INCLUDE(${KOKKOS_SRC_PATH}/cmake/kokkos_check_env.cmake)

#
KOKKOS_OPTION(INSTALL_TESTING OFF BOOL "Whether to build tests and examples against installation")
Copy link
Contributor

Choose a reason for hiding this comment

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

What this is doing effectively is to disable compiling the library and just run the tests.
Hence, I would rather name it DISABLE_COMPILING or similar and make sure Kokkos_ENABLE_TESTS=ON.

@crtrott
Copy link
Member

crtrott commented May 13, 2020

Ok it doesn't look like this fully works. The config is doing something in between where it doesn't get the enabled spaces from the installed version. It would work if I would just give matching options for the second configure but do we want that?

@jjwilke
Copy link
Author

jjwilke commented May 13, 2020

@masterleinad Yeah, we should probaly just make it an internal variable.
@crtrott We would need to make the configurations match. We could make this more sophisticated (and also test kokkos_check), but that's probably overkill for now.

@crtrott
Copy link
Member

crtrott commented May 14, 2020

So I tried something where I stuck the check just before KOKKOS_SETUP_BUILD_ENVIRONMENT in the main CMakeLists:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4ec1bac..24fde2a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -151,6 +151,17 @@ INCLUDE(${KOKKOS_SRC_PATH}/cmake/kokkos_check_env.cmake)
 #
 KOKKOS_OPTION(INSTALL_TESTING OFF BOOL "Whether to build tests and examples against installation")

+# Short circuit all the rest of the processing if we just build tests against installed Kokkos
+IF(KOKKOS_INSTALL_TESTING)
+  SET(KOKKOS_ENABLE_TESTS ON)
+  KOKKOS_PACKAGE_DECL()
+  FIND_PACKAGE(Kokkos REQUIRED)
+  add_subdirectory(core)
+  add_subdirectory(containers)
+  add_subdirectory(algorithms)
+ELSE()
+
+
 # The build environment setup goes in the following steps
 # 1) Check all the enable options. This includes checking Kokkos_DEVICES
 # 2) Check the compiler ID (type and version)
@@ -279,3 +290,6 @@ IF (HAS_PARENT)
     SET_PROPERTY(GLOBAL PROPERTY Kokkos_ENABLE_${DEV} ON)
   ENDFOREACH()
 ENDIF()
+
+# End for not INSTALL_TESTING
+ENDIF()

And that seems to work properly. You only need to set compiler and Kokkos_INSTALL_TESTING and everything else comes from the installed Kokkos instead of first trying to do a configuration of Kokkos and overwriting those settings via a find package.

@masterleinad
Copy link
Contributor

masterleinad commented May 14, 2020

Creating a separate directory with

cmake_minimum_required(VERSION 3.10)
project(KokkosTestInstall CXX)

find_package(Kokkos 3.1 REQUIRED QUIET)

SET(PACKAGE_NAME KOKKOS)
SET(KOKKOS_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/..)
SET(KOKKOS_ENABLE_TESTS ON)
SET(KOKKOS_INSTALL_TESTING ON)

include(${PROJECT_SOURCE_DIR}/../cmake/fake_tribits.cmake)
include(${PROJECT_SOURCE_DIR}/../cmake/kokkos_tribits.cmake)

ADD_SUBDIRECTORY(${PROJECT_SOURCE_DIR}/../core/ core)
ADD_SUBDIRECTORY(${PROJECT_SOURCE_DIR}/../algorithms algorithms)
ADD_SUBDIRECTORY(${PROJECT_SOURCE_DIR}/../containers containers)

would an alternative to be sure to not mess with the configuration of the found package.

@jjwilke
Copy link
Author

jjwilke commented May 14, 2020

I pushed a refactor that combines all the recent feedback (I think).

  • It receives the configuration from the imported package
  • It skips any local configuration
  • It uses an internal cache variable for the configuration

I kept the name Kokkos_INSTALL_TESTING. This seems a more accurate description than DISABLE_COMPILING?

@crtrott
Copy link
Member

crtrott commented May 14, 2020

Yeah this looks good. Let me push again the Jenkins config file change I added which got nuked by your force push :-)

@masterleinad
Copy link
Contributor

I kept the name Kokkos_INSTALL_TESTING. This seems a more accurate description than DISABLE_COMPILING?

Now, Kokkos_INSTALL_TESTING implies Kokkos_ENABLE_TESTS which it didn't before IIRC. The name is fine with me then.
I'm a bit torn between handling all the configuration stuff in CMakeLists.txt and having a separate project (which is probably closer to what a user would have). Anyway, if both of you are in favor of doing it inline, that's OK.

@crtrott
Copy link
Member

crtrott commented May 14, 2020

I think this is fine. I am also happy with the Kokkos_INSTALL_TESTING directly implying Kokkos_ENABLE_TESTS. That INSTALL_TESTING option anyway implies doing something completely different.

@crtrott
Copy link
Member

crtrott commented May 14, 2020

And the install test worked in Jenkins. Great!

@crtrott crtrott merged commit 67a347c into kokkos:develop May 15, 2020
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.

None yet

3 participants