-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename dirs and make executable dir switchable to meet NCO standards #216
Changes from 5 commits
614cd7a
697a8d4
7f8fb28
5452f8f
eaa673a
adff323
5d4776b
eb9b850
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,9 +49,6 @@ ENDFUNCTION() | |
enable_testing() | ||
include(CTest) | ||
|
||
# Build in this subdirectory. | ||
#add_subdirectory(src) | ||
|
||
##### | ||
# Configure and print the ufs-srweather-app.settings file. | ||
##### | ||
|
@@ -91,6 +88,10 @@ SET(host_os "${osrel}") | |
SET(abs_top_builddir "${CMAKE_CURRENT_BINARY_DIR}") | ||
SET(abs_top_srcdir "${CMAKE_CURRENT_SOURCE_DIR}") | ||
|
||
##### Executable directory name (exec_bin): exec or bin ############# | ||
SET(exec_bin exec) | ||
##################################################################### | ||
|
||
SET(CC_VERSION "${CMAKE_C_COMPILER}") | ||
|
||
# Set values for .settings file. | ||
|
@@ -119,7 +120,7 @@ MESSAGE(${UFS-SRWEATHER-APP_SETTINGS}) | |
# Install ufs_srweather_app.settings file into same location | ||
# as the app. | ||
INSTALL(FILES "${CMAKE_BINARY_DIR}/ufs_srweather_app.settings" | ||
DESTINATION bin) | ||
DESTINATION ${exec_bin}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use ${CMAKE_INSTALL_BINDIR} |
||
|
||
##### | ||
# Create 'ufs_srweather_app_meta.h' include file. | ||
|
@@ -130,4 +131,4 @@ configure_file( | |
|
||
FILE(COPY "${CMAKE_CURRENT_BINARY_DIR}/ufs_srweather_app_meta.h" DESTINATION include) | ||
|
||
add_subdirectory(src) | ||
add_subdirectory(sorc) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,7 +80,7 @@ usage_error () { | |
# default settings | ||
LCL_PID=$$ | ||
SRC_DIR=$(cd "$(dirname "$(readlink -f -n "${BASH_SOURCE[0]}" )" )" && pwd -P) | ||
MACHINE_SETUP=${SRC_DIR}/src/UFS_UTILS/sorc/machine-setup.sh | ||
MACHINE_SETUP=${SRC_DIR}/sorc/UFS_UTILS/sorc/machine-setup.sh | ||
BUILD_DIR=${SRC_DIR}/build | ||
INSTALL_DIR=${SRC_DIR} | ||
COMPILER="" | ||
|
@@ -89,12 +89,12 @@ CCPP="" | |
ENABLE_OPTIONS="" | ||
DISABLE_OPTIONS="" | ||
BUILD_TYPE="RELEASE" | ||
BUILD_JOBS=4 | ||
BUILD_JOBS=8 | ||
CLEAN=false | ||
CONTINUE=false | ||
VERBOSE=false | ||
# detect PLATFORM (MACHINE) | ||
source ${SRC_DIR}/env/detect_machine.sh | ||
source ${SRC_DIR}/modulefiles/detect_machine.sh | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The contents of a directory named There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @christopherwharrop-noaa, ok. I'll move it to another dir. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @christopherwharrop-noaa, this file has been moved into the 'test' dir. |
||
|
||
# process required arguments | ||
if [[ ("$1" == "--help") || ("$1" == "-h") ]]; then | ||
|
@@ -161,7 +161,7 @@ if [ "${VERBOSE}" = true ] ; then | |
fi | ||
|
||
# set ENV_FILE for this platform/compiler combination | ||
ENV_FILE="${SRC_DIR}/env/build_${PLATFORM}_${COMPILER}.env" | ||
ENV_FILE="${SRC_DIR}/modulefiles/build_${PLATFORM}_${COMPILER}.env" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you're going to name this directory There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @christopherwharrop-noaa, I agree. This will be my next PR. I don't want to put several topics into one PR. I'll modify the env files into the regular module files in another PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @christopherwharrop-noaa, if you want this change in this PR, I'll do it soon. |
||
if [ ! -f "${ENV_FILE}" ]; then | ||
printf "ERROR: environment file does not exist for platform/compiler\n" >&2 | ||
printf " ENV_FILE=${ENV_FILE}\n" >&2 | ||
|
@@ -231,7 +231,7 @@ fi | |
|
||
# source the environment file for this platform/compiler combination, then build the code | ||
printf "... Source ENV_FILE and create BUILD directory ...\n" | ||
module use ${SRC_DIR}/env | ||
module use ${SRC_DIR}/modulefiles | ||
. ${ENV_FILE} | ||
module list | ||
mkdir -p ${BUILD_DIR} | ||
|
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,30 @@ | ||
include(ExternalProject) | ||
|
||
##### CMAKE Arguments depending on exec_bin ######################### | ||
|
||
if(${exec_bin} STREQUAL exec) | ||
gsketefian marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the point of supporting multiple names for the executable install directory? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @christopherwharrop-noaa, the UFS SRW App will be used as an operational model. We should change the app to meet the NCO standards for this. If you and other reviewers agree, we can use only one name as 'exec'. But I don't think this is what you want. |
||
list(APPEND COMP_ARGS | ||
"-DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}" | ||
"-DEMC_EXEC_DIR=ON" | ||
) | ||
else() | ||
List(APPEND COMP_ARGS | ||
"-DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}" | ||
) | ||
endif() | ||
|
||
##### UFS_UTILS ##################################################### | ||
|
||
ExternalProject_Add(UFS_UTILS | ||
PREFIX ${CMAKE_CURRENT_BINARY_DIR}/UFS_UTILS | ||
SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/UFS_UTILS | ||
INSTALL_DIR ${CMAKE_INSTALL_PREFIX} | ||
CMAKE_ARGS "-DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}" | ||
CMAKE_ARGS ${COMP_ARGS} | ||
BUILD_ALWAYS TRUE | ||
) | ||
|
||
##### ufs-weather-model ############################################# | ||
|
||
if(NOT CCPP_SUITES) | ||
set(CCPP_SUITES "FV3_CPT_v0,FV3_GFS_2017_gfdlmp,FV3_GFS_2017_gfdlmp_regional,FV3_GSD_SAR,FV3_GSD_v0,FV3_GFS_v15p2,FV3_GFS_v16,FV3_RRFS_v1beta,FV3_HRRR,FV3_RRFS_v1alpha,FV3_GFS_v15_thompson_mynn_lam3km") | ||
endif() | ||
|
@@ -58,14 +75,17 @@ ExternalProject_Add(ufs-weather-model | |
SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/ufs-weather-model | ||
INSTALL_DIR ${CMAKE_INSTALL_PREFIX} | ||
CMAKE_ARGS ${UFS_WEATHER_MODEL_ARGS} | ||
INSTALL_COMMAND mkdir -p ${CMAKE_INSTALL_PREFIX}/bin && cp ${CMAKE_CURRENT_BINARY_DIR}/ufs-weather-model/src/ufs-weather-model-build/ufs_model ${CMAKE_INSTALL_PREFIX}/bin/ | ||
INSTALL_COMMAND mkdir -p ${CMAKE_INSTALL_PREFIX}/${exec_bin} && cp ${CMAKE_CURRENT_BINARY_DIR}/ufs-weather-model/src/ufs-weather-model-build/ufs_model ${CMAKE_INSTALL_PREFIX}/${exec_bin}/ | ||
BUILD_ALWAYS TRUE | ||
) | ||
|
||
##### UPP ########################################################### | ||
|
||
ExternalProject_Add(UPP | ||
PREFIX ${CMAKE_CURRENT_BINARY_DIR}/UPP | ||
SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/UPP | ||
INSTALL_DIR ${CMAKE_INSTALL_PREFIX} | ||
CMAKE_ARGS "-DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}" | ||
CMAKE_ARGS ${COMP_ARGS} | ||
BUILD_ALWAYS TRUE | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems hard-coded. The PR description says it is switchable, but I don't see any option for doing that at build time. Also, what is the point of having a configurable directory name for executables? Everything should be adjustable using
$CMAKE_PREFIX_PATH
with its subdirs using standard software industry names. It seems to me that having a configurable directory name for the executables directory has no advantages and increases the complexity a lot. The complexity increases dramatically because now every other component in the App tree will need to support this. I need to be convinced that there is a good reason to make this configurable rather than just setting it to what it needs to be.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, I think this should look like:
or perhaps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also argue that the default should be
bin
notexec
for the simple reason that NCO is the only place whereexec
would be desired. The default should be the value that the community expects.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christopherwharrop-noaa, I am not familiar with cmake. Can you help me for this? I'll invite you to my repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christopherwharrop-noaa, Now I got your point. I'll modify exec_bin soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christopherwharrop-noaa, a new cmake argument '-DEMC_EXEC_DIR' was introduced to change the dir name to 'exec' for the NCO standard. This is the same approach as UFS_UTILS and UPP. Its default is 'bin'. To change the name to 'exec', we just need to add "-DEMC_EXEC_DIR=ON" to CMAKE_SETTINGS as described in devbuild.sh (ln 213).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The appropriate solution would be to provide at cmake configuration time:
-DCMAKE_INSTALL_BINDIR=exec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the default that CMake assigns.