repo.py: enable search paths when spack.repo.PATH is assigned (#50370)

Fixes a bug where `custom_repo.get_pkg_class("foo")` failed executing `import spack_repo.builtin` even if the builtin repo was configured globally.

Instead of assignment of `spack.repo.PATH`, the `spack.repo.enable_repo` function now assigns and enables the repo, meaning that also Python module search paths are modified.

lib/spack/spack/main.py

@@ -550,7 +550,6 @@ def setup_main_options(args):
         spack.config.CONFIG.scopes["command_line"].sections["repos"] = syaml.syaml_dict(
             [(key, [spack.paths.mock_packages_path])]
         )
-        spack.repo.PATH = spack.repo.create(spack.config.CONFIG)
 
     # If the user asked for it, don't check ssl certs.
     if args.insecure:
@@ -561,6 +560,8 @@ def setup_main_options(args):
     for config_var in args.config_vars or []:
         spack.config.add(fullpath=config_var, scope="command_line")
 
+    spack.repo.enable_repo(spack.repo.create(spack.config.CONFIG))
+
     # On Windows10 console handling for ASCI/VT100 sequences is not
     # on by default. Turn on before we try to write to console
     # with color

lib/spack/spack/repo.py

@@ -91,29 +91,8 @@ class ReposFinder:
     Returns a loader based on the inspection of the current repository list.
     """
 
-    def __init__(self):
+    #: The current list of repositories.
-        self._repo_init = _path
+    repo_path: "RepoPath"
-        self._repo: Optional[RepoType] = None
-
-    @property
-    def current_repository(self):
-        if self._repo is None:
-            self._repo = self._repo_init()
-        return self._repo
-
-    @current_repository.setter
-    def current_repository(self, value):
-        self._repo = value
-
-    @contextlib.contextmanager
-    def switch_repo(self, substitute: "RepoType"):
-        """Switch the current repository list for the duration of the context manager."""
-        old = self._repo
-        try:
-            self._repo = substitute
-            yield
-        finally:
-            self._repo = old
 
     def find_spec(self, fullname, python_path, target=None):
         # "target" is not None only when calling importlib.reload()
@@ -134,14 +113,11 @@ def compute_loader(self, fullname: str):
         namespace, dot, module_name = fullname.rpartition(".")
 
         # If it's a module in some repo, or if it is the repo's namespace, let the repo handle it.
-        current_repo = self.current_repository
-        is_repo_path = isinstance(current_repo, RepoPath)
-        if is_repo_path:
-            repos = current_repo.repos
-        else:
-            repos = [current_repo]
 
-        for repo in repos:
+        if not hasattr(self, "repo_path"):
+            return None
+
+        for repo in self.repo_path.repos:
             # We are using the namespace of the repo and the repo contains the package
             if namespace == repo.full_namespace:
                 # With 2 nested conditionals we can call "repo.real_name" only once
@@ -156,9 +132,7 @@ def compute_loader(self, fullname: str):
 
         # No repo provides the namespace, but it is a valid prefix of
         # something in the RepoPath.
-        if is_repo_path and current_repo.by_namespace.is_prefix(
+        if self.repo_path.by_namespace.is_prefix(fullname[len(PKG_MODULE_PREFIX_V1) :]):
-            fullname[len(PKG_MODULE_PREFIX_V1) :]
-        ):
             return SpackNamespaceLoader()
 
         return None
@@ -662,7 +636,6 @@ def __init__(
                 if isinstance(repo, str):
                     assert cache is not None, "cache must hold a value, when repo is a string"
                     repo = Repo(repo, cache=cache, overrides=overrides)
-                repo.finder(self)
                 self.put_last(repo)
             except RepoError as e:
                 tty.warn(
@@ -672,6 +645,20 @@ def __init__(
                     f"    spack repo rm {repo}",
                 )
 
+    def enable(self) -> None:
+        """Set the relevant search paths for package module loading"""
+        REPOS_FINDER.repo_path = self
+        for p in reversed(self.python_paths()):
+            if p not in sys.path:
+                sys.path.insert(0, p)
+
+    def disable(self) -> None:
+        """Disable the search paths for package module loading"""
+        del REPOS_FINDER.repo_path
+        for p in self.python_paths():
+            if p in sys.path:
+                sys.path.remove(p)
+
     def ensure_unwrapped(self) -> "RepoPath":
         """Ensure we unwrap this object from any dynamic wrapper (like Singleton)"""
         return self
@@ -845,9 +832,6 @@ def python_paths(self) -> List[str]:
 
     def get_pkg_class(self, pkg_name: str) -> Type["spack.package_base.PackageBase"]:
         """Find a class for the spec's package and return the class object."""
-        for p in self.python_paths():
-            if p not in sys.path:
-                sys.path.insert(0, p)
         return self.repo_for_pkg(pkg_name).get_pkg_class(pkg_name)
 
     @autospec
@@ -1094,9 +1078,6 @@ def check(condition, msg):
         # Class attribute overrides by package name
         self.overrides = overrides or {}
 
-        # Optional reference to a RepoPath to influence module import from spack.pkg
-        self._finder: Optional[RepoPath] = None
-
         # Maps that goes from package name to corresponding file stat
         self._fast_package_checker: Optional[FastPackageChecker] = None
 
@@ -1108,9 +1089,6 @@ def check(condition, msg):
     def package_api_str(self) -> str:
         return f"v{self.package_api[0]}.{self.package_api[1]}"
 
-    def finder(self, value: RepoPath) -> None:
-        self._finder = value
-
     def real_name(self, import_name: str) -> Optional[str]:
         """Allow users to import Spack packages using Python identifiers.
 
@@ -1363,11 +1341,9 @@ def get_pkg_class(self, pkg_name: str) -> Type["spack.package_base.PackageBase"]
             fullname += ".package"
 
         class_name = nm.pkg_name_to_class_name(pkg_name)
-        if self.python_path and self.python_path not in sys.path:
+
-            sys.path.insert(0, self.python_path)
         try:
-            with REPOS_FINDER.switch_repo(self._finder or self):
+            module = importlib.import_module(fullname)
-                module = importlib.import_module(fullname)
         except ImportError as e:
             raise UnknownPackageError(fullname) from e
         except Exception as e:
@@ -1560,12 +1536,6 @@ def create_or_construct(
     return from_path(repo_yaml_dir)
 
 
-def _path(configuration=None):
-    """Get the singleton RepoPath instance for Spack."""
-    configuration = configuration or spack.config.CONFIG
-    return create(configuration=configuration)
-
-
 def create(configuration: spack.config.Configuration) -> RepoPath:
     """Create a RepoPath from a configuration object.
 
@@ -1588,8 +1558,10 @@ def create(configuration: spack.config.Configuration) -> RepoPath:
     return RepoPath(*repo_dirs, cache=spack.caches.MISC_CACHE, overrides=overrides)
 
 
-#: Singleton repo path instance
+#: Global package repository instance.
-PATH: RepoPath = llnl.util.lang.Singleton(_path)  # type: ignore
+PATH: RepoPath = llnl.util.lang.Singleton(
+    lambda: create(configuration=spack.config.CONFIG)
+)  # type: ignore[assignment]
 
 # Add the finder to sys.meta_path
 REPOS_FINDER = ReposFinder()
@@ -1615,20 +1587,27 @@ def use_repositories(
     Returns:
         Corresponding RepoPath object
     """
-    global PATH
     paths = [getattr(x, "root", x) for x in paths_and_repos]
-    scope_name = "use-repo-{}".format(uuid.uuid4())
+    scope_name = f"use-repo-{uuid.uuid4()}"
     repos_key = "repos:" if override else "repos"
     spack.config.CONFIG.push_scope(
         spack.config.InternalConfigScope(name=scope_name, data={repos_key: paths})
     )
-    PATH, saved = create(configuration=spack.config.CONFIG), PATH
+    old_repo, new_repo = PATH, create(configuration=spack.config.CONFIG)
+    old_repo.disable()
+    enable_repo(new_repo)
     try:
-        with REPOS_FINDER.switch_repo(PATH):  # type: ignore
+        yield new_repo
-            yield PATH
     finally:
         spack.config.CONFIG.remove_scope(scope_name=scope_name)
-        PATH = saved
+        enable_repo(old_repo)
+
+
+def enable_repo(repo_path: RepoPath) -> None:
+    """Set the global package repository and make them available in module search paths."""
+    global PATH
+    PATH = repo_path
+    PATH.enable()
 
 
 class MockRepositoryBuilder:

lib/spack/spack/subprocess_context.py

@@ -106,7 +106,7 @@ def __init__(self):
 
     def restore(self):
         spack.config.CONFIG = self.config
-        spack.repo.PATH = spack.repo.create(self.config)
+        spack.repo.enable_repo(spack.repo.create(self.config))
         spack.platforms.host = self.platform
         spack.store.STORE = self.store
         self.test_patches.restore()
@@ -129,7 +129,6 @@ def restore(self):
 
 
 def store_patches():
-    global patches
     module_patches = list()
     class_patches = list()
     if not patches:

lib/spack/spack/test/cmd/ci.py

@@ -2028,13 +2028,12 @@ def test_ci_verify_versions_valid(
     tmpdir,
 ):
     repo, _, commits = mock_git_package_changes
-    spack.repo.PATH.put_first(repo)
+    with spack.repo.use_repositories(repo):
+        monkeypatch.setattr(spack.repo, "builtin_repo", lambda: repo)
 
-    monkeypatch.setattr(spack.repo, "builtin_repo", lambda: repo)
+        out = ci_cmd("verify-versions", commits[-1], commits[-3])
-
+        assert "Validated diff-test@2.1.5" in out
-    out = ci_cmd("verify-versions", commits[-1], commits[-3])
+        assert "Validated diff-test@2.1.6" in out
-    assert "Validated diff-test@2.1.5" in out
-    assert "Validated diff-test@2.1.6" in out
 
 
 def test_ci_verify_versions_standard_invalid(
@@ -2045,23 +2044,21 @@ def test_ci_verify_versions_standard_invalid(
     verify_git_versions_invalid,
 ):
     repo, _, commits = mock_git_package_changes
-    spack.repo.PATH.put_first(repo)
+    with spack.repo.use_repositories(repo):
+        monkeypatch.setattr(spack.repo, "builtin_repo", lambda: repo)
 
-    monkeypatch.setattr(spack.repo, "builtin_repo", lambda: repo)
+        out = ci_cmd("verify-versions", commits[-1], commits[-3], fail_on_error=False)
-
+        assert "Invalid checksum found diff-test@2.1.5" in out
-    out = ci_cmd("verify-versions", commits[-1], commits[-3], fail_on_error=False)
+        assert "Invalid commit for diff-test@2.1.6" in out
-    assert "Invalid checksum found diff-test@2.1.5" in out
-    assert "Invalid commit for diff-test@2.1.6" in out
 
 
 def test_ci_verify_versions_manual_package(monkeypatch, mock_packages, mock_git_package_changes):
     repo, _, commits = mock_git_package_changes
-    spack.repo.PATH.put_first(repo)
+    with spack.repo.use_repositories(repo):
+        monkeypatch.setattr(spack.repo, "builtin_repo", lambda: repo)
 
-    monkeypatch.setattr(spack.repo, "builtin_repo", lambda: repo)
+        pkg_class = spack.spec.Spec("diff-test").package_class
+        monkeypatch.setattr(pkg_class, "manual_download", True)
 
-    pkg_class = spack.spec.Spec("diff-test").package_class
+        out = ci_cmd("verify-versions", commits[-1], commits[-2])
-    monkeypatch.setattr(pkg_class, "manual_download", True)
+        assert "Skipping manual download package: diff-test" in out
-
-    out = ci_cmd("verify-versions", commits[-1], commits[-2])
-    assert "Skipping manual download package: diff-test" in out

lib/spack/spack/test/repo.py

@@ -312,7 +312,6 @@ class TestRepoPath:
     def test_creation_from_string(self, mock_test_cache):
         repo = spack.repo.RepoPath(spack.paths.mock_packages_path, cache=mock_test_cache)
         assert len(repo.repos) == 1
-        assert repo.repos[0]._finder is repo
         assert repo.by_namespace["builtin.mock"] is repo.repos[0]
 
     def test_get_repo(self, mock_test_cache):

share/spack/qa/run-unit-tests

@@ -40,9 +40,7 @@ spack -p --lines 20 spec mpileaks%gcc
 $coverage_run $(which spack) bootstrap status --dev --optional
 
 # Check that we can import Spack packages directly as a first import
-# TODO: this check is disabled, because sys.path is only updated once
+$coverage_run $(which spack) python -c "from spack_repo.builtin.packages.mpileaks.package import Mpileaks"
-# spack.repo.PATH.get_pkg_class is called.
-# $coverage_run $(which spack) python -c "import spack.pkg.builtin.mpileaks; repr(spack.pkg.builtin.mpileaks.Mpileaks)"
 
 #-----------------------------------------------------------
 # Run unit tests with code coverage