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

Update ESMF from spack-stack fork: fix netcdf static libs and variant combinations #39738

Merged
merged 3 commits into from
Sep 20, 2023
Merged

Update ESMF from spack-stack fork: fix netcdf static libs and variant combinations #39738

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fbed845
Update ESMF from spack-stack fork: fix netcdf static libs and variant…
climbfuji Aug 31, 2023
512d279
Revert invalid updates and simplify version constraints
climbfuji Aug 31, 2023
8481dae
Merge branch 'develop' into feature/update_esmf_from_spack_stack
climbfuji Sep 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 19 additions & 8 deletions var/spack/repos/builtin/packages/esmf/package.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,20 @@ class Esmf(MakefilePackage):
)
variant("debug", default=False, description="Build with debugging symbols and options enabled")
variant("shared", default=True, description="Build shared library")

# 'esmf_os', 'esmf_comm', 'esmf_pio' variants allow override values for their corresponding
# build environment variables. Documentation, including valid values, can be found at
# https://earthsystemmodeling.org/docs/release/latest/ESMF_usrdoc/node10.html#SECTION000105000000000000000
variant("esmf_os", default="auto", description="Override for ESMF_OS variable")
variant("esmf_comm", default="auto", description="Override for ESMF_COMM variable")
variant("esmf_pio", default="auto", description="Override for ESMF_PIO variable")
# Set the 'snapshot' variant any time a beta snapshot is used in order to obtain
# correct module name behavior for MAPL.
variant(
"snapshot",
default="none",
description="Named variant for snapshots versions (e.g., 'b09')",
Comment on lines +88 to +91
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this variant being passed to the build process. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a workaround for the fact that in modules.yaml, spack doesn't distinguish between, say, "@1.2.3" and "@1.2.3b09". I discussed this at some length with Todd once upon a time and there wasn't really another good solution. So this is essentially a dummy variant that let's us distinguish ESMF's "b??" subversions when we customize the modulefile names of one of our dependencies (MAPL).

Copy link
Contributor

@theurich theurich Sep 5, 2023 •
edited

Choose a reason for hiding this comment

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

One question to consider - and I do not know the answer - is how much longer will anything depend on a specific snapshot tag like v8.3.0b09? I think it was unfortunate that some models started depending on that specific snapshot tag, but hopefully that is a thing of the past now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I was the one to make this decision, I would say that any submitted ufs-weather-model code must use an official release of ESMF. But as long as UFS WM developers want to test with a beta snapshot (and have the mapl dependency be built with it, too), and spack(-stack) is used to install that version, this code needs to stay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ok of course to leave this out of mainline spack and keep that "feature" in our fork, just one more difference to carry around.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @climbfuji for clarifying. I have no issue with this "feature" moving into the mainline spack ESMF package. Generally I think the fewer differences between your fork and the mainline, the better.

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to hold up the PR on this but would like to understand the notion better: the module file should be able to access the version of dependencies (e.g. 8.3.0b09). I can see how you might not want to alter all module file names just to account for this.

That being said I think this variant could be named more-clearly, like:

variant(ufs-weather-model)

And furthermore if it's a dummy variant which derives from the version, it might also be useful to encode a conflict

conflicts(+ufs-weather-model, when=)

admittedly this would be difficult to encode given that we don't support not.

Copy link
Member

Choose a reason for hiding this comment

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

We do have require come to think of it, so you can do

require(@8.3.0b09, when=+ufs-weather-model)

)

# Optional dependencies
depends_on("mpi", when="+mpi")
Expand All @@ -90,12 +98,12 @@ class Esmf(MakefilePackage):
depends_on("netcdf-fortran@3.6:", when="+netcdf")
depends_on("parallel-netcdf@1.2.0:", when="+pnetcdf")
depends_on("xerces-c@3.1.0:", when="+xerces")
depends_on("parallelio@2.5.7: +mpi", when="@8.3.0:8.3.99+external-parallelio+mpi")
depends_on("parallelio@2.5.7: ~mpi", when="@8.3.0:8.3.99+external-parallelio~mpi")
depends_on("parallelio@2.5.9: +mpi", when="@8.4.0:8.4.99+external-parallelio+mpi")
depends_on("parallelio@2.5.9: ~mpi", when="@8.4.0:8.4.99+external-parallelio~mpi")
depends_on("parallelio@2.5.10: +mpi", when="@8.5.0:+external-parallelio+mpi")
depends_on("parallelio@2.5.10: ~mpi", when="@8.5.0:+external-parallelio~mpi")
depends_on("parallelio@2.5.7: +mpi", when="@8.3+external-parallelio+mpi")
depends_on("parallelio@2.5.7: ~mpi", when="@8.3+external-parallelio~mpi")
depends_on("parallelio@2.5.9: +mpi", when="@8.4+external-parallelio+mpi")
depends_on("parallelio@2.5.9: ~mpi", when="@8.4+external-parallelio~mpi")
depends_on("parallelio@2.5.10: +mpi", when="@8.5:+external-parallelio+mpi")
depends_on("parallelio@2.5.10: ~mpi", when="@8.5:+external-parallelio~mpi")
depends_on("cmake@3.5.2:", type="build", when="~external-parallelio")

# Testing dependencies
Expand Down Expand Up @@ -241,7 +249,7 @@ def setup_build_environment(self, env):
if (
self.compiler.name in ["gcc", "clang", "apple-clang"]
and gfortran_major_version >= 10
and self.spec.satisfies("@:8.2.99")
and (self.spec.satisfies("@:8.2.99") or self.spec.satisfies("@8.3.0b09"))
):
env.set("ESMF_F90COMPILEOPTS", "-fallow-argument-mismatch")

Expand All @@ -268,6 +276,9 @@ def setup_build_environment(self, env):
if "+mpi" in spec:
if "^cray-mpich" in self.spec:
env.set("ESMF_COMM", "mpi")
# https://github.com/jcsda/spack-stack/issues/517
if self.spec.satisfies("@:8.4.1"):
env.set("ESMF_CXXLINKLIBS", "-lmpifort -lmpi")
elif "^mvapich2" in spec:
env.set("ESMF_COMM", "mvapich2")
elif "^mpich" in spec:
Expand Down Expand Up @@ -327,7 +338,7 @@ def setup_build_environment(self, env):
netcdfc = spec["netcdf-c"]
if netcdfc.satisfies("~shared"):
nc_config = which(os.path.join(netcdfc.prefix.bin, "nc-config"))
nc_flags = nc_config("--libs", output=str).strip()
nc_flags = nc_config("--static", "--libs", output=str).strip()
env.set("ESMF_NETCDF_LIBS", nc_flags)

###################
Expand Down