refactor: packages import spack.package explicitly (#30404)

Explicitly import package utilities in all packages, and corresponding fallout.

This includes:

* rename `spack.package` to `spack.package_base`
* rename `spack.pkgkit` to `spack.package`
* update all packages in builtin, builtin_mock and tutorials to include `from spack.package import *`
* update spack style
  * ensure packages include the import
  * automatically add the new import and remove any/all imports of `spack` and `spack.pkgkit`
    from packages when using `--fix`
  * add support for type-checking packages with mypy when SPACK_MYPY_CHECK_PACKAGES
    is set in the environment
* fix all type checking errors in packages in spack upstream
* update spack create to include the new imports
* update spack repo to inject the new import, injection persists to allow for a deprecation period

Original message below:
 
As requested @adamjstewart, update all packages to use pkgkit.  I ended up using isort to do this,
so repro is easy:

```console
$ isort -a 'from spack.pkgkit import *' --rm 'spack' ./var/spack/repos/builtin/packages/*/package.py
$ spack style --fix
```

There were several line spacing fixups caused either by space manipulation in isort or by packages
that haven't been touched since we added requirements, but there are no functional changes in here.

* [x] add config to isort to make sure this is maintained going forward

lib/spack/docs/developer_guide.rst

@@ -151,7 +151,7 @@ Package-related modules
 ^^^^^^^^^^^^^^^^^^^^^^^
 
 :mod:`spack.package`
-  Contains the :class:`~spack.package.Package` class, which
+  Contains the :class:`~spack.package_base.Package` class, which
   is the superclass for all packages in Spack.  Methods on ``Package``
   implement all phases of the :ref:`package lifecycle
   <package-lifecycle>` and manage the build process.

lib/spack/docs/packaging_guide.rst

@@ -2393,9 +2393,9 @@ Influence how dependents are built or run
 
 Spack provides a mechanism for dependencies to influence the
 environment of their dependents by overriding  the
-:meth:`setup_dependent_run_environment <spack.package.PackageBase.setup_dependent_run_environment>`
+:meth:`setup_dependent_run_environment <spack.package_base.PackageBase.setup_dependent_run_environment>`
 or the
-:meth:`setup_dependent_build_environment <spack.package.PackageBase.setup_dependent_build_environment>`
+:meth:`setup_dependent_build_environment <spack.package_base.PackageBase.setup_dependent_build_environment>`
 methods.
 The Qt package, for instance, uses this call:
 
@@ -2417,7 +2417,7 @@ will have the ``PYTHONPATH``, ``PYTHONHOME`` and ``PATH`` environment
 variables set appropriately before starting the installation. To make things
 even simpler the ``python setup.py`` command is also inserted into the module
 scope of dependents by overriding a third method called
-:meth:`setup_dependent_package <spack.package.PackageBase.setup_dependent_package>`
+:meth:`setup_dependent_package <spack.package_base.PackageBase.setup_dependent_package>`
 :
 
 .. literalinclude:: _spack_root/var/spack/repos/builtin/packages/python/package.py
@@ -3022,7 +3022,7 @@ The classes that are currently provided by Spack are:
 +----------------------------------------------------------+----------------------------------+
 |     **Base Class**                                       |           **Purpose**            |
 +==========================================================+==================================+
-| :class:`~spack.package.Package`                          | General base class not           |
+| :class:`~spack.package_base.Package`                     | General base class not           |
 |                                                          | specialized for any build system |
 +----------------------------------------------------------+----------------------------------+
 | :class:`~spack.build_systems.makefile.MakefilePackage`   | Specialized class for packages   |
@@ -3153,7 +3153,7 @@ for the install phase is:
     For those not used to Python instance methods, this is the
     package itself.  In this case it's an instance of ``Foo``, which
     extends ``Package``.  For API docs on Package objects, see
-    :py:class:`Package <spack.package.Package>`.
+    :py:class:`Package <spack.package_base.Package>`.
 
 ``spec``
     This is the concrete spec object created by Spack from an

lib/spack/spack/analyzers/libabigail.py

@@ -12,7 +12,7 @@
 import spack.error
 import spack.hooks
 import spack.monitor
-import spack.package
+import spack.package_base
 import spack.repo
 import spack.util.executable
 

lib/spack/spack/build_environment.py

@@ -55,7 +55,7 @@
 import spack.config
 import spack.install_test
 import spack.main
-import spack.package
+import spack.package_base
 import spack.paths
 import spack.platforms
 import spack.repo
@@ -722,7 +722,7 @@ def get_std_cmake_args(pkg):
         package were a CMakePackage instance.
 
     Args:
-        pkg (spack.package.PackageBase): package under consideration
+        pkg (spack.package_base.PackageBase): package under consideration
 
     Returns:
         list: arguments for cmake
@@ -738,7 +738,7 @@ def get_std_meson_args(pkg):
         package were a MesonPackage instance.
 
     Args:
-        pkg (spack.package.PackageBase): package under consideration
+        pkg (spack.package_base.PackageBase): package under consideration
 
     Returns:
         list: arguments for meson
@@ -748,12 +748,12 @@ def get_std_meson_args(pkg):
 
 def parent_class_modules(cls):
     """
-    Get list of superclass modules that descend from spack.package.PackageBase
+    Get list of superclass modules that descend from spack.package_base.PackageBase
 
     Includes cls.__module__
     """
-    if (not issubclass(cls, spack.package.PackageBase) or
+    if (not issubclass(cls, spack.package_base.PackageBase) or
-        issubclass(spack.package.PackageBase, cls)):
+        issubclass(spack.package_base.PackageBase, cls)):
         return []
     result = []
     module = sys.modules.get(cls.__module__)
@@ -771,7 +771,7 @@ def load_external_modules(pkg):
     associated with them.
 
     Args:
-        pkg (spack.package.PackageBase): package to load deps for
+        pkg (spack.package_base.PackageBase): package to load deps for
     """
     for dep in list(pkg.spec.traverse()):
         external_modules = dep.external_modules or []
@@ -1109,7 +1109,7 @@ def start_build_process(pkg, function, kwargs):
 
     Args:
 
-        pkg (spack.package.PackageBase): package whose environment we should set up the
+        pkg (spack.package_base.PackageBase): package whose environment we should set up the
             child process for.
         function (typing.Callable): argless function to run in the child
             process.
@@ -1234,7 +1234,7 @@ def make_stack(tb, stack=None):
         if 'self' in frame.f_locals:
             # Find the first proper subclass of PackageBase.
             obj = frame.f_locals['self']
-            if isinstance(obj, spack.package.PackageBase):
+            if isinstance(obj, spack.package_base.PackageBase):
                 break
 
     # We found obj, the Package implementation we care about.

lib/spack/spack/build_systems/aspell_dict.py

@@ -9,7 +9,7 @@
 
 from spack.build_systems.autotools import AutotoolsPackage
 from spack.directives import extends
-from spack.package import ExtensionError
+from spack.package_base import ExtensionError
 from spack.util.executable import which
 
 

lib/spack/spack/build_systems/autotools.py

@@ -16,7 +16,7 @@
 from spack.build_environment import InstallError
 from spack.directives import conflicts, depends_on
 from spack.operating_systems.mac_os import macos_version
-from spack.package import PackageBase, run_after, run_before
+from spack.package_base import PackageBase, run_after, run_before
 from spack.util.executable import Executable
 from spack.version import Version
 

lib/spack/spack/build_systems/cached_cmake.py

@@ -8,7 +8,7 @@
 from llnl.util.filesystem import install, mkdirp
 
 from spack.build_systems.cmake import CMakePackage
-from spack.package import run_after
+from spack.package_base import run_after
 
 
 def cmake_cache_path(name, value, comment=""):

lib/spack/spack/build_systems/cmake.py

@@ -18,7 +18,7 @@
 
 import spack.build_environment
 from spack.directives import conflicts, depends_on, variant
-from spack.package import InstallError, PackageBase, run_after
+from spack.package_base import InstallError, PackageBase, run_after
 from spack.util.path import convert_to_posix_path
 
 # Regex to extract the primary generator from the CMake generator

lib/spack/spack/build_systems/cuda.py

@@ -6,7 +6,7 @@
 import spack.variant
 from spack.directives import conflicts, depends_on, variant
 from spack.multimethod import when
-from spack.package import PackageBase
+from spack.package_base import PackageBase
 
 
 class CudaPackage(PackageBase):

lib/spack/spack/build_systems/gnu.py

@@ -3,14 +3,16 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-import spack.package
+from typing import Optional
+
+import spack.package_base
 import spack.util.url
 
 
-class GNUMirrorPackage(spack.package.PackageBase):
+class GNUMirrorPackage(spack.package_base.PackageBase):
     """Mixin that takes care of setting url and mirrors for GNU packages."""
     #: Path of the package in a GNU mirror
-    gnu_mirror_path = None
+    gnu_mirror_path = None  # type: Optional[str]
 
     #: List of GNU mirrors used by Spack
     base_mirrors = [

lib/spack/spack/build_systems/intel.py

@@ -26,7 +26,7 @@
 
 import spack.error
 from spack.build_environment import dso_suffix
-from spack.package import InstallError, PackageBase, run_after
+from spack.package_base import InstallError, PackageBase, run_after
 from spack.util.environment import EnvironmentModifications
 from spack.util.executable import Executable
 from spack.util.prefix import Prefix
@@ -1115,7 +1115,7 @@ def _setup_dependent_env_callback(
                 raise InstallError('compilers_of_client arg required for MPI')
 
     def setup_dependent_package(self, module, dep_spec):
-        # https://spack.readthedocs.io/en/latest/spack.html#spack.package.PackageBase.setup_dependent_package
+        # https://spack.readthedocs.io/en/latest/spack.html#spack.package_base.PackageBase.setup_dependent_package
         # Reminder: "module" refers to Python module.
         # Called before the install() method of dependents.
 

lib/spack/spack/build_systems/lua.py

@@ -10,7 +10,7 @@
 
 from spack.directives import depends_on, extends
 from spack.multimethod import when
-from spack.package import PackageBase
+from spack.package_base import PackageBase
 from spack.util.executable import Executable
 
 

lib/spack/spack/build_systems/makefile.py

@@ -11,7 +11,7 @@
 from llnl.util.filesystem import working_dir
 
 from spack.directives import conflicts
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 
 
 class MakefilePackage(PackageBase):

lib/spack/spack/build_systems/maven.py

@@ -7,7 +7,7 @@
 from llnl.util.filesystem import install_tree, working_dir
 
 from spack.directives import depends_on
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 from spack.util.executable import which
 
 

lib/spack/spack/build_systems/meson.py

@@ -11,7 +11,7 @@
 from llnl.util.filesystem import working_dir
 
 from spack.directives import depends_on, variant
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 
 
 class MesonPackage(PackageBase):

lib/spack/spack/build_systems/octave.py

@@ -6,7 +6,7 @@
 import inspect
 
 from spack.directives import extends
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 
 
 class OctavePackage(PackageBase):

lib/spack/spack/build_systems/oneapi.py

@@ -14,7 +14,7 @@
 
 from llnl.util.filesystem import find_headers, find_libraries, join_path
 
-from spack.package import Package
+from spack.package_base import Package
 from spack.util.environment import EnvironmentModifications
 from spack.util.executable import Executable
 

lib/spack/spack/build_systems/perl.py

@@ -10,7 +10,7 @@
 from llnl.util.filesystem import filter_file
 
 from spack.directives import extends
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 from spack.util.executable import Executable
 
 

lib/spack/spack/build_systems/python.py

@@ -6,6 +6,7 @@
 import os
 import re
 import shutil
+from typing import Optional
 
 import llnl.util.tty as tty
 from llnl.util.filesystem import (
@@ -19,13 +20,13 @@
 from llnl.util.lang import match_predicate
 
 from spack.directives import depends_on, extends
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 
 
 class PythonPackage(PackageBase):
     """Specialized class for packages that are built using pip."""
     #: Package name, version, and extension on PyPI
-    pypi = None
+    pypi = None  # type: Optional[str]
 
     maintainers = ['adamjstewart']
 
@@ -46,7 +47,7 @@ class PythonPackage(PackageBase):
     # package manually
     depends_on('py-wheel', type='build')
 
-    py_namespace = None
+    py_namespace = None  # type: Optional[str]
 
     @staticmethod
     def _std_args(cls):

lib/spack/spack/build_systems/qmake.py

@@ -9,7 +9,7 @@
 from llnl.util.filesystem import working_dir
 
 from spack.directives import depends_on
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 
 
 class QMakePackage(PackageBase):

lib/spack/spack/build_systems/r.py

@@ -5,9 +5,10 @@
 
 
 import inspect
+from typing import Optional
 
 from spack.directives import extends
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 
 
 class RPackage(PackageBase):
@@ -28,10 +29,10 @@ class RPackage(PackageBase):
     # package attributes that can be expanded to set the homepage, url,
     # list_url, and git values
     # For CRAN packages
-    cran = None
+    cran = None  # type: Optional[str]
 
     # For Bioconductor packages
-    bioc = None
+    bioc = None  # type: Optional[str]
 
     maintainers = ['glennpj']
 

lib/spack/spack/build_systems/racket.py

@@ -3,13 +3,14 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 import os
+from typing import Optional
 
 import llnl.util.tty as tty
 from llnl.util.filesystem import working_dir
 
 from spack.build_environment import SPACK_NO_PARALLEL_MAKE, determine_number_of_jobs
 from spack.directives import extends
-from spack.package import PackageBase
+from spack.package_base import PackageBase
 from spack.util.environment import env_flag
 from spack.util.executable import Executable, ProcessError
 
@@ -36,8 +37,8 @@ class RacketPackage(PackageBase):
     extends('racket')
 
     pkgs = False
-    subdirectory = None
+    subdirectory = None  # type: Optional[str]
-    name = None
+    name = None  # type: Optional[str]
     parallel = True
 
     @property

lib/spack/spack/build_systems/rocm.py

@@ -77,7 +77,7 @@
 
 import spack.variant
 from spack.directives import conflicts, depends_on, variant
-from spack.package import PackageBase
+from spack.package_base import PackageBase
 
 
 class ROCmPackage(PackageBase):

lib/spack/spack/build_systems/ruby.py

@@ -7,7 +7,7 @@
 import inspect
 
 from spack.directives import extends
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 
 
 class RubyPackage(PackageBase):

lib/spack/spack/build_systems/scons.py

@@ -7,7 +7,7 @@
 import inspect
 
 from spack.directives import depends_on
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 
 
 class SConsPackage(PackageBase):

lib/spack/spack/build_systems/sip.py

@@ -11,7 +11,7 @@
 from llnl.util.filesystem import find, join_path, working_dir
 
 from spack.directives import depends_on, extends
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 
 
 class SIPPackage(PackageBase):

lib/spack/spack/build_systems/sourceforge.py

@@ -3,15 +3,17 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-import spack.package
+from typing import Optional
+
+import spack.package_base
 import spack.util.url
 
 
-class SourceforgePackage(spack.package.PackageBase):
+class SourceforgePackage(spack.package_base.PackageBase):
     """Mixin that takes care of setting url and mirrors for Sourceforge
        packages."""
     #: Path of the package in a Sourceforge mirror
-    sourceforge_mirror_path = None
+    sourceforge_mirror_path = None  # type: Optional[str]
 
     #: List of Sourceforge mirrors used by Spack
     base_mirrors = [

lib/spack/spack/build_systems/sourceware.py

@@ -2,16 +2,17 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
+from typing import Optional
 
-import spack.package
+import spack.package_base
 import spack.util.url
 
 
-class SourcewarePackage(spack.package.PackageBase):
+class SourcewarePackage(spack.package_base.PackageBase):
     """Mixin that takes care of setting url and mirrors for Sourceware.org
        packages."""
     #: Path of the package in a Sourceware mirror
-    sourceware_mirror_path = None
+    sourceware_mirror_path = None  # type: Optional[str]
 
     #: List of Sourceware mirrors used by Spack
     base_mirrors = [

lib/spack/spack/build_systems/waf.py

@@ -9,7 +9,7 @@
 from llnl.util.filesystem import working_dir
 
 from spack.directives import depends_on
-from spack.package import PackageBase, run_after
+from spack.package_base import PackageBase, run_after
 
 
 class WafPackage(PackageBase):

lib/spack/spack/build_systems/xorg.py

@@ -3,15 +3,17 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-import spack.package
+from typing import Optional
+
+import spack.package_base
 import spack.util.url
 
 
-class XorgPackage(spack.package.PackageBase):
+class XorgPackage(spack.package_base.PackageBase):
     """Mixin that takes care of setting url and mirrors for x.org
        packages."""
     #: Path of the package in a x.org mirror
-    xorg_mirror_path = None
+    xorg_mirror_path = None  # type: Optional[str]
 
     #: List of x.org mirrors used by Spack
     #  Note: x.org mirrors are a bit tricky, since many are out-of-sync or off.

lib/spack/spack/cmd/checksum.py

@@ -14,7 +14,7 @@
 import spack.repo
 import spack.stage
 import spack.util.crypto
-from spack.package import preferred_version
+from spack.package_base import preferred_version
 from spack.util.naming import valid_fully_qualified_module_name
 from spack.version import Version, ver
 

lib/spack/spack/cmd/create.py

@@ -57,7 +57,7 @@
 # See the Spack documentation for more information on packaging.
 # ----------------------------------------------------------------------------
 
-from spack import *
+from spack.package import *
 
 
 class {class_name}({base_class_name}):

lib/spack/spack/cmd/dependencies.py

@@ -11,7 +11,7 @@
 import spack.cmd
 import spack.cmd.common.arguments as arguments
 import spack.environment as ev
-import spack.package
+import spack.package_base
 import spack.repo
 import spack.store
 
@@ -57,7 +57,7 @@ def dependencies(parser, args):
 
     else:
         spec = specs[0]
-        dependencies = spack.package.possible_dependencies(
+        dependencies = spack.package_base.possible_dependencies(
             spec,
             transitive=args.transitive,
             expand_virtuals=args.expand_virtuals,

lib/spack/spack/cmd/external.py

@@ -200,7 +200,7 @@ def external_list(args):
     list(spack.repo.path.all_packages())
     # Print all the detectable packages
     tty.msg("Detectable packages per repository")
-    for namespace, pkgs in sorted(spack.package.detectable_packages.items()):
+    for namespace, pkgs in sorted(spack.package_base.detectable_packages.items()):
         print("Repository:", namespace)
         colify.colify(pkgs, indent=4, output=sys.stdout)
 

lib/spack/spack/cmd/info.py

@@ -18,7 +18,7 @@
 import spack.fetch_strategy as fs
 import spack.repo
 import spack.spec
-from spack.package import has_test_method, preferred_version
+from spack.package_base import has_test_method, preferred_version
 
 description = 'get detailed information on a particular package'
 section = 'basic'
@@ -269,14 +269,14 @@ def print_tests(pkg):
     names = []
     pkg_cls = pkg if inspect.isclass(pkg) else pkg.__class__
     if has_test_method(pkg_cls):
-        pkg_base = spack.package.PackageBase
+        pkg_base = spack.package_base.PackageBase
         test_pkgs = [str(cls.test) for cls in inspect.getmro(pkg_cls) if
                      issubclass(cls, pkg_base) and cls.test != pkg_base.test]
         test_pkgs = list(set(test_pkgs))
         names.extend([(test.split()[1]).lower() for test in test_pkgs])
 
     # TODO Refactor START
-    # Use code from package.py's test_process IF this functionality is
+    # Use code from package_base.py's test_process IF this functionality is
     # accepted.
     v_names = list(set([vspec.name for vspec in pkg.virtuals_provided]))
 

lib/spack/spack/cmd/install.py

@@ -302,7 +302,7 @@ def install(parser, args, **kwargs):
         )
 
     reporter = spack.report.collect_info(
-        spack.package.PackageInstaller, '_install_task', args.log_format, args)
+        spack.package_base.PackageInstaller, '_install_task', args.log_format, args)
     if args.log_file:
         reporter.filename = args.log_file
 

lib/spack/spack/cmd/mark.py

@@ -12,7 +12,7 @@
 import spack.cmd
 import spack.cmd.common.arguments as arguments
 import spack.error
-import spack.package
+import spack.package_base
 import spack.repo
 import spack.store
 from spack.database import InstallStatuses

lib/spack/spack/cmd/solve.py

@@ -18,7 +18,7 @@
 import spack.config
 import spack.environment
 import spack.hash_types as ht
-import spack.package
+import spack.package_base
 import spack.solver.asp as asp
 
 description = "concretize a specs using an ASP solver"

lib/spack/spack/cmd/style.py

@@ -65,7 +65,7 @@ def is_package(f):
     packages, since we allow `from spack import *` and poking globals
     into packages.
     """
-    return f.startswith("var/spack/repos/")
+    return f.startswith("var/spack/repos/") and f.endswith('package.py')
 
 
 #: decorator for adding tools to the list
@@ -236,7 +236,7 @@ def translate(match):
             continue
         if not args.root_relative and re_obj:
             line = re_obj.sub(translate, line)
-        print("  " + line)
+        print(line)
 
 
 def print_style_header(file_list, args):
@@ -290,20 +290,26 @@ def run_flake8(flake8_cmd, file_list, args):
 @tool("mypy")
 def run_mypy(mypy_cmd, file_list, args):
     # always run with config from running spack prefix
-    mypy_args = [
+    common_mypy_args = [
         "--config-file", os.path.join(spack.paths.prefix, "pyproject.toml"),
-        "--package", "spack",
-        "--package", "llnl",
         "--show-error-codes",
     ]
-    # not yet, need other updates to enable this
+    mypy_arg_sets = [common_mypy_args + [
-    # if any([is_package(f) for f in file_list]):
+        "--package", "spack",
-    #     mypy_args.extend(["--package", "packages"])
+        "--package", "llnl",
+    ]]
+    if 'SPACK_MYPY_CHECK_PACKAGES' in os.environ:
+        mypy_arg_sets.append(common_mypy_args + [
+            '--package', 'packages',
+            '--disable-error-code', 'no-redef',
+        ])
 
-    output = mypy_cmd(*mypy_args, fail_on_error=False, output=str)
+    returncode = 0
-    returncode = mypy_cmd.returncode
+    for mypy_args in mypy_arg_sets:
+        output = mypy_cmd(*mypy_args, fail_on_error=False, output=str)
+        returncode |= mypy_cmd.returncode
 
-    rewrite_and_print_output(output, args)
+        rewrite_and_print_output(output, args)
 
     print_tool_result("mypy", returncode)
     return returncode
@@ -318,16 +324,29 @@ def run_isort(isort_cmd, file_list, args):
 
     pat = re.compile("ERROR: (.*) Imports are incorrectly sorted")
     replacement = "ERROR: {0} Imports are incorrectly sorted"
-    returncode = 0
+    returncode = [0]
-    for chunk in grouper(file_list, 100):
-        packed_args = isort_args + tuple(chunk)
-        output = isort_cmd(*packed_args, fail_on_error=False, output=str, error=str)
-        returncode |= isort_cmd.returncode
 
-        rewrite_and_print_output(output, args, pat, replacement)
+    def process_files(file_list, is_args):
+        for chunk in grouper(file_list, 100):
+            packed_args = is_args + tuple(chunk)
+            output = isort_cmd(*packed_args, fail_on_error=False, output=str, error=str)
+            returncode[0] |= isort_cmd.returncode
 
-    print_tool_result("isort", returncode)
+            rewrite_and_print_output(output, args, pat, replacement)
-    return returncode
+
+    packages_isort_args = ('--rm', 'spack', '--rm', 'spack.pkgkit', '--rm',
+                           'spack.package_defs', '-a', 'from spack.package import *')
+    packages_isort_args = packages_isort_args + isort_args
+
+    # packages
+    process_files(filter(is_package, file_list),
+                  packages_isort_args)
+    # non-packages
+    process_files(filter(lambda f: not is_package(f), file_list),
+                  isort_args)
+
+    print_tool_result("isort", returncode[0])
+    return returncode[0]
 
 
 @tool("black")

lib/spack/spack/cmd/test.py

@@ -20,7 +20,7 @@
 import spack.cmd.common.arguments as arguments
 import spack.environment as ev
 import spack.install_test
-import spack.package
+import spack.package_base
 import spack.repo
 import spack.report
 
@@ -189,7 +189,7 @@ def test_run(args):
     # Set up reporter
     setattr(args, 'package', [s.format() for s in test_suite.specs])
     reporter = spack.report.collect_info(
-        spack.package.PackageBase, 'do_test', args.log_format, args)
+        spack.package_base.PackageBase, 'do_test', args.log_format, args)
     if not reporter.filename:
         if args.log_file:
             if os.path.isabs(args.log_file):
@@ -217,7 +217,7 @@ def test_list(args):
         else set()
 
     def has_test_and_tags(pkg_class):
-        return spack.package.has_test_method(pkg_class) and \
+        return spack.package_base.has_test_method(pkg_class) and \
             (not args.tag or pkg_class.name in tagged)
 
     if args.list_all:

lib/spack/spack/cmd/uninstall.py

@@ -15,7 +15,7 @@
 import spack.cmd.common.arguments as arguments
 import spack.environment as ev
 import spack.error
-import spack.package
+import spack.package_base
 import spack.repo
 import spack.store
 from spack.database import InstallStatuses
@@ -221,7 +221,7 @@ def do_uninstall(env, specs, force):
         except spack.repo.UnknownEntityError:
             # The package.py file has gone away -- but still
             # want to uninstall.
-            spack.package.Package.uninstall_by_spec(item, force=True)
+            spack.package_base.Package.uninstall_by_spec(item, force=True)
 
     # A package is ready to be uninstalled when nothing else references it,
     # unless we are requested to force uninstall it.

lib/spack/spack/cmd/url.py

@@ -422,7 +422,7 @@ def url_list_parsing(args, urls, url, pkg):
         urls (set): List of URLs that have already been added
         url (str or None): A URL to potentially add to ``urls`` depending on
             ``args``
-        pkg (spack.package.PackageBase): The Spack package
+        pkg (spack.package_base.PackageBase): The Spack package
 
     Returns:
         set: The updated set of ``urls``
@@ -470,7 +470,7 @@ def name_parsed_correctly(pkg, name):
     """Determine if the name of a package was correctly parsed.
 
     Args:
-        pkg (spack.package.PackageBase): The Spack package
+        pkg (spack.package_base.PackageBase): The Spack package
         name (str): The name that was extracted from the URL
 
     Returns:
@@ -487,7 +487,7 @@ def version_parsed_correctly(pkg, version):
     """Determine if the version of a package was correctly parsed.
 
     Args:
-        pkg (spack.package.PackageBase): The Spack package
+        pkg (spack.package_base.PackageBase): The Spack package
         version (str): The version that was extracted from the URL
 
     Returns:

lib/spack/spack/detection/common.py

@@ -240,7 +240,7 @@ def compute_windows_program_path_for_package(pkg):
     program files location, return list of best guesses
 
     Args:
-        pkg (spack.package.Package): package for which
+        pkg (spack.package_base.Package): package for which
                            Program Files location is to be computed
     """
     if not is_windows:

lib/spack/spack/fetch_strategy.py

@@ -1556,7 +1556,7 @@ def _extrapolate(pkg, version):
     try:
         return URLFetchStrategy(pkg.url_for_version(version),
                                 fetch_options=pkg.fetch_options)
-    except spack.package.NoURLError:
+    except spack.package_base.NoURLError:
         msg = ("Can't extrapolate a URL for version %s "
                "because package %s defines no URLs")
         raise ExtrapolationError(msg % (version, pkg.name))

lib/spack/spack/installer.py

@@ -50,7 +50,7 @@
 import spack.error
 import spack.hooks
 import spack.monitor
-import spack.package
+import spack.package_base
 import spack.package_prefs as prefs
 import spack.repo
 import spack.store
@@ -103,7 +103,7 @@ def _check_last_phase(pkg):
     package already.
 
     Args:
-        pkg (spack.package.PackageBase): the package being installed
+        pkg (spack.package_base.PackageBase): the package being installed
 
     Raises:
         ``BadInstallPhase`` if stop_before or last phase is invalid
@@ -125,7 +125,7 @@ def _handle_external_and_upstream(pkg, explicit):
     database if it is external package.
 
     Args:
-        pkg (spack.package.Package): the package whose installation is under
+        pkg (spack.package_base.Package): the package whose installation is under
             consideration
         explicit (bool): the package was explicitly requested by the user
     Return:
@@ -265,7 +265,7 @@ def _install_from_cache(pkg, cache_only, explicit, unsigned=False):
     Extract the package from binary cache
 
     Args:
-        pkg (spack.package.PackageBase): the package to install from the binary cache
+        pkg (spack.package_base.PackageBase): package to install from the binary cache
         cache_only (bool): only extract from binary cache
         explicit (bool): ``True`` if installing the package was explicitly
             requested by the user, otherwise, ``False``
@@ -355,7 +355,7 @@ def _process_binary_cache_tarball(pkg, binary_spec, explicit, unsigned,
     Process the binary cache tarball.
 
     Args:
-        pkg (spack.package.PackageBase): the package being installed
+        pkg (spack.package_base.PackageBase): the package being installed
         binary_spec (spack.spec.Spec): the spec  whose cache has been confirmed
         explicit (bool): the package was explicitly requested by the user
         unsigned (bool): ``True`` if binary package signatures to be checked,
@@ -394,7 +394,7 @@ def _try_install_from_binary_cache(pkg, explicit, unsigned=False):
     Try to extract the package from binary cache.
 
     Args:
-        pkg (spack.package.PackageBase): the package to be extracted from binary cache
+        pkg (spack.package_base.PackageBase): package to be extracted from binary cache
         explicit (bool): the package was explicitly requested by the user
         unsigned (bool): ``True`` if binary package signatures to be checked,
             otherwise, ``False``
@@ -530,7 +530,7 @@ def log(pkg):
     Copy provenance into the install directory on success
 
     Args:
-        pkg (spack.package.Package): the package that was built and installed
+        pkg (spack.package_base.Package): the package that was built and installed
     """
     packages_dir = spack.store.layout.build_packages_path(pkg.spec)
 
@@ -616,7 +616,7 @@ def package_id(pkg):
     and packages for combinatorial environments.
 
     Args:
-        pkg (spack.package.PackageBase): the package from which the identifier is
+        pkg (spack.package_base.PackageBase): the package from which the identifier is
             derived
     """
     if not pkg.spec.concrete:
@@ -769,7 +769,7 @@ def _add_bootstrap_compilers(
         Args:
             compiler: the compiler to boostrap
             architecture: the architecture for which to bootstrap the compiler
-            pkgs (spack.package.PackageBase): the package with possible compiler
+            pkgs (spack.package_base.PackageBase): the package with possible compiler
                 dependencies
             request (BuildRequest): the associated install request
             all_deps (defaultdict(set)): dictionary of all dependencies and
@@ -786,7 +786,7 @@ def _add_init_task(self, pkg, request, is_compiler, all_deps):
         Creates and queus the initial build task for the package.
 
         Args:
-            pkg (spack.package.Package): the package to be built and installed
+            pkg (spack.package_base.Package): the package to be built and installed
             request (BuildRequest or None): the associated install request
                  where ``None`` can be used to indicate the package was
                  explicitly requested by the user
@@ -968,7 +968,7 @@ def _cleanup_task(self, pkg):
         Cleanup the build task for the spec
 
         Args:
-            pkg (spack.package.PackageBase): the package being installed
+            pkg (spack.package_base.PackageBase): the package being installed
         """
         self._remove_task(package_id(pkg))
 
@@ -982,7 +982,7 @@ def _ensure_install_ready(self, pkg):
         already locked.
 
         Args:
-            pkg (spack.package.PackageBase): the package being locally installed
+            pkg (spack.package_base.PackageBase): the package being locally installed
         """
         pkg_id = package_id(pkg)
         pre = "{0} cannot be installed locally:".format(pkg_id)
@@ -1014,7 +1014,8 @@ def _ensure_locked(self, lock_type, pkg):
 
         Args:
             lock_type (str): 'read' for a read lock, 'write' for a write lock
-            pkg (spack.package.PackageBase): the package whose spec is being installed
+            pkg (spack.package_base.PackageBase): the package whose spec is being
+                                                  installed
 
         Return:
             (lock_type, lock) tuple where lock will be None if it could not
@@ -1228,7 +1229,7 @@ def _install_task(self, task):
 
             # Create a child process to do the actual installation.
             # Preserve verbosity settings across installs.
-            spack.package.PackageBase._verbose = (
+            spack.package_base.PackageBase._verbose = (
                 spack.build_environment.start_build_process(
                     pkg, build_process, install_args)
             )
@@ -1373,7 +1374,7 @@ def _setup_install_dir(self, pkg):
         Write a small metadata file with the current spack environment.
 
         Args:
-            pkg (spack.package.Package): the package to be built and installed
+            pkg (spack.package_base.Package): the package to be built and installed
         """
         if not os.path.exists(pkg.spec.prefix):
             tty.debug('Creating the installation directory {0}'.format(pkg.spec.prefix))
@@ -1447,7 +1448,7 @@ def _flag_installed(self, pkg, dependent_ids=None):
         known dependents.
 
         Args:
-            pkg (spack.package.Package): Package that has been installed locally,
+            pkg (spack.package_base.Package): Package that has been installed locally,
                 externally or upstream
             dependent_ids (list or None): list of the package's
                 dependent ids, or None if the dependent ids are limited to
@@ -1536,7 +1537,7 @@ def install(self):
         Install the requested package(s) and or associated dependencies.
 
         Args:
-            pkg (spack.package.Package): the package to be built and installed"""
+            pkg (spack.package_base.Package): the package to be built and installed"""
 
         self._init_queue()
         fail_fast_err = 'Terminating after first install failure'
@@ -1788,7 +1789,7 @@ def __init__(self, pkg, install_args):
         process in the build.
 
         Arguments:
-            pkg (spack.package.PackageBase) the package being installed.
+            pkg (spack.package_base.PackageBase) the package being installed.
             install_args (dict) arguments to do_install() from parent process.
 
         """
@@ -1848,8 +1849,8 @@ def run(self):
 
         # get verbosity from do_install() parameter or saved value
         self.echo = self.verbose
-        if spack.package.PackageBase._verbose is not None:
+        if spack.package_base.PackageBase._verbose is not None:
-            self.echo = spack.package.PackageBase._verbose
+            self.echo = spack.package_base.PackageBase._verbose
 
         self.pkg.stage.keep = self.keep_stage
 
@@ -2001,7 +2002,7 @@ def build_process(pkg, install_args):
     This function's return value is returned to the parent process.
 
     Arguments:
-        pkg (spack.package.PackageBase): the package being installed.
+        pkg (spack.package_base.PackageBase): the package being installed.
         install_args (dict): arguments to do_install() from parent process.
 
     """
@@ -2049,7 +2050,7 @@ def __init__(self, pkg, request, compiler, start, attempts, status,
         Instantiate a build task for a package.
 
         Args:
-            pkg (spack.package.Package): the package to be built and installed
+            pkg (spack.package_base.Package): the package to be built and installed
             request (BuildRequest or None): the associated install request
                  where ``None`` can be used to indicate the package was
                  explicitly requested by the user
@@ -2062,7 +2063,7 @@ def __init__(self, pkg, request, compiler, start, attempts, status,
         """
 
         # Ensure dealing with a package that has a concrete spec
-        if not isinstance(pkg, spack.package.PackageBase):
+        if not isinstance(pkg, spack.package_base.PackageBase):
             raise ValueError("{0} must be a package".format(str(pkg)))
 
         self.pkg = pkg
@@ -2240,11 +2241,11 @@ def __init__(self, pkg, install_args):
         Instantiate a build request for a package.
 
         Args:
-            pkg (spack.package.Package): the package to be built and installed
+            pkg (spack.package_base.Package): the package to be built and installed
             install_args (dict): the install arguments associated with ``pkg``
         """
         # Ensure dealing with a package that has a concrete spec
-        if not isinstance(pkg, spack.package.PackageBase):
+        if not isinstance(pkg, spack.package_base.PackageBase):
             raise ValueError("{0} must be a package".format(str(pkg)))
 
         self.pkg = pkg
@@ -2314,7 +2315,7 @@ def get_deptypes(self, pkg):
         """Determine the required dependency types for the associated package.
 
         Args:
-            pkg (spack.package.PackageBase): explicit or implicit package being
+            pkg (spack.package_base.PackageBase): explicit or implicit package being
                 installed
 
         Returns:
@@ -2337,7 +2338,7 @@ def run_tests(self, pkg):
         """Determine if the tests should be run for the provided packages
 
         Args:
-            pkg (spack.package.PackageBase): explicit or implicit package being
+            pkg (spack.package_base.PackageBase): explicit or implicit package being
                 installed
 
         Returns:

lib/spack/spack/patch.py

@@ -356,7 +356,7 @@ def patch_for_package(self, sha256, pkg):
 
         Arguments:
             sha256 (str): sha256 hash to look up
-            pkg (spack.package.Package): Package object to get patch for.
+            pkg (spack.package_base.Package): Package object to get patch for.
 
         We build patch objects lazily because building them requires that
         we have information about the package's location in its repo.

lib/spack/spack/pkgkit.py

@@ -1,83 +0,0 @@
-# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
-# Spack Project Developers. See the top-level COPYRIGHT file for details.
-#
-# SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-# flake8: noqa: F401
-"""pkgkit is a set of useful build tools and directives for packages.
-
-Everything in this module is automatically imported into Spack package files.
-"""
-import llnl.util.filesystem
-from llnl.util.filesystem import *
-
-import spack.directives
-import spack.util.executable
-from spack.build_systems.aspell_dict import AspellDictPackage
-from spack.build_systems.autotools import AutotoolsPackage
-from spack.build_systems.cached_cmake import (
-    CachedCMakePackage,
-    cmake_cache_option,
-    cmake_cache_path,
-    cmake_cache_string,
-)
-from spack.build_systems.cmake import CMakePackage
-from spack.build_systems.cuda import CudaPackage
-from spack.build_systems.gnu import GNUMirrorPackage
-from spack.build_systems.intel import IntelPackage
-from spack.build_systems.lua import LuaPackage
-from spack.build_systems.makefile import MakefilePackage
-from spack.build_systems.maven import MavenPackage
-from spack.build_systems.meson import MesonPackage
-from spack.build_systems.octave import OctavePackage
-from spack.build_systems.oneapi import (
-    IntelOneApiLibraryPackage,
-    IntelOneApiPackage,
-    IntelOneApiStaticLibraryList,
-)
-from spack.build_systems.perl import PerlPackage
-from spack.build_systems.python import PythonPackage
-from spack.build_systems.qmake import QMakePackage
-from spack.build_systems.r import RPackage
-from spack.build_systems.racket import RacketPackage
-from spack.build_systems.rocm import ROCmPackage
-from spack.build_systems.ruby import RubyPackage
-from spack.build_systems.scons import SConsPackage
-from spack.build_systems.sip import SIPPackage
-from spack.build_systems.sourceforge import SourceforgePackage
-from spack.build_systems.sourceware import SourcewarePackage
-from spack.build_systems.waf import WafPackage
-from spack.build_systems.xorg import XorgPackage
-from spack.dependency import all_deptypes
-from spack.directives import *
-from spack.install_test import get_escaped_text_output
-from spack.installer import (
-    ExternalPackageError,
-    InstallError,
-    InstallLockError,
-    UpstreamPackageError,
-)
-from spack.mixins import filter_compiler_wrappers
-from spack.multimethod import when
-from spack.package import (
-    BundlePackage,
-    DependencyConflictError,
-    Package,
-    build_system_flags,
-    env_flags,
-    flatten_dependencies,
-    inject_flags,
-    install_dependency_symlinks,
-    on_package_attributes,
-    run_after,
-    run_before,
-)
-from spack.spec import InvalidSpecDetected, Spec
-from spack.util.executable import *
-from spack.variant import (
-    any_combination_of,
-    auto_or_any_combination_of,
-    conditional,
-    disjoint_sets,
-)
-from spack.version import Version, ver

lib/spack/spack/repo.py

@@ -202,17 +202,11 @@ 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 were originally expected to call `from spack import *`
+    #: Spack packages are expected to call `from spack.package import *`
-    #: themselves, but it became difficult to manage and imports in the Spack
+    #: themselves, but we are allowing a deprecation period before breaking
-    #: core the top-level namespace polluted by package symbols this way.  To
+    #: external repos that don't do this yet.
-    #: solve this, the top-level ``spack`` package contains very few symbols
-    #: of its own, and importing ``*`` is essentially a no-op.  The common
-    #: routines and directives that packages need are now in ``spack.pkgkit``,
-    #: and the import system forces packages to automatically include
-    #: this. This way, old packages that call ``from spack import *`` will
-    #: continue to work without modification, but it's no longer required.
     _package_prepend = ('from __future__ import absolute_import;'
-                        'from spack.pkgkit import *')
+                        'from spack.package import *')
 
     def __init__(self, fullname, repo, package_name):
         self.repo = repo
@@ -450,10 +444,10 @@ def is_package_file(filename):
     # Package files are named `package.py` and are not in lib/spack/spack
     # We have to remove the file extension because it can be .py and can be
     # .pyc depending on context, and can differ between the files
-    import spack.package  # break cycle
+    import spack.package_base  # break cycle
     filename_noext = os.path.splitext(filename)[0]
     packagebase_filename_noext = os.path.splitext(
-        inspect.getfile(spack.package.PackageBase))[0]
+        inspect.getfile(spack.package_base.PackageBase))[0]
     return (filename_noext != packagebase_filename_noext and
             os.path.basename(filename_noext) == 'package')
 

lib/spack/spack/report.py

@@ -15,7 +15,7 @@
 
 import spack.build_environment
 import spack.fetch_strategy
-import spack.package
+import spack.package_base
 from spack.install_test import TestSuite
 from spack.reporter import Reporter
 from spack.reporters.cdash import CDash
@@ -131,7 +131,7 @@ def gather_info(do_fn):
             """
             @functools.wraps(do_fn)
             def wrapper(instance, *args, **kwargs):
-                if isinstance(instance, spack.package.PackageBase):
+                if isinstance(instance, spack.package_base.PackageBase):
                     pkg = instance
                 elif hasattr(args[0], 'pkg'):
                     pkg = args[0].pkg

lib/spack/spack/reporters/cdash.py

@@ -22,7 +22,7 @@
 
 import spack.build_environment
 import spack.fetch_strategy
-import spack.package
+import spack.package_base
 from spack.error import SpackError
 from spack.reporter import Reporter
 from spack.util.crypto import checksum

lib/spack/spack/reporters/junit.py

@@ -7,7 +7,7 @@
 
 import spack.build_environment
 import spack.fetch_strategy
-import spack.package
+import spack.package_base
 from spack.reporter import Reporter
 
 __all__ = ['JUnit']

lib/spack/spack/solver/asp.py

@@ -41,7 +41,7 @@
 import spack.directives
 import spack.environment as ev
 import spack.error
-import spack.package
+import spack.package_base
 import spack.package_prefs
 import spack.platforms
 import spack.repo
@@ -1822,7 +1822,7 @@ def setup(self, driver, specs, reuse=None):
         self.possible_virtuals = set(
             x.name for x in specs if x.virtual
         )
-        possible = spack.package.possible_dependencies(
+        possible = spack.package_base.possible_dependencies(
             *specs,
             virtuals=self.possible_virtuals,
             deptype=spack.dependency.all_deptypes

lib/spack/spack/test/build_systems.py

@@ -128,7 +128,7 @@ def test_cmake_bad_generator(config, mock_packages):
     s.concretize()
     pkg = spack.repo.get(s)
     pkg.generator = 'Yellow Sticky Notes'
-    with pytest.raises(spack.package.InstallError):
+    with pytest.raises(spack.package_base.InstallError):
         get_std_cmake_args(pkg)
 
 

lib/spack/spack/test/cmd/analyze.py

@@ -10,7 +10,7 @@
 
 import spack.cmd.install
 import spack.config
-import spack.package
+import spack.package_base
 import spack.util.spack_json as sjson
 from spack.main import SpackCommand
 from spack.spec import Spec

lib/spack/spack/test/cmd/clean.py

@@ -9,7 +9,7 @@
 
 import spack.caches
 import spack.main
-import spack.package
+import spack.package_base
 import spack.stage
 
 clean = spack.main.SpackCommand('clean')
@@ -31,7 +31,7 @@ def __init__(self, name):
         def __call__(self, *args, **kwargs):
             counts[self.name] += 1
 
-    monkeypatch.setattr(spack.package.PackageBase, 'do_clean',
+    monkeypatch.setattr(spack.package_base.PackageBase, 'do_clean',
                         Counter('package'))
     monkeypatch.setattr(spack.stage, 'purge', Counter('stages'))
     monkeypatch.setattr(

lib/spack/spack/test/cmd/install.py

@@ -20,7 +20,7 @@
 import spack.config
 import spack.environment as ev
 import spack.hash_types as ht
-import spack.package
+import spack.package_base
 import spack.util.executable
 from spack.error import SpackError
 from spack.main import SpackCommand
@@ -66,7 +66,7 @@ def test_install_package_and_dependency(
 def test_install_runtests_notests(monkeypatch, mock_packages, install_mockery):
     def check(pkg):
         assert not pkg.run_tests
-    monkeypatch.setattr(spack.package.PackageBase, 'unit_test_check', check)
+    monkeypatch.setattr(spack.package_base.PackageBase, 'unit_test_check', check)
     install('-v', 'dttop')
 
 
@@ -75,7 +75,7 @@ def test_install_runtests_root(monkeypatch, mock_packages, install_mockery):
     def check(pkg):
         assert pkg.run_tests == (pkg.name == 'dttop')
 
-    monkeypatch.setattr(spack.package.PackageBase, 'unit_test_check', check)
+    monkeypatch.setattr(spack.package_base.PackageBase, 'unit_test_check', check)
     install('--test=root', 'dttop')
 
 
@@ -84,7 +84,7 @@ def test_install_runtests_all(monkeypatch, mock_packages, install_mockery):
     def check(pkg):
         assert pkg.run_tests
 
-    monkeypatch.setattr(spack.package.PackageBase, 'unit_test_check', check)
+    monkeypatch.setattr(spack.package_base.PackageBase, 'unit_test_check', check)
     install('--test=all', 'a')
 
 
@@ -257,7 +257,7 @@ def test_install_commit(
 
     """
     repo_path, filename, commits = mock_git_version_info
-    monkeypatch.setattr(spack.package.PackageBase,
+    monkeypatch.setattr(spack.package_base.PackageBase,
                         'git', 'file://%s' % repo_path,
                         raising=False)
 

lib/spack/spack/test/cmd/pkg.py

@@ -22,7 +22,7 @@
 
 #: new fake package template
 pkg_template = '''\
-from spack import *
+from spack.package import *
 
 class {name}(Package):
     homepage = "http://www.example.com"

lib/spack/spack/test/cmd/stage.py

@@ -28,7 +28,7 @@ def test_stage_spec(monkeypatch):
     def fake_stage(pkg, mirror_only=False):
         expected.remove(pkg.name)
 
-    monkeypatch.setattr(spack.package.PackageBase, 'do_stage', fake_stage)
+    monkeypatch.setattr(spack.package_base.PackageBase, 'do_stage', fake_stage)
 
     stage('trivial-install-test-package', 'mpileaks')
 
@@ -42,7 +42,7 @@ def check_stage_path(monkeypatch, tmpdir):
     def fake_stage(pkg, mirror_only=False):
         assert pkg.path == expected_path
 
-    monkeypatch.setattr(spack.package.PackageBase, 'do_stage', fake_stage)
+    monkeypatch.setattr(spack.package_base.PackageBase, 'do_stage', fake_stage)
 
     return expected_path
 
@@ -69,7 +69,7 @@ def fake_stage(pkg, mirror_only=False):
         assert pkg.name == 'trivial-install-test-package'
         assert pkg.path is None
 
-    monkeypatch.setattr(spack.package.PackageBase, 'do_stage', fake_stage)
+    monkeypatch.setattr(spack.package_base.PackageBase, 'do_stage', fake_stage)
 
     e = ev.create('test')
     e.add('mpileaks')
@@ -87,7 +87,7 @@ def fake_stage(pkg, mirror_only=False):
         assert pkg.name == 'mpileaks'
         assert pkg.version == Version('100.100')
 
-    monkeypatch.setattr(spack.package.PackageBase, 'do_stage', fake_stage)
+    monkeypatch.setattr(spack.package_base.PackageBase, 'do_stage', fake_stage)
 
     e = ev.create('test')
     e.add('mpileaks@100.100')
@@ -115,7 +115,7 @@ def test_stage_full_env(mutable_mock_env_path, monkeypatch):
     def fake_stage(pkg, mirror_only=False):
         expected.remove(pkg.name)
 
-    monkeypatch.setattr(spack.package.PackageBase, 'do_stage', fake_stage)
+    monkeypatch.setattr(spack.package_base.PackageBase, 'do_stage', fake_stage)
 
     with e:
         stage()

lib/spack/spack/test/cmd/style.py

@@ -76,7 +76,9 @@ def flake8_package_with_errors(scope="function"):
         package = FileFilter(filename)
         package.filter("state = 'unmodified'", "state    =    'modified'", string=True)
         package.filter(
-            "from spack import *", "from spack import *\nimport os", string=True
+            "from spack.package import *",
+            "from spack.package import *\nimport os",
+            string=True
         )
         yield filename
     finally:
@@ -273,17 +275,17 @@ def test_style(flake8_package, tmpdir):
         relative = os.path.relpath(flake8_package)
 
         # no args
-        output = style()
+        output = style(fail_on_error=False)
         assert relative in output
         assert "spack style checks were clean" in output
 
         # one specific arg
-        output = style(flake8_package)
+        output = style(flake8_package, fail_on_error=False)
         assert relative in output
         assert "spack style checks were clean" in output
 
         # specific file that isn't changed
-        output = style(__file__)
+        output = style(__file__, fail_on_error=False)
         assert relative not in output
         assert __file__ in output
         assert "spack style checks were clean" in output

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

@@ -13,7 +13,7 @@
 
 import spack.cmd.install
 import spack.config
-import spack.package
+import spack.package_base
 import spack.paths
 import spack.store
 from spack.main import SpackCommand
@@ -239,7 +239,7 @@ def test_test_list(
                     reason="Not supported on Windows (yet)")
 def test_has_test_method_fails(capsys):
     with pytest.raises(SystemExit):
-        spack.package.has_test_method('printing-package')
+        spack.package_base.has_test_method('printing-package')
 
     captured = capsys.readouterr()[1]
     assert 'is not a class' in captured

lib/spack/spack/test/conftest.py

@@ -35,7 +35,7 @@
 import spack.database
 import spack.directory_layout
 import spack.environment as ev
-import spack.package
+import spack.package_base
 import spack.package_prefs
 import spack.paths
 import spack.platforms
@@ -532,7 +532,7 @@ def _pkg_install_fn(pkg, spec, prefix):
 
 @pytest.fixture
 def mock_pkg_install(monkeypatch):
-    monkeypatch.setattr(spack.package.PackageBase, 'install',
+    monkeypatch.setattr(spack.package_base.PackageBase, 'install',
                         _pkg_install_fn, raising=False)
 
 
@@ -934,7 +934,7 @@ def mock_fetch(mock_archive, monkeypatch):
     mock_fetcher.append(URLFetchStrategy(mock_archive.url))
 
     monkeypatch.setattr(
-        spack.package.PackageBase, 'fetcher', mock_fetcher)
+        spack.package_base.PackageBase, 'fetcher', mock_fetcher)
 
 
 class MockLayout(object):

lib/spack/spack/test/database.py

@@ -29,7 +29,7 @@
 from llnl.util.tty.colify import colify
 
 import spack.database
-import spack.package
+import spack.package_base
 import spack.repo
 import spack.spec
 import spack.store
@@ -792,7 +792,7 @@ def test_uninstall_by_spec(mutable_database):
     with mutable_database.write_transaction():
         for spec in mutable_database.query():
             if spec.installed:
-                spack.package.PackageBase.uninstall_by_spec(spec, force=True)
+                spack.package_base.PackageBase.uninstall_by_spec(spec, force=True)
             else:
                 mutable_database.remove(spec)
     assert len(mutable_database.query()) == 0

lib/spack/spack/test/flag_handlers.py

@@ -10,7 +10,7 @@
 import spack.build_environment
 import spack.repo
 import spack.spec
-from spack.pkgkit import build_system_flags, env_flags, inject_flags
+from spack.package import build_system_flags, env_flags, inject_flags
 
 
 @pytest.fixture()

lib/spack/spack/test/install.py

@@ -15,7 +15,7 @@
 import spack.repo
 import spack.store
 import spack.util.spack_json as sjson
-from spack.package import (
+from spack.package_base import (
     InstallError,
     PackageBase,
     PackageStillNeededError,
@@ -238,7 +238,7 @@ def test_flatten_deps(
     assert dependency_name not in os.listdir(pkg.prefix)
 
     # Flatten the dependencies and ensure the dependency directory is there.
-    spack.package.flatten_dependencies(spec, pkg.prefix)
+    spack.package_base.flatten_dependencies(spec, pkg.prefix)
 
     dependency_dir = os.path.join(pkg.prefix, dependency_name)
     assert os.path.isdir(dependency_dir)
@@ -322,7 +322,7 @@ def test_partial_install_keep_prefix(install_mockery, mock_fetch, monkeypatch):
     # If remove_prefix is called at any point in this test, that is an
     # error
     pkg.succeed = False  # make the build fail
-    monkeypatch.setattr(spack.package.Package, 'remove_prefix', mock_remove_prefix)
+    monkeypatch.setattr(spack.package_base.Package, 'remove_prefix', mock_remove_prefix)
     with pytest.raises(spack.build_environment.ChildError):
         pkg.do_install(keep_prefix=True)
     assert os.path.exists(pkg.prefix)
@@ -340,7 +340,7 @@ def test_partial_install_keep_prefix(install_mockery, mock_fetch, monkeypatch):
 def test_second_install_no_overwrite_first(install_mockery, mock_fetch, monkeypatch):
     spec = Spec('canfail').concretized()
     pkg = spack.repo.get(spec)
-    monkeypatch.setattr(spack.package.Package, 'remove_prefix', mock_remove_prefix)
+    monkeypatch.setattr(spack.package_base.Package, 'remove_prefix', mock_remove_prefix)
 
     pkg.succeed = True
     pkg.do_install()

lib/spack/spack/test/installer.py

@@ -69,7 +69,8 @@ def create_build_task(pkg, install_args={}):
     Create a built task for the given (concretized) package
 
     Args:
-        pkg (spack.package.PackageBase): concretized package associated with the task
+        pkg (spack.package_base.PackageBase): concretized package associated with
+                                              the task
         install_args (dict): dictionary of kwargs (or install args)
 
     Return:
@@ -716,7 +717,7 @@ def _add(_compilers):
     task.compiler = True
 
     # Preclude any meaningful side-effects
-    monkeypatch.setattr(spack.package.PackageBase, 'unit_test_check', _true)
+    monkeypatch.setattr(spack.package_base.PackageBase, 'unit_test_check', _true)
     monkeypatch.setattr(inst.PackageInstaller, '_setup_install_dir', _noop)
     monkeypatch.setattr(spack.build_environment, 'start_build_process', _noop)
     monkeypatch.setattr(spack.database.Database, 'add', _noop)
@@ -1049,7 +1050,7 @@ def test_install_fail_fast_on_except(install_mockery, monkeypatch, capsys):
     # This will prevent b from installing, which will cause the build of a
     # to be skipped.
     monkeypatch.setattr(
-        spack.package.PackageBase,
+        spack.package_base.PackageBase,
         'do_patch',
         _test_install_fail_fast_on_except_patch
     )

lib/spack/spack/test/package_class.py

@@ -17,7 +17,7 @@
 
 import llnl.util.filesystem as fs
 
-import spack.package
+import spack.package_base
 import spack.repo
 
 
@@ -80,7 +80,7 @@ def test_possible_dependencies_virtual(mock_packages, mpi_names):
     # only one mock MPI has a dependency
     expected['fake'] = set()
 
-    assert expected == spack.package.possible_dependencies(
+    assert expected == spack.package_base.possible_dependencies(
         "mpi", transitive=False)
 
 
@@ -125,7 +125,7 @@ def test_possible_dependencies_with_multiple_classes(
         'dt-diamond-bottom': set(),
     })
 
-    assert expected == spack.package.possible_dependencies(*pkgs)
+    assert expected == spack.package_base.possible_dependencies(*pkgs)
 
 
 def setup_install_test(source_paths, install_test_root):

lib/spack/spack/test/package_sanity.py

@@ -16,7 +16,7 @@
 # do sanity checks only on packagess modified by a PR
 import spack.cmd.style as style
 import spack.fetch_strategy
-import spack.package
+import spack.package_base
 import spack.paths
 import spack.repo
 import spack.util.crypto as crypto
@@ -265,7 +265,7 @@ def test_all_dependencies_exist():
     """Make sure no packages have nonexisting dependencies."""
     missing = {}
     pkgs = [pkg for pkg in spack.repo.path.all_package_names()]
-    spack.package.possible_dependencies(
+    spack.package_base.possible_dependencies(
         *pkgs, transitive=True, missing=missing)
 
     lines = [

lib/spack/spack/test/packages.py

@@ -135,10 +135,10 @@ def test_urls_for_versions(mock_packages, config):
 
 def test_url_for_version_with_no_urls(mock_packages, config):
     pkg = spack.repo.get('git-test')
-    with pytest.raises(spack.package.NoURLError):
+    with pytest.raises(spack.package_base.NoURLError):
         pkg.url_for_version('1.0')
 
-    with pytest.raises(spack.package.NoURLError):
+    with pytest.raises(spack.package_base.NoURLError):
         pkg.url_for_version('1.1')
 
 
@@ -377,7 +377,7 @@ def test_fetch_options(mock_packages, config):
 
 def test_has_test_method_fails(capsys):
     with pytest.raises(SystemExit):
-        spack.package.has_test_method('printing-package')
+        spack.package_base.has_test_method('printing-package')
 
     captured = capsys.readouterr()[1]
     assert 'is not a class' in captured

lib/spack/spack/test/packaging.py

@@ -21,7 +21,7 @@
 
 import spack.binary_distribution as bindist
 import spack.cmd.buildcache as buildcache
-import spack.package
+import spack.package_base
 import spack.repo
 import spack.store
 import spack.util.gpg
@@ -575,10 +575,10 @@ def fetch(self):
     def fake_fn(self):
         return fetcher
 
-    orig_fn = spack.package.PackageBase.fetcher
+    orig_fn = spack.package_base.PackageBase.fetcher
-    spack.package.PackageBase.fetcher = fake_fn
+    spack.package_base.PackageBase.fetcher = fake_fn
     yield
-    spack.package.PackageBase.fetcher = orig_fn
+    spack.package_base.PackageBase.fetcher = orig_fn
 
 
 @pytest.mark.parametrize("manual,instr", [(False, False), (False, True),
@@ -598,7 +598,7 @@ def _instr(pkg):
 
     pkg.manual_download = manual
     if instr:
-        monkeypatch.setattr(spack.package.PackageBase, 'download_instr',
+        monkeypatch.setattr(spack.package_base.PackageBase, 'download_instr',
                             _instr)
 
     expected = pkg.download_instr if manual else 'All fetchers failed'
@@ -616,7 +616,7 @@ def fetch(self):
             raise Exception("Sources are fetched but shouldn't have been")
     fetcher = FetchStrategyComposite()
     fetcher.append(FetchingNotAllowed())
-    monkeypatch.setattr(spack.package.PackageBase, 'fetcher', fetcher)
+    monkeypatch.setattr(spack.package_base.PackageBase, 'fetcher', fetcher)
 
 
 def test_fetch_without_code_is_noop(install_mockery, fetching_not_allowed):

lib/spack/spack/test/repo.py

@@ -7,7 +7,7 @@
 
 import pytest
 
-import spack.package
+import spack.package_base
 import spack.paths
 import spack.repo
 
@@ -116,11 +116,12 @@ def test_absolute_import_spack_packages_as_python_modules(mock_packages):
     assert hasattr(spack.pkg.builtin.mock, 'mpileaks')
     assert hasattr(spack.pkg.builtin.mock.mpileaks, 'Mpileaks')
     assert isinstance(spack.pkg.builtin.mock.mpileaks.Mpileaks,
-                      spack.package.PackageMeta)
+                      spack.package_base.PackageMeta)
-    assert issubclass(spack.pkg.builtin.mock.mpileaks.Mpileaks, spack.package.Package)
+    assert issubclass(spack.pkg.builtin.mock.mpileaks.Mpileaks,
+                      spack.package_base.Package)
 
 
 def test_relative_import_spack_packages_as_python_modules(mock_packages):
     from spack.pkg.builtin.mock.mpileaks import Mpileaks
-    assert isinstance(Mpileaks, spack.package.PackageMeta)
+    assert isinstance(Mpileaks, spack.package_base.PackageMeta)
-    assert issubclass(Mpileaks, spack.package.Package)
+    assert issubclass(Mpileaks, spack.package_base.Package)

lib/spack/spack/test/spec_dag.py

@@ -8,7 +8,7 @@
 import pytest
 
 import spack.error
-import spack.package
+import spack.package_base
 import spack.util.hash as hashutil
 from spack.dependency import Dependency, all_deptypes, canonical_deptype
 from spack.spec import Spec

lib/spack/spack/test/test_activations.py

@@ -14,7 +14,7 @@
 
 from llnl.util.link_tree import MergeConflictError
 
-import spack.package
+import spack.package_base
 import spack.spec
 from spack.directory_layout import DirectoryLayout
 from spack.filesystem_view import YamlFilesystemView

lib/spack/spack/test/versions.py

@@ -14,7 +14,7 @@
 
 from llnl.util.filesystem import working_dir
 
-import spack.package
+import spack.package_base
 import spack.spec
 from spack.util.executable import which
 from spack.version import Version, VersionList, VersionRange, ver
@@ -590,7 +590,7 @@ def test_invalid_versions(version_str):
                     reason="Not supported on Windows (yet)")
 def test_versions_from_git(mock_git_version_info, monkeypatch, mock_packages):
     repo_path, filename, commits = mock_git_version_info
-    monkeypatch.setattr(spack.package.PackageBase, 'git', 'file://%s' % repo_path,
+    monkeypatch.setattr(spack.package_base.PackageBase, 'git', 'file://%s' % repo_path,
                         raising=False)
 
     for commit in commits:
@@ -614,7 +614,7 @@ def test_git_hash_comparisons(
     """Check that hashes compare properly to versions
     """
     repo_path, filename, commits = mock_git_version_info
-    monkeypatch.setattr(spack.package.PackageBase,
+    monkeypatch.setattr(spack.package_base.PackageBase,
                         'git', 'file://%s' % repo_path,
                         raising=False)
 

lib/spack/spack/util/mock_package.py

@@ -15,7 +15,7 @@
 
 
 class MockPackageBase(object):
-    """Internal base class for mocking ``spack.package.PackageBase``.
+    """Internal base class for mocking ``spack.package_base.PackageBase``.
 
     Use ``MockPackageMultiRepo.add_package()`` to create new instances.
 

lib/spack/spack/util/package_hash.py

@@ -7,7 +7,7 @@
 
 import spack.directives
 import spack.error
-import spack.package
+import spack.package_base
 import spack.repo
 import spack.spec
 import spack.util.hash
@@ -63,7 +63,7 @@ def __init__(self, spec):
         # list of URL attributes and metadata attributes
         # these will be removed from packages.
         self.metadata_attrs = [s.url_attr for s in spack.fetch_strategy.all_strategies]
-        self.metadata_attrs += spack.package.Package.metadata_attrs
+        self.metadata_attrs += spack.package_base.Package.metadata_attrs
 
         self.spec = spec
         self.in_classdef = False  # used to avoid nested classdefs

lib/spack/spack/util/web.py

@@ -594,7 +594,7 @@ def find_versions_of_archive(
         list_depth (int): max depth to follow links on list_url pages.
             Defaults to 0.
         concurrency (int): maximum number of concurrent requests
-        reference_package (spack.package.Package or None): a spack package
+        reference_package (spack.package_base.Package or None): a spack package
             used as a reference for url detection.  Uses the url_for_version
             method on the package to produce reference urls which, if found,
             are preferred.

lib/spack/spack/variant.py

@@ -95,7 +95,7 @@ def validate_or_raise(self, vspec, pkg=None):
 
         Args:
             vspec (Variant): instance to be validated
-            pkg (spack.package.Package): the package that required the validation,
+            pkg (spack.package_base.Package): the package that required the validation,
                 if available
 
         Raises:

lib/spack/spack/version.py

@@ -177,6 +177,7 @@ class Version(object):
     ]
 
     def __init__(self, string):
+        # type: (str) -> None
         if not isinstance(string, str):
             string = str(string)
 

pyproject.toml

@@ -16,8 +16,9 @@ honor_noqa = true
 
 [tool.mypy]
 python_version = 3.7
-files = ['lib/spack/llnl/**/*.py', 'lib/spack/spack/**/*.py']
+files = ['lib/spack/llnl/**/*.py', 'lib/spack/spack/**/*.py', './var/spack/repos/builtin/packages/*/package.py']
 mypy_path = ['bin', 'lib/spack', 'lib/spack/external', 'var/spack/repos/builtin']
+allow_redefinition = true
 
 # This and a generated import file allows supporting packages
 namespace_packages = true
@@ -36,6 +37,8 @@ ignore_missing_imports = true
   module = 'packages.*'
   ignore_errors = false
   ignore_missing_imports = false
+  # we can't do this here, not a module scope option, in spack style instead
+  # disable_error_code = 'no-redef'
 
   [[tool.mypy.overrides]]
   module = 'llnl.*'
@@ -58,6 +61,15 @@ ignore_missing_imports = true
   module = 'jinja2'
   follow_imports = 'skip'
 
+[tool.pyright]
+useLibraryCodeForTypes = true
+reportMissingImports = true
+reportWildcardImportFromLibrary = false
+include = ['lib/spack']
+ignore = ['lib/spack/external']
+extraPaths = ['lib/spack', 'lib/spack/external']
+
+
 [tool.coverage.run]
 parallel = true
 concurrency = ["multiprocessing"]

share/spack/qa/flake8_formatter.py

@@ -16,10 +16,10 @@
 pattern_exemptions = {
     # exemptions applied only to package.py files.
     r"package.py$": {
-        # Allow 'from spack import *' in packages, but no other wildcards
+        # Allow 'from spack.package import *' in packages, but no other wildcards
         "F403": [
-            r"^from spack import \*$",
+            r"^from spack.package import \*$",
-            r"^from spack.pkgkit import \*$",
+            r"^from spack.package_defs import \*$",
         ],
         # Exempt lines with urls and descriptions from overlong line errors.
         "E501": [

var/spack/repos/builtin.mock/packages/a/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class A(AutotoolsPackage):

var/spack/repos/builtin.mock/packages/archive-files/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class ArchiveFiles(AutotoolsPackage):

var/spack/repos/builtin.mock/packages/autotools-conditional-variants-test/package.py

@@ -4,6 +4,9 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 
+from spack.package import *
+
+
 class AutotoolsConditionalVariantsTest(AutotoolsPackage):
     homepage = "https://www.example.com"
     has_code = False

var/spack/repos/builtin.mock/packages/autotools-config-replacement/package.py

@@ -5,7 +5,7 @@
 
 import os
 
-from spack import *
+from spack.package import *
 
 
 class AutotoolsConfigReplacement(AutotoolsPackage):

var/spack/repos/builtin.mock/packages/b/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class B(Package):

var/spack/repos/builtin.mock/packages/boost/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class Boost(Package):

var/spack/repos/builtin.mock/packages/both-link-and-build-dep-a/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class BothLinkAndBuildDepA(Package):

var/spack/repos/builtin.mock/packages/both-link-and-build-dep-b/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class BothLinkAndBuildDepB(Package):

var/spack/repos/builtin.mock/packages/both-link-and-build-dep-c/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class BothLinkAndBuildDepC(Package):

var/spack/repos/builtin.mock/packages/bowtie/package.py

@@ -2,6 +2,9 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
+from spack.package import *
+
+
 class Bowtie(Package):
     """Mock package to test conflicts on compiler ranges"""
 

var/spack/repos/builtin.mock/packages/brillig/package.py

@@ -4,7 +4,7 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 
-from spack import *
+from spack.package import *
 
 
 class Brillig(Package):

var/spack/repos/builtin.mock/packages/build-error/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class BuildError(Package):

var/spack/repos/builtin.mock/packages/build-warnings/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class BuildWarnings(Package):

var/spack/repos/builtin.mock/packages/c/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class C(Package):

var/spack/repos/builtin.mock/packages/callpath/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class Callpath(Package):

var/spack/repos/builtin.mock/packages/canfail/package.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
-from spack import *
+from spack.package import *
 
 
 class Canfail(Package):

var/spack/repos/builtin.mock/packages/cmake-client/package.py

@@ -5,7 +5,7 @@
 
 import os
 
-from spack import *
+from spack.package import *
 
 
 def check(condition, msg):

var/spack/repos/builtin.mock/packages/cmake-conditional-variants-test/package.py

@@ -3,6 +3,9 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+from spack.package import *
+
+
 class CmakeConditionalVariantsTest(CMakePackage):
     homepage = "https://dev.null"
     version('1.0')