-
Notifications
You must be signed in to change notification settings - Fork 369
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
CMake: overhaul options #1490
CMake: overhaul options #1490
Conversation
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.
Looks very good - we are now informing & waiting for downstream codes to anticipate the breaking change without much friction.
# This will stop the subsequent option( AMReX_FORTRAN "Enable Fortran language" ON ) | ||
# from being executed and no entry AMReX_FORTRAN will be created in the cache | ||
# | ||
option( USE_XSDK_DEFAULTS "Enable xSDK mode" OFF ) |
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.
It would be pretty nice if we could fulfill the xSDK policies by default for high usability, also because it would be less logic to test.
Do you and CC @WeiqunZhang see a way towards that? Maybe this is a good time to introduce this as well, just curious.
Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
endif () | ||
|
||
|
||
set(AMReX_PRECISION_VALUES SINGLE DOUBLE) |
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.
Just a minor taste quesion:
Since this is not an abbreviation of brand name, I picked lowercase single
/double
last when I added this to WarpX. I am ok to go all caps, too.
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 picked upper case because the options names are upper case anyways. I can make it case-insensitive tho.
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.
Oh, case insensitive is nice for this one, too. Only if it's not too much work.
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.
Actually, making it case-insensitive requires a little bit of extra logic. Also, XSDK seems to prescribe the use of upper case.At this point, I will leave it upper case. Sorry :-(
Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
…fix" This reverts commit 924cf15.
Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>
AMReX CMake changes in AMReX-Codes/amrex#1490
Summary
<unreleased-WarpX-code>
to be readyAdditional background
Checklist
The proposed changes: