Rename dirs and make executable dir switchable to meet NCO standards #216

chan-hoo · 2022-02-18T18:10:23Z

DESCRIPTION OF CHANGES:

Some directories are renamed to meet the NCO standards (Ver 11.0.0, January 19, 2022, pp 19 Directory Structures):

docs -> doc
env -> modulefiles
src -> sorc
bin -> exec

The executable directory name can be switched between exec and bin. A PR pre-required for this is under review in NOAA-EMC/UPP.
The workflow environment files in the 'env' directory are moved to the 'regional_workflow/env' directory.

DEPENDENCIES:

Update the directory names of UFS SRW App renamed to meet NCO standards regional_workflow#679
Make executable directory name switchable with exec NOAA-EMC/UPP#448

ISSUE:

Fixes issue mentioned in #215

…her-app into feature/move_wflow_env

sorc/CMakeLists.txt

christopherwharrop-noaa · 2022-02-23T19:50:50Z

CMakeLists.txt

@@ -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)


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.

To clarify, I think this should look like:

set(exec_bin "exec" CACHE STRING "Executable directory name")

or perhaps

set(exec_bin "exec" CACHE INTERNAL "")

I would also argue that the default should be bin not exec for the simple reason that NCO is the only place where exec would be desired. The default should be the value that the community expects.

@christopherwharrop-noaa, I am not familiar with cmake. Can you help me for this? I'll invite you to my repo.

@christopherwharrop-noaa, Now I got your point. I'll modify exec_bin soon.

@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).

The appropriate solution would be to provide at cmake configuration time:
-DCMAKE_INSTALL_BINDIR=exec

I would also argue that the default should be bin not exec for the simple reason that NCO is the only place where exec would be desired. The default should be the value that the community expects.

That is the default that CMake assigns.

christopherwharrop-noaa · 2022-02-23T19:53:50Z

devbuild.sh

 CLEAN=false
 CONTINUE=false
 VERBOSE=false
 # detect PLATFORM (MACHINE)
-source ${SRC_DIR}/env/detect_machine.sh
+source ${SRC_DIR}/modulefiles/detect_machine.sh


The contents of a directory named modulefiles should probably contain module files. This is confusing.

@christopherwharrop-noaa, ok. I'll move it to another dir.

@christopherwharrop-noaa, this file has been moved into the 'test' dir.

christopherwharrop-noaa · 2022-02-23T19:57:54Z

devbuild.sh

@@ -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"


If you're going to name this directory modulefiles then all of the *.env files should be converted to actual module files and should be loaded with, for example, module load build_hera_intel rather than sourced. This would also remove the user's shell from the equation as the module load would work regardless of the shell the user is using.

@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.

@christopherwharrop-noaa, if you want this change in this PR, I'll do it soon.

christopherwharrop-noaa · 2022-02-23T20:00:01Z

sorc/CMakeLists.txt

+##### CMAKE Arguments depending on exec_bin #########################
+
+list(APPEND COMP_ARGS "-DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}")
+if(${exec_bin} STREQUAL exec)


What is the point of supporting multiple names for the executable install directory?

@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.

christopherwharrop-noaa · 2022-02-23T20:48:45Z

I understand that the point of this PR is to make the ufs-srweather-app conform to NCO naming conventions. However, I can't in good conscience let this go by without expressing a few thoughts about that. I'd like to remind everyone that this is a community model, and as such, its development should always be guided by standard software industry practices and principles. Departures from well accepted ubiquitous industry standards should only be done for reasons that can be objectively evaluated and justified on their technical merits and which clearly demonstrate why the departure improves the codebase. Departures from industry standards present customers with unexpected attributes, features, and behaviors. This has at least two consequences: (1) Usability by the community is decreased because customers have to learn non-standard behaviors and quirks (2) Nonstandard nomenclature and naming conventions undermines the otherwise professional quality of the product; it may be harmful to the organization's reputation because it will not appear "professional". This may sound like a minor point, but the software industry has well established conventions for how the various parts of a code base are organized. NCO is absolutely an outlier when it comes to their naming convention standards. Literally no one else on Earth uses those conventions (e.g. sorc/ ). As a community model, ufs-srweather-app is public facing and how it is perceived and used by customers in the research community matters.

