Initial CMake Refactoring

[program--]

This PR resolves #629, resolves #585, resolves #581, resolves #586, and resolves #565.

Changes

• Reorganizes the root CMakeLists.txt into clearly defined sections, in the order of:

  1. Policies
  2. Project Variables
  3. Options
  4. Dependencies
  5. Project Targets
  6. Build Summary

• Flattens the cmake/ directory and sets it as the module dir, since there is no need for cmake/modules.

• Updates ngen CMake configure options to have consistent naming:

  • NETCDF_ACTIVE -> NGEN_WITH_NETCDF
  • MPI_ACTIVE -> NGEN_WITH_MPI
  • UDUNITS_ACTIVE -> NGEN_WITH_UDUNITS
  • ... etc. See the third section of the root CMakeLists for the rest

  Note
  This does not currently affect preprocessor definitions.

  Note
  Previous configure options/env vars will be supported, but will
  notify at configure-time that they are deprecated. Users should switch
  to using new option names.

• Removes all calls to cmake_minimum_required outside of the root CMakeLists

• Adds imported target for NetCDF

• Adds imported target for iso_c_fortran_bmi

• Update ngen-defined add_test to ngen_add_test and use CMake argument parsing rather than explicit numbering to define test sources/linking/construction dependencies.

• Fixes target linking, such that new targets should only link to direct dependencies, and not transitive dependencies. (i.e. test targets linking directly to NetCDF when NGen components expose NetCDF linking)

• Updates dynamic_sourced_cxx_library to use file(GLOB_RECURSE ... CONFIGURE_DEPENDS ...) instead of calling a system command.

• Adds deprecated_option function, which allows for compatibility between new and old options.

Notes

I removed the FindNetCDF.cmake module in cmake/ rather than the one in cmake/modules/ since it was more up to date (or at least had more recent commits...) -- however, I should note the one in the root cmake/ directory created NetCDF targets. I'm not 100% on the reason for both, but I'm assuming here the root one was not working correctly.

TODO

• Create imported target for iso_c_fortran_bmi library when NGEN_WITH_FORTRAN is enabled.

• Update relevant documentation.

  Important
  Maybe this should be updated in the github wiki?
  We are a bit fragmented as far as documentation goes.

Checklist

• PR has an informative and human-readable title
• Changes are limited to a single goal (no scope creep)
• Code can be automatically merged (no conflicts)
• Code follows project standards (link if applicable)
• Passes all existing automated tests
• Any change in functionality is tested
• New functions are documented (with a description, list of inputs, and expected output)
• Placeholder code is flagged / future todos are captured in comments
• Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
• Reviewers requested with the Reviewers tool ➡️

[PhilMiller]

It would vaguely make sense to move this and everything that touches the target ngen down to src/CMakeLists.txt, but I don't see that as essential

[PhilMiller]

Should bmi_cxx be referenced directly from the top-level CMakeLists.txt?

[program--]

No I don't think so, but I'm not sure the reason for the extern/bmi-cxx submodule, since we already have a vendored/modified version in include/bmi.hpp. I think bmi.hxx is used for models in extern/, whereas ngen uses include/bmi.hpp (based on a quick search for bmi.hxx)

[mattw-nws]

The thinking here was actually that we should stop using include/bmi.hpp--I think it is identical--and point to the upstream version...sort of as a matter of principle. Dogfooding, in a certain way. We are adamant that formulations must use an identical header to "ours" (for fairly obvious reasons of compatability)... what we really mean is that conformance to the official BMI header is required, both for semantic and technical reasons. So, do what we say we do* and use a copy that can't be modified (practically)... and encourage the formulations to do the same, in fact.

*Except in the case of Fortran where we modify it to work with ISO_C, and then we say use our modified version. There's an argument that should be an additional "language" and live up at CSDMS's GitHub, in fact, but that's another story and this is an oddball exception that doesn't really change any of the API signatures in a usual sense but changes how it's interpreted by the compiler.

[PhilMiller]

I'm really happy with this. It's a big enough change that it's maybe worth having others look over it, too.

Regardless, everyone on the team needs to be aware of its impact

[mattw-nws]

I'm not 100% on the reason for both...
I think I added the 2nd one (the one kept?) not realizing that there was already one not in modules. Plain accident.

[mattw-nws]

For other reviewers... that is, it uses them also... So this makes either NetCDF_ROOT or NETCDF_ROOT work.

[mattw-nws]

I added this when trying to get the build working on Hera... I don't remember exactly the symptoms, but I was having a hard time getting everything to use icpx, and adding this fixed it. This was a scenario where you had to load up the GCC9 module to get its stdlib, and then the OneAPI module to get icpx... but certain things (gfortran/mpif90?) kept going back to using GCC toolchain, and when I added this that problem stopped happening.... if I remember correctly (that was close to a year ago). It could be that something else is the "right" solution and this fixed it by accident, but I'm not sure what the right answer is then.

