Some touchups to reflect that C++20 is now required for Axom - #1957
Conversation
c7ba710 to
59b0a74
Compare
59b0a74 to
939077f
Compare
| * CMake with a minimum required version of 3.14 for CPU-only and CUDA builds, | ||
| * C++ compiler with C++20 support at a minimum | ||
| * CMake with a minimum required version of 3.14 for CPU-only builds, | ||
| a minimum version of 3.18 when building with CUDA support, |
There was a problem hiding this comment.
I see the logic in the top-level CMakeLists.txt file for CUDA builds requiring CMake 3.18+. However, this comment seems to conflict with that: https://github.com/llnl/axom/blob/develop/src/CMakeLists.txt#L13
Is this still an issue; i.e., CUDA build is broken depending on which CMake version is used?
Also, should the choice of CMake min version be done the same way for all cases; i.e., using cmake_minimum_required. We have some comments about not using that. Seems confusing to me how we handle different cases.
There was a problem hiding this comment.
That's fair -- I think that the following two statements are true:
(a) the CMake version for CUDA needs to be at least 3.18,
(b) we need to set the cmake_minimum_required to 3.14 at the top of src/CMakeLists.txt for CUDA builds. The cmake_minimum_version function affects the CMake policies and their default values.
Presumably, this will be improved once llnl/blt#769 is merged (?)
@white238 -- should this docs change be rolled back or clarified? Alternatively, should we add a clarifying note to https://github.com/llnl/axom/blob/develop/src/CMakeLists.txt#L13 to reflect the actual CMake requirement for CUDA as opposed to the listed cmake_minimum_version
(Also, please let me know if I got this wrong)
There was a problem hiding this comment.
CMake 3.18 + CUDA + c++20 actually is a failure due to CMake not knowing that CUDA supports C++20 that early in CMake. It was set to this because many moons ago we noticed that it was changing cmake policies based on the cmake_minimum_required and this caused failures. We should reevaluate this at some point.
There was a problem hiding this comment.
Thanks. Should I add a note here or in src/CMakeLists.txt and/or roll back the changes in this paragraph?
Or is it good for now, and we'll revisit in the future?
There was a problem hiding this comment.
If it passes all CI checks, I say let's go with as is for now. Please make an issue to reevaluate so we don't forget to do that.
939077f to
5ca3ab1
Compare
5ca3ab1 to
ea4a596
Compare
Removes C++20 from name since all jobs are now C++20.
Also adds troubleshooting sections about shroud and about symlinked paths.
ea4a596 to
fa3ff25
Compare
Summary
config.hppwas missing a compiler define forAXOM_USE_MULTIMAT