Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cmake #110

Draft
wants to merge 5 commits into
base: noah_om_grid
Choose a base branch
from
Draft

Cmake #110

wants to merge 5 commits into from

Conversation

drakest123
Copy link

@drakest123 drakest123 commented May 7, 2024

This PR contains a first version of CMake files that build noah-owp-modular.exe and noahowp_driver_test.exe in the noah_om_grid branch. The CMake files reference a new file, FindNetCDF.cmake, which searches the filesystem for netCDF-related files. These build files were developed and tested on MacOS Sonoma (v14.4.1) and contain stubs for building on other OSes, however, they have not been tested on other OSes and are unlikely to work on them without modification. Although the original Makefiles are not referenced by the CMake files, they are left in-place until this new build system has been tested on all supported platforms. Three files were moved to limit circular dependencies. These are:

bmi/bmi_noahowp.f90 -> src/bmi_noahowp.f90
driver/AsciiReadModule.f90 -> src/AsciiReadModule.f90
driver/OutputModule.f90 -> src/OutputModule.f90

Compilation occurs outside of the source code directory. CMake usage is documented in the root CMakeLists.txt file and is duplicated here:

# Usage: from noah-owp-modular directory type:
# mkdir ../build
# cd ../build
# cmake ../noah-owp-modular
# cmake --build . -- VERBOSE=1
# cd ../noah-owp-modular/run
# ../../build/noah-owp-modular.exe namelist.input
# cd ../test
# ../../build/noahowp_driver_test.exe ../run/namelist.input

CMakeLists.txt Outdated
endif()

# NetCDF compiler flags
add_compile_options("$<$<COMPILE_LANGUAGE:Fortran>:-I ${NETCDF_INC_DIR};-L ${NETCDF_LIB_DIR} -lnetcdff>")
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, these flags would come from target_link_libraries

Copy link
Author

Choose a reason for hiding this comment

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

Wow, that was fast! I was still writing the PR description. I'll try your suggestion in then next version. Thanks!

CMakeLists.txt Outdated
message(FATAL_ERROR "G95 Fortran compiler not configured.")
elseif(CMAKE_Fortran_COMPILER_ID STREQUAL "GNU")
# Mac OS Gfortran F90FLAGS and FREESOURCE flags
add_compile_options("$<$<COMPILE_LANGUAGE:Fortran>:-g;-cpp;-fbacktrace;-Wall;-fcheck=all>")
Copy link
Contributor

Choose a reason for hiding this comment

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

Options like -g and -Wall are specific to what any particular user wants in a given build. Thus, they should not be hard-coded in the project's CMake file.

If the project wants to enforce warning clean development, -Wall -Werror could go in CI instead.

Copy link
Author

Choose a reason for hiding this comment

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

Sound good. I was replicating the flags in config/user_build_options* in PR #102.

src/CMakeLists.txt Outdated Show resolved Hide resolved
@drakest123
Copy link
Author

@PhilMiller: I responded to your notes. Could you please take another look?

FindNetCDF.cmake Outdated
Comment on lines 23 to 24
set (NETCDF_F90 "YES")
find_package (NetCDF REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be uncommented here?

Copy link
Author

Choose a reason for hiding this comment

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

Removed these lines from CMakeLists.txt and removed find_package() from FindNetCDF.cmake .

@PhilMiller
Copy link
Contributor

The changes look fine so far. Ping me again when this comes out of draft, please. Thanks for digging into these build system improvements.

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.

2 participants