Add missing MultiMethodMeta metaclass in builders (#45879)

* Add missing MultiMethodMeta metaclass in builders

and remove the Python 2 fallback option in favor of hard errors to catch
similar issues going forward.

The fallback option can cause about 10K stat calls due to use of
`realpath` in the inspect module, depending on how deep Spack itself is
nested in the file system, which is ... undesirable.

* code shuffling to avoid circular import

* more reshuffling

* move reserved variant names into variants module

11 files changed, 288 insertions, 289 deletions

diff --git a/lib/spack/llnl/util/lang.py b/lib/spack/llnl/util/lang.py
index c3e67f205d..8449c4ca8c 100644
--- a/lib/spack/llnl/util/lang.py
+++ b/lib/spack/llnl/util/lang.py
@@ -84,20 +84,6 @@ def index_by(objects, *funcs):
     return result
 
 
-def caller_locals():
-    """This will return the locals of the *parent* of the caller.
-    This allows a function to insert variables into its caller's
-    scope.  Yes, this is some black magic, and yes it's useful
-    for implementing things like depends_on and provides.
-    """
-    # Passing zero here skips line context for speed.
-    stack = inspect.stack(0)
-    try:
-        return stack[2][0].f_locals
-    finally:
-        del stack
-
-
 def attr_setdefault(obj, name, value):
     """Like dict.setdefault, but for objects."""
     if not hasattr(obj, name):
diff --git a/lib/spack/spack/builder.py b/lib/spack/spack/builder.py
index 00734d5533..2f947335a8 100644
--- a/lib/spack/spack/builder.py
+++ b/lib/spack/spack/builder.py
@@ -12,6 +12,7 @@ from typing import List, Optional, Tuple
 from llnl.util import lang
 
 import spack.build_environment
+import spack.multimethod
 
 #: Builder classes, as registered by the "builder" decorator
 BUILDER_CLS = {}
@@ -295,7 +296,11 @@ class PhaseCallbacksMeta(type):
         return _decorator
 
 
-class BuilderMeta(PhaseCallbacksMeta, type(collections.abc.Sequence)):  # type: ignore
+class BuilderMeta(
+    PhaseCallbacksMeta,
+    spack.multimethod.MultiMethodMeta,
+    type(collections.abc.Sequence),  # type: ignore
+):
     pass
 
 
diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index d3d0d7ec48..55b040e68a 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -32,10 +32,9 @@ The available directives are:
 """
 import collections
 import collections.abc
-import functools
 import os.path
 import re
-from typing import TYPE_CHECKING, Any, Callable, List, Optional, Set, Tuple, Union
+from typing import TYPE_CHECKING, Any, Callable, List, Optional, Tuple, Union
 
 import llnl.util.lang
 import llnl.util.tty.color
@@ -48,6 +47,7 @@ import spack.url
 import spack.util.crypto
 import spack.variant
 from spack.dependency import Dependency
+from spack.directives_meta import DirectiveError, DirectiveMeta
 from spack.fetch_strategy import from_kwargs
 from spack.resource import Resource
 from spack.version import (
@@ -80,22 +80,6 @@ __all__ = [
     "redistribute",
 ]
 
-#: These are variant names used by Spack internally; packages can't use them
-reserved_names = [
-    "arch",
-    "architecture",
-    "dev_path",
-    "namespace",
-    "operating_system",
-    "os",
-    "patches",
-    "platform",
-    "target",
-]
-
-#: Names of possible directives. This list is mostly populated using the @directive decorator.
-#: Some directives leverage others and in that case are not automatically added.
-directive_names = ["build_system"]
 
 _patch_order_index = 0
 
@@ -155,219 +139,6 @@ def _make_when_spec(value: WhenType) -> Optional["spack.spec.Spec"]:
     return spack.spec.Spec(value)
 
 
-class DirectiveMeta(type):
-    """Flushes the directives that were temporarily stored in the staging
-    area into the package.
-    """
-
-    # Set of all known directives
-    _directive_dict_names: Set[str] = set()
-    _directives_to_be_executed: List[str] = []
-    _when_constraints_from_context: List[str] = []
-    _default_args: List[dict] = []
-
-    def __new__(cls, name, bases, attr_dict):
-        # Initialize the attribute containing the list of directives
-        # to be executed. Here we go reversed because we want to execute
-        # commands:
-        # 1. in the order they were defined
-        # 2. following the MRO
-        attr_dict["_directives_to_be_executed"] = []
-        for base in reversed(bases):
-            try:
-                directive_from_base = base._directives_to_be_executed
-                attr_dict["_directives_to_be_executed"].extend(directive_from_base)
-            except AttributeError:
-                # The base class didn't have the required attribute.
-                # Continue searching
-                pass
-
-        # De-duplicates directives from base classes
-        attr_dict["_directives_to_be_executed"] = [
-            x for x in llnl.util.lang.dedupe(attr_dict["_directives_to_be_executed"])
-        ]
-
-        # Move things to be executed from module scope (where they
-        # are collected first) to class scope
-        if DirectiveMeta._directives_to_be_executed:
-            attr_dict["_directives_to_be_executed"].extend(
-                DirectiveMeta._directives_to_be_executed
-            )
-            DirectiveMeta._directives_to_be_executed = []
-
-        return super(DirectiveMeta, cls).__new__(cls, name, bases, attr_dict)
-
-    def __init__(cls, name, bases, attr_dict):
-        # The instance is being initialized: if it is a package we must ensure
-        # that the directives are called to set it up.
-
-        if "spack.pkg" in cls.__module__:
-            # Ensure the presence of the dictionaries associated with the directives.
-            # All dictionaries are defaultdicts that create lists for missing keys.
-            for d in DirectiveMeta._directive_dict_names:
-                setattr(cls, d, {})
-
-            # Lazily execute directives
-            for directive in cls._directives_to_be_executed:
-                directive(cls)
-
-            # Ignore any directives executed *within* top-level
-            # directives by clearing out the queue they're appended to
-            DirectiveMeta._directives_to_be_executed = []
-
-        super(DirectiveMeta, cls).__init__(name, bases, attr_dict)
-
-    @staticmethod
-    def push_to_context(when_spec):
-        """Add a spec to the context constraints."""
-        DirectiveMeta._when_constraints_from_context.append(when_spec)
-
-    @staticmethod
-    def pop_from_context():
-        """Pop the last constraint from the context"""
-        return DirectiveMeta._when_constraints_from_context.pop()
-
-    @staticmethod
-    def push_default_args(default_args):
-        """Push default arguments"""
-        DirectiveMeta._default_args.append(default_args)
-
-    @staticmethod
-    def pop_default_args():
-        """Pop default arguments"""
-        return DirectiveMeta._default_args.pop()
-
-    @staticmethod
-    def directive(dicts=None):
-        """Decorator for Spack directives.
-
-        Spack directives allow you to modify a package while it is being
-        defined, e.g. to add version or dependency information.  Directives
-        are one of the key pieces of Spack's package "language", which is
-        embedded in python.
-
-        Here's an example directive:
-
-        .. code-block:: python
-
-            @directive(dicts='versions')
-            version(pkg, ...):
-                ...
-
-        This directive allows you write:
-
-        .. code-block:: python
-
-            class Foo(Package):
-                version(...)
-
-        The ``@directive`` decorator handles a couple things for you:
-
-          1. Adds the class scope (pkg) as an initial parameter when
-             called, like a class method would.  This allows you to modify
-             a package from within a directive, while the package is still
-             being defined.
-
-          2. It automatically adds a dictionary called "versions" to the
-             package so that you can refer to pkg.versions.
-
-        The ``(dicts='versions')`` part ensures that ALL packages in Spack
-        will have a ``versions`` attribute after they're constructed, and
-        that if no directive actually modified it, it will just be an
-        empty dict.
-
-        This is just a modular way to add storage attributes to the
-        Package class, and it's how Spack gets information from the
-        packages to the core.
-        """
-        global directive_names
-
-        if isinstance(dicts, str):
-            dicts = (dicts,)
-
-        if not isinstance(dicts, collections.abc.Sequence):
-            message = "dicts arg must be list, tuple, or string. Found {0}"
-            raise TypeError(message.format(type(dicts)))
-
-        # Add the dictionary names if not already there
-        DirectiveMeta._directive_dict_names |= set(dicts)
-
-        # This decorator just returns the directive functions
-        def _decorator(decorated_function):
-            directive_names.append(decorated_function.__name__)
-
-            @functools.wraps(decorated_function)
-            def _wrapper(*args, **_kwargs):
-                # First merge default args with kwargs
-                kwargs = dict()
-                for default_args in DirectiveMeta._default_args:
-                    kwargs.update(default_args)
-                kwargs.update(_kwargs)
-
-                # Inject when arguments from the context
-                if DirectiveMeta._when_constraints_from_context:
-                    # Check that directives not yet supporting the when= argument
-                    # are not used inside the context manager
-                    if decorated_function.__name__ == "version":
-                        msg = (
-                            'directive "{0}" cannot be used within a "when"'
-                            ' context since it does not support a "when=" '
-                            "argument"
-                        )
-                        msg = msg.format(decorated_function.__name__)
-                        raise DirectiveError(msg)
-
-                    when_constraints = [
-                        spack.spec.Spec(x) for x in DirectiveMeta._when_constraints_from_context
-                    ]
-                    if kwargs.get("when"):
-                        when_constraints.append(spack.spec.Spec(kwargs["when"]))
-                    when_spec = spack.spec.merge_abstract_anonymous_specs(*when_constraints)
-
-                    kwargs["when"] = when_spec
-
-                # If any of the arguments are executors returned by a
-                # directive passed as an argument, don't execute them
-                # lazily. Instead, let the called directive handle them.
-                # This allows nested directive calls in packages.  The
-                # caller can return the directive if it should be queued.
-                def remove_directives(arg):
-                    directives = DirectiveMeta._directives_to_be_executed
-                    if isinstance(arg, (list, tuple)):
-                        # Descend into args that are lists or tuples
-                        for a in arg:
-                            remove_directives(a)
-                    else:
-                        # Remove directives args from the exec queue
-                        remove = next((d for d in directives if d is arg), None)
-                        if remove is not None:
-                            directives.remove(remove)
-
-                # Nasty, but it's the best way I can think of to avoid
-                # side effects if directive results are passed as args
-                remove_directives(args)
-                remove_directives(list(kwargs.values()))
-
-                # A directive returns either something that is callable on a
-                # package or a sequence of them
-                result = decorated_function(*args, **kwargs)
-
-                # ...so if it is not a sequence make it so
-                values = result
-                if not isinstance(values, collections.abc.Sequence):
-                    values = (values,)
-
-                DirectiveMeta._directives_to_be_executed.extend(values)
-
-                # wrapped function returns same result as original so
-                # that we can nest directives
-                return result
-
-            return _wrapper
-
-        return _decorator
-
-
 SubmoduleCallback = Callable[["spack.package_base.PackageBase"], Union[str, List[str], bool]]
 directive = DirectiveMeta.directive
 
@@ -846,7 +617,7 @@ def variant(
         msg += " @*r{{[{0}, variant '{1}']}}"
         return llnl.util.tty.color.colorize(msg.format(pkg.name, name))
 
-    if name in reserved_names:
+    if name in spack.variant.reserved_names:
 
         def _raise_reserved_name(pkg):
             msg = "The name '%s' is reserved by Spack" % name
@@ -1110,10 +881,6 @@ def _language(lang_spec_str: str, *, when: Optional[Union[str, bool]] = None):
     return _execute_languages
 
 
-class DirectiveError(spack.error.SpackError):
-    """This is raised when something is wrong with a package directive."""
-
-
 class DependencyError(DirectiveError):
     """This is raised when a dependency specification is invalid."""
 
diff --git a/lib/spack/spack/directives_meta.py b/lib/spack/spack/directives_meta.py
new file mode 100644
index 0000000000..0768a40664
--- /dev/null
+++ b/lib/spack/spack/directives_meta.py
@@ -0,0 +1,234 @@
+# Copyright 2013-2024 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)
+
+import collections.abc
+import functools
+from typing import List, Set
+
+import llnl.util.lang
+
+import spack.error
+import spack.spec
+
+#: Names of possible directives. This list is mostly populated using the @directive decorator.
+#: Some directives leverage others and in that case are not automatically added.
+directive_names = ["build_system"]
+
+
+class DirectiveMeta(type):
+    """Flushes the directives that were temporarily stored in the staging
+    area into the package.
+    """
+
+    # Set of all known directives
+    _directive_dict_names: Set[str] = set()
+    _directives_to_be_executed: List[str] = []
+    _when_constraints_from_context: List[str] = []
+    _default_args: List[dict] = []
+
+    def __new__(cls, name, bases, attr_dict):
+        # Initialize the attribute containing the list of directives
+        # to be executed. Here we go reversed because we want to execute
+        # commands:
+        # 1. in the order they were defined
+        # 2. following the MRO
+        attr_dict["_directives_to_be_executed"] = []
+        for base in reversed(bases):
+            try:
+                directive_from_base = base._directives_to_be_executed
+                attr_dict["_directives_to_be_executed"].extend(directive_from_base)
+            except AttributeError:
+                # The base class didn't have the required attribute.
+                # Continue searching
+                pass
+
+        # De-duplicates directives from base classes
+        attr_dict["_directives_to_be_executed"] = [
+            x for x in llnl.util.lang.dedupe(attr_dict["_directives_to_be_executed"])
+        ]
+
+        # Move things to be executed from module scope (where they
+        # are collected first) to class scope
+        if DirectiveMeta._directives_to_be_executed:
+            attr_dict["_directives_to_be_executed"].extend(
+                DirectiveMeta._directives_to_be_executed
+            )
+            DirectiveMeta._directives_to_be_executed = []
+
+        return super(DirectiveMeta, cls).__new__(cls, name, bases, attr_dict)
+
+    def __init__(cls, name, bases, attr_dict):
+        # The instance is being initialized: if it is a package we must ensure
+        # that the directives are called to set it up.
+
+        if "spack.pkg" in cls.__module__:
+            # Ensure the presence of the dictionaries associated with the directives.
+            # All dictionaries are defaultdicts that create lists for missing keys.
+            for d in DirectiveMeta._directive_dict_names:
+                setattr(cls, d, {})
+
+            # Lazily execute directives
+            for directive in cls._directives_to_be_executed:
+                directive(cls)
+
+            # Ignore any directives executed *within* top-level
+            # directives by clearing out the queue they're appended to
+            DirectiveMeta._directives_to_be_executed = []
+
+        super(DirectiveMeta, cls).__init__(name, bases, attr_dict)
+
+    @staticmethod
+    def push_to_context(when_spec):
+        """Add a spec to the context constraints."""
+        DirectiveMeta._when_constraints_from_context.append(when_spec)
+
+    @staticmethod
+    def pop_from_context():
+        """Pop the last constraint from the context"""
+        return DirectiveMeta._when_constraints_from_context.pop()
+
+    @staticmethod
+    def push_default_args(default_args):
+        """Push default arguments"""
+        DirectiveMeta._default_args.append(default_args)
+
+    @staticmethod
+    def pop_default_args():
+        """Pop default arguments"""
+        return DirectiveMeta._default_args.pop()
+
+    @staticmethod
+    def directive(dicts=None):
+        """Decorator for Spack directives.
+
+        Spack directives allow you to modify a package while it is being
+        defined, e.g. to add version or dependency information.  Directives
+        are one of the key pieces of Spack's package "language", which is
+        embedded in python.
+
+        Here's an example directive:
+
+        .. code-block:: python
+
+            @directive(dicts='versions')
+            version(pkg, ...):
+                ...
+
+        This directive allows you write:
+
+        .. code-block:: python
+
+            class Foo(Package):
+                version(...)
+
+        The ``@directive`` decorator handles a couple things for you:
+
+          1. Adds the class scope (pkg) as an initial parameter when
+             called, like a class method would.  This allows you to modify
+             a package from within a directive, while the package is still
+             being defined.
+
+          2. It automatically adds a dictionary called "versions" to the
+             package so that you can refer to pkg.versions.
+
+        The ``(dicts='versions')`` part ensures that ALL packages in Spack
+        will have a ``versions`` attribute after they're constructed, and
+        that if no directive actually modified it, it will just be an
+        empty dict.
+
+        This is just a modular way to add storage attributes to the
+        Package class, and it's how Spack gets information from the
+        packages to the core.
+        """
+        global directive_names
+
+        if isinstance(dicts, str):
+            dicts = (dicts,)
+
+        if not isinstance(dicts, collections.abc.Sequence):
+            message = "dicts arg must be list, tuple, or string. Found {0}"
+            raise TypeError(message.format(type(dicts)))
+
+        # Add the dictionary names if not already there
+        DirectiveMeta._directive_dict_names |= set(dicts)
+
+        # This decorator just returns the directive functions
+        def _decorator(decorated_function):
+            directive_names.append(decorated_function.__name__)
+
+            @functools.wraps(decorated_function)
+            def _wrapper(*args, **_kwargs):
+                # First merge default args with kwargs
+                kwargs = dict()
+                for default_args in DirectiveMeta._default_args:
+                    kwargs.update(default_args)
+                kwargs.update(_kwargs)
+
+                # Inject when arguments from the context
+                if DirectiveMeta._when_constraints_from_context:
+                    # Check that directives not yet supporting the when= argument
+                    # are not used inside the context manager
+                    if decorated_function.__name__ == "version":
+                        msg = (
+                            'directive "{0}" cannot be used within a "when"'
+                            ' context since it does not support a "when=" '
+                            "argument"
+                        )
+                        msg = msg.format(decorated_function.__name__)
+                        raise DirectiveError(msg)
+
+                    when_constraints = [
+                        spack.spec.Spec(x) for x in DirectiveMeta._when_constraints_from_context
+                    ]
+                    if kwargs.get("when"):
+                        when_constraints.append(spack.spec.Spec(kwargs["when"]))
+                    when_spec = spack.spec.merge_abstract_anonymous_specs(*when_constraints)
+
+                    kwargs["when"] = when_spec
+
+                # If any of the arguments are executors returned by a
+                # directive passed as an argument, don't execute them
+                # lazily. Instead, let the called directive handle them.
+                # This allows nested directive calls in packages.  The
+                # caller can return the directive if it should be queued.
+                def remove_directives(arg):
+                    directives = DirectiveMeta._directives_to_be_executed
+                    if isinstance(arg, (list, tuple)):
+                        # Descend into args that are lists or tuples
+                        for a in arg:
+                            remove_directives(a)
+                    else:
+                        # Remove directives args from the exec queue
+                        remove = next((d for d in directives if d is arg), None)
+                        if remove is not None:
+                            directives.remove(remove)
+
+                # Nasty, but it's the best way I can think of to avoid
+                # side effects if directive results are passed as args
+                remove_directives(args)
+                remove_directives(list(kwargs.values()))
+
+                # A directive returns either something that is callable on a
+                # package or a sequence of them
+                result = decorated_function(*args, **kwargs)
+
+                # ...so if it is not a sequence make it so
+                values = result
+                if not isinstance(values, collections.abc.Sequence):
+                    values = (values,)
+
+                DirectiveMeta._directives_to_be_executed.extend(values)
+
+                # wrapped function returns same result as original so
+                # that we can nest directives
+                return result
+
+            return _wrapper
+
+        return _decorator
+
+
+class DirectiveError(spack.error.SpackError):
+    """This is raised when something is wrong with a package directive."""
diff --git a/lib/spack/spack/multimethod.py b/lib/spack/spack/multimethod.py
index 4c21da3c91..b245c6fc1b 100644
--- a/lib/spack/spack/multimethod.py
+++ b/lib/spack/spack/multimethod.py
@@ -28,11 +28,9 @@ import functools
 import inspect
 from contextlib import contextmanager
 
-from llnl.util.lang import caller_locals
-
-import spack.directives
+import spack.directives_meta
 import spack.error
-from spack.spec import Spec
+import spack.spec
 
 
 class MultiMethodMeta(type):
@@ -165,9 +163,9 @@ class when:
             condition (str): condition to be met
         """
         if isinstance(condition, bool):
-            self.spec = Spec() if condition else None
+            self.spec = spack.spec.Spec() if condition else None
         else:
-            self.spec = Spec(condition)
+            self.spec = spack.spec.Spec(condition)
 
     def __call__(self, method):
         """This annotation lets packages declare multiple versions of
@@ -229,11 +227,9 @@ class when:
            platform-specific versions.  There's not much we can do to get
            around this because of the way decorators work.
         """
-        # In Python 2, Get the first definition of the method in the
-        # calling scope by looking at the caller's locals. In Python 3,
-        # we handle this using MultiMethodMeta.__prepare__.
-        if MultiMethodMeta._locals is None:
-            MultiMethodMeta._locals = caller_locals()
+        assert (
+            MultiMethodMeta._locals is not None
+        ), "cannot use multimethod, missing MultiMethodMeta metaclass?"
 
         # Create a multimethod with this name if there is not one already
         original_method = MultiMethodMeta._locals.get(method.__name__)
@@ -266,17 +262,17 @@ class when:
         and add their constraint to whatever may be already present in the directive
         `when=` argument.
         """
-        spack.directives.DirectiveMeta.push_to_context(str(self.spec))
+        spack.directives_meta.DirectiveMeta.push_to_context(str(self.spec))
 
     def __exit__(self, exc_type, exc_val, exc_tb):
-        spack.directives.DirectiveMeta.pop_from_context()
+        spack.directives_meta.DirectiveMeta.pop_from_context()
 
 
 @contextmanager
 def default_args(**kwargs):
-    spack.directives.DirectiveMeta.push_default_args(kwargs)
+    spack.directives_meta.DirectiveMeta.push_default_args(kwargs)
     yield
-    spack.directives.DirectiveMeta.pop_default_args()
+    spack.directives_meta.DirectiveMeta.pop_default_args()
 
 
 class MultiMethodError(spack.error.SpackError):
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index b3f1248ce7..8a1933b749 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -32,7 +32,6 @@ import spack.compilers
 import spack.config
 import spack.config as sc
 import spack.deptypes as dt
-import spack.directives
 import spack.environment as ev
 import spack.error
 import spack.package_base
@@ -1878,8 +1877,7 @@ class SpackSolverSetup:
 
                 # validate variant value only if spec not concrete
                 if not spec.concrete:
-                    reserved_names = spack.directives.reserved_names
-                    if not spec.virtual and vname not in reserved_names:
+                    if not spec.virtual and vname not in spack.variant.reserved_names:
                         pkg_cls = self.pkg_class(spec.name)
                         try:
                             variant_def, _ = pkg_cls.variants[vname]
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index eddbbf934c..19242bc06a 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1639,7 +1639,7 @@ class Spec:
         Known flags currently include "arch"
         """
 
-        if propagate and name in spack.directives.reserved_names:
+        if propagate and name in vt.reserved_names:
             raise UnsupportedPropagationError(
                 f"Propagation with '==' is not supported for '{name}'."
             )
@@ -2935,9 +2935,7 @@ class Spec:
         pkg_variants = pkg_cls.variants
         # reserved names are variants that may be set on any package
         # but are not necessarily recorded by the package's class
-        not_existing = set(spec.variants) - (
-            set(pkg_variants) | set(spack.directives.reserved_names)
-        )
+        not_existing = set(spec.variants) - (set(pkg_variants) | set(vt.reserved_names))
         if not_existing:
             raise vt.UnknownVariantError(spec, not_existing)
 
diff --git a/lib/spack/spack/test/util/package_hash.py b/lib/spack/spack/test/util/package_hash.py
index b613c6630d..5a0251b2d0 100644
--- a/lib/spack/spack/test/util/package_hash.py
+++ b/lib/spack/spack/test/util/package_hash.py
@@ -9,6 +9,7 @@ import os
 import pytest
 
 import spack.directives
+import spack.directives_meta
 import spack.paths
 import spack.repo
 import spack.util.package_hash as ph
@@ -211,13 +212,13 @@ class HasManyDirectives:
 
 {directives}
 """.format(
-    directives="\n".join("    %s()" % name for name in spack.directives.directive_names)
+    directives="\n".join("    %s()" % name for name in spack.directives_meta.directive_names)
 )
 
 
 def test_remove_all_directives():
     """Ensure all directives are removed from packages before hashing."""
-    for name in spack.directives.directive_names:
+    for name in spack.directives_meta.directive_names:
         assert name in many_directives
 
     tree = ast.parse(many_directives)
@@ -225,7 +226,7 @@ def test_remove_all_directives():
     tree = ph.RemoveDirectives(spec).visit(tree)
     unparsed = unparse(tree, py_ver_consistent=True)
 
-    for name in spack.directives.directive_names:
+    for name in spack.directives_meta.directive_names:
         assert name not in unparsed
 
 
diff --git a/lib/spack/spack/util/package_hash.py b/lib/spack/spack/util/package_hash.py
index a0552b13cc..7a4fd39115 100644
--- a/lib/spack/spack/util/package_hash.py
+++ b/lib/spack/spack/util/package_hash.py
@@ -5,7 +5,7 @@
 
 import ast
 
-import spack.directives
+import spack.directives_meta
 import spack.error
 import spack.package_base
 import spack.repo
@@ -82,7 +82,7 @@ class RemoveDirectives(ast.NodeTransformer):
                 node.value
                 and isinstance(node.value, ast.Call)
                 and isinstance(node.value.func, ast.Name)
-                and node.value.func.id in spack.directives.directive_names
+                and node.value.func.id in spack.directives_meta.directive_names
             )
             else node
         )
diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py
index bb4138116f..71a31aa914 100644
--- a/lib/spack/spack/variant.py
+++ b/lib/spack/spack/variant.py
@@ -17,10 +17,22 @@ import llnl.util.lang as lang
 import llnl.util.tty.color
 from llnl.string import comma_or
 
-import spack.directives
 import spack.error as error
 import spack.parser
 
+#: These are variant names used by Spack internally; packages can't use them
+reserved_names = [
+    "arch",
+    "architecture",
+    "dev_path",
+    "namespace",
+    "operating_system",
+    "os",
+    "patches",
+    "platform",
+    "target",
+]
+
 special_variant_values = [None, "none", "*"]
 
 
@@ -679,7 +691,7 @@ def substitute_abstract_variants(spec):
     # in $spack/lib/spack/spack/spec_list.py
     failed = []
     for name, v in spec.variants.items():
-        if name in spack.directives.reserved_names:
+        if name in reserved_names:
             if name == "dev_path":
                 new_variant = SingleValuedVariant(name, v._original_value)
                 spec.variants.substitute(new_variant)
diff --git a/var/spack/repos/builtin/packages/icu4c/package.py b/var/spack/repos/builtin/packages/icu4c/package.py
index 3cb4eb1d98..97f89cfc29 100644
--- a/var/spack/repos/builtin/packages/icu4c/package.py
+++ b/var/spack/repos/builtin/packages/icu4c/package.py
@@ -76,18 +76,15 @@ class Icu4c(AutotoolsPackage, MSBuildPackage):
         return (None, flags, None)
 
 
-class BuildEnvironment:
-    # Need to make sure that locale is UTF-8 in order to process source
-    # files in UTF-8.
+class AutotoolsBuilder(spack.build_systems.autotools.AutotoolsBuilder):
+
+    configure_directory = "source"
+
+    # Need to make sure that locale is UTF-8 in order to process source files in UTF-8.
     @when("@59:")
     def setup_build_environment(self, env):
         env.set("LC_ALL", "en_US.UTF-8")
 
-
-class AutotoolsBuilder(spack.build_systems.autotools.AutotoolsBuilder, BuildEnvironment):
-
-    configure_directory = "source"
-
     def configure_args(self):
         args = []
 
@@ -104,7 +101,12 @@ class AutotoolsBuilder(spack.build_systems.autotools.AutotoolsBuilder, BuildEnvi
         return args
 
 
-class MSBuildBuilder(spack.build_systems.msbuild.MSBuildBuilder, BuildEnvironment):
+class MSBuildBuilder(spack.build_systems.msbuild.MSBuildBuilder):
+    # Need to make sure that locale is UTF-8 in order to process source files in UTF-8.
+    @when("@59:")
+    def setup_build_environment(self, env):
+        env.set("LC_ALL", "en_US.UTF-8")
+
     def msbuild_args(self):
         return [
             "allinone.sln",