Skip to content

Conversation

@dougiesquire
Copy link
Collaborator

@dougiesquire dougiesquire commented Dec 3, 2025

This PR modifies the CMake build system to make the access3 and solo ocean builds non-exclusive.

Changes

  • Adds new option MOM6_SOLO that can be used at the same time as MOM6_ACCESS3 to build a MOM6 executable with the solo_driver.
  • Adds a new target mom6shared to build the shared components of the mom6lib ACCESS3 library and mom6-solo executable.
  • Uses CMake subdirectories for modularity/organisation. This could easily be extended to other drivers.
  • Actually links OpenMP when MOM6_OPENMP is set - see CMake: Not linking OpenMP #35

Related PRs

  • The changes to expose the MOM6_SOLO option as a variant in the access-mom6 SPR are here.
  • A prerelease deployment of ACCESS-OM3 using this option is here.

Copy link
Collaborator

@anton-seaice anton-seaice left a comment

Choose a reason for hiding this comment

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

Weird stylistic questions only:

I was imagining a set() for drivers, so that the set could be extended in the future?

e.g.

set(drivers)

foreach (_drive IN LISTS drivers)
    add_subdirectory(... <something about _drive>)
endif()

similar to access3 - https://github.com/ACCESS-NRI/access3-share/blob/main/cmake/Access3BinInstall.cmake

and why do the CMakeLists.txt files go in the cmake folder rather than the relevant src folder?

This could get rid of these variables:

# set paths for the source code and config source
set(SRC "${CMAKE_SOURCE_DIR}/src")
set(CONFIG_SRC "${CMAKE_SOURCE_DIR}/config_src")

and use relative paths

@dougiesquire
Copy link
Collaborator Author

dougiesquire commented Dec 5, 2025

Weird stylistic questions only:

I was imagining a set() for drivers, so that the set could be extended in the future?

e.g.

set(drivers)

foreach (_drive IN LISTS drivers)
add_subdirectory(... )
endif()

similar to access3 - https://github.com/ACCESS-NRI/access3-share/blob/main/cmake/Access3BinInstall.cmake

We chatted about this in person and decided the current approach is simplest for the SPR. We are unlikely to add additional targets.

and why do the CMakeLists.txt files go in the cmake folder rather than the relevant src folder?

This could get rid of these variables:

# set paths for the source code and config source
set(SRC "${CMAKE_SOURCE_DIR}/src")
set(CONFIG_SRC "${CMAKE_SOURCE_DIR}/config_src")

@anton-seaice I tried this in ce0ee7c and I'm not sure how I feel about it. It's probably more in line with typical CMake practice... Thoughts/opinions?

Copy link
Collaborator

@anton-seaice anton-seaice left a comment

Choose a reason for hiding this comment

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

In testing:

For completeness, it would be good to have builds for both +mom6_solo, and ~mom6_solo and also +mom6_solo without access3 at all.

@micaeljtoliveira - would you be able to review this ? I think we're mainly looking for advice on if this is a clear/standard(ish) way to organise the CMake build files and their contents.

@dougiesquire
Copy link
Collaborator Author

dougiesquire commented Dec 7, 2025

For completeness, it would be good to have builds for both +mom6_solo, and ~mom6_solo and also +mom6_solo without access3 at all.

Added

@micaeljtoliveira
Copy link
Member

@micaeljtoliveira - would you be able to review this ? I think we're mainly looking for advice on if this is a clear/standard(ish) way to organise the CMake build files and their contents.

@anton-seaice I had a look, but your use case is so unusual that I don't think "standard" CMake organisation really applies here, except the usual basic stuff, like putting common code in libraries that can be reused.

The only thing I would do different, is putting the source files specific to the solo driver in their own library and not modify the add_fortran_library function. But this is more a matter of personal taste.

@anton-seaice
Copy link
Collaborator

Thanks @micaeljtoliveira

I don't think I have any comments @dougiesquire

I think the CI should work though ?

@dougiesquire
Copy link
Collaborator Author

I think the CI should work though ?

Yeah we need this first: #32. I'll get to it today.

@dougiesquire dougiesquire force-pushed the cmake-allow-exe branch 2 times, most recently from 2a35972 to c61ff5c Compare December 16, 2025 00:00
@dougiesquire
Copy link
Collaborator Author

dougiesquire commented Dec 16, 2025

@anton-seaice CI now works, but I think I'd like to move the manifest changes into a subsequent PR

@dougiesquire
Copy link
Collaborator Author

@anton-seaice are you happy to approve?

Copy link
Collaborator

@anton-seaice anton-seaice left a comment

Choose a reason for hiding this comment

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

Thanks @dougiesquire

@dougiesquire dougiesquire merged commit 77801f8 into 2025.07 Dec 16, 2025
107 checks passed
@dougiesquire dougiesquire deleted the cmake-allow-exe branch December 16, 2025 02:51
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.

4 participants