From ea082539e43eaaa489b4bc68f1c823bd6fa278c8 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Fri, 11 Aug 2023 00:04:40 -0700 Subject: [PATCH] Revert "package import: remove magic import line (#39183)" (#39380) This reverts commit 8e7c53a8ba32e0793e36e66b5fdac8f4f61e644d. --- lib/spack/spack/build_environment.py | 6 ++- lib/spack/spack/repo.py | 46 +++++++++++++++---- lib/spack/spack/test/concretize.py | 4 -- .../spack/test/concretize_requirements.py | 10 ---- lib/spack/spack/test/packages.py | 23 ---------- .../templates/mock-repository/package.pyt | 2 - 6 files changed, 43 insertions(+), 48 deletions(-) diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py index 28df13771e9..ff99515d668 100644 --- a/lib/spack/spack/build_environment.py +++ b/lib/spack/spack/build_environment.py @@ -1265,8 +1265,12 @@ def make_stack(tb, stack=None): # Point out the location in the install method where we failed. filename = inspect.getfile(frame.f_code) lineno = frame.f_lineno + if os.path.basename(filename) == "package.py": + # subtract 1 because we inject a magic import at the top of package files. + # TODO: get rid of the magic import. + lineno -= 1 - lines = [f"{filename}:{lineno:d}, in {frame.f_code.co_name}:"] + lines = ["{0}:{1:d}, in {2}:".format(filename, lineno, frame.f_code.co_name)] # Build a message showing context in the install method. sourcelines, start = inspect.getsourcelines(frame) diff --git a/lib/spack/spack/repo.py b/lib/spack/spack/repo.py index d24c27be101..f57a79aaeaa 100644 --- a/lib/spack/spack/repo.py +++ b/lib/spack/spack/repo.py @@ -78,6 +78,43 @@ def namespace_from_fullname(fullname): return namespace +class _PrependFileLoader(importlib.machinery.SourceFileLoader): + def __init__(self, fullname, path, prepend=None): + super(_PrependFileLoader, self).__init__(fullname, path) + self.prepend = prepend + + def path_stats(self, path): + stats = super(_PrependFileLoader, self).path_stats(path) + if self.prepend: + stats["size"] += len(self.prepend) + 1 + return stats + + def get_data(self, path): + data = super(_PrependFileLoader, self).get_data(path) + if path != self.path or self.prepend is None: + return data + else: + return self.prepend.encode() + b"\n" + data + + +class RepoLoader(_PrependFileLoader): + """Loads a Python module associated with a package in specific repository""" + + #: Code in ``_package_prepend`` is prepended to imported packages. + #: + #: Spack packages are expected to call `from spack.package import *` + #: themselves, but we are allowing a deprecation period before breaking + #: external repos that don't do this yet. + _package_prepend = "from spack.package import *" + + def __init__(self, fullname, repo, package_name): + self.repo = repo + self.package_name = package_name + self.package_py = repo.filename_for_package_name(package_name) + self.fullname = fullname + super().__init__(self.fullname, self.package_py, prepend=self._package_prepend) + + class SpackNamespaceLoader: def create_module(self, spec): return SpackNamespace(spec.name) @@ -118,9 +155,7 @@ def compute_loader(self, fullname): # With 2 nested conditionals we can call "repo.real_name" only once package_name = repo.real_name(module_name) if package_name: - return importlib.machinery.SourceFileLoader( - fullname, repo.filename_for_package_name(package_name) - ) + return RepoLoader(fullname, repo, package_name) # We are importing a full namespace like 'spack.pkg.builtin' if fullname == repo.full_namespace: @@ -1201,11 +1236,6 @@ def get_pkg_class(self, pkg_name): raise UnknownPackageError(fullname) except Exception as e: msg = f"cannot load package '{pkg_name}' from the '{self.namespace}' repository: {e}" - if isinstance(e, NameError): - msg += ( - ". This usually means `from spack.package import *` " - "is missing at the top of the package.py file." - ) raise RepoError(msg) from e cls = getattr(module, class_name) diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py index 031eb999e42..465edfdb083 100644 --- a/lib/spack/spack/test/concretize.py +++ b/lib/spack/spack/test/concretize.py @@ -145,8 +145,6 @@ def repo_with_changing_recipe(tmpdir_factory, mutable_mock_repo): packages_dir = repo_dir.ensure("packages", dir=True) root_pkg_str = """ -from spack.package import * - class Root(Package): homepage = "http://www.example.com" url = "http://www.example.com/root-1.0.tar.gz" @@ -159,8 +157,6 @@ class Root(Package): packages_dir.join("root", "package.py").write(root_pkg_str, ensure=True) changing_template = """ -from spack.package import * - class Changing(Package): homepage = "http://www.example.com" url = "http://www.example.com/changing-1.0.tar.gz" diff --git a/lib/spack/spack/test/concretize_requirements.py b/lib/spack/spack/test/concretize_requirements.py index 4436e428c9e..ff0e354965f 100644 --- a/lib/spack/spack/test/concretize_requirements.py +++ b/lib/spack/spack/test/concretize_requirements.py @@ -29,8 +29,6 @@ def update_packages_config(conf_str): _pkgx = ( "x", """\ -from spack.package import * - class X(Package): version("1.1") version("1.0") @@ -47,8 +45,6 @@ class X(Package): _pkgy = ( "y", """\ -from spack.package import * - class Y(Package): version("2.5") version("2.4") @@ -63,8 +59,6 @@ class Y(Package): _pkgv = ( "v", """\ -from spack.package import * - class V(Package): version("2.1") version("2.0") @@ -75,8 +69,6 @@ class V(Package): _pkgt = ( "t", """\ -from spack.package import * - class T(Package): version('2.1') version('2.0') @@ -89,8 +81,6 @@ class T(Package): _pkgu = ( "u", """\ -from spack.package import * - class U(Package): version('1.1') version('1.0') diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py index bf34155aba8..4a5831dce70 100644 --- a/lib/spack/spack/test/packages.py +++ b/lib/spack/spack/test/packages.py @@ -4,13 +4,11 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) import os -import re import pytest import spack.directives import spack.fetch_strategy -import spack.package_base import spack.repo from spack.paths import mock_packages_path from spack.spec import Spec @@ -320,24 +318,3 @@ def test_package_deprecated_version(mock_packages, mock_fetch, mock_stage): assert spack.package_base.deprecated_version(pkg_cls, "1.1.0") assert not spack.package_base.deprecated_version(pkg_cls, "1.0.0") - - -def test_package_with_deprecated_magic_import_has_a_useful_error(tmpdir, mutable_config): - """Test that a package that's missing `from spack.package import *` gets a useful error, - suggesting that it be added.""" - tmpdir.join("repo.yaml").write("repo:\n namespace: old_package") - tmpdir.join("packages", "old-package").ensure(dir=True).join("package.py").write( - """\ -class OldPackage(Package): - version('1.0', '0123456789abcdef0123456789abcdef') -""" - ) - with spack.repo.use_repositories(str(tmpdir)) as repo: - with pytest.raises( - spack.repo.RepoError, - match=re.escape( - "This usually means `from spack.package import *` " - "is missing at the top of the package.py file." - ), - ): - repo.get_pkg_class("old-package") diff --git a/share/spack/templates/mock-repository/package.pyt b/share/spack/templates/mock-repository/package.pyt index a4a52ec700c..82bd50bd053 100644 --- a/share/spack/templates/mock-repository/package.pyt +++ b/share/spack/templates/mock-repository/package.pyt @@ -1,5 +1,3 @@ -from spack.package import * - class {{ cls_name }}(Package): homepage = "http://www.example.com" url = "http://www.example.com/root-1.0.tar.gz"