search

ufs-community / ufs-weather-model

UFS Weather Model
Other
136 stars 244 forks source link

Possible issue with FindESMF.cmake #39

Closed mathomp4 closed 4 years ago

mathomp4 commented 4 years ago

All,

I was trying to build following the instructions and was getting frustrated by the fact that ESMF could not be found. Eventually, I tracked down my issue to this line: https://github.com/ufs-community/ufs-weather-model/blob/52795b83f0febae0fe030d5cb1da3e5bbafba5e8/cmake/FindESMF.cmake#L21

Why? Well, if we look in my esmf.mk file:

ESMF_F90COMPILEPATHS=-I/discover/swdev/mathomp4/NCEPLIBS/install-Intel18/mod -I/discover/swdev/mathomp4/NCEPLIBS/install-Intel18/include -I/discover/swdev/mathomp4/NCEPLIBS/install-Intel18/include

The issue is that I installed NCEPLIBS with -DCMAKE_INSTALL_PREFIX=../install-Intel18. And what's in the center of that? -I. Thus, CMake was seeing:

-- MATMAT fv3cpl ESMF_MOD: /discover/swdev/mathomp4/NCEPLIBS/installntel18/mod;/discover/swdev/mathomp4/NCEPLIBS/installntel18/include;/discover/swdev/mathomp4/NCEPLIBS/installntel18/include

I solved it by installing to a different directory, but I'm lucky only in that nowhere else in my full path was a -I.

It's not a fatal flaw, but it might be useful to make a bit more robust.

climbfuji commented 4 years ago

Thanks for reporting this issue. Please note that the entire ufs model, including the NCEPLIBS umbrella build, are currently under development and testing (basically what you did, thank you). We are currently refactoring the NCEPLIBS build, by the end of the week we hope to have it finalized. We will make sure that the issue you reported (that is, a -I in the filepath) is addressed in the refactored build system. Meanwhile, I need to ask you for your patience. Thanks!

DusanJovic-NOAA commented 4 years ago

How about changing:

string(REPLACE "-I" "" ESMF_F90COMPILEPATHS ${ESMF_F90COMPILEPATHS})

to:

string(REPLACE " -I" " " ESMF_F90COMPILEPATHS ${ESMF_F90COMPILEPATHS})

@mathomp4 Can you install ESMF in install-Intel18 directory again and try this change?

climbfuji commented 4 years ago

This will not catch the leading -I in ESMF_F90COMPILEPATHS. The script will also have to look for a -I at the beginning of the string.

  message(INFO "ESMF_F90COMPILEPATHS 1: ${ESMF_F90COMPILEPATHS}")
  string(REPLACE "-I" "" ESMF_F90COMPILEPATHS ${ESMF_F90COMPILEPATHS})
  message(INFO "ESMF_F90COMPILEPATHS 2: ${ESMF_F90COMPILEPATHS}")
  string(REPLACE " " ";" ESMF_F90COMPILEPATHS ${ESMF_F90COMPILEPATHS})
  message(INFO "ESMF_F90COMPILEPATHS 3: ${ESMF_F90COMPILEPATHS}")
INFO ESMF_F90COMPILEPATHS 1: -I/usr/local/gnu/esmf-8.0.0/mod -I/usr/local/gnu/esmf-8.0.0/include -I/usr/local/gnu/include
INFO ESMF_F90COMPILEPATHS 2: /usr/local/gnu/esmf-8.0.0/mod /usr/local/gnu/esmf-8.0.0/include /usr/local/gnu/include
INFO ESMF_F90COMPILEPATHS 3: /usr/local/gnu/esmf-8.0.0/mod;/usr/local/gnu/esmf-8.0.0/include;/usr/local/gnu/include
climbfuji commented 4 years ago

We should use the following imo:

  string(REGEX REPLACE "^-I" "" ESMF_F90COMPILEPATHS ${ESMF_F90COMPILEPATHS})
  string(REPLACE " -I" " " ESMF_F90COMPILEPATHS ${ESMF_F90COMPILEPATHS})
  string(REPLACE " " ";" ESMF_F90COMPILEPATHS ${ESMF_F90COMPILEPATHS})

This also takes care of filenames containing spaces unless it's something really weird, but who would do that?

-I/some/stupid/filepath/containing\ -I/and/something/else

Well, probably someone will. @kgerheiser @mark-a-potts I will create a PR for Kyle's refactored version.

