Skip to content
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

[develop] Added environment variable LDFLAGS on macOS as requested by the corresponding module file #729

Merged
merged 4 commits into from
May 1, 2023

Conversation

ywangwof
Copy link
Collaborator

@ywangwof ywangwof commented Apr 13, 2023

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.

  • hera.intel
  • orion.intel
  • cheyenne.intel
  • cheyenne.gnu
  • gaea.intel
  • jet.intel
  • wcoss2.intel
  • NOAA Cloud (indicate which platform)
  • Jenkins
  • fundamental test suite
  • comprehensive tests (specify which if a subset was used)

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:

  • Work In Progress
  • bug
  • enhancement
  • documentation
  • release
  • high priority
  • run_ci
  • run_we2e_fundamental_tests
  • run_we2e_comprehensive_tests
  • Needs Cheyenne test
  • Needs Jet test
  • Needs Hera test
  • Needs Orion test
  • help wanted

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@ywangwof ywangwof Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. export LDFLAGS+=" -L$MPI_ROOT/lib " does not work in a Lua module file. Note that it is a shell command.
  2. 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ywangwof -
Thank you for your responses!

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

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

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

@MichaelLueken 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 MichaelLueken added the enhancement New feature or request label Apr 14, 2023
devbuild.sh Outdated
@@ -450,7 +450,7 @@ else
module use ${SRW_DIR}/modulefiles
module load ${MODULE_FILE}
if [[ "${MODULE_FILE}" == "build_macos_gnu" ]]; then
Copy link
Collaborator

@natalie-perlin natalie-perlin Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@natalie-perlin natalie-perlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ywangwof -
Thank you for your responses!

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

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

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

Copy link
Collaborator

@natalie-perlin natalie-perlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -60,7 +60,7 @@ setenv("FFLAGS", " -DNO_QUAD_PRECISION -fallow-argument-mismatch ")
if mode() == "load" then
LmodMsgRaw([===[
Please export env. variable LDFLAGS after the module is successfully loaded:
> export LDFLAGS=\"-L\$MPI_ROOT/lib \" "
> export LDFLAGS+=\"-L\$MPI_ROOT/lib \" "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Changed with a new commit.

Copy link
Collaborator

@MichaelLueken MichaelLueken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request run_we2e_coverage_tests Run the coverage set of SRW end-to-end tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants