Changes to move SRW app towards NCO complaince#337
Conversation
877fdfc to
406fd9e
Compare
406fd9e to
c3e93aa
Compare
c3e93aa to
3eb3d65
Compare
0cfd36e to
5572495
Compare
|
Machine: hera |
christinaholtNOAA
left a comment
There was a problem hiding this comment.
@danielabdi-noaa I think this is a nice, clean implementation of the NCO standards that we were looking for. I left just a few comments/questions below.
|
|
||
| add_subdirectory(src) | ||
| add_custom_target(mbuild DEPENDS sorc/pbuild) | ||
| add_custom_target(minstall DEPENDS sorc/pinstall) |
There was a problem hiding this comment.
Is this the magic that separates the build and install steps?
Are the names of the targets arbitrary? I suppose I'm curious why mbuild and minstall instead of just build and install.
There was a problem hiding this comment.
Yes, but the cause of the problem was the external projects add. You would expect a make all would just build the binaries and not install but default behavior of ExternalProject_Add is non-intiutive -- it builds and installs on a make all. So I added STEP_TARGETS build install to separate them and then connect to a custom target at the top level. I think future CMake version could potentially simplify this. I used mbuild and minstall because I couldn't use "install" as target since it is a reserved keyword. "build" was fine though but changed it to be consistent.
There was a problem hiding this comment.
I have removed the custom install target because there was already one anyway, so we have a build target instead of mbuild now. Still couldn't find a solution to prevent make all from installing the project...
| --install-dir=INSTALL_DIR | ||
| installation prefix | ||
| --bin-dir=BIN_DIR | ||
| installation binary directory name ("bin" or "exec") |
There was a problem hiding this comment.
Are these the only two options? Could this be set arbitrarily?
There was a problem hiding this comment.
Unfortunately no. UPP and UFS_UTILS do not use CMAKE_INSTALL_BINDIR as the destination target, instead they set to either "bin" or "exec" where the latter is used if an "EMC_EXEC_DIR" variable is turned on. But once these are fixed, we should be able to install to any bin directory name.
| if [ $build_it -eq 0 ] ; then | ||
| ./devbuild.sh --platform=${machine} --compiler=${compiler} --build-dir=${BUILD_DIR} --install-dir=${BIN_DIR} \ | ||
| --clean --rrfs || fail "Build ${machine} ${compiler} FAILED" | ||
| --clean all || fail "Build ${machine} ${compiler} FAILED" |
There was a problem hiding this comment.
Does this --clean behave the same as it did before? Should we have both --clean and --remove?
There was a problem hiding this comment.
Thanks for catching that bug! --clean does a make clean on the target you selected. I am not sure if --remove that does rm -rf build is needed now but I kept it just in case.
|
@danielabdi-noaa, "./devbuild.sh -p=hera --bin-dir=exec upp" doesn't work correctly. The executable "upp.x" is not created in the "exec" directory but in "bin" directory. |
|
@chan-hoo Hmm that is weird. It does put the upp.x executable in the exec directory for me. Maybe removing the build directory first can help? |
|
@danielabdi-noaa, I don't know what happened. I removed the 'build' directory before I tested. What I got: [Chan-hoo.Jeon@hfe09 exec]$ ls [Chan-hoo.Jeon@hfe09 ufs-srweather-app]$ cd bin |
|
@chan-hoo It looks like a recent PR has removed "EMC_EXEC_DIR" option which is what is causing the problem. |
|
@chan-hoo Could you please try again? I have updated for the UPP change regarding removal of |
|
@danielabdi-noaa, It works well now. Thank you for the quick fix! |
|
@chan-hoo Thanks for testing! I have created a PR to modify |
panll
left a comment
There was a problem hiding this comment.
Tested on Hera. Works fine. Thanks!
0a937e7 to
ddf1363
Compare
9bba7f3 to
552c78f
Compare
DESCRIPTION OF CHANGES:
Some changes needed for NCO standard compliance.
execdirectory instead ofbinI don't think a separate script for each of them is needed especially because you can build two of them at a time
with the latter approach which the former can't do.
srctosorcalong with necessary changes inExternals.cfg--clean,--buildand--installto do separate build and installTESTS CONDUCTED:
Run one test locally on hera, will run automated test
DEPENDENCIES:
Currently none, but will need to modify
regional_workflowwhen default bin directly is changed back toexecDOCUMENTATION:
Currently none, but changes in
devbuild.shand directory structure need to be documented later.