[develop] Added environment variable LDFLAGS on macOS as requested by the corresponding module file by ywangwof · Pull Request #729 · ufs-community/ufs-srweather-app

ywangwof · 2023-04-13T20:53:52Z

DESCRIPTION OF CHANGES:

On macOS with GNU compiler, once the module file build_macos_gnu.lua is loaded, it outputs a message to request for setting environment variable LDFLAGS:

Please export env. variable LDFLAGS after the module is successfully loaded:
> export LDFLAGS="-L$MPI_ROOT/lib " "
%

When using script devbuild.sh however, it does not give users a chance to read/set this environment variable. I added three lines in the script devbuild.sh, which will set this environment variable automatically when the module build_macos_gnu is detected.

if [[ "${MODULE_FILE}" == "build_macos_gnu" ]]; then
    export LDFLAGS="-L$MPI_ROOT/lib "
fi

This will make script devbuild.sh working on macOS as it is on other platforms and will also avoid a library not found error on macOS.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

TESTS CONDUCTED:

It is tested on Mac OS Monterey using GNU compiler and the latest HPC-stack installation (from develop branch). It should not affect the performance of this script on other platforms and it is also confirmed on Jet using Intel compiler.

DEPENDENCIES:

None

DOCUMENTATION:

N/A

CHECKLIST

My code follows the style guidelines in the Contributor's Guide
I have performed a self-review of my own code using the Code Reviewer's Guide
I have commented my code, particularly in hard-to-understand areas
My changes need updates to the documentation. I have made corresponding changes to the documentation
My changes do not require updates to the documentation (explain).
My changes generate no new warnings
New and existing tests pass with my changes
Any dependent changes have been merged and published

LABELS (optional):

A Code Manager needs to add the following labels to this PR:

…sponding module file

natalie-perlin · 2023-04-13T21:21:13Z

Suggesting changes to the modulefile build_macos_gnu.lua instead of adding conditionals to devbuild.sh
Lines 59-65 of build_macos_gnu.lua to be changed to a single line adding a flag to the existing ones (note "+=" instead of "=", and spaces before "L" and after "lib")

export LDFLAGS+=" -L$MPI_ROOT/lib "

Another change that is needed for build_macos_gnu.lua in lines 49-50:

setenv("CMAKE_Platform", "macosx.gnu") --setenv("CMAKE_Platform", "macosx.intel")

instead of "macos.<compiler>".
The ufs-weather-model expects "macosx.<compiler>" as a platform name, and it has ufs-weather-model/cmake/configure_macosx.gnu.cmake and configure_macosx.intel.cmake platform-dependent configuration files.

export LDFLAGS+=" -L$MPI_ROOT/lib " does not work in a Lua module file. Note that it is a shell command.

If we change the platform name inside the module file, build_macos_gnu.lua, the file name should also be changed to build_macosx_gnu.lua to be consistent. And all other scripts that mentioned build_macos_gnushould also be changed.

It will be involved too many modifications and is error prone. I prefer to leaving it to the next major version update? My current modification is not perfect, but it requests the minimal changes that make the script devbuild.sh work on the macOS platform.

@ywangwof -
Thank you for your responses!

