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

Fix for dependent specs in view #468

Conversation

rhoneyager-tomorrow
Copy link

@rhoneyager-tomorrow rhoneyager-tomorrow commented Aug 22, 2024

This PR fixes an issue with spack environment views.

Let's say that you have this in a spack.yaml file:

spack:
  # ...
  view:
    jedi-skylab-env:
      root: /path/to/view
      select: [ jedi-skylab-env ]
      link: all

As it stands right now, if you try to generate this view (e.g. with spack env view regenerate), you get nothing even though link: all is set, which should pull in the dependencies of jedi-skylab-env. That's the bug.

The main issue is that Spack's SimpleFilesystemView is missing the logic to fill in these dependencies. It should have this logic according to the base class' comments. To fix, I've copied the correct logic over from YamlFileSystemView.

Then, I'm also ensuring that the with_dependencies option needs to be passed to when calling view.add_specs in lib/spack/spack/environment/environment.py.

I'm going to also open this upstream, but figured it's good to get this in before spack-stack 1.8.0.

Note: this now causes the exclude tests to fail, so it's not a complete fix. Work in progress for today.

@rhoneyager-tomorrow rhoneyager-tomorrow marked this pull request as draft August 22, 2024 14:12
@climbfuji
Copy link
Collaborator

Thanks for this @rhoneyager-tomorrow!

Can you link the upstream spack PR here? Also, the unit tests are failing.

I'd prefer to wait with merging this until the spack upstream PR is merged (assuming with fixed tests). Given that we don't use views except for in the JEDI CI containers, and that the release branches will be cut tomorrow with several developments still in the pipeline, it's probably not feasible and necessary to squeeze this into the release?

@rhoneyager-tomorrow
Copy link
Author

I've reported upstream, so it's fine to close. As noted in the description, the patch needs a bit of help so that it can pass the unit tests, though really the existing unit tests should also be extended.

spack#45883

@climbfuji
Copy link
Collaborator

I would actually like to keep this open. Once the upstream PR is merged, we can update this PR and get it merged in quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants