fixes for cmake 32/64bit builds and enable CI testing via ctest #1787
Conversation
rem1776
requested review from
bensonr,
uramirez8707 and
vithikashah001
as code owners
| target_compile_definitions(${libTgt}_f PRIVATE "${fms_defs}") | ||
| target_compile_definitions(${libTgt}_f PRIVATE "${${kind}_defs}") | ||
| set_target_properties(${libTgt}_f PROPERTIES COMPILE_FLAGS ${${kind}_flags}) | ||
| set_target_properties(${libTgt}_f PROPERTIES COMPILE_FLAGS "${${kind}_flags}") |
There was a problem hiding this comment.
comments not for this line, but just looking at line 76 - 79, the messages seem misleading. Could they be changed to something like "building select modules and procedures in 32bit precision"?
There was a problem hiding this comment.
Also, line 278 - 278, I don't think the OVERLOAD macros are there anymore
There was a problem hiding this comment.
Also, line 278 - 278, I don't think the OVERLOAD macros are there anymore
nice catch! should be removed now
| set(USING_CMAKE "true") | ||
| # set the fms library to link tests with based on whats built | ||
| if(NOT kinds) | ||
| set(fmsLibraryName FMS::fms) |
There was a problem hiding this comment.
line 439, isn't kind always defined?
There was a problem hiding this comment.
line 439 uses kind as opposed to kinds which is only set if one of the 32/64bit options is enabled.
CMakeLists.txt
Outdated
| set(fmsLibraryName FMS::fms) | ||
| elseif(64BIT) | ||
| set(fmsLibraryName FMS::fms_r8) | ||
| else() |
There was a problem hiding this comment.
maybe this should explicitly be elseif(32BIT)
| use time_interp_external2_mod, only: init_external_field, time_interp_external_init, time_interp_external | ||
|
|
||
| use time_manager_mod, only: JULIAN, time_type, set_date, set_calendar_type, time_manager_init | ||
| use platform_mod, only: r4_kind, r8_kind |
There was a problem hiding this comment.
ah this probably worked because DTEST_FMS_KIND_ is set to 4 or 8 in Makefile.am
Is this use statement needed?
There was a problem hiding this comment.
unless cmake substitutes them as r4_kind and r8_kind somewhere?
There was a problem hiding this comment.
unless cmake substitutes them as r4_kind and r8_kind somewhere?
Yeah cmake uses r4/r8_kind so that its uniform across all the different unit tests.
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| omp-flags: [ -DOPENMP=on, -DOPENMP=off ] |
There was a problem hiding this comment.
just curious, why was this removed?
There was a problem hiding this comment.
That was mainly to make the amount of jobs smaller. It'll just compile with openmp everytime.
I was thinking as we move away from autotools we can add more options to this one, just didn't want to make the CI too bloated.
| endforeach() | ||
|
|
||
| # skip parallel netcdf tests if its not enabled in given install | ||
| IF(NOT NetCDF_PARALLEL) |
There was a problem hiding this comment.
Leaving what @J-Lentz pointed out, line 290 should say "parallel netcdf io" instead of "collective netcdf io"
mlee03
left a comment
mlee03
left a comment
There was a problem hiding this comment.
All comments addressed :)
|
|
||
| if(32BIT) | ||
| list(APPEND kinds "r4") | ||
| message(STATUS "Building library with 4-byte real defaults (with mixed precision real support for most modules).") |
There was a problem hiding this comment.
one more request, line 68 needs to be removed
Description
Fixes errors that come up when building using the 32 or 64 bit build options. Now it will link in whatever's built so it won't fail when it gets to the unit test executables.
Also includes a few small changes to get the unit tests from cmake running in the CI. Skips two tests (#1786) until we can get them working since they have cmake/arm specific bugs.
Fixes #1783
How Has This Been Tested?
tested on amd box with gcc 13/14 + mpich
Checklist:
make distcheckpasses