CMake: Export targets so the build directory can be used directly

[ZedThree]

Along with #2751, this fully enables using the build directory directly in other projects, allowing other projects to include netCDF using things like FetchContent.

I've had to delete the paths in netCDFConfig.cmake.in because they won't exist until netCDF is installed (sort of defeating the point of using the build directory!) -- this should be fine as long as people are using the netCDF::netcdf target rather than these variables. It might be possible to support both, but it might get a bit complicated!

[WardF]

We historically discourage people from using the build directory directly, and encourage people to use the install directory instead; this has been a best practice for a very long time. I'm not opposed to making it easier for people to use their build/ directory if that is what they want to do, but I'm not certain what you mean, specifically, by It might be possible to support both, but it might get a bit complicated. Forgive my obtuseness, I'd like to merge this :), I just want to make sure I understand what you mean. I presume this PR does not make it more difficult for people who use the standard make && make install process? Thanks!

[ZedThree]

The major reason to enable using the build directory directly is to allow embedding in another project, for example as part of a superbuild or a bundled dependency. There are at least a couple of ways to do this in practice, either with a git submodule (or just bundled) and add_subdirectory, or using FetchContent to download it as part of the build process. There are also various CMake package managers like CPM and vcpkg, which generally work using FetchContent -- some of these tools already patch netCDF to make this work.

Basically, in modern CMake, it's a really nice user experience to be able to use the build directory for a project, as well as installing it.

It might be possible to support both, but it might get a bit complicated

Sorry, I was not very clear here! Just so we're all on the same page (and for others reading this), pre-CMake 3 and the introduction of targets, the standard way of using a dependency was with variables like package_LIBRARIES and package_INCLUDE_DIRS. Modern CMake wraps these all up into a target like package::package. The current netCDFConfig.cmake file that gets installed sets both the target and the netCDF_LIBRARIES/INCLUDE_DIR variables (as well as netCDF_INSTALL_PREFIX and netCDF_LIB_DIR) -- but only the target are required.

So, if we delete the netCDF_* variables from the config file, this might possibly break people's CMake files -- but the fix is to switch to use the targets instead, so pretty easy. The targets have been available since 2014 in netCDF v4.3.3, so quite some time!

It is possible to keep the variables, but it would involve generating the config file twice, once for the build directory, once for the install directory. Not a problem, it's just more code to do so!

I presume this PR does not make it more difficult for people who use the standard make && make install process?

Nope! Just adds another way to consume netCDF

[LecrisUT]

I don't think that this will address using FetchContent because that would require running find_package after FetchContent in order to use netCDFConfig.cmake from the build directory which will clash with FetchContent(FIND_PACKAGE_ARGS).

Regardless, this change is still necessary to do functional tests of examples, e.g. to test the packaging example.

If you want to address FetchContent, some necessary points are:

• Make sure relevant target_include_directories, etc. have appropriate PUBLIC interface where needed
• Make sure the options and targets are namespaces, with the latter having add_library(ALIAS)
• Make sure you export the netCDF_VERSION and other such variables that are available in netCDFConfig.cmake via return(PROPAGATE)

[LecrisUT]

There is also configure_package_config_file which seems excessive with

  PATH_VARS
  CMAKE_INSTALL_PREFIX
  CMAKE_INSTALL_INCLUDEDIR
  CMAKE_INSTALL_LIBDIR

And since it uses PACKAGE_INIT it should no have NO_CHECK_REQUIRED_COMPONENTS_MACRO

Otherwise, functionally 👍 this PR should work as is

[ZedThree]

Thanks for the comments here @LecrisUT!

I don't think that this will address using FetchContent because that would require running find_package after FetchContent in order to use netCDFConfig.cmake from the build directory which will clash with FetchContent(FIND_PACKAGE_ARGS).

Previously, I was just checking that netcdf-cxx4 could use the netcdf-c build directory directly, which as you note is a necessary but not sufficient condition for FetchContent to also work. I've now checked FetchContent with the head of this branch in netcdf-cxx4:

include(FetchContent)
FetchContent_Declare(
  netcdf
  GIT_REPOSITORY https://github.com/ZedThree/netcdf-c
  GIT_TAG 25dc1faa603ff885197cbc09983ecb848f570470
)
set(ENABLE_TESTS OFF CACHE BOOL "" FORCE)
FetchContent_MakeAvailable(netcdf)

(+ commenting out the existing machinery for finding netcdf-c)

This does work, but I did also have to turn off tests in netcdf-cxx4 due to a clash with the add_bin_test macros that appear in both projects, as well as tests in netcdf-c due to some internal includes not resolving. But otherwise, it did just work out of the box.
Turning off the tests for NOT PROJECT_IS_TOP_LEVEL would help here, as well as either namespacing those macros, or gating them behind ENABLE_TESTS as well. (@K20shores maybe this is already on your radar?)

Regardless, this change is still necessary to do functional tests of examples, e.g. to test the packaging example.

This is a good idea! Worth adding here, or in a separate PR perhaps? Ideally, I guess this sort of test would check several ways of consuming the package (build dir vs install, for example).

If you want to address FetchContent, some necessary points are:

• Make sure relevant target_include_directories, etc. have appropriate PUBLIC interface where needed
• Make sure the options and targets are namespaces, with the latter having add_library(ALIAS)
• Make sure you export the netCDF_VERSION and other such variables that are available in netCDFConfig.cmake via return(PROPAGATE)

I think @K20shores is already looking into most of these? The ALIAS target is already available with a namespace, and return(PROPAGATE) would require bumping the min cmake version, but everything else is definitely important.

[LecrisUT]

Just some short clarifications:

which as you note is a necessary but not sufficient condition for FetchContent to also work.

FetchContent has different inclusion methods, i.e. it uses add_subdirectory instead of find_package(CONFIG), so getting the project to work for either is an independent task

check several ways of consuming the package (build dir vs install, for example)

All except for install. For that one I run tests on Fedora packaging on top of that (which calls the same test-suite). See the tmt + packit setup if you're curious on how to set it up. Probably I need more explicit documentation on that though.

return(PROPAGATE) would require bumping the min cmake version

Not really, the workaround is a bit verbose but this one you can support both versions with policy CMP0140.

[K20shores]

@ZedThree yes, I am looking into those things you mentioned