From 8e7c53a8ba32e0793e36e66b5fdac8f4f61e644d Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Wed, 9 Aug 2023 16:28:06 +0200 Subject: [PATCH] package import: remove magic import line (#39183) --- 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, 48 insertions(+), 43 deletions(-) diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py index ddcbc9acc2..f0b95ab7de 100644 --- a/lib/spack/spack/build_environment.py +++ b/lib/spack/spack/build_environment.py @@ -1266,12 +1266,8 @@ 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 = ["{0}:{1:d}, in {2}:".format(filename, lineno, frame.f_code.co_name)] + lines = [f"{filename}:{lineno:d}, in {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 f301f13e8c..4fb5402216 100644 --- a/lib/spack/spack/repo.py +++ b/lib/spack/spack/repo.py @@ -78,43 +78,6 @@ 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) @@ -155,7 +118,9 @@ 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 RepoLoader(fullname, repo, package_name) + return importlib.machinery.SourceFileLoader( + fullname, repo.filename_for_package_name(package_name) + ) # We are importing a full namespace like 'spack.pkg.builtin' if fullname == repo.full_namespace: @@ -1236,6 +1201,11 @@ 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 22baead777..3b83fa25a7 100644 --- a/lib/spack/spack/test/concretize.py +++ b/lib/spack/spack/test/concretize.py @@ -145,6 +145,8 @@ 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" @@ -157,6 +159,8 @@ 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 ff0e354965..4436e428c9 100644 --- a/lib/spack/spack/test/concretize_requirements.py +++ b/lib/spack/spack/test/concretize_requirements.py @@ -29,6 +29,8 @@ def update_packages_config(conf_str): _pkgx = ( "x", """\ +from spack.package import * + class X(Package): version("1.1") version("1.0") @@ -45,6 +47,8 @@ class X(Package): _pkgy = ( "y", """\ +from spack.package import * + class Y(Package): version("2.5") version("2.4") @@ -59,6 +63,8 @@ class Y(Package): _pkgv = ( "v", """\ +from spack.package import * + class V(Package): version("2.1") version("2.0") @@ -69,6 +75,8 @@ class V(Package): _pkgt = ( "t", """\ +from spack.package import * + class T(Package): version('2.1') version('2.0') @@ -81,6 +89,8 @@ 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 3fb4a169f5..0ac0bec0ba 100644 --- a/lib/spack/spack/test/packages.py +++ b/lib/spack/spack/test/packages.py @@ -4,11 +4,13 @@ # 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 @@ -318,3 +320,24 @@ 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 82bd50bd05..a4a52ec700 100644 --- a/share/spack/templates/mock-repository/package.pyt +++ b/share/spack/templates/mock-repository/package.pyt @@ -1,3 +1,5 @@ +from spack.package import * + class {{ cls_name }}(Package): homepage = "http://www.example.com" url = "http://www.example.com/root-1.0.tar.gz"