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

Merge spack/develop as of 2024/01/26 into jcsda_emc_spack_stack #401

Merged

Conversation

climbfuji
Copy link
Collaborator

@climbfuji climbfuji commented Jan 27, 2024

Description

Pull in spack develop as of 2024/01/26 - needed for JEDI+GEOS development, and generally good to stay up to date. Every single package has changed because of the copyright date change (2013-2023 --> 2013-2024).

I would like the reviewers to specifically look at the following packages for which I had to resolve merge conflicts that weren't entirely trivial, or for which I needed to add more code (nco and openmpi).

var/spack/repos/builtin/packages/cmake/package.py
var/spack/repos/builtin/packages/ecmwf-atlas/package.py
var/spack/repos/builtin/packages/libjpeg-turbo/package.py
var/spack/repos/builtin/packages/mapl/package.py
var/spack/repos/builtin/packages/met/package.py
var/spack/repos/builtin/packages/mysql/package.py
var/spack/repos/builtin/packages/nco/package.py
var/spack/repos/builtin/packages/openmpi/package.py

Issues

This is all part of compiling JEDI+GEOS with spack-stack (i.e. not using Baselibs) on more than just Discover: https://github.com/JCSDA-internal/AOP23/issues/33

Testing

tgamblin and others added 30 commits January 9, 2024 00:26
Part 3 of reworking all package metadata to key by `when` conditions.

Changes conflict dictionary structure from this:

    { (requirement_spec, ...): [(when_spec, policy, msg)] }

to this:

    { when_spec: [((requirement_spec, ...), policy, msg), ...] }
`make_when_spec()` was being used in the solver, but it has semantics that are specific
to parsing when specs from `package.py`. In particular, it returns `None` when the
`when` spec is `False`, and directives are responsible for ignoring that case and not
adding requirements, deps, etc. when there's an actual `False` passed in from
`package.py`.

In `asp.py`, we know that there won't ever be a raw boolean when spec or constraint, so
we know we can parse them without any of the special boolean handling. However, we
should report where in the file the error happened on error, so this adds some parsing
logic to extract the `mark` from YAML and alert the user where the bad parse is.

- [x] refactor `config.py` so that basic `spack_yaml` mark info is in its own method
- [x] refactor `asp.py` so that it uses the smarter YAML parsing routine
- [x] refactor `asp.py` so that YAML input validation for requirements is done up front
Part 4 of reworking all package metadata to key by `when` conditions.

Changes conflict dictionary structure from this:

    { provided_spec: {when_spec, ...} }

to this:

    { when_spec: {provided_spec, ...} }
Shows how to modify environment variables using
"setup_build_environment" instead of overriding "edit"
…spack#41986)

This fixes an issue where pkg.stage throws because a patch cannot be found,
but the patch is redundant because the spec is reused from a build cache and
will be installed from existing binaries.
…k#41727)

* Move in vs. satisfies to a note and mention special cases of in
* Address feedback: oveoverlap -> intersect
* Re-word the satisfies versus in note.

---------

Co-authored-by: Massimiliano Culpo <[email protected]>
* perl-memoize: add new package with version 1.16=
* fix styling
* glow: add latest version v1.5.1

* update: glow build from source
* py-pyvista: fix import tests
* Skip additional modules
@climbfuji climbfuji marked this pull request as ready for review January 31, 2024 18:47
…penmpi@5:, autoreconf is enforced and we need to apply the two-level namespace patch again after the autoreconf step
@AlexanderRichert-NOAA
Copy link
Collaborator

The only thing that stands out to me is MySQL-- Let's restore the +download_boost functionality since we're still using it in common/packages.yaml, unless we have a different solution there.

Copy link
Collaborator

@RatkoVasic-NOAA RatkoVasic-NOAA left a comment

Choose a reason for hiding this comment

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

I checked mentioned package.py's and differences with old ones. Looks good to me.

@climbfuji
Copy link
Collaborator Author

The only thing that stands out to me is MySQL-- Let's restore the +download_boost functionality since we're still using it in common/packages.yaml, unless we have a different solution there.

Do we need this functionality? On all the HPCs as well as on all the user platforms we rely on mysql being provided as an external package? I'd rather remove that option unless you remind me of why we need it ;-)

@AlexanderRichert-NOAA
Copy link
Collaborator

JCSDA/spack-stack#705

@climbfuji
Copy link
Collaborator Author

