-
Notifications
You must be signed in to change notification settings - Fork 36
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
Build static libjpeg if requested, use hpc-stack libjpeg if built in jasper and NCEPLIBS; add NCAR Cheyenne #38
Build static libjpeg if requested, use hpc-stack libjpeg if built in jasper and NCEPLIBS; add NCAR Cheyenne #38
Conversation
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.
Looks good.
Just a couple minor comments.
Also, can you please add shared: NO
in stack_noaa.yaml
section for jpeg
.
See bugfix/jpeg
branch. it will save us another PR.
Thanks.
libs/build_jasper.sh
Outdated
@@ -56,13 +56,18 @@ else | |||
useCmake=NO | |||
fi | |||
|
|||
if [[ "${STACK_jpeg_build}" == "YES" ]]; then | |||
module load jpeg |
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.
Would just a module try-load jpeg
work? Because if one installed jpeg previously and now just updating jasper, the if block would return false.
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.
Yes, I came across that only later when I looked at the NCEPLIBS scripts. Should be fixed now, please check.
libs/build_jpeg.sh
Outdated
cmake $sourceDir \ | ||
-DCMAKE_INSTALL_PREFIX=$prefix \ | ||
-DBUILD_TESTS=ON \ | ||
-DBUILD_EXECUTABLES=ON \ | ||
-DCMAKE_BUILD_TYPE=RELEASE | ||
-DCMAKE_BUILD_TYPE=RELEASE \ | ||
${shared_flags} |
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 is fine.
Can you see bugfix/jpeg
and please add the check_flags
logic from that branch. It will save us another PR.
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.
Should be done now, please check if I missed something.
config/config_cheyenne_gnu.sh
Outdated
export STACK_EXIT_ON_FAIL=Y | ||
export WGET="wget -nv" | ||
|
||
# Load these basic modules for Hera |
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.
you mean Cheyenne
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.
fixed for both intel and gnu
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.
A few more comments.
config/config_cheyenne_intel.sh
Outdated
export STACK_EXIT_ON_FAIL=Y | ||
export WGET="wget -nv" | ||
|
||
# Load these basic modules for Hera |
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.
Cheyenne here too.
@@ -0,0 +1,243 @@ | |||
cmake: |
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.
how different is stack_ncar.yaml
to stack_noaa.yaml
?
Just curious.
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.
Just the build-jpeg YES|NO, correct.
@@ -4,6 +4,7 @@ cmake: | |||
|
|||
jpeg: | |||
build: NO | |||
shared: NO |
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.
If build: NO
is the only difference between stack_noaa.yaml
and stack_ncar.yaml
, do you see any harm in turning jpeg to YES in stack_noaa.yaml
?
I don't. May be @kgerheiser has some thoughts.
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 prefer building it per default on NOAA platforms, too. In that case we could ditch the stack_ncar.yaml file again.
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 in that case suggest just build jpeg on NOAA platforms and ditch stack_ncar.yaml
libs/build_jasper.sh
Outdated
@@ -56,13 +56,18 @@ else | |||
useCmake=NO | |||
fi | |||
|
|||
# Load jpeg module if created by hpc-stack; requires setting | |||
# MAKE_POLICY_DEFAULT_CMP0074 to new below so that JPEG_ROOT is searched | |||
module try-load jpeg |
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 has to be up top in the if $MODULES
block.
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.
It hopefully is now.
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.
All good.
Thanks for the fixes and porting to Cheyenne.
Look at the comment on jpeg for noaa. We can build jpeg on noaa too and ditch stack_ncar.yaml.
I am fine with having both too.
@@ -4,6 +4,7 @@ cmake: | |||
|
|||
jpeg: | |||
build: NO | |||
shared: NO |
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 in that case suggest just build jpeg on NOAA platforms and ditch stack_ncar.yaml
Ok, thanks. I'll wait if @kgerheiser has anything to add. I am also trying a fix for the ESMF build on Cheyenne right now. |
I just pushed the esmf configuration update for SGI MPT (used on Cheyenne). Works just fine. After deciding what to do with building jpeg on NOAA platforms or not (and potentially removing the NCAR config), this PR is ready to merge. |
@@ -0,0 +1,21 @@ | |||
#!/bin/bash |
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.
One last comment otherwise happy to merge.
Do we need config_cheyenne_gnu.sh
and config_cheyenne_intel.sh
Can we just have one config_cheyenne.sh
See config_hera.sh
for example.
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 remember we "slacked" about this before. I want to minimize the amount of work and reduce the potential of making mistakes when rolling out a new version of the stack. All it should take is a no-brainer exercise of checking out the code, running the seup_modules script and the build_stack script. No editing of whatsoever files. This should be a failsafe exercise at 3am in the morning when somebody calls you that the libraries got deleted accidentally and they need them back right now. We should do the same for every other platform for which we support multiple compilers, in my opinion. Right now only hera, i.e. config_hera_gnu.sh
and config_hera_intel.sh
. And possibly append _intel
for all the other platforms.
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.
sold!
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.
Thanks, appreciate it. All CI tests passed, let's wait to hear from @kgerheiser about the jpeg build on NOAA platforms.
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.
or maybe not ;-)
This PR fixes a few issues with libjpeg that surfaced when trying to install libjpeg on Cheyenne, see #39.
It also adds the configuration files for Cheyenne with Intel and GNU, and a separate NCAR YAML stack config. The latter differs from the NOAA config in building a static version of libjpeg.
Limitations: