MetOffice / MetOffice/shumlib

CMake build does not link the OpenMP targets, so BUILD_OPENMP has no effect

Open Beginner friendly
#37 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Fortran
Stars
4
Forks
10
PR merge metrics
No merged PRs in 30d

Description

## Summary

`BUILD_OPENMP` defaults to `ON`, but the CMake build never links the OpenMP targets it finds, so the resulting library is serial. Nothing in the configure output indicates that the option had no effect.

## Detail

`cmake/ShumOptions.cmake` takes the option as far as locating OpenMP:

```cmake
if(BUILD_OPENMP)
# FIXME: this probably needs newer version of cmake on the Cray
find_package(OpenMP 3.0 REQUIRED)
...
endif()
```

`find_package` defines the imported targets `OpenMP::OpenMP_C` and `OpenMP::OpenMP_Fortran`, but no `target_link_libraries` call consumes them. As a result:

- `-fopenmp` never reaches a compile line,
- `_OPENMP` is never defined,
- the OpenMP regions are preprocessed away.

The affected sources are `shum_wgdos_packing`, `shum_horizontal_field_interp`, `shum_latlon_eq_grids`, `shum_thread_utils` and `shum_byteswap` (`#pragma omp` in `c_shum_byteswap.c`, `!$omp` in the Fortran).

The Makefile build does honour the equivalent setting — `SHUM_OPENMP ?= true` in `Makefile`, applying `FCFLAGS_OPENMP` / `CCFLAGS_OPENMP` — so the two build systems disagree, with CMake silently producing a serial library.

## Reproduce

```console
$ cmake -S . -B build -DCMAKE_BUILD_TYPE=Release -DBUILD_OPENMP=ON
-- Found OpenMP_C: -fopenmp (found suitable version "4.5", minimum required is "3.0")
-- Found OpenMP_Fortran: -fopenmp (found suitable version "4.5", minimum required is "3.0")

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

No `-fopenmp`, despite OpenMP having been found and required. After building, `ldd libshum.so` shows no `libgomp`.

## Suggested fix

Link the imported targets to `shum` inside the existing `BUILD_OPENMP` guard. `PRIVATE` is sufficient, since `_OPENMP` is used in `c_shum_byteswap.c` and never in an installed header, so consumers need no OpenMP flags of their own. The `shum_thread_utils` unit tests exercise the OpenMP paths directly, so `shumlib-tests` needs the same linkage in its own right.

I have opened a PR with that change.

## How this surfaced

I hit this while packaging shumlib for conda-forge (conda-forge/staged-recipes#34717), where a reviewer asked why the recipe requested OpenMP but produced a library with no OpenMP runtime dependency.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with cmake/ShumOptions.cmake and the CMake target definitions for shum and shumlib-tests; configure with BUILD_OPENMP=ON and inspect the generated compile and link flags. Confirm the OpenMP flags and runtime are present, then run the shum_thread_utils unit tests to verify the OpenMP paths.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, cmake, fortran
Domain
build-system, testing-qa
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
84/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.