-
Notifications
You must be signed in to change notification settings - Fork 45
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
Update MPI, HDF5, and Netcdf TriBITS find modules (#533, #534) #535
Update MPI, HDF5, and Netcdf TriBITS find modules (#533, #534) #535
Conversation
this is required to fix TriBITSPub#534
when this CMake cache variable is set to a true value, the TriBITS Netcdf TPL code will only look for externally installed CMake config files for CMake packages called netCDF and possibly hdf5. Such files are installed by the latest versions of NetCDF and HDF5. This effectively allows loading NetCDF and its HDF5 dependency using the most modern CMake approach. This is needed to resolve TriBITSPub#533
This reverts commit e49070d29fdd8c1d525bb22c52d7b2745271d7e2. This causes configure failures.
This makes Netcdf depend on HDF5 using modern CMake targets. ToDo: Finish this commit message.
1. set the minimum version to the one I'm testing with 2. use HDF5_EXPORT_LIBRARIES since the targets defined are hard to predict
1. don't expect HDF5 target to be set. in the general case, Netcdf may not use HDF5, but also HDF5 has no predictable catch-all target 2. TPL_Netcdf_NOT_FOUND still has to be manually set to FALSE
Okay, given above, hopefully these new I will merge this after the merge of: We can always tweak this later if needed. |
With the merge of:
we can finally merge this PR. |
Hello @KyleFromKitware, could you please take a look at this PR. This is an example of where this updated TriBITS external package/TPL system provides the tools to fix up these external packages that are not quite following proper usage of CMake. If you focus on the new code added to these modules, you will see it is pretty minimal (and is mostly print statements). And, actually, I just realized that I did not add a test for TriBITS core change |
endif() | ||
|
||
endif() | ||
|
||
set(HDF5_FOUND_MODERN_CONFIG_FILE ${INTERNAL_IS_MODERN} CACHE BOOL "True if HDF5 was found by the modern method" FORCE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
${INTERNAL_IS_MODERN}
should be quoted to ensure that set()
receives a value... though I see from above that it will always have a value, so perhaps it doesn't matter. Still, quoting it makes this explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point I never considered. If INTERNAL_IS_MODERN
is empty (or undefined), this statement just produces a regular list variable with the value:
"CACHE;BOOL;True if HDF5 was found by the modern method;FORCE"
Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it turns out that if it's empty or undefined, it will simply set a cache variable with an empty value. However, readers won't know this just from glancing at the code, and for that reason I still recommend quoting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that for regular command calls, not quoting an arg can help catch errors where an input args is empty. For example, if you have a function:
function(my_func arg1 arg2 arg3)
...
endfunction()
and calling this with:
my_func(${var1} ${var2} ${var3})
will result in a CMake error if any of these 3 vars are empty.
But this logic does not apply to set()
statements.
We really need a linter for CMake code to catch issues like missing quotes for ${val}
in set(<var> ${val} CACHE ...)
calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it turns out that if it's empty or undefined, it will simply set a cache variable with an empty value. However, readers won't know this just from glancing at the code, and for that reason I still recommend quoting it.
@KyleFromKitware, then what is the benefit of quoting the ${val}
argument in set(<var> ${val} CACHE ...)
if it does not impact behavior? In my example above, quoting the argument can change behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then what is the benefit of quoting the
${val}
argument inset(<var> ${val} CACHE ...)
if it does not impact behavior?
It assures the reader that there won't be a problem. I had to test the behavior of set(... CACHE)
to check that it would work without quoting, whereas I knew instantly just by looking at it that it would work with quoting. I believe it's better to be explicit than implicit. Just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point. There really needs to be a CMake linter to catch issues like this automatically. We should not expend too much time in PR reviews dealing with low-level language issues like this. That is why they make linters in the first place.
In the future, we will need your help reviewing these types of changes when they are being created and tested in the Trilinos github repo. Once we being this back to TriBITS (as the official repo for these common TPLs) we can't test them easily.
This file gets included in a TriBITS project which must have CMake 3.17+.
I am looking over how to test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the other changes are very hard to test as noted, I will approve this PR to be merged.
In the future, we need to be reviewing these changes in the Trilinos and SEACAS PRs where they can be tested.
Addressed one change but the other suggested changes are quite hard to fully test and these are not defects
if (TPL_ENABLE_MPI) | ||
set(REQUIRED_LIBS_NAMES ${REQUIRED_LIBS_NAMES} pnetcdf) | ||
endif() | ||
if (Netcdf_ALLOW_MODERN AND HDF5_FOUND_MODERN_CONFIG_FILE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After seeing how to use this in SEACAS --
This does not seem to work if HDF5 is added as a dependency only because of netCDF since at this point, I don't think that we have yet found HDF5, so HDF5_FOUND_MODERN_CONFIG_FILE
will not be set?
At least for SEACAS, I think that the Netcdf_ALLOW_MODERN
should drive this and determine whether to find HDF5 the modern/legacy way? Does that make sense?
Or, do I reorder things such that CGNS (which depends on HDF5) comes first and then netCDF?
@bartlettroscoe @ibaned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, do I reorder things such that CGNS (which depends on HDF5) comes first and then netCDF?
@gsjaardema, you specify the dependencies as: HDF5 <-- CGNS <-- Netcdf
See:
It looks like GGNS already has a required dependency on HDF5 as per:
TriBITS/tribits/common_tpls/FindTPLCGNSDependencies.cmake
Lines 1 to 2 in 9fd56d2
tribits_extpkg_define_dependencies( CGNS | |
DEPENDENCIES HDF5) |
But I had no idea that Netcdf depended on CGNS?
Also not that, currently, TriBITS does not automatically enable upstream TPL dependencies for an enabled TPL. So the configure script will need to manually enable all of the TPLs it wants. (See NOTES at bottom of slide 16 "TriBITS External Packages/TPLs Dependencies (New!)" in the presentation TriBITS Modernization Update I gave last Thursday, Oct 27, 2022.)
NetCDF doesn't depend on CGNS. They both depend on HDF5, so I thought that maybe configuring CGNS first would then set the If netCDF is configured before CGNS, then that if won't be triggered because it is netCDF that triggers HDF5 if I am understanding things correctly (probably not) |
@gsjaardema, the HDF5 TPL will always be configured before Netcdf TPL, because the TPLs are processed in the order they are listed in the TPLsList.cmake file which is the case with SEACAS as shown at: |
This is a portion of the output of my CMake configuration:
Doesn't that show that Netcdf is being found and then since netcdf depends on HDF5, then HDF5 is found/processed? Note that HDF5 is an optional dependency wherever it is mentioned in the SEACAS
Thanks for trying to explain. |
@gsjaardema, can you please attach the full cmake STDOUT+STDERR? What are the dependencies of Pnetcdf? Do we need to add |
endif() | ||
if (Netcdf_ALLOW_MODERN) | ||
|
||
set(minimum_modern_HDF5_version 1.13.2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ibaned @bartlettroscoe
Sorry for not noticing this prior to merging.
What is needed in the HDF5-1.10.X and HDF5-1.12.X versions to make them usable for this. Both of those versions are still in development, so it would be good to fix their CMake configurations if possible.
I can either contact the HDF5 group with the suggested changes, or one of you can also. Note that we have priority support with HDF5, so they will listen and most likely make useful changes that we suggest.
PnetCDF only depends on MPI and netCDF optionally depends on PnetCDF in a parallel build. SEACAS only accesses / depends on PnetCDF if netCDF was configured in parallel and with PnetCDF capability enabled. There isn't any direct access from SEACAS that bypassed netCDF and hits PnetCDF. |
|
Then this dependency between the TriBITS
In case we do turn on TPL dependencies by default, this might get changed to:
That would mean that if the user set How common is it to enable MPI but not build |
@gsjaardema, the problem is that the HDF5 TPL is not being enabled as can be see in the output seacas-config-debug.txt showing:
Someone needs to set Currently, TriBITS is not automatically enabling the Can you add |
I still confused as to why the netCDF configuration isn't the place where this information is handled. That is the current behavior and it seems to be working in most situations... netCDF has a few optional dependencies depending on how it is configured:
We will also have that issue with HDF5 soon if we start adding some of the advanced HDF5 capabilties (VOLs will add optional dependencies); multiple compression library options. As I see it, only the netCDF configuration files can fully know what optional dependencies are present in the library... |
The TriBITS allows us to fix issues with these external packages when they are not doing the correct behavior. |
Ok, that was the missing piece for me -- |
Hi! I'm coming in late to the conversation, I gather essentially that this PR broke something? Could we maybe open an issue on the SEACAS or TriBITS repositories just to track what broke and I can get a better understanding? |
@ibaned Nothing broke that I am aware of, I am just not sure it is working the way it should -- I am unable to get the |
Ah I see! If it helps, here is the CMake configuration that I'm using:
|
It is probably the Also, do you know what HDF5-1.10.X is missing to allow it to use the |
Maybe nothing! I was being overly conservative in listing just the versions installed on my system. |
@gsjaardema, can we set up a short MS Teams meeting so I can explain to you what is happening and how TriBITS is fixing this? |
@gsjaardema, the key is that when setting
Look at the file:
and you will see this. And it is possible to extend this TriBITS support to automatically fix other situations as well. |
Description
These commits address #533 and #534
These commits were pulled off of the branch from:
(which also matches the patches in sandialabs/seacas#336).
This seems to maintain near perfect backwards compatibility.
Tasks
Add test for change to... Cost of adding a test for this is not worth the value (see below).tribits/core/package_arch/TribitsTplFindIncludeDirsAndLibraries.cmake