-
Notifications
You must be signed in to change notification settings - Fork 15
Update spack packages #332
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
base: master
Are you sure you want to change the base?
Conversation
.uberenv_config.json
Outdated
| "spack_configs_path": "scripts/spack_configs", | ||
| "spack_packages_path": "scripts/spack_packages/packages", | ||
| "spack_packages_commit": "a75a7f75182ffc7a51c6ca7f0fec4bf9b2705be8" | ||
| "spack_packages_commit": "cfa8d650480c409de2d568cf1355bf7e509f4c1c" // (Jan 21st 2026) |
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'll need to update the uberenv submodule as well.
There is a bug in the current uberenv that locks the spack_packages commit being used.
More details in this uberenv PR: llnl/uberenv#152
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 for the heads up, Brian! Saved some time!
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.
No problem! I have a branch here (https://github.com/GEOS-DEV/thirdPartyLibs/tree/feature/han12/spack_updates) that is testing out Spack 1.1.0 . Hopefully the other changes might be useful!
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.
Oh nice, yes those do look useful and more comprehensive than this branch. Are you close to finishing it? Maybe we can put all together
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.
Feel free to integrate the parts that work into your PR!
Last I checked, I was running into compilation errors when building GEOS related to newer hypre API changes.
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.
Also, I think json is particular about comments, so C-style // comments may not work.
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.
That's a bummer, wish uberenv could understand jsonc
I fixed the hypre API changes here: GEOS-DEV/GEOS#3939
I'll go ahead and merge your stuff into this PR
…retization error (compiler mixing error)
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 @victorapm!
Just have some comments related to cleanup/clarity.
EDIT: For reference, related PR GEOS-DEV/GEOS#3939
| # Run uberenv | ||
| # Have to create install directory first for uberenv | ||
| # -k flag is to ignore SSL errors |
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.
We should preserve comments like these for future reference.
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.
Good catch, restored
| scl enable gcc-toolset-13 ' \ | ||
| export CXXFLAGS="--gcc-toolchain=/opt/rh/gcc-toolset-13/root/usr" && \ | ||
| export CFLAGS="--gcc-toolchain=/opt/rh/gcc-toolset-13/root/usr" && \ |
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.
There looks to be 3 methods to pass the specific toolchain to clang here and elsewhere:
- Creating a
bashwrapper script that invokes clang with--gcc-toolchainand setting those paths for thellvmexternal dependency insidedocker/rocky-spack.yaml. - Setting
CFLAGSandCXXFLAGShere with--gcc-toolchain - Specifying
--gcc-toolchainthroughcxxflags/cflagsunder the spack toolchain insidedocker/rocky-spack.yaml.
Are these all necessary? Is there any redundancy or methods that didn't end up working here?
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.
We should only need the first one. I had tried the other two solutions, but couldn't make it work. I cleaned up the stuff that I didn't need anymore in the code
| ./scripts/uberenv/uberenv.py \ | ||
| --spec "+cuda~uncrustify~openmp~pygeosx cuda_arch=70 %clang-10 ^cuda@11.8.0+allow-unsupported-compilers ^caliper~gotcha~sampler~libunwind~libdw~papi" \ | ||
| --spack-env-file=${SRC_DIR}/docker/spack.yaml \ | ||
| --spack-env-file=${SRC_DIR}/docker/ubuntu20-spack.yaml \ |
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.
Maybe rename this file as ubuntu20-clang-cuda.yaml, spack-cuda.yaml, etc. ?
To indicate that this is now cuda-specific/config-specific.
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.
Renamed to docker/ubuntu20-clang-cuda-spack.yaml
| - ../versions.yaml | ||
|
|
||
| # Ubuntu 20.04 clang/cuda image only has clang/llvm 10.x available. | ||
| # Keep this environment llvm10-only to avoid concretizer conflicts with llvm@15. |
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 curious - was spack failing to concretize and/or was it trying to use llvm@15?
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 think here spack has hitting concretization issues because the environment could see multiple llvm externals and spack would either treat %llvm deps ambiguous (multiple external satisfies) and/or concretize into constraints that pulled in llvm@15 even though the image is clang/llvm 10.
spack passes clang-cuda build by not building hypre (false positive)
|
Hm, looks like the clang-cuda configuration is passing, but spack does so by not building hypre: Here's the geosx concretization from the CI run(https://github.com/GEOS-DEV/thirdPartyLibs/actions/runs/21274976099) :
That would explain why the associated CI job is failing inside GEOS with error: |
|
Thanks for catching this! Trying to force |
scotchpackage and rely on upstream one (from spack-packages) by @bmhan1221st Jan 2026v1.1.0v4.1.2(preferable version by spack-packages)v1.14.6(preferable version by spack-packages)