spack.repo.PATH: fix lazy constructor#50777
Merged
Conversation
85c6c29 to
9738117
Compare
spack.repo.PATH is constructed either in: 1. spack.main.setup_main_options (entrypoint of Spack) 2. or spack.repo.PATH.__getattr__ (singleton, in sub-processes that don't run main) The latter was using an old named constructor of RepoPath, which had regressed.
9738117 to
44628bb
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR addresses two regressions in repository construction by unifying the initialization of spack.repo.PATH using descriptors instead of legacy paths and by ensuring that package overrides from packages.yaml are properly applied.
- Updates test cases to use RepoDescriptors instead of legacy path lists.
- Replaces the use of from_paths with from_descriptors in RepoPath construction and adjusts related function signatures.
- Revises command and repo modules to pass required parameters (cache and overrides) for repository construction.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/spack/spack/test/repo.py | Updated tests to use descriptors and validate both Repo and RepoPath. |
| lib/spack/spack/test/cmd/repo.py | Adjusted initialization and construction to pass cache and overrides. |
| lib/spack/spack/repo.py | Replaced from_paths with from_descriptors and updated construct method signatures. |
| lib/spack/spack/main.py | Simplified repo enabling by using RepoPath.from_config. |
| lib/spack/spack/cmd/repo.py | Updated repo commands to supply required cache parameter in construct calls. |
Comments suppressed due to low confidence (3)
lib/spack/spack/repo.py:671
- Consider updating the docstring for 'from_descriptors' to clearly document the purpose and usage of the new 'fetch' and 'overrides' parameters, to help users transitioning from the deprecated 'from_paths'.
def from_descriptors(descriptors: "RepoDescriptors", cache: spack.util.file_cache.FileCache, overrides: Optional[Dict[str, Any]] = None) -> "RepoPath":
lib/spack/spack/test/repo.py:291
- [nitpick] Consider splitting tests for 'Repo' and 'RepoPath' into separate test cases to isolate and more precisely diagnose any issues arising from the unified construction logic.
def test_all_package_names(self, mock_test_cache):
lib/spack/spack/test/cmd/repo.py:263
- Ensure that all call sites for 'initialize' and 'construct' consistently pass a valid 'cache' instance, and consider adding inline comments or tests to verify that the new parameters are handled correctly across different repositories.
def initialize(self, fetch=True, git=None) -> None:
tgamblin
requested changes
Jun 3, 2025
Member
tgamblin
left a comment
There was a problem hiding this comment.
One change request, otherwise LGTM
Member
|
We agreed to restore unloadable repositories-as-errors in a later (but very soon) PR and to merge this for now. |
tgamblin
approved these changes
Jun 3, 2025
kshea21
pushed a commit
to kshea21/spack
that referenced
this pull request
Jun 18, 2025
spack.repo.PATH is constructed either in: 1. spack.main.setup_main_options (entrypoint of Spack) 2. or spack.repo.PATH.__getattr__ (singleton, in sub-processes that don't run main) The latter was using an old named constructor of RepoPath, which had regressed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two regressions introduced in #50650.
Regression 1
spack.repo.PATHis constructed either inspack.mainor through a singleton inspack.repo. Those initialization functions were out of sync; the latter used a named constructor that was not updated as part of #50650. That broke Spack on macOS where sub-processes instantiatespack.repo.PATHwith the singleton.Now both
spack.mainandspack.repouseRepoPath.from_configconsistently.Regression 2
When
spack.repo.PATHwas instantiated fromspack.mainit no longer receivedoverridesfrom packages.yaml config.Additionally, allow Spack to be run without any package repo configured, this is at least useful this week.