mathomp4 commented 4 years ago

This could also work:

separate_arguments(ESMF_F90COMPILEPATHS)
foreach (ITEM ${ESMF_F90COMPILEPATHS})
   string(REGEX REPLACE "^-I" "" ITEM "${ITEM}")
   list(APPEND tmp ${ITEM})
endforeach()
set(ESMF_F90COMPILEPATHS ${tmp})
climbfuji commented 4 years ago

See https://github.com/kgerheiser/CMakeModules/pull/2

DusanJovic-NOAA commented 4 years ago

This file is in ufs-weather-model (this repository). We should thoroughly test this change and commit it in one of our next PRs.

mathomp4 commented 4 years ago

This could also work:

Actually, this still might cause issue with paths with spaces in. Okay. New challenge...

climbfuji commented 4 years ago

Thanks @mathomp4. I tested your version to see if it fixes the shortcoming I noted for my suggestion, and unfortunately it doesn't solve this problem either:

INFOESMF_F90COMPILEPATHS: -I/usr/local/gnu/esmf-8.0.0/mod -I/usr/local/gnu/esmf-8.0.0/include -I/usr/local/gnu/include -I/something/stupid -Ibla/stuff
INFOESMF_F90COMPILEPATHS: /usr/local/gnu/esmf-8.0.0/mod;/usr/local/gnu/esmf-8.0.0/include;/usr/local/gnu/include;/something/stupid;bla/stuff
INFOESMF_F90COMPILEPATHS: -I/usr/local/gnu/esmf-8.0.0/mod -I/usr/local/gnu/esmf-8.0.0/include -I/usr/local/gnu/include -I'/something/stupid -Ibla/stuff'
INFOESMF_F90COMPILEPATHS: /usr/local/gnu/esmf-8.0.0/mod;/usr/local/gnu/esmf-8.0.0/include;/usr/local/gnu/include;'/something/stupid;bla/stuff'

So, for now I will keep what is in https://github.com/kgerheiser/CMakeModules/pull/2. Thanks for helping!

climbfuji commented 4 years ago

@mathomp4 if course we could replace \ -I with something else temporarily, then do the decomposition using your or my method, and then replace the temporary string with \ -I again. But I don't know if it is worth the effort.

mathomp4 commented 4 years ago

This seems to handle spaces in paths:

separate_arguments(ESMF_F90COMPILEPATHS NATIVE_COMMAND ${ESMF_F90COMPILEPATHS})
foreach (ITEM ${ESMF_F90COMPILEPATHS})
   string(REGEX REPLACE "^-I" "" ITEM "${ITEM}")
   list(APPEND tmp ${ITEM})
endforeach()
set(ESMF_F90COMPILEPATHS ${tmp})

ETA: Should perhaps work on Windows as well.

climbfuji commented 4 years ago

This works as long as the ESMF_F90COMPILEPATHS variable puts the filepath with spaces in quotes, otherwise not. I hope it does.

INFO ESMF_F90COMPILEPATHS: -I/usr/local/gnu/esmf-8.0.0/mod -I/usr/local/gnu/esmf-8.0.0/include -I/usr/local/gnu/include -I'/something/stupid -Ibla/stuff'
...
INFO ESMF_F90COMPILEPATHS: /usr/local/gnu/esmf-8.0.0/mod;/usr/local/gnu/esmf-8.0.0/include;/usr/local/gnu/include;/something/stupid -Ibla/stuff

INFO ESMF_F90COMPILEPATHS: -I/usr/local/gnu/esmf-8.0.0/mod -I/usr/local/gnu/esmf-8.0.0/include -I/usr/local/gnu/include -I/something/stupid -Ibla/stuff
...
INFO ESMF_F90COMPILEPATHS: /usr/local/gnu/esmf-8.0.0/mod;/usr/local/gnu/esmf-8.0.0/include;/usr/local/gnu/include;/something/stupid;bla/stuff

Anyway, your solution is better. I'll update the PR. Thanks.

climbfuji commented 4 years ago

This has been fixed in the new branch release/public-v1, which will replace all other branches associated with the UFS public release. Have a look here, and if satisfied, please close the issue:

https://github.com/NOAA-EMC/CMakeModules/blob/7f5b1b3f51ddaf2d33b0e658587b978887c524f7/Modules/FindESMF.cmake#L75

Thanks!

junwang-noaa commented 4 years ago

The problem was fixed. Close issue.