JCSDA/spack-stack#705

That would have been my guess but I don't recall what the download_boost option gives us? We need to build boost in spack anyway for other reasons (not just ecflow or mysql, but JEDI itself requires it).

@AlexanderRichert-NOAA
Copy link
Collaborator

It downloads and uses its own internal boost libraries, so it doesn't impose version constraints on the Spack-installed boost.

@climbfuji
Copy link
Collaborator Author

It downloads and uses its own internal boost libraries, so it doesn't impose version constraints on the Spack-installed boost.

Ok, so because of the version constraints for mysql. Let me try to put it back in. CI won't test it though.

@climbfuji
Copy link
Collaborator Author

@AlexanderRichert-NOAA I have another suggestion. What about if we take mysql out of the default dependencies and only enable it (as external package) for macOS users who wish to run JEDI-Skylab experiments on their laptops, and on 2 particular systems managed by JCSDA (on AWS)? There is only one particular use case for a local mysql server, and that is running JEDI-Skylab in "localhost" mode. This only happens on macOS laptops at JCSDA, on the JEDI Singlenode AMI for AWS, and on AWS ParallelCluster - all HPCs, all non-JCSDA user installations, all other cloud systems can be installed w/o mysql.

@AlexanderRichert-NOAA
Copy link
Collaborator

Take it off the HPC systems? Yes that sounds lovely :)

Copy link
Collaborator

@srherbener srherbener left a comment

Choose a reason for hiding this comment

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

The changes in the listed package.py scripts seem okay. I noticed that the openmpi patch for apple-clang two level namespace has this addition:

+    @when("@5.0.0:5.0.1")
+    def autoreconf(self, spec, prefix):
+        perl = which("perl")
+        perl("autogen.pl", "--force")
+        if "+two_level_namespace" in spec and spec.satisfies("platform=darwin"):
+            print("Re-applying configure patch for two_level_namespace on MacOS after autoreconf")
+            filter_file(r"-flat_namespace", "-commons,use_dylibs", "configure")
+

which looks like re-applying the patch for two level namespace when autoreconf is run. This seem like a good thing for us to pick up. Just wanted to point that out.

@climbfuji
Copy link
Collaborator Author

Take it off the HPC systems? Yes that sounds lovely :)

Ok, then I'll do that. I leave the PR as is, and file a bug to redo the mysql dependency in the way I described above. Is that ok with you?

@AlexanderRichert-NOAA
Copy link
Collaborator

Yep works for me! Thanks

@climbfuji
Copy link
Collaborator Author

The changes in the listed package.py scripts seem okay. I noticed that the openmpi patch for apple-clang two level namespace has this addition:

+    @when("@5.0.0:5.0.1")
+    def autoreconf(self, spec, prefix):
+        perl = which("perl")
+        perl("autogen.pl", "--force")
+        if "+two_level_namespace" in spec and spec.satisfies("platform=darwin"):
+            print("Re-applying configure patch for two_level_namespace on MacOS after autoreconf")
+            filter_file(r"-flat_namespace", "-commons,use_dylibs", "configure")
+

which looks like re-applying the patch for two level namespace when autoreconf is run. This seem like a good thing for us to pick up. Just wanted to point that out.

Yes, that is needed. As you can see from the code snippet, for openmpi@5: it currently enforces an autoreconf, which overwrites the configure file you patched earlier. I kept the original logic above in place (always patch the default configure) in case someone removes these forced autoreconf in the future.

@climbfuji
Copy link
Collaborator Author

@AlexanderRichert-NOAA See JCSDA/spack-stack#990 - if satisfied, please approve. Thanks!

Copy link
Collaborator

@AlexanderRichert-NOAA AlexanderRichert-NOAA left a comment

Choose a reason for hiding this comment

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

Built unified-dev and ufs-weather-model-static on Gaea, everything checks out. Checked recipe diffs and everything looks reasonable.

@climbfuji
Copy link
Collaborator Author

Thanks @AlexanderRichert-NOAA and all other reviewers!

@climbfuji climbfuji merged commit 03ecae6 into JCSDA:jcsda_emc_spack_stack Feb 7, 2024
15 checks passed
@climbfuji climbfuji deleted the feature/merge_spack_dev_20240126 branch February 7, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INFRA JEDI Infrastructure
Projects
No open projects
Development

Successfully merging this pull request may close these issues.