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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
May 1, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3677147
Added environment variable LDFLAGS on macOS as requested by the corre…
ywangwof Apr 13, 2023
eb77115
Changed to append to LDFLAGS but not replace
ywangwof Apr 14, 2023
82f1276
Modified changes based on comments
ywangwof Apr 17, 2023
a1c2543
Modify modulefiles/build_macos_gnu.lua based on comments from Natelie
ywangwof Apr 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Modified changes based on comments
  • Loading branch information
@ywangwof
ywangwof committed Apr 17, 2023
commit 82f12763b394ca25288b9c73f3712430ef89d4df
2 changes: 1 addition & 1 deletion devbuild.sh
View file
Open in desktop
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 •
edited
Loading

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

Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ if [ $USE_SUB_MODULES = true ]; then
else
module use ${SRW_DIR}/modulefiles
module load ${MODULE_FILE}
if [[ "${MODULE_FILE}" == "build_macos_gnu" ]]; then
if [[ "${PLATFORM}" == "macos" ]]; then
export LDFLAGS+=" -L$MPI_ROOT/lib "
fi
fi
Expand Down
2 changes: 1 addition & 1 deletion modulefiles/build_macos_gnu.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

]===])
end

Expand Down