FindNetCDF.cmake should not ignore NetCDF_<COMP>_PATHS if MPI_<COMP>_FOUND is TRUE.

[bartgol]

The helper function initialize_paths completely overwrites the user-provided NetCDF_<COMP>_PATHS variable if MPI_<COMP>_FOUND is TRUE.

Instead, it should append additional paths to it.

[dqwu]

@bartgol Could you please provide more information on possible configuration failures caused by this issue? Also, if you can share your steps and settings on your testing machines to reproduce this issue, we can try to reproduce it on other machines we have access to.
PS, do you already have a fix? Otherwise we can start working on it if we are able to reproduce it on some machines.

[bartgol]

@dqwu , here's a simple reproducer of the problem:

set (MyVar "S_val_pre" CACHE STIRNG "") 
function (my_func VAR)
  set (Temp)
  # Uncomment these to see the issue
  # list (APPEND Temp "some_value")  # <--- Key line
  set (${VAR} ${Temp} PARENT_SCOPE)
endfunction()
 
message ("MyVar pre: ${MyVar}")
my_func(MyVar)
message ("MyVar post: ${MyVar}")

Some comments:

1. With the key line commented, MyVar retains its value even after the call. That's due to the fact that MyVar is a CACHE var, so if Temp is empty, the last line in the function is just set (${VAR}), which, for CACHE vars, has no effect (it would be different if VAR was a normal variable)
2. With the key line uncommented, then MyVar does change value after the fcn call. That's because set (VAR_NAME VALUE) does alter a CACHE var.
3. In the case of initialize_paths, the function is called passing INCLUDE_DIRECTORIES ${MPI_${NCDFcomp}_INCLUDE_PATH} LIBRARIES ${MPI_${NCDFcomp}_LIBRARIES}. There are a few issues with this call:

• First of all, it would be nice to have initialize_paths verify that when the optional keywords are used, they have a valid value. Upon vars resolution, if, say, MPI_${NCDFcomp}_INCLUDE_PATH is empty, cmake sees INCLUDE_DIRECTORIES LIBRARIES ${MPI_${NCDFcomp}_LIBRARIES}. This should issue a cmake warning (which probably scorpio doesn't see b/c of the minimum version being "old" (2.8)). However, cmake_parse_arguments also sets a ${prefix}_UNPARSER_ARGUMENTS and ${prefix}_KEYWORDS_MISSING_VALUE, which allow to spot potential buggy calls of the macro/function
• If MPI_${NCDFcomp}_INCLUDE_PATH and/or MPI_${NCDFcomp}_LIBRARIES are non-empty, for what we said at point 2, the function initialize_paths will alter the value of the ${PATHLIST} var, effectively overwriting what the user had.

4. Depending on how MPI is located (i.e., depending on which FindMPI.cmake module gets called and maybe even cmake version), the find module might set MPI_<LANG>_INCLUDE_PATH and MPI_<LANG>_LIBRARIES. This is happening on one of our testbeds after a recent change (I'm not sure what triggered it, I suspect cmake ends up using the FindMPI.cmake module shipped by cmake, rather than the one in scorpio).

tl;dr: the function initialize_paths, in presence of non-empty MPI_<LANG>_INCLUDE_PATH and/or MPI_<LANG>_LIBRARIES, can effectively discard what the user passed via the CACHE vars <LIBNAME>_<LANG>_PATHS.

I have a quick fix (make initialize_paths append to the cache var, rather than overwrite it). I will push a branch soon.

[jayeshkrishna]

@bartgol : So if I understand correctly in your system after a recent change/upgrade the NetCDF_C_PATH / NETCDF_FORTRAN_PATH that you set stopped working (was no longer used by SCORPIO when searching for the NetCDF lib)?

[bartgol]

I can't be 100% sure what the culprit was, but a call to find_package(MPI ...) now sets a lot of the MPI_XYZ cmake vars, including MPI_<LANG>_INCLUDE_PATH and MPI_<LANG>_LIBRARIES. If these are set, then the current impl of initialize_paths would overwrite completely what the user may have set in <LIBNAME>_<LANG>_PATHS, which defies the purpose of those cache vars.

In #457 I make it so that initialize_paths can still add the extra paths coming from the MPI cmake vars, but it appends them to the input ${PATHLIST} var, so that any attempt of the user to specify a path does not get lost.

[jayeshkrishna]

@bartgol : Are you using NetCDF_C_PATHS or NetCDF_Fortran_PATHS (instead of NetCDF_C_PATH / NetCDF_Fortran_PATH) to set the paths to the C / Fortran NetCDF libraries?

[bartgol]

Yes. It seems to me that those are the ones that scorpio passes to find_package_component.

[jayeshkrishna]

Does using NetCDF_C_PATH and NetCDF_Fortran_PATH work for you (these are the vars used by E3SM & recommended in the FindNetCDF cmake module)?

[bartgol]

Ah, yes, that seems to work.

I guess that solves the problem, though I have to say that the fact that both <PKG>_<COMP>_PATH and <PKG>_<COMP>_PATHS are supported is a bit confusing. CMake usually uses the plural (PATHS) keyword, since libs could potentially be scattered across different folders. In the scorpio case, <PKG>_<COMP>_PATH is still a list, so it still works. But I find it a bit confusing.

Anyhow, since it boils down to personal taste, I think it's not a big deal. I can use __PATH, without the S at the end.

Thanks!

[jayeshkrishna]

Thanks, I agree that the fact that *_PATHS is not available to the user is a bit confusing. We will fix that soon.