MPAS-Dev / MPAS-Model

Repository for MPAS models and shared framework releases.
239 stars 319 forks source link

Added support for building the atmosphere and init atmosphere cores with CMake. #1169

Closed amstokely closed 4 months ago

amstokely commented 4 months ago

Overview

Notes

Building with CMake

jim-p-w commented 4 months ago

Shouldn't there be a CMakeLists.txt for all of the cores (core_landice, etc)

amstokely commented 4 months ago

Shouldn't there be a CMakeLists.txt for all of the cores (core_landice, etc)

To my knowledge, only the init atmosphere and atmosphere cores are maintained by MPAS-Dev (@mgduda is this correct?), and I don't want to make changes to cores in the MPAS-Dev managed MPAS-Model that they do not maintain.

mgduda commented 4 months ago

Shouldn't there be a CMakeLists.txt for all of the cores (core_landice, etc)

MPAS-Ocean, MPAS-Albany Land Ice, and MPAS-Seaice are no longer actively developed in the MPAS-Dev/MPAS-Model repository, so I think there's no need to worry about extending the CMake build to those cores. In future, though, we should support the test core and perhaps the sw (shallow water) core.

mgduda commented 4 months ago

@amstokely For the PR description, which we'll use for the commit message in the merge commit, I think we could be a bit more direct in our explanation of the purpose of this PR. How about something that covers at least the following points?

  1. This PR introduces an initial CMake build capability for atmosphere and init_atmosphere core
  2. The initial focus is on supporting MPAS-JEDI, which uses ecbuild / CMake (and so we need MPAS-Model to be a CMake-managed project)
  3. In future, the CMake build capability introduced here can be extended
  4. The changes in this PR are based on work from the JCSDA-internal/MPAS-Model fork, with contributions from ???
mgduda commented 4 months ago

@amstokely For the PR description, which we'll use for the commit message in the merge commit, I think we could be a bit more direct in our explanation of the purpose of this PR. How about something that covers at least the following points?

1. This PR introduces an initial CMake build capability for atmosphere and init_atmosphere core

2. The initial focus is on supporting MPAS-JEDI, which uses ecbuild / CMake (and so we need MPAS-Model to be a CMake-managed project)

3. In future, the CMake build capability introduced here can be extended

4. The changes in this PR are based on work from the JCSDA-internal/MPAS-Model fork, with contributions from ???

Maybe another point:

  1. Which variables / options are recognized or supported?
mgduda commented 4 months ago

@amstokely Also, maybe we should mention that PIO is required (no SMIOL support)?

amstokely commented 4 months ago

@amstokely For the PR description, which we'll use for the commit message in the merge commit, I think we could be a bit more direct in our explanation of the purpose of this PR. How about something that covers at least the following points?

  1. This PR introduces an initial CMake build capability for atmosphere and init_atmosphere core
  2. The initial focus is on supporting MPAS-JEDI, which uses ecbuild / CMake (and so we need MPAS-Model to be a CMake-managed project)
  3. In future, the CMake build capability introduced here can be extended
  4. The changes in this PR are based on work from the JCSDA-internal/MPAS-Model fork, with contributions from ???

Instead of listing specific people as contributors (maybe they don't want to be listed?), can I just link the contributors page from the JCSDA-Internal MPAS-Model fork?

mgduda commented 4 months ago

Instead of listing specific people as contributors (maybe they don't want to be listed?), can I just link the contributors page from the JCSDA-Internal MPAS-Model fork?

Agreed regarding not trying to list individual contributors -- they might not want to be listed, and there's a risk of accidentally leaving someone out of the list. Perhaps something like this?

The CMake build for MPAS-Atmosphere introduced in this PR is based on work from the JCSDA-internal/MPAS-Model fork,
with contributions from the NSF NCAR Mesoscale and Microscale Meteorology (MMM) Laboratory and the Joint Center for
Satellite Data Assimilation (JCSDA).
mgduda commented 4 months ago

I was able to successfully build this PR branch and run a test case with the resulting mpas_atmosphere binary. From my perspective, we're all set after updating the PR description, and perhaps also cleaning up the commit history and commit messages.

amstokely commented 4 months ago

@amstokely For the PR description, which we'll use for the commit message in the merge commit, I think we could be a bit more direct in our explanation of the purpose of this PR. How about something that covers at least the following points?

1. This PR introduces an initial CMake build capability for atmosphere and init_atmosphere core

2. The initial focus is on supporting MPAS-JEDI, which uses ecbuild / CMake (and so we need MPAS-Model to be a CMake-managed project)

3. In future, the CMake build capability introduced here can be extended

4. The changes in this PR are based on work from the JCSDA-internal/MPAS-Model fork, with contributions from ???

Maybe another point: 5. Which variables / options are recognized or supported?

@mgduda What do you think about creating a seperate markdown file (BUILD_CMAKE.md?) that has cmake build instructions. This word avoid editing the README to a significant extent and would allow people to use the cmake build outside of the mpas-jedi team.

mgduda commented 4 months ago

@mgduda What do you think about creating a seperate markdown file (BUILD_CMAKE.md?) that has cmake build instructions. This word avoid editing the README to a significant extent and would allow people to use the cmake build outside of the mpas-jedi team.

I'm not sure if the CMake build in its current form is something we want to widely advertise and support for general stand-alone MPAS-Atmosphere applications. It doesn't support SMIOL, it doesn't support other cores, and it produces a directory structure that doesn't match any of our documentation and tutorial material. So for now, I would propose not changing the README file.

amstokely commented 4 months ago

@mgduda What do you think about creating a seperate markdown file (BUILD_CMAKE.md?) that has cmake build instructions. This word avoid editing the README to a significant extent and would allow people to use the cmake build outside of the mpas-jedi team.

I'm not sure if the CMake build in its current form is something we want to widely advertise and support for general stand-alone MPAS-Atmosphere applications. It doesn't support SMIOL, it doesn't support other cores, and it produces a directory structure that doesn't match any of our documentation and tutorial material. So for now, I would propose not changing the README file.

Ok, that's fair! For now I'll just include general build instructions in the PR description.

mgduda commented 4 months ago

@amstokely The PR description states "For detailed instructions on how to build the atmosphere core with CMake, please refer to BUILD_CMAKE.md." I assume this sentence will be replaced with a few notes on build instructions / options?

amstokely commented 4 months ago

@amstokely The PR description states "For detailed instructions on how to build the atmosphere core with CMake, please refer to BUILD_CMAKE.md." I assume this sentence will be replaced with a few notes on build instructions / options?

Yes! Sorry I was in the middle of writing the updated build instructions that are now in the PR description.