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 fixes for netCDF #2493

Merged
merged 6 commits into from
Feb 1, 2022
Merged

CMake fixes for netCDF #2493

merged 6 commits into from
Feb 1, 2022

Conversation

johnomotani
Copy link
Contributor

  1. Linking to the github.com master-branch version of netCDF-cxx4 seems to require backporting a fix from next Small CMake changes for MacOS #2455.
  2. Also, using the master-branch version makes the output of ncxx4-config --version be 4.3.2-developer. CMake doesn't like the -developer suffix, so update the regex in FindnetCDFCxx.cmake to remove it.
  3. Remove propagation of NCXX_BINARY_DIR in bout++Config.cmake.in since this variable seems to never be used.
  4. set() the variables NC_CONFIG and NCXX4_CONFIG, if they exist. This allows packages that import BOUT++ to make use of these variables when finding netCDF and netCDF-cxx4, without having to pass them again.

bendudson and others added 5 commits February 1, 2022 14:17
CMake 3.21 on MacOS needs `\\.` rather than `\.`
Making string lowercase before regex matching the version.
When compiled from the git repo, `ncxx4-config --version` may return
something like `4.3.2-developer`. However, CMake's `find_dependency()`
function does not recognize that as a valid version argument. Modifying
the regex to remove anything trailing the `#.#.#` version number fixes
the problem.
NCXX_BINARY_DIR is not defined in cmake/FindnetCDFCxx.cmake, so never
exists.
These variables can be used (rather than netCDFCxx_ROOT and netCDF_ROOT)
to find the netCDF and netCDF-cxx4 libraries. `set`'ing them if they
exist means packages importing BOUT++ should link to the same libraries.
@johnomotani johnomotani added bugfix build-system issues with make/configure/... labels Feb 1, 2022
@johnomotani
Copy link
Contributor Author

@ZedThree this PR seems to fix the problems I was posting about on Slack yesterday. At least, I've managed to compile STORM against this branch.

I'm not at all sure about point 3 in the description above - I couldn't see NCXX_BINARY_DIR being used or set anywhere, but I'm still a CMake newbie, so might well have missed something.

I did also try to use -DBOUT_DOWNLOAD_NETCDF_CXX4=ON, but it was trying to link to the (ancient) system HDF5, not the HDF5 that netCDF-c was linked to. As I have something working now, I'm not going to chase that failure up.

@ZedThree
Copy link
Member

ZedThree commented Feb 1, 2022

I have a feeling NCXX_BINARY_DIR might be used for BOUT_DOWNLOAD_NETCDF_CXX4 -- I'll investigate

Copy link
Member

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

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

LGTM, but the removed NCXX_BINARY_DIR branch is needed for BOUT_DOWNLOAD_NETCDF_CX4

bout++Config.cmake.in Show resolved Hide resolved
Now with explanation of why it is needed.

Co-authored-by: Peter Hill <zed.three@gmail.com>
Copy link
Member

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

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

Lovely, thanks @johnomotani !

@ZedThree ZedThree merged commit 1c44d41 into master Feb 1, 2022
@ZedThree ZedThree deleted the cmake-netcdf-fixes branch February 1, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix build-system issues with make/configure/...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants