Skip to content

Provide Transient Properties on netCDF Library#1540

Merged
WardF merged 2 commits intoUnidata:masterfrom
kprussing:feature/cmake-targets
Nov 22, 2019
Merged

Provide Transient Properties on netCDF Library#1540
WardF merged 2 commits intoUnidata:masterfrom
kprussing:feature/cmake-targets

Conversation

@kprussing
Copy link
Copy Markdown
Contributor

CMake provides the means to track the transient dependencies of a library, like its include directories, by setting the appropriate property on the target(s). This pull request does two things. The first add the include directories to the netcdf library so that downstream usage only requires adding the link library. Specifically

add_executable(myprog ...)
target_link_libraries(myprog netcdf ...)
target_include_directories(myprog ${netCDF_INCLUDE_DIRS} ...)

becomes

add_executable(myprog ...)
target_link_libraries(myprog netcdf ...)

This helps CMake to correctly track the implicit dependencies and get linking orders correct.

The second change (second commit) is adding a namespace for the netCDF targets. Namespaces are advised by most tutorials (I'm thinking this one in particular), but are in reality not critical.

Closes #1539

Modern CMake tracks the properties of targets so that down stream
libraries do not have to worry about include directories.  However, the
include directories must be added to the target and not just at the
directory level.
The usual convention with modern CMake is to namespace the targets.
This gives the netCDF namespace to the package.
@kprussing kprussing requested a review from WardF as a code owner November 19, 2019 20:06
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Nov 19, 2019

CLA assistant check
All committers have signed the CLA.

kprussing added a commit to kprussing/cloudgen that referenced this pull request Nov 19, 2019
The changes to netCDF have been submitted as a [PR](Unidata/netcdf-c#1540).
This update is a temporary fix while the request is reviewed.
@WardF WardF self-assigned this Nov 19, 2019
@WardF WardF added this to the 4.7.3 milestone Nov 19, 2019
@WardF
Copy link
Copy Markdown
Member

WardF commented Nov 19, 2019

Thanks!

@kprussing
Copy link
Copy Markdown
Contributor Author

It just dawned on me that I might have gotten in my own way with the namespacing. The second commit could break downstream builds that rely on the library being named netcdf. I should do a couple more checks before you merge this back to master. I'll check when I get back to the office in the morning and update with my findings (or tonight if I need to fiddle ;-))

@kprussing
Copy link
Copy Markdown
Contributor Author

kprussing commented Nov 20, 2019

I just checked, and CMake usage works with both netcdf and the namespaced version netCDF::netcdf in a target_link_libraries. So, merging should not cause issues with downstream projects.

As a followup, should a note on usage with CMake be added to a particular part of the documentation? This pull request notes how to use netCDF in a CMake build, but I don't see anywhere in the documentation that looks appropriate.

@WardF WardF merged commit 808803c into Unidata:master Nov 22, 2019
kprussing added a commit to kprussing/cloudgen that referenced this pull request Apr 29, 2021
The patch that was referenced in the CMakeLists was [merged upstream][merge].

[merge]: Unidata/netcdf-c#1540
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.

Add Include Directories as Transient Properties for CMake

3 participants