-
Notifications
You must be signed in to change notification settings - Fork 156
fixes for cmake 32/64bit builds and enable CI testing via ctest #1787
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
base: main
Are you sure you want to change the base?
Changes from all commits
2e3576b
54a625a
5630618
e65bb82
acf8c1c
c4230a1
985d6e7
c5b2fbd
46678d5
65c65e9
f19f7f4
fcf5849
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,42 +12,46 @@ jobs: | |
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| omp-flags: [ -DOPENMP=on, -DOPENMP=off ] | ||
| libyaml-flag: [ "", -DWITH_YAML=on ] | ||
| build-type: [ "-DCMAKE_BUILD_TYPE=Release", "-DCMAKE_BUILD_TYPE=Debug" ] | ||
| container: | ||
| image: ghcr.io/noaa-gfdl/fms/fms-ci-rocky-gnu:13.2.0 | ||
| env: | ||
| CMAKE_FLAGS: "${{ matrix.build-type }} ${{ matrix.omp-flags }} ${{ matrix.libyaml-flag }}" | ||
| CMAKE_FLAGS: "${{ matrix.build-type }} ${{ matrix.libyaml-flag }}" | ||
| EXCLUDE_TESTS: "test_mpp_nesting|test_bc_restart|test_collective_io|test_fms2_io|test_io_with_mask" | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/[email protected] | ||
| - name: Generate makefiles with CMake | ||
| run: | | ||
| mkdir build | ||
| cd build | ||
| cmake $CMAKE_FLAGS -DNetCDF_ROOT=/opt/view -DLIBYAML_ROOT=/opt/view .. | ||
| cmake $CMAKE_FLAGS -DOPENMP=on -DNetCDF_ROOT=/opt/view -DLIBYAML_ROOT=/opt/view .. | ||
| - name: Build the library | ||
| run: make -C build | ||
| - name: Run the unit tests | ||
| run: cd build && ctest -E "${EXCLUDE_TESTS}" --output-on-failure | ||
|
|
||
| build_arm: | ||
| runs-on: ubuntu-24.04-arm | ||
| strategy: | ||
| matrix: | ||
| omp-flags: [ -DOPENMP=on, -DOPENMP=off ] | ||
| libyaml-flag: [ "", -DWITH_YAML=on ] | ||
| build-type: [ "-DCMAKE_BUILD_TYPE=Release", "-DCMAKE_BUILD_TYPE=Debug" ] | ||
| container: | ||
| image: ghcr.io/noaa-gfdl/fms/fms-ci-rocky-gnu:13.2.0-arm | ||
| env: | ||
| CMAKE_FLAGS: "${{ matrix.build-type }} ${{ matrix.omp-flags }} ${{ matrix.libyaml-flag }}" | ||
| CMAKE_FLAGS: "${{ matrix.build-type }} ${{ matrix.libyaml-flag }}" | ||
| EXCLUDE_TESTS: "test_mpp_nesting|test_bc_restart|test_collective_io|test_fms2_io|test_io_with_mask|test_sat_vapor_pres" | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/[email protected] | ||
| - name: Generate makefiles with CMake | ||
| run: | | ||
| mkdir build | ||
| cd build | ||
| cmake $CMAKE_FLAGS -DNetCDF_ROOT=/opt/view -DLIBYAML_ROOT=/opt/view .. | ||
| cmake $CMAKE_FLAGS -DOPENMP=on -DNetCDF_ROOT=/opt/view -DLIBYAML_ROOT=/opt/view .. | ||
| - name: Build the library | ||
| run: make -C build | ||
| - name: Run the unit tests | ||
| run: cd build && ctest -E "${EXCLUDE_TESTS}" --output-on-failure | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,18 +64,17 @@ option(PORTABLE_KINDS "Enable compiler definition -DPORTABLE_KINDS" | |
| option(GFS_PHYS "Enable compiler definition -DGFS_PHYS" OFF) | ||
| option(LARGEFILE "Enable compiler definition -Duse_LARGEFILE" OFF) | ||
| option(WITH_YAML "Enable compiler definition -Duse_yaml" OFF) | ||
| option(USE_DEPRECATED_IO "THIS OPTION HAS NO EFFECT AND WILL BE REMOVED IN A FUTURE RELEASE" OFF) | ||
|
|
||
| if(32BIT) | ||
| list(APPEND kinds "r4") | ||
| message(STATUS "Building library with 4-byte real defaults (with mixed precision real support for most modules).") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one more request, line 68 needs to be removed |
||
| endif() | ||
| if(64BIT) | ||
| list(APPEND kinds "r8") | ||
| message(STATUS "Building library with 8-byte real defaults (with mixed precision real support for most modules).") | ||
| endif() | ||
| if(NOT kinds) | ||
| message(STATUS "Single Precision 32BIT: ${32BIT}") | ||
| message(STATUS "Double Precision 64BIT: ${64BIT}") | ||
| message(STATUS "No kind specified, building default double precision (with mixed precision support). Real size will not be included in built library name.") | ||
| message(STATUS "Building library with 8-byte real defaults (with mixed precision 4-byte real support for most modules). Real size will not be included in built library name.") | ||
| endif() | ||
|
|
||
| # Find dependencies | ||
|
|
@@ -271,11 +270,6 @@ if(LARGEFILE) | |
| list(APPEND fms_defs use_LARGEFILE) | ||
| endif() | ||
|
|
||
| # Precision-based compiler definitions | ||
| if(32BIT) | ||
| list(APPEND r4_defs OVERLOAD_R4 OVERLOAD_R8) | ||
| endif() | ||
|
|
||
| # Add platform specific compiler definitions | ||
| if(APPLE) | ||
| list(APPEND fms_defs __APPLE__) | ||
|
|
@@ -286,7 +280,7 @@ include(fms_compiler_flags) | |
|
|
||
| # If netCDF was not built with HDF5 parallel I/O features, set up the macro -DNO_NC_PARALLEL4 | ||
| IF(NOT NetCDF_PARALLEL) | ||
| MESSAGE(WARNING "netCDF was not build with HDF5 parallel I/O features, so collective netcdf io is not allowed") | ||
| MESSAGE(WARNING "netCDF was not build with HDF5 parallel I/O features, so parallel netcdf io is not allowed") | ||
| list(APPEND fms_defs NO_NC_PARALLEL4) | ||
| ENDIF() | ||
|
|
||
|
|
@@ -342,7 +336,7 @@ foreach(kind ${kinds}) | |
| amip_interp/include) | ||
| 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}") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, line 278 - 278, I don't think the OVERLOAD macros are there anymore
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
nice catch! should be removed now |
||
| set_target_properties(${libTgt}_f PROPERTIES Fortran_MODULE_DIRECTORY | ||
| ${moduleDir}) | ||
| target_link_libraries(${libTgt}_f PRIVATE NetCDF::NetCDF_Fortran | ||
|
|
@@ -587,6 +581,14 @@ include(CTest) | |
| set(MPI_LAUNCHER "mpirun") | ||
| # used in the test-lib.sh.in to make it behave differently when parsed by cmake | ||
| set(USING_CMAKE "true") | ||
| # set the fms library to link tests with based on whats built | ||
| if(NOT kinds) | ||
| set(fmsLibraryName FMS::fms) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. line 439, isn't kind always defined?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. line 439 uses |
||
| elseif(64BIT) | ||
| set(fmsLibraryName FMS::fms_r8) | ||
| elseif(32BIT) | ||
| set(fmsLibraryName FMS::fms_r4) | ||
| endif() | ||
|
|
||
| # parse and add build info to test script util file | ||
| configure_file(${CMAKE_CURRENT_SOURCE_DIR}/test_fms/test-lib.sh.in ${CMAKE_CURRENT_SOURCE_DIR}/test-lib.sh | ||
|
|
@@ -609,7 +611,7 @@ list(APPEND TEST_MODS_SRC | |
| test_fms/mpp/test_system_clock.F90) | ||
|
|
||
| add_library(testLibs "${TEST_MODS_SRC}") | ||
| target_link_libraries(testLibs FMS::fms) | ||
| target_link_libraries(testLibs ${fmsLibraryName}) | ||
| target_compile_definitions(testLibs PRIVATE TEST_MOS_KIND_=r8_kind) | ||
| target_include_directories(testLibs PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/include | ||
| PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/test_fms/fms/include | ||
|
|
@@ -690,6 +692,7 @@ list(APPEND TEST_SRC_SINGLE_TARGET | |
| test_fms/fms2_io/test_domain_io.F90 | ||
| test_fms/fms2_io/test_collective_io.F90 | ||
| test_fms/fms2_io/test_generalized_indices.F90 | ||
| test_fms/fms2_io/test_parallel_writes.F90 | ||
| test_fms/fms/test_fms.F90 | ||
| test_fms/interpolator/test_interpolator.F90 | ||
| test_fms/mpp/test_clock_init.F90 | ||
|
|
@@ -768,21 +771,25 @@ list(APPEND TEST_SRC_MULTI_TARGET | |
| test_fms/sat_vapor_pres/test_sat_vapor_pres.F90 | ||
| test_fms/time_interp/test_time_interp.F90 | ||
| test_fms/time_interp/test_time_interp_external.F90 | ||
| test_fms/time_interp/test_time_interp_conservative_hi.F90 | ||
| test_fms/topography/test_topography.F90 | ||
| test_fms/tracer_manager/test_tracer_manager.F90 | ||
| test_fms/tridiagonal/test_tridiagonal.F90 | ||
| ) | ||
|
|
||
| # add all the test executables and set up linking + compile flags | ||
| foreach (testFile ${TEST_SRC_SINGLE_TARGET}) | ||
| get_filename_component (TName ${testFile} NAME_WE) | ||
|
|
||
| get_filename_component (TName ${testFile} NAME_WE) | ||
| add_executable(${TName} ${testFile}) | ||
| target_compile_definitions(${TName} PRIVATE "${r8_defs}") | ||
| set_target_properties(${TName} PROPERTIES COMPILE_FLAGS ${r8_flags}) | ||
| target_link_libraries(${TName} PUBLIC FMS::fms | ||
| PRIVATE testLibs | ||
| ) | ||
| target_link_libraries(${TName} PUBLIC ${fmsLibraryName} | ||
| PRIVATE testLibs) | ||
| if(32BIT) | ||
| target_compile_definitions(${TName} PRIVATE "${r4_defs} ${fms_defs}") | ||
| else() | ||
| target_compile_definitions(${TName} PRIVATE "${r8_defs} ${fms_defs} ") | ||
| set_target_properties(${TName} PROPERTIES COMPILE_FLAGS ${r8_flags}) | ||
| endif() | ||
|
|
||
| if(WITH_YAML) | ||
| target_link_libraries(${TName} PRIVATE yaml ${LIBYAML_LIBRARIES}) | ||
|
|
@@ -805,18 +812,16 @@ foreach (testFile ${TEST_SRC_MULTI_TARGET}) | |
| get_filename_component (TName ${testFile} NAME_WE) | ||
|
|
||
| add_executable(${TName}_r8 ${testFile}) | ||
| target_compile_definitions(${TName}_r8 PRIVATE "${r8_defs}") | ||
| target_compile_definitions(${TName}_r8 PRIVATE "${r8_defs} ${fms_defs}") | ||
| set_target_properties(${TName}_r8 PROPERTIES COMPILE_FLAGS ${r8_flags}) | ||
| target_link_libraries(${TName}_r8 PUBLIC FMS::fms | ||
| PRIVATE testLibs | ||
| ) | ||
|
|
||
| target_link_libraries(${TName}_r8 PUBLIC ${fmsLibraryName} | ||
| PRIVATE testLibs) | ||
| add_executable(${TName}_r4 ${testFile}) | ||
| target_compile_definitions(${TName}_r4 PRIVATE "${r4_defs}") | ||
| target_compile_definitions(${TName}_r4 PRIVATE "${r4_defs} ${fms_defs}") | ||
| # seems counterintuitive but r4 tests use r8 default | ||
| # they specify kind values explicitly with the preprocessor where needed (TEST_FMS_KIND_) | ||
| set_target_properties(${TName}_r4 PROPERTIES COMPILE_FLAGS ${r8_flags}) | ||
| target_link_libraries(${TName}_r4 PUBLIC FMS::fms | ||
| target_link_libraries(${TName}_r4 PUBLIC ${fmsLibraryName} | ||
| PRIVATE testLibs | ||
| ) | ||
|
|
||
|
|
@@ -878,8 +883,16 @@ foreach (testScript ${TEST_SCRIPTS}) | |
| ENVIRONMENT "parser_skip=skip" | ||
| ) | ||
| endif() | ||
|
|
||
| endforeach() | ||
|
|
||
| # skip parallel netcdf tests if its not enabled in given install | ||
| IF(NOT NetCDF_PARALLEL) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving what @J-Lentz pointed out, line 290 should say "parallel netcdf io" instead of "collective netcdf io" |
||
| set_tests_properties(test_collective_io PROPERTIES | ||
| ENVIRONMENT "parallel_skip=skip" | ||
| ) | ||
| endif() | ||
|
|
||
| set(CMAKE_CTEST_ARGUMENTS "--output-on-failure") | ||
|
|
||
| ### Package config | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ program test_time_interp_with_interp | |
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah this probably worked because DTEST_FMS_KIND_ is set to 4 or 8 in Makefile.am
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unless cmake substitutes them as r4_kind and r8_kind somewhere?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah cmake uses r4/r8_kind so that its uniform across all the different unit tests. |
||
|
|
||
| implicit none | ||
|
|
||
|
|
||
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 curious, why was this removed?
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 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.