Link the OpenMP targets when BUILD_OPENMP is enabled

[Kolen Cheung (ickc)]

PR Summary

Link the OpenMP imported targets so that BUILD_OPENMP actually enables OpenMP.

Code Reviewer: Sam Clarke-Green (@t00sa)

BUILD_OPENMP defaults to ON and cmake/ShumOptions.cmake runs find_package(OpenMP 3.0 REQUIRED), but nothing links the resulting OpenMP::OpenMP_C / OpenMP::OpenMP_Fortran targets. -fopenmp therefore never reaches a compile line, _OPENMP is never defined, and the OpenMP regions in shum_wgdos_packing, shum_horizontal_field_interp, shum_latlon_eq_grids, shum_thread_utils and shum_byteswap are preprocessed away. The Makefile build defaults SHUM_OPENMP to true and does apply the flags, so the two build systems currently disagree and the CMake one gives no sign that the option did nothing.

This links the targets inside the existing BUILD_OPENMP guard:

• shum gets them PRIVATE. That is sufficient because _OPENMP is used inside c_shum_byteswap.c and never in an installed header, so consumers do not need OpenMP flags of their own.

• shumlib-tests gets the same linkage in its own right, since the shum_thread_utils tests exercise the OpenMP paths directly and would not otherwise be compiled with them.

• closes CMake build does not link the OpenMP targets, so BUILD_OPENMP has no effect #37

Code Quality Checklist

(Some checks are automatically carried out via the CI pipeline)

• I have performed a self-review of my own code
• My code follows the project's style guidelines
• Comments have been included that aid understanding and enhance the readability of the code
• My changes generate no new warnings

Testing

• I have tested this change locally, using the rose-stem suite
• If any tests fail (rose-stem or CI) the reason is understood and acceptable (eg. kgo changes)
• I have added tests to cover new functionality as appropriate (eg. system tests, unit tests, etc.)

I am an external contributor and have no access to the rose-stem suite, so that box is unticked. What I did verify locally with GCC:

• -fopenmp now reaches the C and Fortran compile lines of both targets:

  $ grep -E '^(C|Fortran)_FLAGS' build/CMakeFiles/shum.dir/flags.make
  C_FLAGS = -O3 -DNDEBUG -std=gnu99 -fPIC -fopenmp
  Fortran_FLAGS = -O3 -DNDEBUG -O3 -Jmodules -fPIC -fopenmp
  
  $ grep -E '^(C|Fortran)_FLAGS' build/CMakeFiles/shumlib-tests.dir/flags.make
  C_FLAGS = -O3 -DNDEBUG -std=gnu99 -fopenmp
  Fortran_FLAGS = -O3 -DNDEBUG -O3 -Jmodules -fopenmp

• libshum.so now links the runtime: libgomp.so.1 => /usr/lib64/libgomp.so.1.

• The fruit suite still passes: 1/1 Test #1: shumlib-tests ... Passed, 100% tests passed, 0 tests failed out of 1.

• -DBUILD_OPENMP=OFF still configures and builds, with no OpenMP flags on either target.

No new tests are added here: this restores the behaviour the existing shum_thread_utils tests were written against, rather than adding functionality.

trac.log

Not applicable — I have no rose-stem access, so there is no trac.log to attach.

Security Considerations

• This change does not introduce security vulnerabilities
• I have reviewed the code for potential security issues
• Sensitive data is properly handled (if applicable)
• Authentication and authorisation are properly implemented (if applicable)

Performance Impact

• Performance of the code has been considered and, if applicable, suitable performance measurements have been conducted

This is a performance change by nature: the parallel regions in the affected sub-libraries go from being compiled out to being compiled in, so a CMake-built shumlib becomes threaded where it previously was not. That matches what the Makefile build has always produced by default. I have not run comparative benchmarks, and results on Met Office platforms would be more meaningful than mine — worth a look during review if you want numbers.

AI Assistance and Attribution

• Some of the content of this change has been produced with the assistance of Generative AI tool name (e.g., Met Office Github Copilot Enterprise, Github Copilot Personal, ChatGPT GPT-4, etc) and I have followed the Simulation Systems AI policy(including attribution labels)

Documentation

• Where appropriate I have updated documentation related to this change and confirmed that it builds correctly

No documentation change: BUILD_OPENMP is already documented as building with OpenMP parallelisation, which is what it now does.

[Sam Clarke-Green (t00sa)]

Thanks for fixing this. I've confirmed that the change works and the OpenMP option are now correctly applied.

However, I've discovered that the shumlib regression tests now hangs on my platform - I'm using GCC 12.2.0 and CMake 3.26.5 on RHEL 9.7 - e.g. in test_unlock_foreign. It's not clear to me if this is a problem with the version of CMake I'm using or if it's a problem with the build configuration. I'm going to investigate further before I approve the PR to make sure I understand what is going on.