Skip to content

CMake build system modifications for fms#6

Open
hctorres wants to merge 2 commits into
dev/turbofrom
192-feature-cmake-build-system-for-FMS
Open

CMake build system modifications for fms#6
hctorres wants to merge 2 commits into
dev/turbofrom
192-feature-cmake-build-system-for-FMS

Conversation

@hctorres
Copy link
Copy Markdown

@hctorres hctorres commented May 27, 2026

Description

Two small CMake changes needed by turbo-stack's find_package(FMS) consumer:

  1. Add diag_manager/mppnccombine.c to the cmake C source list (was already in the makefile build).
  2. Add NetCDF::NetCDF_C to the PUBLIC link interface of fms_r4 and fms_r8. Without it, downstream consumers see only NetCDF::NetCDF_Fortran + bare -lnetcdf flags from nf-config --flibs, which fail to resolve at link time on NetCDF installs with split lib/lib64 layouts (e.g. NCAR Derecho's NetCDF module).

This is part of the issue-192 cross-repo CMake build-system effort:

Repo PR Base
FMS this PR dev/turbo
TIM TURBO-ESM/TIM#16 main
MOM6 TURBO-ESM/MOM6#25 dev/turbo
turbo-stack TURBO-ESM/turbo-stack#233 main

Recommended merge order: FMS → TIM → MOM6 → turbo-stack. turbo-stack consumes the other three via
find_package(FMS|TIM) and add_subdirectory(${MOM6_SOURCE_DIR} ...).

How Has This Been Tested?

Exercised end-to-end via the turbo-stack PR, which builds FMS2 from this branch and then links MOM6 against FMS::fms_r8.

hctorres and others added 2 commits May 18, 2026 17:48
The combined fms_r{4,8} library bundles both C and Fortran object
files; the C objects reference libnetcdf, but only NetCDF::NetCDF_Fortran
was being re-exported to consumers. On NetCDF installs with split
lib/lib64 layouts (e.g. NCAR Derecho module: libnetcdf in lib64,
libnetcdff in lib), bare -lnetcdf flags from nf-config --flibs fail
to resolve at link time. Re-exporting NetCDF::NetCDF_C ensures its
absolute path reaches the link line.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@hctorres hctorres requested a review from Copilot May 27, 2026 05:50
@hctorres hctorres changed the title 192 feature cmake build system for fms CMake build system for fms May 27, 2026
@hctorres hctorres changed the title CMake build system for fms CMake build system modifications for fms May 27, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates FMS’s CMake build so downstream find_package(FMS) consumers correctly build/link when using NetCDF installations where the C and Fortran libraries are split across different library directories.

Changes:

  • Add diag_manager/mppnccombine.c to the C source list so it is built under CMake (matching the existing automake build).
  • Export NetCDF::NetCDF_C as a PUBLIC link dependency of fms_r4 / fms_r8 so downstream link lines reliably include the NetCDF C library via the imported target.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@hctorres hctorres self-assigned this May 28, 2026
Copy link
Copy Markdown

@johnmauff johnmauff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hctorres, Here is what Claude said about one of your changes.

Change 2: diag_manager/mppnccombine.c — likely a build-breaking bug

mppnccombine is a standalone post-processing utility that combines distributed NetCDF output files written by parallel MPI tasks. It has its own main() function. Adding it to the fms_c_src_files list means it gets compiled into libfms_r4.a and libfms_r8.a.

Any program that links against FMS will then have two main() symbols — its own and the one from mppnccombine.c — which causes a link error:

error: multiple definition of main

In FMS's existing Makefile build, mppnccombine is compiled as a separate standalone executable, not as part of the library. The CMake change should mirror that: a separate add_executable(mppnccombine diag_manager/mppnccombine.c) target with its own target_link_libraries, not an entry in the library source list.

The reason this may not have been caught in the turbo-stack end-to-end validation is that the test builds the FMS library and links MOM6 against it — but if no program in that test chain ever exercises the specific translation unit containing main() from mppnccombine.c, some linkers (particularly with static libraries) may silently omit it and only fail when a linker with stricter symbol resolution is used.

Recommendation: Remove diag_manager/mppnccombine.c from fms_c_src_files and replace with:
add_executable(mppnccombine diag_manager/mppnccombine.c)
target_link_libraries(mppnccombine PRIVATE NetCDF::NetCDF_C)
install(TARGETS mppnccombine RUNTIME DESTINATION bin)

@hctorres
Copy link
Copy Markdown
Author

hctorres commented May 29, 2026

@johnmauff I cant seem to get the duplicate main bug mentioned in the previous comment to trigger. I think we might be ok.

Copy link
Copy Markdown

@johnmauff johnmauff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hctorres, thanks for looking into the Claude comment. This now looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants