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.

Sign up for GitHub

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

Jump to bottom

Explore CMake-config-only for NetCDF and HDF5 #336

Merged
merged 15 commits into from
Oct 26, 2022
Merged

Explore CMake-config-only for NetCDF and HDF5 #336

merged 15 commits into from
Oct 26, 2022

Conversation

ibaned
Copy link
Contributor

@ibaned ibaned commented Oct 19, 2022

No description provided.

@github-actions
Copy link
Contributor

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

You can retrigger this bot by commenting recheck in this Pull Request

@ibaned ibaned changed the title try to change FindTPLNetcdf.cmake Explore CMake-config-only for NetCDF and HDF5 Oct 19, 2022
@ibaned
Copy link
Contributor Author

ibaned commented Oct 19, 2022

This is only meant as a vehicle to communicate with the TriBITS team on TriBITSPub/TriBITS#533

ibaned added 12 commits October 19, 2022 22:03
@ibaned
Revert "try to change FindTPLNetcdf.cmake"
3b589a5 
This reverts commit 84459b0.
@ibaned
new approach is working for static case
c772d0d
@ibaned
workaround missing find_dependency(hdf5) in netCDF
1a6674c
@ibaned
fix usage and naming of a variable
debeaee
@ibaned
remove debug check for a target
88aada3
@ibaned
support TPL_Netcdf_Enables_Netcdf4 in modern mode
5a44626
@ibaned
add REQUIRED_HEADERS for MPI TPL
876fc4d 
without this, TriBITS will ignore
the variable TPL_MPI_INCLUDE_DIRS
@ibaned
Don't wipe out TPL_<tplName>_INCLUDE_DIRS if set by user (TriBITSPub/…
7eaabfc 
…TriBITS#534)
@ibaned
Revert "add REQUIRED_HEADERS for MPI TPL"
c0bb481 
This reverts commit 876fc4d.
@ibaned
Make Netcdf depend on HDF5, fix TriBITS find modules (TriBITSPub/TriB…
f49ebe6 
…ITS#533)
@ibaned
fix modern HDF5 import (TriBITSPub/TriBITS#533)
2735e7b 
1. provide the minimum version as the one I'm testing with
2. use HDF5_EXPORTED_LIBRARIES since the set of defined
   targets is very hard to predict otherwise
@ibaned
fix modern Netcdf import (TriBITSPub/TriBITS#533)
a4b7510 
TPL_Netcdf_NOT_FOUND still has to be manually
set to FALSE for some reason
@gsjaardema
Copy link
Member

@bartlettroscoe @ibaned Related to this PR, see the following on the netCDF github -- Unidata/netcdf-c#1733 (comment)

@bartlettroscoe
Copy link
Contributor

FYI: This PR is a duplicate of the PR trilinos/Trilinos#11175 to test these changes against Trilinos.

Once these PRs are fully tested, I will pull off these commits and apply them to TriBITS proper. (TriBITS itself has no way to test these changes. The TriBITS repo is just a place to store a common implementation of these that our projects can share.)

@bartlettroscoe
Copy link
Contributor

@gsjaardema, you can go ahead and merge this if you want. This is what I will bring over to TriBITS proper (see trilinos/Trilinos#11175 (comment)). Or, you can wait until I update TriBITS 'master' and then you can update the SEACAS TriBITS snapshot. It is up to you.

@gsjaardema gsjaardema self-assigned this Oct 26, 2022
@gsjaardema gsjaardema marked this pull request as ready for review October 26, 2022 19:36
gsjaardema
gsjaardema reviewed Oct 26, 2022
View reviewed changes
cmake/tribits/common_tpls/FindTPLHDF5.cmake
endif()
if (Netcdf_ALLOW_MODERN)

set(minimum_modern_HDF5_version 1.13.2)
Copy link
Member

Choose a reason for hiding this comment

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

@ibaned Is 1.13.2 really the minimum version that can be used for modern? I thought THG was keeping cmake information current in their 1.12 branches also...

gsjaardema
gsjaardema reviewed Oct 26, 2022
View reviewed changes
cmake/tribits/common_tpls/FindTPLHDF5.cmake
endif()

if (TPL_ENABLE_MPI)
set(HDF5_PREFER_PARALLEL TRUE)
set(REQUIRED_LIBS_NAMES ${REQUIRED_LIBS_NAMES} z)
Copy link
Member

Choose a reason for hiding this comment

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

libz is required for serial builds also. Is there some issue where it isn't specified correctly in the parallel build/config?

@gsjaardema gsjaardema merged commit b32dbdc into sandialabs:master Oct 26, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 26, 2022
@ibaned ibaned deleted the netcdf-hdf5-config branch October 26, 2022 20:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gsjaardema gsjaardema gsjaardema left review comments

Assignees

@gsjaardema gsjaardema

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ibaned @gsjaardema @bartlettroscoe