Cmake changes: Now and into the future

[K20shores]

Howdy!

As part of a pilot program between ACOM and Unidata with @WardF hosting me, I've been doing some work in the netCDF repository. I have some decent experience with cmake and the first (and maybe only thing since it's a short program) I am doing is trying to modernize cmake with the explicit goal of making consumption of netCDF by other projects easier, allowing fetch_content to be used, if it's not already available.

Most of my knowledge has come from the book Professional CMake: A Practical Guide (which I highly recommend).

Much of what I'm doing will address some things in #2713.

Below is a list of PRs that I've already submitted and what they contained.

• Manage project version with cmake #2835
  • Places the project version in the cmake project command which populates cmake internal variables and then uses them throughout the project
• Place dependencies into separate file #2838
  • Part of enabling fetch_content is making it easy for cmake to manage dependencies, or at least find them. This PR pulled all of the dependencies out of the top level cmake and placed them into cmake/depdendencies.cmake
• Macros functions #2842
  • I removed all of the functions and macros from the top level cmake file and placed them into cmake/netcdf_functions_macros.cmake
• Use cmake netCDF with target_* for many options #2847
  • moved the netcdf cmake target to the top level directory so that things like compile defintions, linked libraries, and include directories are set only on the cmake target and not globally
• Remove now unused cmake #2890
  • shows that netcdf no longer adds hdf5 libraries to it's cmake and pkg config files that don't exist
• adding an integration test to show fetch_content can work #2894
  • shows that netcdf can be included successfully with fetch content and adds an integration test for this, as well as documentation
• Prefix all options with NETCDF_ #2895
  • prefixes all cmake options with NETCDF_

Here a list of changes that I would to make over the remaining 2+ months that I have left. I'm listing them here so that the broader community can see what work is going on and comment about it so ensure that we arrive at a solution that works for everyone. I'm doing my best to keep the PRs small on purpose, though task 1. below will likely be the largest.

What I'm working on right now

1. Part of the aforementioned book includes a recommend directory structure.

:: tree . 
.
├── CMakeLists.txt
├── cmake
├── packaging
└── tests

as well as a recommended top level CMakeLists.txt setup, which you may recognize as something I'm working towards

# Preamble
cmake_minimum_required(VERSION 3.21)
project(MyProj)

# Project wide setup
list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR}/cmake)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED YES)
set(CMAKE_CXX_EXTENSIONS NO)

# Externally provided content brought into the current scope.
include(dependencies.cmake)

# Main targets built by this project
add_subdirectory(src)

# Not typically needed if there is a parent project
if(PROJECT_IS_TOP_LEVEL)
    add_subdirectory(tests)
    add_subdirectory(packaging)
endif()

Explicitly, at present I am attempting to place all packaging into the packaging directory so that netCDF could be built and delivered entirely within cmake. At the same time, I've had to define the netCDF library as a top level target earlier on than before so that compile definitions are not applied globally.

This is a step in the direction of supporting #2843 since we would no longer need autotools to support libtools versioning.

Future

2. ensure that the install includes all necessary cmake things to generated a FindNetDDF.cmake file if it doesn't already and test/fix anything that doesn't work with fetch_content
3. Place all options near the top of the file
4. move autotools definitions to an separate file
5. Rework the hdf5 package finding. If possible, place it into a FindHDF5.cmake file, or bump the cmake required version to 3.24 and bring in hdf5 with fetch_content if hdf5 supports this. If we can bump the version, then fetch_content will preferably use installed version of hdf5 before pulling it from git and compiling it from source
6. If it makes sense, place all set commands used for working variables shortly after the the options, but this

Up until this point I've just done things, mostly because they've been small. But moving forward I would like to create issues for each thing I do, since, you know, that's the better and more appropriate way to do things.

@WardF, @DennisHeimbigner, @LecrisUT, @edhartnett, @ZedThree I would appreciate any feedback or direction you'd like to give.

Cmake conventions:

• lowercase function calls
• set compile definitions, include directories, only on targets
• prefix each option with NETCDF_ (needs to be applied

[LecrisUT]

Overall 👍. Just some minor details which would probably already be addressed in the actual PRs

About the folder structure, I'm not sure how common /packaging is. I usually go with that content at the bottom of src, especially when it's a multi-project format. That would be useful if some files are to be copied as-is.

Also, instead of if(PROJECT_IS_TOP_LEVEL) go for individual options like NETCDF_INSTALL so that downstream has controll. I usually also include NETCDF_TESTS and NETCDF_SHARED_LIBS.

[ZedThree]

+1 for individual options that default to PROJECT_IS_TOP_LEVEL:

option(NETCDF_TESTS "Build the tests" ${PROJECT_IS_TOP_LEVEL})

[LecrisUT]

Crucial point that you should add: namespacing of options and targets. Currently it is inconsistent. Among those it is also important to check all cache variables such that no CMAKE_ ones are set as cache within the project (it will conflict with downstream).

[LecrisUT]

If possible, place it into a FindHDF5.cmake file, or bump the cmake required version to 3.24 and bring in hdf5 with fetch_content if hdf5 supports this.

Probably you are already familiar, but don't use FetchContent within Find<Package>.cmake. Instead if you want FetchContent(FIND_PACKAGE_ARGS), implement a small wrapper. Here's a simple implementation (note a few bugs, such as these should be macro instead of function)

[ZedThree]

I would also say it would hopefully be possible to entirely rely on CMake's built in FindHDF5. Essentially all of the logic for finding HDF5 currently could be as simple as:

find_package(HDF5 ${HDF5_VERSION_REQUIRED} COMPONENTS C HL REQUIRED)

and then using the imported target hdf5::hdf5

[K20shores]

#2847 includes this change (thanks @ZedThree!)

There are two related issues to hdf5 that I want to investigate next, listed here for posterity:

• cmake exported target uses undefined hdf5::hdf5 imported target #1444
• netcdf-c: An incorrect hdf5 library name is added to pkconfig and cmake config  spack/spack#42878
• and this: netCDFTargets.cmake and pkgconfig netcdf.pc have wrong hdf5 libraries #2869

[K20shores]

Related, #2890

[ZedThree]

Thanks @K20shores, this looks really good! Maybe you could look at #2774, which I think is the minimal changes required to get fetch_content to work.

I've found this CMake project template really useful, it has quite a few modules that can be dropped in, like coverage and sanitizers.
That template also moves all the options to a separate file, which is quite nice.

Something that's not on your list that I think perhaps should be is removing all the global add_definitions and include_directories, and replacing them with target_compile_definitions and target_link_libraries

[K20shores]

#2894 shows that fetch_content indeed already works. I added some github actions to run an integration test to show that it works. Only the windows builds are failing, but that has nothing to do with netcdf, and everything to do with other libraries not being found or linking incorrectly. Hopefully those issue will be fixed soon

[DennisHeimbigner]

prefix each option with NETCDF_ (needs to be applied)

I assume that this means that we specify options as "-DNETCDF_XXX".
This would cause a lot of complaints from our users.

[LecrisUT]

Yes, but not having namespaced options means that the project cannot always be consumed via FetchContent which is the modern recommendation of how to link to projects. This issue is particularly important when there are chains of dependencies.

Unfortunately CMake does not have option deprecation, only target deprecation, and although such deprecation can be manually implemented, I am not sure it is worth it over proper communication.

In principle this change will primarily affect packagers, but they will find out about this breakage immediately as files will be missing. For the general users, they would be reading the documentation on how to build and what options are available anyway, so the renaming will not really affect them.

[K20shores]

#2895 addressed this

[K20shores]

Another possible thing to do is figure out how to remove the global HAVE_CONFIG_H https://github.com/Unidata/netcdf-c/pull/2847/files#r1469377349

[K20shores]

#2847 (comment)

[K20shores]

Remove uses of glob: https://github.com/Unidata/netcdf-c/pull/2847/files#r1468249196

[K20shores]

Create an interface library to replace TLL_LIBS #2847 (comment)

[K20shores]

use build in modules to find the jpeg library: https://github.com/Unidata/netcdf-c/pull/2847/files#r1468245297