[program--]

Hmm maybe the IntelDPCPPConfig.cmake file is different on Hera -- the one I have doesn't modify anything besides SYCL variables. I also have to explicitly set INTEL_DPCPP for this branch to execute, even with IntelLLVM as my toolchain... so maybe it's an environment requirement. In any case, I don't think there's an issue with keeping this section of code in, so I'll modify the comment to provide context and leave it as is 😄

[mattw-nws]

I'll add (for posterity) that there is probably a smarter way of figuring out whether it ought to be turned on (i.e. turn on if CMAKE_CXX_COMPILER is icpx (which is non-trivial if it's mpicxx) or something) but I just kludged in a -D solution.

[PhilMiller]

"If CMAKE_CXX_COMPILER is icpx" would be a direct test of if (CMAKE_CXX_COMPILER_ID STREQ IntelLLVM)

This block is sketchy, but not really a big deal right now.

For context, the Intel compilers (and Nvidia, not that it matters for us) don't ship their own implementation of the standard library or the compiler-ABI-defining headers. Rather, they defer to a reference version of GCC either based on PATH or a command-line argument setting the reference explicitly to get their libstdc++. That's why they need the GCC9 module (for instance) loaded - it has to be a version that supports the language standard icpx is compiling for, but crucially, not one whose headers rely on language version support beyond what icpc implements. It's a really delicate dance, with poorly documented compatibility.

[mattw-nws]

Consideration for follow-on might be to go ahead and add_subdirectory (plus build directory) for iso_c_fortran_bmi, similar to what is done for test_bmi_cpp, since the former is essentially a requirement when NGEN_WITH_BMI_FORTRAN is ON--this would guarantee that it gets built. Right now if the user enables fortran they're required to manually build iso_c_fortran_bmi.

[PhilMiller]

I was seeing a local build failure with this branch that doesn't occur on master:

cd \
/Users/phil/Code/noaa/ngen/build_bmi_conformance/test \
&& \
/Library/Developer/CommandLineTools/usr/bin/c++ \
-DNETCDF_ACTIVE \
-DNGEN_BMI_CPP_LIB_TESTS_ACTIVE \
-DNGEN_BMI_C_LIB_ACTIVE \
-DNGEN_BMI_C_LIB_TESTS_ACTIVE \
-DNGEN_SHARED_LIB_EXTENSION \
-DNGEN_WITH_SQLITE3 \
-DNGEN_WITH_UDUNITS \
-I/Users/phil/Code/noaa/ngen/include \
-I/Users/phil/Code/noaa/ngen/include/simulation_time \
-I/Users/phil/Code/noaa/ngen/include/core \
-I/Users/phil/Code/noaa/ngen/include/core/catchment \
-I/Users/phil/Code/noaa/ngen/include/core/nexus \
-I/Users/phil/Code/noaa/ngen/include/core/hydrolocation \
-I/Users/phil/Code/noaa/ngen/include/core/mediator \
-I/Users/phil/Code/noaa/ngen/include/realizations/catchment \
-I/Users/phil/Code/noaa/ngen/include/geojson \
-I/Users/phil/Code/noaa/ngen/include/forcing \
-I/Users/phil/Code/noaa/ngen/include/utilities \
-I/Users/phil/Code/noaa/ngen/extern/pybind11/include \
-isystem \
/opt/homebrew/include \
-isystem \
/Users/phil/Code/noaa/ngen/test/googletest/googletest/include \
-isystem \
/Users/phil/Code/noaa/ngen/test/googletest/googletest \
-std=gnu++14 \
-arch \
arm64 \
-isysroot \
/Library/Developer/CommandLineTools/SDKs/MacOSX13.1.sdk \
-mmacosx-version-min=12.6 \
-fvisibility=hidden \
-MD \
-MT \
test/CMakeFiles/test_partition.dir/utils/Partition_Test.cpp.o \
-MF \
CMakeFiles/test_partition.dir/utils/Partition_Test.cpp.o.d \
-o \
CMakeFiles/test_partition.dir/utils/Partition_Test.cpp.o \
-c \
/Users/phil/Code/noaa/ngen/test/utils/Partition_Test.cpp


/Users/phil/Code/noaa/ngen/test/utils/Partition_Test.cpp:2:10: fatal error: 'gmock/gmock.h' file not found
#include "gmock/gmock.h"
         ^~~~~~~~~~~~~~~
1 error generated.

I just pushed a (rebase and) fix

[PhilMiller]

Matt and I are seeing various issues with this on the UCS systems. We'll try to get that worked out today, but merging should be held until then.