package import: remove magic import line (#39183)

5 files changed, 46 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 get_package_context(traceback, context=3):
     # 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 @@ class ReposFinder:
                 # 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 @@ class Repo:
             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 @@ 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")