Yes, indeed this will not work as in the bash shell. Even if we use Lua syntax, such as
append_path (”LDFLAGS”, “ -L$MPI_ROOT/lib ", “ ”)
it will not work either, because the $MPI_ROOT could only be expanded after the modulefile is loaded; it is not yet available inside the modulefile.

Your solution to modify devbuild.sh is then a good option. My only suggestion is to use a conditional based on a $PLATFORM variable, because we may have other compilers Intel, Clang. The line 452 in devbuild.sh then would be:
if [[ "${PLATFORM}" == "macos" ]]; then

Agree. If a platform+compiler naming for the SRW differs from that for the UFS-WM, but still works for the SRW system, it's OK to leave it unchanged.

Modulefiles build_macos_gnu.lua may still need a correction to the syntax for the export LDFLAGS string, suggesting:

if mode() == "load" then LmodMsgRaw([===[ Please export env. variable LDFLAGS after the module is successfully loaded: export LDFLAGS+=" -L$MPI_ROOT/lib " ]===]) end

natalie-perlin · 2023-04-14T20:31:33Z

@@ -450,7 +450,7 @@ else
    module use ${SRW_DIR}/modulefiles
    module load ${MODULE_FILE}
    if [[ "${MODULE_FILE}" == "build_macos_gnu" ]]; then


Suggesting the following for the line 452:
if [[ "${PLATFORM}" == "macos" ]]; then

Thanks for your comments. The modifications are committed as you suggested. Please review them again.

natalie-perlin

Your solution to modify devbuild.sh is then a good option. My only suggestion is to use a conditional based on a $PLATFORM variable, because we may have other compilers Intel, Clang. The line 452 in devbuild.sh then would be:

if [[ "${PLATFORM}" == "macos" ]]; then

Modulefile build_macos_gnu.lua may still need a correction to the syntax for the export LDFLAGS string, suggesting:

if mode() == "load" then
  LmodMsgRaw([===[
   Please export env. variable LDFLAGS after the module is successfully loaded:
        export LDFLAGS+=" -L$MPI_ROOT/lib "
  ]===])
end

natalie-perlin · 2023-04-14T20:51:57Z

@ywangwof -
Thank you for your responses!

Yes, indeed this will not work as in the bash shell. Even if we use Lua syntax, such as
append_path (”LDFLAGS”, “ -L$MPI_ROOT/lib ", “ ”)
it will not work either, because the $MPI_ROOT could only be expanded after the modulefile is loaded; it is not yet available inside the modulefile.

Your solution to modify devbuild.sh is then a good option. My only suggestion is to use a conditional based on a $PLATFORM variable, because we may have other compilers Intel, Clang. The line 452 in devbuild.sh then would be:
if [[ "${PLATFORM}" == "macos" ]]; then

Agree. If a platform+compiler naming for the SRW differs from that for the UFS-WM, but still works for the SRW system, it's OK to leave it unchanged.

Modulefiles build_macos_gnu.lua may still need a correction to the syntax for the export LDFLAGS string, suggesting:

if mode() == "load" then LmodMsgRaw([===[ Please export env. variable LDFLAGS after the module is successfully loaded: export LDFLAGS+=" -L$MPI_ROOT/lib " ]===]) end

natalie-perlin

The line 63 needs in modulefiles/build_macos_gnu.lua to be changed to:

export LDFLAGS+=" -L$MPI_ROOT/lib "

there are extra characters that need to be removed. They were related to a message when written in a bash script, and the conversion to *.lua format has not been done properly.

natalie-perlin · 2023-04-17T20:42:59Z

  LmodMsgRaw([===[
   Please export env. variable LDFLAGS after the module is successfully loaded:
-       > export LDFLAGS=\"-L\$MPI_ROOT/lib \" "
+       > export LDFLAGS+=\"-L\$MPI_ROOT/lib \" "


The line 63 needs to be changed to:

export LDFLAGS+=" -L$MPI_ROOT/lib "

there are extra characters that need to be removed. They were related to a message when written in a bash script, and the conversion to *.lua format not done properly.

@ywangwof - please see my comment above regarding the change!..

Thanks. Changed with a new commit.

MichaelLueken

@ywangwof Thank you so much for working with @natalie-perlin on this PR. Approving now.

MichaelLueken · 2023-05-01T16:07:57Z

@ywangwof The automated Jenkins testing showed a failure in get_from_NOMADS_ics_FV3GFS_lbcs_FV3GFS on Hera GNU. A rerun of the test on Hera GNU successfully passed.

There was also the failure of the grid_RRFS_CONUS_13km_ics_FV3GFS_lbcs_FV3GFS_suite_RRFS_v1beta on Hera Intel. Issue #731 notes that this test can fail. I merged in the latest update to develop this morning into a local clone of your branch, reran the grid_RRFS_CONUS_13km_ics_FV3GFS_lbcs_FV3GFS_suite_RRFS_v1beta test. The test successfully passed.

Now moving forward with merging this work into develop.

Added environment variable LDFLAGS on macOS as requested by the corre…

3677147

…sponding module file

natalie-perlin requested changes Apr 13, 2023

View reviewed changes

RatkoVasic-NOAA approved these changes Apr 13, 2023

View reviewed changes

MichaelLueken changed the title ~~Added environment variable LDFLAGS on macOS as requested by the corre…~~ [develop] Added environment variable LDFLAGS on macOS as requested by the corresponding module file Apr 14, 2023

MichaelLueken added the enhancement New feature or request label Apr 14, 2023

Changed to append to LDFLAGS but not replace

eb77115

natalie-perlin reviewed Apr 14, 2023

View reviewed changes

natalie-perlin requested changes Apr 17, 2023

View reviewed changes

Modified changes based on comments

82f1276

natalie-perlin requested changes Apr 28, 2023

View reviewed changes

Modify modulefiles/build_macos_gnu.lua based on comments from Natelie

a1c2543

natalie-perlin approved these changes Apr 28, 2023

View reviewed changes

MichaelLueken approved these changes Apr 28, 2023

View reviewed changes

MichaelLueken added the run_we2e_coverage_tests Run the coverage set of SRW end-to-end tests label Apr 28, 2023

MichaelLueken merged commit 99944d2 into ufs-community:develop May 1, 2023

MichaelLueken mentioned this pull request May 1, 2023

WE2E test grid_RRFS_NA_3km_ics_FV3GFS_lbcs_FV3GFS_suite_RRFS_v1beta fails at the run_fcst step #731

Closed

MichaelLueken mentioned this pull request May 16, 2023

[develop] Update WM and UPP hashes and minor rearrangement of WE2E coverage tests that fail on certain platforms #799

Merged

19 tasks

Conversation

ywangwof commented Apr 13, 2023 • edited by natalie-perlin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DESCRIPTION OF CHANGES:

Type of change

TESTS CONDUCTED:

DEPENDENCIES:

DOCUMENTATION:

CHECKLIST

LABELS (optional):

Uh oh!

natalie-perlin Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

ywangwof Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natalie-perlin Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

natalie-perlin Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangwof Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

natalie-perlin left a comment

Choose a reason for hiding this comment

Uh oh!

natalie-perlin Apr 14, 2023

Choose a reason for hiding this comment

Uh oh!

natalie-perlin left a comment

Choose a reason for hiding this comment

Uh oh!

natalie-perlin Apr 17, 2023

Choose a reason for hiding this comment

Uh oh!

natalie-perlin Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

ywangwof Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

MichaelLueken left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelLueken commented May 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ywangwof commented Apr 13, 2023 •

edited by natalie-perlin

Loading

ywangwof Apr 14, 2023 •

edited

Loading

natalie-perlin Apr 14, 2023 •

edited

Loading