christopherwharrop-noaa

I can't approve until the following are addressed:

A CMake option is added for the value of exec_bin needs to be chosen at build time with a CMake option or it is
Either the contents of modulefiles directory are changed to be actual module files or the directory is not named "modulefiles"

christopherwharrop-noaa · 2022-02-23T21:34:18Z

CMakeLists.txt

@@ -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)


To clarify, I think this should look like:

set(exec_bin "exec" CACHE STRING "Executable directory name")

or perhaps

set(exec_bin "exec" CACHE INTERNAL "")

chan-hoo · 2022-02-23T23:29:07Z

@christopherwharrop-noaa, I understand what your point is. However, most users of the UFS SRW App and the regional workflow, including myself, are not familiar with the standard software industry practices and principles because their backgrounds are not software engineering or computer science. Only some knowledgeable people like you perceive it. I've never heard that 'bin' is the standard name for executables before. This might be my lack of knowledge. I don't care about the name of the executable directory. However, the NCO standards describe that the executable directory should be named to 'exec'. Simply put yourself in my position. There exists one standards describing the rules specifically even though it is only for one organization. Other institutes don't have standards like this. In this case, it will be more professional to follow the existing standards. I agree that it may be harmful to the organization's reputation. However, it is not something we can decide at this moment. I suggest you to meet the NOAA management group and update the standards with the software industry principles :) :) Can you share the standard software industry practices and principles with us if you have any documents? It will be definitely helpful at least for me to be a professional software engineer in the far future.

aerorahul

A few comments on the discussion of bin vs exec.
BTW, NCO does not object to src or bin vs sorc or exec.
I am not sure about the envs case.
Hope this helps.

aerorahul · 2022-02-24T21:49:55Z

CMakeLists.txt

+# Executable directory name (exec_bin): exec or bin
+SET(exec_bin "bin")
+IF(EMC_EXEC_DIR)
+  SET(exec_bin "exec")
+ENDIF()


This should not be necessary.

aerorahul · 2022-02-24T21:50:35Z

CMakeLists.txt

@@ -119,7 +122,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})


Use ${CMAKE_INSTALL_BINDIR}

aerorahul · 2022-02-24T21:54:46Z

devbuild.sh

@@ -210,6 +210,7 @@ fi
 # cmake settings
 CMAKE_SETTINGS="-DCMAKE_INSTALL_PREFIX=${INSTALL_DIR}"
 CMAKE_SETTINGS="${CMAKE_SETTINGS} -DCMAKE_BUILD_TYPE=${BUILD_TYPE}"
+#CMAKE_SETTINGS="${CMAKE_SETTINGS} -DEMC_EXEC_DIR=ON"


Instead use
-DCMAKE_INSTALL_BINDIR=exec
where exec is desired.

christopherwharrop-noaa · 2022-02-24T22:39:24Z

If @aerorahul is correct that NCO doesn't object to the use of src and bin then I would like to simply close this pull request (or limit it to doc and modulefiles). How would we get positive confirmation of that?

chan-hoo · 2022-02-28T14:01:00Z

@christopherwharrop-noaa, @aerorahul is the right person who can confirm this. Since he said ok, I'll close this pr and submit a new one for modulefiles soon.

move workflow env files to regional workflow

614cd7a

chan-hoo requested review from mkavulich, gsketefian and JeffBeck-NOAA as code owners February 18, 2022 18:10

chan-hoo changed the title ~~move workflow env files to regional workflow~~ Rename dirs and make executable dir switchable to meet NCO standards Feb 20, 2022

chan-hoo marked this pull request as draft February 20, 2022 21:47

chan-hoo added 3 commits February 20, 2022 21:48

Rename dirs and make exe dir switchable

697a8d4

Merge branch 'develop' of https://github.com/ufs-community/ufs-srweat…

7f8fb28

…her-app into feature/move_wflow_env

