-
Notifications
You must be signed in to change notification settings - Fork 34
Some touchups to reflect that C++20 is now required for Axom #1957
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
2c57609
Require C++20 at config time
kennyweiss 1d3c07d
Bugfix: config.hpp was missing AXOM_USE_MULTIMAT define
kennyweiss 931d55b
Updates docs and tutorials about C++20 requirement
kennyweiss 23c6d2b
Updates CI job name
kennyweiss 7788b71
Updates C++ version for python interface
kennyweiss aa73ec3
Updates RELEASE-NOTES about C++20 requirement
kennyweiss fa3ff25
Updates the build skill about running MPI commands
kennyweiss File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_requiredto 3.14 at the top ofsrc/CMakeLists.txtfor CUDA builds. Thecmake_minimum_versionfunction 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_requiredand this caused failures. We should reevaluate this at some point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Should I add a note here or in
src/CMakeLists.txtand/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the following issue for this: #1965