-
Notifications
You must be signed in to change notification settings - Fork 71
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
More systematic compiler flags for CI; separate Conda-style builds #360
Conversation
I don't understand why |
@makepath-alex The |
Co-authored-by: Sergey Kosukhin <sergey.kosukhin@mpimet.mpg.de>
Thanks for the suggestion @skosukhin; it worked perfectly . Any other thoughts? |
@makepath-alex Maybe you want to change the target of #361 to this branch? Then you could approve the PR if I've made things more clear. |
@skosukhin @makepath-alex To explain the logic: the idea is to test a range of options with |
@makepath-alex @skosukhin I believe I've made the changes you wanted - one or both of you can look briefly and approve? (It's nice to have multiple eyes on these changes, thanks.) |
fpmodel: [DP, SP] | ||
build-type: [Debug, RelWithDebugInfo] |
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.
We test RelWithDebugInfo
with no extra flags in conda-style-builds.yml
. Instead of Debug
with extra flags and RelWithDebugInfo
without extra flags, I would test only RelWithDebugInfo
with extra flags in this workflow.
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.
One minor(?) difference is that continuous-integration
uses the system gfortran
while conda-style-builds
uses the conda
-installed gfortran
.
I was thinking of continuous-integration
as a way to run the code with maximum debugging, which motivated Debug
builds with extra flags. @skosukhin Do you think we'll be able to isolate any errors introduced with RelWithDebugInfo
?
- build-type: RelWithDebugInfo | ||
rte-kernels: accel |
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 don't see why we should not run this case.
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 thought to exclude this because it would never be used. Builds for use on CPU architectures will bundle the default kernels. I did want to test the accel
kernels with max debugging in case we introduce coding errors.
- name: Build libraries and tests | ||
run: | | ||
cmake -S . -B build -G "Ninja" \ | ||
-DCMAKE_BUILD_TYPE=${{ matrix.build-type}} \ |
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.
Since there is only one value for the build-type
in the matrix now, should we remove it and have the following here?
-DCMAKE_BUILD_TYPE=${{ matrix.build-type}} \ | |
-DCMAKE_BUILD_TYPE=RelWithDebugInfo \ |
This PR separates plain condo-style builds across many platforms into one workflow. More systematic debug and release testing with gfortran is done in
continuous-integration.yml
"-march=native" flag is removed. Seems to resolve #358 and #318, making #359 obsolete.