Move exec_bin to the upper level cmakelists

5452f8f

chan-hoo marked this pull request as ready for review February 20, 2022 22:48

Remove unnecessary line

eaa673a

chan-hoo requested a review from RatkoVasic-NOAA as a code owner February 22, 2022 18:44

gsketefian reviewed Feb 22, 2022

View reviewed changes

sorc/CMakeLists.txt Outdated Show resolved Hide resolved

chan-hoo added 2 commits February 22, 2022 20:20

Simplify if condition for exec_bin

adff323

Update hash of UPP

5d4776b

chan-hoo requested review from ywangwof, panll, christinaholtNOAA, christopherwharrop-noaa, danielabdi-noaa, mark-a-potts, jkbk2004, willmayfield, jwolff-ncar and dmwright526 as code owners February 23, 2022 17:38

gsketefian approved these changes Feb 23, 2022

View reviewed changes

christopherwharrop-noaa reviewed Feb 23, 2022

View reviewed changes

christopherwharrop-noaa self-requested a review February 23, 2022 21:01

christopherwharrop-noaa requested changes Feb 23, 2022

View reviewed changes

Add -DEMC_EXEC_DIR to cmake conf for exec dir

eb9b850

aerorahul reviewed Feb 24, 2022

View reviewed changes

chan-hoo closed this Feb 28, 2022

chan-hoo deleted the feature/move_wflow_env branch March 1, 2022 13:09

SamuelTrahanNOAA pushed a commit to SamuelTrahanNOAA/ufs-srweather-app that referenced this pull request Sep 15, 2023

Update hash for using the new lam fix files. (ufs-community#216)

7fb6aad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename dirs and make executable dir switchable to meet NCO standards #216

Rename dirs and make executable dir switchable to meet NCO standards #216

chan-hoo commented Feb 18, 2022 •

edited

Loading

christopherwharrop-noaa Feb 23, 2022

christopherwharrop-noaa Feb 23, 2022

christopherwharrop-noaa Feb 23, 2022

chan-hoo Feb 23, 2022

chan-hoo Feb 24, 2022

chan-hoo Feb 24, 2022

aerorahul Feb 24, 2022

aerorahul Feb 24, 2022

christopherwharrop-noaa Feb 23, 2022

chan-hoo Feb 23, 2022

chan-hoo Feb 24, 2022

christopherwharrop-noaa Feb 23, 2022

chan-hoo Feb 23, 2022 •

edited

Loading

chan-hoo Feb 24, 2022

christopherwharrop-noaa Feb 23, 2022

chan-hoo Feb 24, 2022

christopherwharrop-noaa commented Feb 23, 2022 •

edited

Loading

christopherwharrop-noaa left a comment •

edited

Loading

christopherwharrop-noaa Feb 23, 2022

chan-hoo commented Feb 23, 2022

aerorahul left a comment

aerorahul Feb 24, 2022

aerorahul Feb 24, 2022

aerorahul Feb 24, 2022

christopherwharrop-noaa commented Feb 24, 2022 •

edited

Loading

chan-hoo commented Feb 28, 2022

Rename dirs and make executable dir switchable to meet NCO standards #216

Rename dirs and make executable dir switchable to meet NCO standards #216

Conversation

chan-hoo commented Feb 18, 2022 • edited Loading

DESCRIPTION OF CHANGES:

DEPENDENCIES:

ISSUE:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chan-hoo Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherwharrop-noaa commented Feb 23, 2022 • edited Loading

christopherwharrop-noaa left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chan-hoo commented Feb 23, 2022

aerorahul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherwharrop-noaa commented Feb 24, 2022 • edited Loading

chan-hoo commented Feb 28, 2022

chan-hoo commented Feb 18, 2022 •

edited

Loading

chan-hoo Feb 23, 2022 •

edited

Loading

christopherwharrop-noaa commented Feb 23, 2022 •

edited

Loading

christopherwharrop-noaa left a comment •

edited

Loading

christopherwharrop-noaa commented Feb 24, 2022 •

edited

Loading