-
Notifications
You must be signed in to change notification settings - Fork 17
Add spack:packages
to recipe schema to allow specifying a fixed commit of the builtin packages
#236
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
Conversation
@DropD pinging you as well, since you may have input and wishes for the icon use cases. |
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.
Pull Request Overview
This PR adds the spack_packages configuration to the recipe schema so that a fixed commit of the builtin spack-packages repository can be specified.
- Introduces a new spack_packages schema in config.json.
- Implements spack-packages cloning, fetching, and checkout logic in builder.py.
- Updates various Makefiles, environment, and template files to use packages.yaml instead of compilers.yaml.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
stackinator/templates/repos.yaml | Adds new keys for alps and builtin repositories. |
stackinator/templates/environments.spack.yaml | Updates include logic by removing compilers.yaml reference. |
stackinator/templates/Makefile.* | Adjusts file dependencies from compilers.yaml to packages.yaml. |
stackinator/schema/config.json | Adds the spack_packages schema definition. |
stackinator/recipe.py | Modifies gcc specs handling to append the bootstrap flag. |
stackinator/etc/envvars.py | Transitions the logic from using compilers.yaml to packages.yaml. |
stackinator/builder.py | Introduces cloning/fetching/checkout for the spack-packages repo and updates repos.yaml rendering. |
Comments suppressed due to low confidence (3)
stackinator/etc/envvars.py:451
- [nitpick] Since the data is now loaded from the packages.yaml file, renaming the variable (for example, to packages_compilers) might reduce confusion and better reflect the source of the data.
compilers = []
stackinator/templates/Makefile.compilers:47
- [nitpick] Please consider adding a comment to explain the purpose of appending the '~bootstrap' variant here to assist future maintainers.
yq write -i $@ 'packages.gcc.externals[0].spec' "$$ (yq read $@ 'packages.gcc.externals[0].spec') ~bootstrap"
stackinator/templates/Makefile:113
- [nitpick] Now that the configuration files have been transitioned from compilers.yaml to packages.yaml, updating the accompanying comments could help clarify these changes for future maintainers.
rm -rf -- $(wildcard ... $(wildcard environments/*/packages.yaml) post-install modules-done env-meta store.squashfs
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.
naming: should this go under spack:packages:... or spack:builtin:... or something else instead?
not a strong opinion on this, but I like more the idea of
config:
spack:
repo:
commit:
packages:
repo: ...
commit:
The main reason is that by nesting the packages repo configuration in the spack
one it keeps them closer in the config file, while having them separated would allow for more freedom and possible separation between the entries. It sounds more "natural" to me.
should the spack-packages repo go into the squashfs? I've currently put it into the squashfs next to repo. I would argue that this could simplify many use cases and documentation for using a uenv as a spack upstream.
Not sure about this. I have to double check, but IIRC when you use a spack instance as upstream, the concretization happens with your personal instance using its own set of recipes (ignoring upstream ones). Hence, from the upstream it just uses the install tree.
This make me wonder about another aspect: should we now suggest to use the same spack-packages
version rather than the same spack
version?
I like the sound of this 👍
Good point. I think if all you do is set
Yeah, I think this is probably the right thing to recommend. It doesn't hurt to keep a warning about spack commits being too far away the one used to build the uenv, but generally speaking that should be less of a problem post-spack 1.0. |
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 am leaning towards putting the spack-packages repo inside the uenv image
A quick overview of how the repo
path inside uenv is constructed today:
- copy the site repo from
alps-cluster-config
- add packages from
repo/packages
in the recipe (overwriting) existing packages, so that the recipe author can fully customise the packages
There is some more detail documented here:
https://eth-cscs.github.io/stackinator/cluster-config/#site-and-system-configurations
And the implementation is here:
https://github.com/eth-cscs/stackinator/blob/main/stackinator/builder.py#L371
We could leverage this to copy the packages from repos/spack_repo/builtin
into this repo, or alternatively store it alongside the "custom repo" that we already construct as a standalone repo, and add it to the repos.yaml
file in $mount/config
with lower precedence than the custom repo.
The path repos/spack_repo/builtin
is ~90 MB uncompressed, but shrinks down to ~7 MB when compressed with -Xcompression-level 3
by mksquashfs, so it wouldn't significantly increase the size of the squashfs images.
I agree. Assuming the repo goes into the squashfs (which I also like), I'd lean towards keeping spack-packages/builtin separate from the alps namespace because 1. it keeps the separation of upstream packages vs. customized packages clean (exactly as it is right now) with |
spack_packages
to recipe schema to allow specifying a fixed commit of the builtin packagesspack:packages
to recipe schema to allow specifying a fixed commit of the builtin packages
508dbe1
to
9ac6c0c
Compare
Thanks for checking that. I'm currently actually including the whole git repo, but that makes little sense because:
I'll change that to only include the package repository, not the git repository, in the squashfs. |
In theory this is ready, but I haven't been able to do a proper full test build with this on alps today, so leaving it a draft until I've done that. |
Successfully built a uenv exposing the builtin repo today after fixing this: 326be75. This is now ready to be merged. |
326be75
to
40276d8
Compare
Fixes #225.
This adds a way to specify the spack-packages repo and commit for a recipe. In config.yaml, one can now say
exactly as for
spack
.