KokkosKernels: Don't list include for non-existant 'batched' build dir (trilinos/Trilinos#11966) #1867
Conversation
trilinos/Trilinos#11966) See the embedded comment. This fixes the generated KokkosKernelsTargets.cmake file in the build dir when tests are not enabled. NOTE: This is a very non-conventional CMake build system. Your never see CMakeLists.txt files that are included with include(). These should always be pulled in using add_subdirectory(). If you want to just include() some CMake code, you call it `<SomeLogicalBaseName>.cmake` and then include() it.
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
ndellingwood
added
CompatibilityUpdate
TrilinosPatchMatch
|
This will be cherry-picked to the release-candidate-4.1.00 branch |
ndellingwood
added
the
AT: PRE-TEST INSPECTED
kokkos-devops-admin
removed
the
AT: PRE-TEST INSPECTED
|
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_CUDA11_CUDA11_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC930_Light_Tpls_GCC930_Tpls_CLANG13CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC1020
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC1020_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC1020
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_INTEL19
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG1001
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_A64FX_Tpls_ARMPL2110
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_A64FX_GCC1020
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_VEGA908_ROCM520
Jenkins Parameters
Using Repos:
Pull Request Author: bartlettroscoe |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_CUDA11_CUDA11_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC930_Light_Tpls_GCC930_Tpls_CLANG13CUDA10
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC1020
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC1020_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC1020
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_INTEL19
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG1001
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_A64FX_Tpls_ARMPL2110
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_A64FX_GCC1020
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_VEGA908_ROCM520
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ ndellingwood ]! |
|
Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge |
| KOKKOSKERNELS_INCLUDE_DIRECTORIES(REQUIRED_DURING_INSTALLATION_TESTING ${CMAKE_CURRENT_SOURCE_DIR}/batched) | ||
| IF(KokkosKernels_ENABLE_TESTS OR KokkosKernels_ENABLE_TESTS_AND_PERFSUITE) | ||
| # Adding unit-tests | ||
| KOKKOSKERNELS_INCLUDE_DIRECTORIES(${CMAKE_CURRENT_BINARY_DIR}/batched) |
There was a problem hiding this comment.
I apologize in advance for any misunderstanding on my part. Given that we append the correct include directories to KK_INCLUDE_DIRS above these lines and have this for-each loop in our top-level CMake:
FOREACH(DIR ${KK_INCLUDE_DIRS})
KOKKOSKERNELS_INCLUDE_DIRECTORIES(${DIR})
APPEND_GLOB(HEADERS ${DIR}/*.hpp)
ENDFOREACH()
I do not see a need to invoke KOKKOSKERNELS_INCLUDE_DIRECTORIES here.
Where are these includes required in the KokkosKernels build system? Does Trilinos build without them?
Why isn't this same change needed in blas/CMakeLists.txt?
Why don't we have similar KOKKOSKERNELS_INCLUDE_DIRECTORIES invocations with REQUIRED_DURING_INSTALLATION_TESTING in sparse/CMakeLists.txt?
There was a problem hiding this comment.
Really these include directories should be added in the CMakeLists.txt files for the tests themselves and NOT added to the exported include directories for the package that show up in the KokkosKernelsConfig.cmake file. Also, someone should consider refactoring this build system to not include CMakeLists.txt files. I have never seen that before and that is confusing. A CMakeLists.txt file is meant to be brought into a project using add_subdirectory().
I think some straightforward refactorings of this build system like the proper usage of CMakeLists.txt files might make it more standard and easier to understand and maintain.
There was a problem hiding this comment.
That may be true but I am still at a loss for where these include directories are currently required. No where in the code base do I see #include "dense/..." or #include "sparse/...". I also deleted these two KOKKOSKERNELS_INCLUDE_DIRECTORIES calls and found that the KK code base configures and builds without errors. Is KOKKOSKERNELS_INCLUDE_DIRECTORIES(REQUIRED_DURING_INSTALLATION_TESTING ${CMAKE_CURRENT_SOURCE_DIR}/batched) only required for KokkosKernelsTargets.cmake in Trilinos?
There was a problem hiding this comment.
That may be true but I am still at a loss for where these include directories are currently required.
If these lines can be deleted, even better. But to validate that, we should make this same change in the Trilinos copy of KokkosKernels and ensure that all of the KokkosKernels tests pass under Trilinos as well.
|
@e10harvey @bartlettroscoe thanks for raising these questions and the discussion, adding @lucbv so he is in the loop The desired next steps, either here or a potential follow up PR, are unclear to me. For now, does the following sequence sound acceptable?
If I'm interpreting the discussion correctly, it sounds like there are a couple follow ups to consider:
If I'm misunderstanding parts of the discussion, please correct me |
👍 |
…ncl-dir KokkosKernels: Don't list include for non-existant 'batched' build dir (trilinos/Trilinos#11966) (cherry picked from commit 6fdd658)
|
Cherry-picked to release-candidate-4.1.00, sha 7871bd2 |
This matches the commit bartlettroscoe@aee776e in Trilinos PR:
This is a clear defect that just happens to not cause a problem unless you try to use the KokkosKernelsTargets.cmake file generated for the build dir (see trilinos/Trilinos#11966 (comment)).
See the log message and embedded comment for more details.