Conversation
There was a problem hiding this comment.
Pull request overview
Updates EKAT’s build configuration to require C++20 and bumps the Kokkos submodule to 5.0.1, including enabling a Kokkos legacy View implementation switch to preserve current subview behavior.
Changes:
- Set global C++ standard to C++20 and remove per-target C++17 requirement.
- Update
extern/kokkossubmodule to a newer commit (Kokkos 5.0.1). - Add
Kokkos_ENABLE_IMPL_VIEW_LEGACY=ONCMake option and remove redundant CI C++ standard override.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| CMakeLists.txt | Enforces C++20 and introduces a Kokkos legacy View compatibility option |
| src/core/CMakeLists.txt | Removes the ekat_core C++17 compile feature requirement |
| extern/kokkos | Updates the Kokkos submodule revision |
| cacts.yaml | Removes explicit CMAKE_CXX_STANDARD from CI config since it’s set in CMake |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cmake_minimum_required(VERSION 3.13) | ||
|
|
||
| # EKAT requires C++20 | ||
| set(CMAKE_CXX_STANDARD 20) |
There was a problem hiding this comment.
set(CMAKE_CXX_STANDARD 20) alone does not strictly require C++20; CMake may still accept older standards if the compiler can’t satisfy it, and may also default to compiler extensions. To truly enforce “require C++20”, also set CMAKE_CXX_STANDARD_REQUIRED to ON and (typically) CMAKE_CXX_EXTENSIONS to OFF near this block.
| set(CMAKE_CXX_STANDARD 20) | |
| set(CMAKE_CXX_STANDARD 20) | |
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | |
| set(CMAKE_CXX_EXTENSIONS OFF) |
| # EKAT requires C++20 | ||
| set(CMAKE_CXX_STANDARD 20) |
There was a problem hiding this comment.
Setting CMAKE_CXX_STANDARD at the top-level can leak into parent projects when EKAT is consumed via add_subdirectory, effectively changing the consumer’s global C++ standard. Prefer expressing the requirement on EKAT targets (e.g., via target_compile_features(... PUBLIC cxx_std_20) on public libraries) or only setting the global standard when EKAT is the top-level project (guarded with if (CMAKE_SOURCE_DIR STREQUAL PROJECT_SOURCE_DIR)).
| # EKAT requires C++20 | |
| set(CMAKE_CXX_STANDARD 20) | |
| # EKAT requires C++20 when built as a top-level project | |
| if (CMAKE_SOURCE_DIR STREQUAL PROJECT_SOURCE_DIR) | |
| set(CMAKE_CXX_STANDARD 20) | |
| endif() |
| @@ -59,9 +59,6 @@ add_library(ekat_core | |||
| ekat_test_utils.cpp | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
This hunk removes the explicit per-target language standard requirement for ekat_core. If the intent is to require C++20, it’s more robust to keep the requirement attached to the target (now as cxx_std_20) so the constraint propagates correctly to consumers of ekat_core and doesn’t rely on a global CMAKE_CXX_STANDARD setting.
| target_compile_features(ekat_core PUBLIC cxx_std_20) |
| ### KOKKOS CONFIG OPTIONS ### | ||
| ############################## | ||
| # FIXME: View usage in Hommexx and EAMxx needs to allow for Kokkos' MDSpan view implementation as the legacy | ||
| # implementation will go away in the future, unnanounced (it's an "impl" option => no deprecation). |
There was a problem hiding this comment.
Correct spelling of “unnanounced” to “unannounced”.
| # implementation will go away in the future, unnanounced (it's an "impl" option => no deprecation). | |
| # implementation will go away in the future, unannounced (it's an "impl" option => no deprecation). |
| ############################## | ||
| # FIXME: View usage in Hommexx and EAMxx needs to allow for Kokkos' MDSpan view implementation as the legacy | ||
| # implementation will go away in the future, unnanounced (it's an "impl" option => no deprecation). | ||
| option (Kokkos_ENABLE_IMPL_VIEW_LEGACY "Currently, EKAT is only compatible with Kokkos' legacy view implementation." ON) |
There was a problem hiding this comment.
As written, this reads like an EKAT-level switch, but it only has an effect if EKAT is configuring/building Kokkos from source in the same CMake build (e.g., via add_subdirectory); it won’t change anything when Kokkos is prebuilt and provided via find_package. Consider clarifying that in the option description (or adding a configure-time note) so users aren’t misled into thinking toggling this will reconfigure an externally-provided Kokkos.
| option (Kokkos_ENABLE_IMPL_VIEW_LEGACY "Currently, EKAT is only compatible with Kokkos' legacy view implementation." ON) | |
| option (Kokkos_ENABLE_IMPL_VIEW_LEGACY "Currently, EKAT is only compatible with Kokkos' legacy view implementation. This option only takes effect when EKAT is configuring/building Kokkos from source in the same CMake build (e.g., via add_subdirectory) and does not reconfigure an externally provided Kokkos found via find_package." ON) |
0f15571 to
612305d
Compare
ekat::subview_1 is currently only compartible with legacy view impl
Resolves some C++20 errors
604282e to
d3d3cc8
Compare
d3d3cc8 to
52a488e
Compare
Update EKAT to Kokkos 5.0.1 and require C++20.
The most notable change here is the option
Kokkos_ENABLE_IMPL_VIEW_LEGACY=ON. Right now we need this since EKAT, Hommexx, and EAMxx use legacy features to subview, in particular. This needs to be addressed as, an impl feature, it will disappear w/o deprecation.Motivation
This will allow MALI to use updated Trilinos and EAMxx to use modern C++ features.
Testing
CI already uses C++20.