Bug fixes for netcdf-c cmake build

[climbfuji]

Description

This PR fixes two bugs in the netcdf-c cmake build that we identified and fixed in our fork.

1. ncgen is missing rpaths for hdf5 and won't run - see JCSDA@27bc9e5
2. Bug fixto find MPI functions when build system is cmake. See CMake can't find MPI_Comm_f2c resulting in unsafe cast in nc_create_par_fortran Unidata/netcdf-c#3199 for details. The solution adopted here is the same as the suggestion made in CMake can't find MPI_Comm_f2c resulting in unsafe cast in nc_create_par_fortran Unidata/netcdf-c#3199, which looks like will make its way into the authoritative codebase soon

Note. This patch is admittedly a bit of a hack. But given the overall state of the netcdf-c CMake build system I believe that this duct-tape solution is good enough. I hope that a overhaul of the entire CMake build for the package will solve this problem in a better way.

Testing

Both patches were tested in https://github.com/JCSDA/spack-stack branch feature/update_to_spack_v1 for intel-oneapi-compilers + intel-oneapi-mpi and gcc + openmpi.

[climbfuji]

"@when" is not working as expected, will fix together with the style errors.

[climbfuji]

Reverting to draft while I am still working on this

[climbfuji]

@skosukhin @WardF This is ready for your review. I found that the original logic to check if the function exists in CMake was not working reliably. It worked for gcc + openmpi when I included the MPI link libraries as in my original attempt. It didn't work for Intel OneAPI. And I think it wouldn't work for many other MPI providers, because there is no way add an #include mpi.h into the CheckFunctionExists.c cmake file that is used for the test.

I will give this a test on one of the Cray machines that I have access to, but the caveat is that the machine doesn't use they Cray compiler wrappers in our spack config (it uses the vendor compilers like on non-Cray machines).

[climbfuji]

@skosukhin @WardF This is ready for your review. I found that the original logic to check if the function exists in CMake was not working reliably. It worked for gcc + openmpi when I included the MPI link libraries as in my original attempt. It didn't work for Intel OneAPI. And I think it wouldn't work for many other MPI providers, because there is no way add an #include mpi.h into the CheckFunctionExists.c cmake file that is used for the test.

I will give this a test on one of the Cray machines that I have access to, but the caveat is that the machine doesn't use they Cray compiler wrappers in our spack config (it uses the vendor compilers like on non-Cray machines).

Update. I tested this on a number of machines, including Cray systems with at least two compilers (GNU and Intel LLVM). For each system/compiler/mpi combination the symbols MPI_{Comm,Info}_f2c are found with this patch.

[climbfuji]

Pinging the reviewers to take a look at this PR, please. Thanks!

[climbfuji]

Pinging the reviewers to take a look at this PR, please. Thanks!

Pinging all the reviewers once more before asking the spack developers to proceed w/o approval. Thanks very much in advance!

@skosukhin @WardF @johnwparent

[climbfuji]

@johnwparent Is there anything I can do to move this PR forward? Thanks!

[johnwparent]

@johnwparent Is there anything I can do to move this PR forward? Thanks!

Nothing from your end, more a bandwidth issue from my end, will be getting to this shortly!

[climbfuji]

@johnwparent Is there anything I can do to move this PR forward? Thanks!

Nothing from your end, more a bandwidth issue from my end, will be getting to this shortly!

Thanks, very much appreciated!

[climbfuji]

@johnwparent Did you have a chance to look at this? Thank you.

[johnwparent]

@johnwparent Did you have a chance to look at this? Thank you.

Thanks for the ping, resolved by #4293

[climbfuji]

@johnwparent Did you have a chance to look at this? Thank you.

Thanks for the ping, resolved by #4293

This PR does more/other stuff than #4293. Which part of this PR #2384 is resolved by #4293?

[johnwparent]

@johnwparent Did you have a chance to look at this? Thank you.

Thanks for the ping, resolved by #4293

This PR does more/other stuff than #4293. Which part of this PR #2384 is resolved by #4293?

Sorry, should have been clear, the CI failure I promised to look into is fixed by that PR

[climbfuji]

@johnwparent Did you have a chance to look at this? Thank you.

Thanks for the ping, resolved by #4293

This PR does more/other stuff than #4293. Which part of this PR #2384 is resolved by #4293?

Sorry, should have been clear, the CI failure I promised to look into is fixed by that PR

Ahh, that makes sense. I'll wait for #4293 to be merged then. Thanks!

[climbfuji]

@johnwparent I pulled in your changes from #4293 and the basic unit tests now pass for Windows. The gitlab-ci tests are still being queued.

[climbfuji]

@johnwparent What do we need to do to move your PR forward so that I can update this PR and we can review & merge?

[johnwparent]

@johnwparent What do we need to do to move your PR forward so that I can update this PR and we can review & merge?

@climbfuji I just need to be able to successfully reproduce the paraview build failure locally. So far no dice. I'll let you know as soon as that's ready

[johnwparent]

Blocker fix is merged, let's rebase and LGTM

[climbfuji]

Blocker fix is merged, let's rebase and LGTM

Thanks, done! Please take another look.

[climbfuji]

This error is new, I haven't changed anything, though, just pulled in develop:

spack.repo.RepoError: cannot load package 'netcdf-c' from the 'builtin' repository: NetcdfC.patch() got an unexpected keyword argument 'sha256'

[johnwparent]

Looks good, thanks!