From 98c08ce5c69bd6d69689f47d6f8e1945a44cb755 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Thu, 8 May 2025 13:42:20 +0200 Subject: [PATCH] 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 | 3 +- lib/spack/spack/repo.py | 101 ++++++++++---------------- lib/spack/spack/subprocess_context.py | 3 +- lib/spack/spack/test/cmd/ci.py | 35 ++++----- lib/spack/spack/test/repo.py | 1 - share/spack/qa/run-unit-tests | 4 +- 6 files changed, 60 insertions(+), 87 deletions(-) diff --git a/lib/spack/spack/main.py b/lib/spack/spack/main.py index 458ef7d2671..d55f5ec9c4c 100644 --- a/lib/spack/spack/main.py +++ b/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 diff --git a/lib/spack/spack/repo.py b/lib/spack/spack/repo.py index e82f59e648a..580d1ab2412 100644 --- a/lib/spack/spack/repo.py +++ b/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): - self._repo_init = _path - 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 + #: The current list of repositories. + repo_path: "RepoPath" 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( - fullname[len(PKG_MODULE_PREFIX_V1) :] - ): + if self.repo_path.by_namespace.is_prefix(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 -PATH: RepoPath = llnl.util.lang.Singleton(_path) # type: ignore +#: Global package repository instance. +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 PATH + yield new_repo 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: diff --git a/lib/spack/spack/subprocess_context.py b/lib/spack/spack/subprocess_context.py index 1463cfab7a3..02c135f3469 100644 --- a/lib/spack/spack/subprocess_context.py +++ b/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: diff --git a/lib/spack/spack/test/cmd/ci.py b/lib/spack/spack/test/cmd/ci.py index 07c3d7bdf04..d2894a08a79 100644 --- a/lib/spack/spack/test/cmd/ci.py +++ b/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 - assert "Validated diff-test@2.1.6" in out + out = ci_cmd("verify-versions", commits[-1], commits[-3]) + 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 - assert "Invalid commit for diff-test@2.1.6" in out + out = ci_cmd("verify-versions", commits[-1], commits[-3], fail_on_error=False) + 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 - monkeypatch.setattr(pkg_class, "manual_download", True) - - out = ci_cmd("verify-versions", commits[-1], commits[-2]) - 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 diff --git a/lib/spack/spack/test/repo.py b/lib/spack/spack/test/repo.py index eb89ae9c1cb..13a3cb63de1 100644 --- a/lib/spack/spack/test/repo.py +++ b/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): diff --git a/share/spack/qa/run-unit-tests b/share/spack/qa/run-unit-tests index 66cca0e2a7c..60775a82862 100755 --- a/share/spack/qa/run-unit-tests +++ b/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 -# 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)" +$coverage_run $(which spack) python -c "from spack_repo.builtin.packages.mpileaks.package import Mpileaks" #----------------------------------------------------------- # Run unit tests with code coverage