Feature/netcdf mpi #43
Conversation
Modules/FindNetCDF.cmake
Outdated
| @@ -231,6 +243,10 @@ foreach( _comp IN LISTS _search_components ) | |||
| if( NOT _comp MATCHES "^(C)$" ) | |||
| target_link_libraries(NetCDF::NetCDF_${_comp} INTERFACE NetCDF::NetCDF_C) | |||
| endif() | |||
| if(NetCDF_PARALLEL) | |||
| find_package(MPI REQUIRED COMPONENTS ${_comp}) | |||
| target_link_libraries(NetCDF::NetCDF_${_comp} INTERFACE MPI::MPI_${_comp}) | |||
There was a problem hiding this comment.
I don't think this is appropriate in FindNetCDF.cmake.
I think the application using NetCDF should be responsible for linking with the MPI library.
e.g.
in wgrib2 or any other application e.g. chgres_cube, one could do one of two things:
- in the application
CMakeLists.txt
if(NetCDF_PARALLEL)
find_package(MPI REQUIRED COMPONENTS C)
target_link_libraries(app/lib INTERFACE|PUBLIC|PRIVATE MPI::MPI_C)
endif()
I think only the C library is required, but I will have to test that.
- explicitly set CC and FC with the MPI wrapper e.g.
export CC=mpiicc
export FC=mpiifort
etc. or whatever the compiler is instead of e.g. CC=icc, FC=ifort
There was a problem hiding this comment.
I'm not really against either of those. In general, find_package links in its dependencies, but I could see it being different for MPI.
There was a problem hiding this comment.
If a serial (non-MPI) application depends on (parallel) netcdf library, it will be much simpler to not need to add if(NetCDF_PARALLEL) .... endif() logic in application's CMakeLists.txt or require CC/FC to point to mpi wrapper scripts. Such program should not need to know anything about internal implementation/dependencies of netcdf.
There was a problem hiding this comment.
I agree with @DusanJovic-NOAA and @kgerheiser that if netCDF depends on MPI, it needs to automatically configure the MPI dependencies for the application that it is used in.
Modules/FindNetCDF.cmake
Outdated
| @@ -171,6 +171,19 @@ function(netcdf_config exec flag output_var) | |||
| endif() | |||
| endfunction() | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
why the additional empty line?
Modules/FindNetCDF.cmake
Outdated
| @@ -231,6 +244,9 @@ foreach( _comp IN LISTS _search_components ) | |||
| if( NOT _comp MATCHES "^(C)$" ) | |||
| target_link_libraries(NetCDF::NetCDF_${_comp} INTERFACE NetCDF::NetCDF_C) | |||
| endif() | |||
| if(NetCDF_PARALLEL) | |||
| target_link_libraries(NetCDF::NetCDF_${_comp} INTERFACE MPI::MPI_C) | |||
There was a problem hiding this comment.
Need indent here.
Also, why does the NetCDF_Fortran library need to interface with the MPI_C library?
This should be done before this for-loop and only for the NetCDF_C library. All other components automatically add NetCDF_C to their interface.
There was a problem hiding this comment.
That makes sense. Admittedly, I didn't really know what INTERFACE was doing.
You can't do that before the for-loop. The library is created a couple lines above.
|
How about now? |
Modules/FindNetCDF.cmake
Outdated
| if(NetCDF_PARALLEL) | ||
| target_link_libraries(NetCDF::NetCDF_${_comp} INTERFACE MPI::MPI_C) | ||
| endif() |
There was a problem hiding this comment.
_comp is C in this else block. Making it explicit doesn't change the result, just makes reading easy-er.
Is an indent missing? Or is this a tab issue?
Otherwise looks good.
There was a problem hiding this comment.
I made it explicit.
I don't know why it shows like that. See how it looks from here
There was a problem hiding this comment.
When you checkout your branch, the indentation is bonkers. See attached screen shot.
Also, get rid of trailing blanks on Line 181.
You are using tabs and I think the tab-expansion in your editor is turned off or messed up.
|
I think it's actually fixed now |
Find and link to MPI when NetCDF parallel is detected. Otherwise, user has to do it manually.
See: NOAA-EMC/wgrib2#15
Also, affects UFS_UTILS and anything else that links to NetCDF.
Tested on Jet and Orion