From 2b4f2daa730d02172434de433a04453500b336ab Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Mon, 5 May 2025 10:52:16 +0200 Subject: [PATCH] package API v2.0: new repo layout (#49256) This implements Package API v2.0, and is an opt-in feature for repos. It can be enabled with ```yaml repo: ... api: v2.0 ``` It differs from the current default v1.0 as follows: 1. Package names can only contain `-` as a separator. 2. Package names can only be lowercase. 3. Package directory names are valid Python module names. 4. The repo namespace and its directory name are the same. 5. The `packages` subdir, which is configurable, should be a directory name that is also a valid Python module name. 6. There is a one to one mapping between Spack package names and Python module names. 7. Import statements `import spack.pkg.namespace.package_module` in `package.py` files need to specify the canonical package module. To go from Spack package name to Python module name: - Replace `-` by `_` - Add a leading `_` if the package name starts with a digit To go from Python module name to Spack package name: - Strip leading `_` - Replace `_` by `-`. --- lib/spack/docs/developer_guide.rst | 4 +- lib/spack/spack/__init__.py | 2 +- lib/spack/spack/builder.py | 4 +- lib/spack/spack/ci/__init__.py | 4 +- lib/spack/spack/cmd/ci.py | 4 +- lib/spack/spack/cmd/create.py | 6 +- lib/spack/spack/cmd/pkg.py | 14 +- lib/spack/spack/cmd/repo.py | 14 +- lib/spack/spack/cmd/style.py | 2 +- lib/spack/spack/directives_meta.py | 2 +- lib/spack/spack/environment/environment.py | 8 +- lib/spack/spack/installer.py | 9 +- lib/spack/spack/package_base.py | 39 +- lib/spack/spack/repo.py | 480 +++++++++++++------- lib/spack/spack/spec.py | 4 +- lib/spack/spack/test/cmd/ci.py | 6 +- lib/spack/spack/test/cmd/env.py | 4 +- lib/spack/spack/test/cmd/pkg.py | 8 +- lib/spack/spack/test/cmd/repo.py | 11 +- lib/spack/spack/test/concretization/core.py | 12 +- lib/spack/spack/test/conftest.py | 4 +- lib/spack/spack/test/package_class.py | 27 +- lib/spack/spack/test/packages.py | 29 +- lib/spack/spack/test/repo.py | 170 ++++++- lib/spack/spack/util/naming.py | 158 ++++--- 25 files changed, 679 insertions(+), 346 deletions(-) diff --git a/lib/spack/docs/developer_guide.rst b/lib/spack/docs/developer_guide.rst index d90259f9336..5ddcc51f61f 100644 --- a/lib/spack/docs/developer_guide.rst +++ b/lib/spack/docs/developer_guide.rst @@ -154,9 +154,7 @@ Package-related modules :mod:`spack.util.naming` Contains functions for mapping between Spack package names, - Python module names, and Python class names. Functions like - :func:`~spack.util.naming.mod_to_class` handle mapping package - module names to class names. + Python module names, and Python class names. :mod:`spack.directives` *Directives* are functions that can be called inside a package definition diff --git a/lib/spack/spack/__init__.py b/lib/spack/spack/__init__.py index 000f8a2214f..ccd6265ca7b 100644 --- a/lib/spack/spack/__init__.py +++ b/lib/spack/spack/__init__.py @@ -18,7 +18,7 @@ #: version is incremented when the package API is extended in a backwards-compatible way. The major #: version is incremented upon breaking changes. This version is changed independently from the #: Spack version. -package_api_version = (1, 0) +package_api_version = (2, 0) #: The minimum Package API version that this version of Spack is compatible with. This should #: always be a tuple of the form ``(major, 0)``, since compatibility with vX.Y implies diff --git a/lib/spack/spack/builder.py b/lib/spack/spack/builder.py index 1ff2f777c0a..23dac2c548c 100644 --- a/lib/spack/spack/builder.py +++ b/lib/spack/spack/builder.py @@ -59,7 +59,7 @@ def __call__(self, spec, prefix): def get_builder_class(pkg, name: str) -> Optional[Type["Builder"]]: """Return the builder class if a package module defines it.""" cls = getattr(pkg.module, name, None) - if cls and cls.__module__.startswith(spack.repo.ROOT_PYTHON_NAMESPACE): + if cls and cls.__module__.startswith(spack.repo.PKG_MODULE_PREFIX_V1): return cls return None @@ -121,6 +121,7 @@ def __init__(self, wrapped_pkg_object, root_builder): new_cls_name, bases, { + "__module__": package_cls.__module__, "run_tests": property(lambda x: x.wrapped_package_object.run_tests), "test_requires_compiler": property( lambda x: x.wrapped_package_object.test_requires_compiler @@ -129,7 +130,6 @@ def __init__(self, wrapped_pkg_object, root_builder): "tester": property(lambda x: x.wrapped_package_object.tester), }, ) - new_cls.__module__ = package_cls.__module__ self.__class__ = new_cls self.__dict__.update(wrapped_pkg_object.__dict__) diff --git a/lib/spack/spack/ci/__init__.py b/lib/spack/spack/ci/__init__.py index 14be3bf4ada..9f3a1ca0d9c 100644 --- a/lib/spack/spack/ci/__init__.py +++ b/lib/spack/spack/ci/__init__.py @@ -150,10 +150,10 @@ def get_stack_changed(env_path, rev1="HEAD^", rev2="HEAD"): return False -def compute_affected_packages(rev1="HEAD^", rev2="HEAD"): +def compute_affected_packages(rev1: str = "HEAD^", rev2: str = "HEAD") -> Set[str]: """Determine which packages were added, removed or changed between rev1 and rev2, and return the names as a set""" - return spack.repo.get_all_package_diffs("ARC", rev1=rev1, rev2=rev2) + return spack.repo.get_all_package_diffs("ARC", spack.repo.builtin_repo(), rev1=rev1, rev2=rev2) def get_spec_filter_list(env, affected_pkgs, dependent_traverse_depth=None): diff --git a/lib/spack/spack/cmd/ci.py b/lib/spack/spack/cmd/ci.py index 156cb9410ad..0a3b68af674 100644 --- a/lib/spack/spack/cmd/ci.py +++ b/lib/spack/spack/cmd/ci.py @@ -791,7 +791,9 @@ def ci_verify_versions(args): """ # Get a list of all packages that have been changed or added # between from_ref and to_ref - pkgs = spack.repo.get_all_package_diffs("AC", args.from_ref, args.to_ref) + pkgs = spack.repo.get_all_package_diffs( + "AC", spack.repo.builtin_repo(), args.from_ref, args.to_ref + ) failed_version = False for pkg_name in pkgs: diff --git a/lib/spack/spack/cmd/create.py b/lib/spack/spack/cmd/create.py index 7e81c9eb4ba..a4df8dfd8e4 100644 --- a/lib/spack/spack/cmd/create.py +++ b/lib/spack/spack/cmd/create.py @@ -23,7 +23,7 @@ from spack.util.editor import editor from spack.util.executable import which from spack.util.format import get_version_lines -from spack.util.naming import mod_to_class, simplify_name, valid_fully_qualified_module_name +from spack.util.naming import pkg_name_to_class_name, simplify_name description = "create a new package file" section = "packaging" @@ -95,7 +95,7 @@ class BundlePackageTemplate: def __init__(self, name: str, versions, languages: List[str]): self.name = name - self.class_name = mod_to_class(name) + self.class_name = pkg_name_to_class_name(name) self.versions = versions self.languages = languages @@ -874,7 +874,7 @@ def get_name(name, url): result = simplify_name(result) - if not valid_fully_qualified_module_name(result): + if not re.match(r"^[a-z0-9-]+$", result): tty.die("Package name can only contain a-z, 0-9, and '-'") return result diff --git a/lib/spack/spack/cmd/pkg.py b/lib/spack/spack/cmd/pkg.py index 56d38b535bb..21c5a41a562 100644 --- a/lib/spack/spack/cmd/pkg.py +++ b/lib/spack/spack/cmd/pkg.py @@ -89,17 +89,17 @@ def setup_parser(subparser): def pkg_add(args): """add a package to the git stage with `git add`""" - spack.repo.add_package_to_git_stage(args.packages) + spack.repo.add_package_to_git_stage(args.packages, spack.repo.builtin_repo()) def pkg_list(args): """list packages associated with a particular spack git revision""" - colify(spack.repo.list_packages(args.rev)) + colify(spack.repo.list_packages(args.rev, spack.repo.builtin_repo())) def pkg_diff(args): """compare packages available in two different git revisions""" - u1, u2 = spack.repo.diff_packages(args.rev1, args.rev2) + u1, u2 = spack.repo.diff_packages(args.rev1, args.rev2, spack.repo.builtin_repo()) if u1: print("%s:" % args.rev1) @@ -114,21 +114,23 @@ def pkg_diff(args): def pkg_removed(args): """show packages removed since a commit""" - u1, u2 = spack.repo.diff_packages(args.rev1, args.rev2) + u1, u2 = spack.repo.diff_packages(args.rev1, args.rev2, spack.repo.builtin_repo()) if u1: colify(sorted(u1)) def pkg_added(args): """show packages added since a commit""" - u1, u2 = spack.repo.diff_packages(args.rev1, args.rev2) + u1, u2 = spack.repo.diff_packages(args.rev1, args.rev2, spack.repo.builtin_repo()) if u2: colify(sorted(u2)) def pkg_changed(args): """show packages changed since a commit""" - packages = spack.repo.get_all_package_diffs(args.type, args.rev1, args.rev2) + packages = spack.repo.get_all_package_diffs( + args.type, spack.repo.builtin_repo(), args.rev1, args.rev2 + ) if packages: colify(sorted(packages)) diff --git a/lib/spack/spack/cmd/repo.py b/lib/spack/spack/cmd/repo.py index 1420067d083..0f14190c029 100644 --- a/lib/spack/spack/cmd/repo.py +++ b/lib/spack/spack/cmd/repo.py @@ -4,6 +4,7 @@ import os import sys +from typing import List import llnl.util.tty as tty @@ -24,9 +25,7 @@ def setup_parser(subparser): create_parser = sp.add_parser("create", help=repo_create.__doc__) create_parser.add_argument("directory", help="directory to create the repo in") create_parser.add_argument( - "namespace", - help="namespace to identify packages in the repository (defaults to the directory name)", - nargs="?", + "namespace", help="name or namespace to identify packages in the repository" ) create_parser.add_argument( "-d", @@ -138,7 +137,7 @@ def repo_remove(args): def repo_list(args): """show registered repositories and their namespaces""" roots = spack.config.get("repos", scope=args.scope) - repos = [] + repos: List[spack.repo.Repo] = [] for r in roots: try: repos.append(spack.repo.from_path(r)) @@ -146,17 +145,14 @@ def repo_list(args): continue if sys.stdout.isatty(): - msg = "%d package repositor" % len(repos) - msg += "y." if len(repos) == 1 else "ies." - tty.msg(msg) + tty.msg(f"{len(repos)} package repositor" + ("y." if len(repos) == 1 else "ies.")) if not repos: return max_ns_len = max(len(r.namespace) for r in repos) for repo in repos: - fmt = "%%-%ds%%s" % (max_ns_len + 4) - print(fmt % (repo.namespace, repo.root)) + print(f"{repo.namespace:<{max_ns_len + 4}}{repo.package_api_str:<8}{repo.root}") def repo(parser, args): diff --git a/lib/spack/spack/cmd/style.py b/lib/spack/spack/cmd/style.py index aff309cc65b..440d268c760 100644 --- a/lib/spack/spack/cmd/style.py +++ b/lib/spack/spack/cmd/style.py @@ -380,7 +380,7 @@ def run_black(black_cmd, file_list, args): def _module_part(root: str, expr: str): parts = expr.split(".") # spack.pkg is for repositories, don't try to resolve it here. - if ".".join(parts[:2]) == spack.repo.ROOT_PYTHON_NAMESPACE: + if expr.startswith(spack.repo.PKG_MODULE_PREFIX_V1) or expr == "spack.pkg": return None while parts: f1 = os.path.join(root, "lib", "spack", *parts) + ".py" diff --git a/lib/spack/spack/directives_meta.py b/lib/spack/spack/directives_meta.py index a02d11d6d40..afb459b5d4a 100644 --- a/lib/spack/spack/directives_meta.py +++ b/lib/spack/spack/directives_meta.py @@ -65,7 +65,7 @@ def __init__(cls: "DirectiveMeta", name: str, bases: tuple, attr_dict: dict): # The instance is being initialized: if it is a package we must ensure # that the directives are called to set it up. - if cls.__module__.startswith(spack.repo.ROOT_PYTHON_NAMESPACE): + if spack.repo.is_package_module(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: diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index 96d023026f9..95476677986 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -2312,8 +2312,12 @@ def update_environment_repository(self) -> None: def _add_to_environment_repository(self, spec_node: Spec) -> None: """Add the root node of the spec to the environment repository""" - repository_dir = os.path.join(self.repos_path, spec_node.namespace) - repository = spack.repo.create_or_construct(repository_dir, spec_node.namespace) + namespace: str = spec_node.namespace + repository = spack.repo.create_or_construct( + root=os.path.join(self.repos_path, namespace), + namespace=namespace, + package_api=spack.repo.PATH.get_repo(namespace).package_api, + ) pkg_dir = repository.dirname_for_package_name(spec_node.name) fs.mkdirp(pkg_dir) spack.repo.PATH.dump_provenance(spec_node, pkg_dir) diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py index 37a1301733a..c51ea278a05 100644 --- a/lib/spack/spack/installer.py +++ b/lib/spack/spack/installer.py @@ -566,10 +566,11 @@ def dump_packages(spec: "spack.spec.Spec", path: str) -> None: tty.warn(f"Warning: Couldn't copy in provenance for {node.name}") # Create a destination repository - dest_repo_root = os.path.join(path, node.namespace) - if not os.path.exists(dest_repo_root): - spack.repo.create_repo(dest_repo_root) - repo = spack.repo.from_path(dest_repo_root) + pkg_api = spack.repo.PATH.get_repo(node.namespace).package_api + repo_root = os.path.join(path, node.namespace) if pkg_api < (2, 0) else path + repo = spack.repo.create_or_construct( + repo_root, namespace=node.namespace, package_api=pkg_api + ) # Get the location of the package in the dest repo. dest_pkg_dir = repo.dirname_for_package_name(node.name) diff --git a/lib/spack/spack/package_base.py b/lib/spack/spack/package_base.py index 7fa444d2e86..c4e7fc20f5f 100644 --- a/lib/spack/spack/package_base.py +++ b/lib/spack/spack/package_base.py @@ -47,6 +47,7 @@ import spack.url import spack.util.environment import spack.util.executable +import spack.util.naming import spack.util.path import spack.util.web import spack.variant @@ -838,26 +839,36 @@ def fullname(cls): def fullnames(cls): """Fullnames for this package and any packages from which it inherits.""" fullnames = [] - for cls in cls.__mro__: - namespace = getattr(cls, "namespace", None) - if namespace: - fullnames.append("%s.%s" % (namespace, cls.name)) - if namespace == "builtin": - # builtin packages cannot inherit from other repos + for base in cls.__mro__: + if not spack.repo.is_package_module(base.__module__): break + fullnames.append(base.fullname) return fullnames @classproperty def name(cls): - """The name of this package. - - The name of a package is the name of its Python module, without - the containing module names. - """ + """The name of this package.""" if cls._name is None: - cls._name = cls.module.__name__ - if "." in cls._name: - cls._name = cls._name[cls._name.rindex(".") + 1 :] + # We cannot know the exact package API version, but we can distinguish between v1 + # v2 based on the module. We don't want to figure out the exact package API version + # since it requires parsing the repo.yaml. + module = cls.__module__ + + if module.startswith(spack.repo.PKG_MODULE_PREFIX_V1): + version = (1, 0) + elif module.startswith(spack.repo.PKG_MODULE_PREFIX_V2): + version = (2, 0) + else: + raise ValueError(f"Package {cls.__qualname__} is not a known Spack package") + + if version < (2, 0): + # spack.pkg.builtin.package_name. + _, _, pkg_module = module.rpartition(".") + else: + # spack_repo.builtin.packages.package_name.package + pkg_module = module.rsplit(".", 2)[-2] + + cls._name = spack.util.naming.pkg_dir_to_pkg_name(pkg_module, version) return cls._name @classproperty diff --git a/lib/spack/spack/repo.py b/lib/spack/spack/repo.py index 2e1b613bd00..e82f59e648a 100644 --- a/lib/spack/spack/repo.py +++ b/lib/spack/spack/repo.py @@ -47,40 +47,34 @@ import spack.util.path import spack.util.spack_yaml as syaml -#: Package modules are imported as spack.pkg.. -ROOT_PYTHON_NAMESPACE = "spack.pkg" +PKG_MODULE_PREFIX_V1 = "spack.pkg." +PKG_MODULE_PREFIX_V2 = "spack_repo." _API_REGEX = re.compile(r"^v(\d+)\.(\d+)$") -def python_package_for_repo(namespace): - """Returns the full namespace of a repository, given its relative one - - For instance: - - python_package_for_repo('builtin') == 'spack.pkg.builtin' - - Args: - namespace (str): repo namespace - """ - return "{0}.{1}".format(ROOT_PYTHON_NAMESPACE, namespace) +def is_package_module(fullname: str) -> bool: + """Check if the given module is a package module.""" + return fullname.startswith(PKG_MODULE_PREFIX_V1) or fullname.startswith(PKG_MODULE_PREFIX_V2) -def namespace_from_fullname(fullname): +def namespace_from_fullname(fullname: str) -> str: """Return the repository namespace only for the full module name. For instance: - namespace_from_fullname('spack.pkg.builtin.hdf5') == 'builtin' + namespace_from_fullname("spack.pkg.builtin.hdf5") == "builtin" + namespace_from_fullname("spack_repo.x.y.z.packages.pkg_name.package") == "x.y.z" Args: - fullname (str): full name for the Python module + fullname: full name for the Python module """ - namespace, dot, module = fullname.rpartition(".") - prefix_and_dot = "{0}.".format(ROOT_PYTHON_NAMESPACE) - if namespace.startswith(prefix_and_dot): - namespace = namespace[len(prefix_and_dot) :] - return namespace + if fullname.startswith(PKG_MODULE_PREFIX_V1): + namespace, _, _ = fullname.rpartition(".") + return namespace[len(PKG_MODULE_PREFIX_V1) :] + elif fullname.startswith(PKG_MODULE_PREFIX_V2) and fullname.endswith(".package"): + return ".".join(fullname.split(".")[1:-3]) + return fullname class SpackNamespaceLoader: @@ -92,14 +86,14 @@ def exec_module(self, module): class ReposFinder: - """MetaPathFinder class that loads a Python module corresponding to a Spack package. + """MetaPathFinder class that loads a Python module corresponding to an API v1 Spack package. Returns a loader based on the inspection of the current repository list. """ def __init__(self): self._repo_init = _path - self._repo = None + self._repo: Optional[RepoType] = None @property def current_repository(self): @@ -127,7 +121,7 @@ def find_spec(self, fullname, python_path, target=None): raise RuntimeError('cannot reload module "{0}"'.format(fullname)) # Preferred API from https://peps.python.org/pep-0451/ - if not fullname.startswith(ROOT_PYTHON_NAMESPACE): + if not fullname.startswith(PKG_MODULE_PREFIX_V1) and fullname != "spack.pkg": return None loader = self.compute_loader(fullname) @@ -135,16 +129,17 @@ def find_spec(self, fullname, python_path, target=None): return None return importlib.util.spec_from_loader(fullname, loader) - def compute_loader(self, fullname): + def compute_loader(self, fullname: str): # namespaces are added to repo, and package modules are leaves. namespace, dot, module_name = fullname.rpartition(".") # If it's a module in some repo, or if it is the repo's namespace, let the repo handle it. - is_repo_path = isinstance(self.current_repository, RepoPath) + current_repo = self.current_repository + is_repo_path = isinstance(current_repo, RepoPath) if is_repo_path: - repos = self.current_repository.repos + repos = current_repo.repos else: - repos = [self.current_repository] + repos = [current_repo] for repo in repos: # We are using the namespace of the repo and the repo contains the package @@ -161,7 +156,9 @@ def compute_loader(self, fullname): # No repo provides the namespace, but it is a valid prefix of # something in the RepoPath. - if is_repo_path and self.current_repository.by_namespace.is_prefix(fullname): + if is_repo_path and current_repo.by_namespace.is_prefix( + fullname[len(PKG_MODULE_PREFIX_V1) :] + ): return SpackNamespaceLoader() return None @@ -179,12 +176,12 @@ def compute_loader(self, fullname): NOT_PROVIDED = object() -def packages_path(): +def builtin_repo() -> "Repo": """Get the test repo if it is active, otherwise the builtin repo.""" try: - return PATH.get_repo("builtin.mock").packages_path + return PATH.get_repo("builtin.mock") except UnknownNamespaceError: - return PATH.get_repo("builtin").packages_path + return PATH.get_repo("builtin") class GitExe: @@ -192,24 +189,25 @@ class GitExe: # invocations. # # Not using -C as that is not supported for git < 1.8.5. - def __init__(self): + def __init__(self, packages_path: str): self._git_cmd = spack.util.git.git(required=True) + self.packages_dir = packages_path - def __call__(self, *args, **kwargs): - with working_dir(packages_path()): - return self._git_cmd(*args, **kwargs) + def __call__(self, *args, **kwargs) -> str: + with working_dir(self.packages_dir): + return self._git_cmd(*args, **kwargs, output=str) -def list_packages(rev): +def list_packages(rev: str, repo: "Repo") -> List[str]: """List all packages associated with the given revision""" - git = GitExe() + git = GitExe(repo.packages_path) # git ls-tree does not support ... merge-base syntax, so do it manually if rev.endswith("..."): ref = rev.replace("...", "") - rev = git("merge-base", ref, "HEAD", output=str).strip() + rev = git("merge-base", ref, "HEAD").strip() - output = git("ls-tree", "-r", "--name-only", rev, output=str) + output = git("ls-tree", "-r", "--name-only", rev) # recursively list the packages directory package_paths = [ @@ -217,54 +215,54 @@ def list_packages(rev): ] # take the directory names with one-level-deep package files - package_names = sorted(set([line[0] for line in package_paths if len(line) == 2])) + package_names = [ + nm.pkg_dir_to_pkg_name(line[0], repo.package_api) + for line in package_paths + if len(line) == 2 + ] - return package_names + return sorted(set(package_names)) -def diff_packages(rev1, rev2): +def diff_packages(rev1: str, rev2: str, repo: "Repo") -> Tuple[Set[str], Set[str]]: """Compute packages lists for the two revisions and return a tuple containing all the packages in rev1 but not in rev2 and all the packages in rev2 but not in rev1.""" - p1 = set(list_packages(rev1)) - p2 = set(list_packages(rev2)) + p1 = set(list_packages(rev1, repo)) + p2 = set(list_packages(rev2, repo)) return p1.difference(p2), p2.difference(p1) -def get_all_package_diffs(type, rev1="HEAD^1", rev2="HEAD"): - """Show packages changed, added, or removed (or any combination of those) - since a commit. +def get_all_package_diffs(type: str, repo: "Repo", rev1="HEAD^1", rev2="HEAD") -> Set[str]: + """Get packages changed, added, or removed (or any combination of those) since a commit. Arguments: - type (str): String containing one or more of 'A', 'R', 'C' - rev1 (str): Revision to compare against, default is 'HEAD^' - rev2 (str): Revision to compare to rev1, default is 'HEAD' - - Returns: - - A set contain names of affected packages. + type: String containing one or more of 'A', 'R', 'C' + rev1: Revision to compare against, default is 'HEAD^' + rev2: Revision to compare to rev1, default is 'HEAD' """ lower_type = type.lower() if not re.match("^[arc]*$", lower_type): tty.die( - "Invald change type: '%s'." % type, - "Can contain only A (added), R (removed), or C (changed)", + f"Invalid change type: '{type}'. " + "Can contain only A (added), R (removed), or C (changed)" ) - removed, added = diff_packages(rev1, rev2) + removed, added = diff_packages(rev1, rev2, repo) - git = GitExe() - out = git("diff", "--relative", "--name-only", rev1, rev2, output=str).strip() + git = GitExe(repo.packages_path) + out = git("diff", "--relative", "--name-only", rev1, rev2).strip() lines = [] if not out else re.split(r"\s+", out) - changed = set() + changed: Set[str] = set() for path in lines: - pkg_name, _, _ = path.partition("/") + dir_name, _, _ = path.partition("/") + pkg_name = nm.pkg_dir_to_pkg_name(dir_name, repo.package_api) if pkg_name not in added and pkg_name not in removed: changed.add(pkg_name) - packages = set() + packages: Set[str] = set() if "a" in lower_type: packages |= added if "r" in lower_type: @@ -275,14 +273,14 @@ def get_all_package_diffs(type, rev1="HEAD^1", rev2="HEAD"): return packages -def add_package_to_git_stage(packages): +def add_package_to_git_stage(packages: List[str], repo: "Repo") -> None: """add a package to the git stage with `git add`""" - git = GitExe() + git = GitExe(repo.packages_path) for pkg_name in packages: filename = PATH.filename_for_package_name(pkg_name) if not os.path.isfile(filename): - tty.die("No such package: %s. Path does not exist:" % pkg_name, filename) + tty.die(f"No such package: {pkg_name}. Path does not exist:", filename) git("add", filename) @@ -352,9 +350,10 @@ class FastPackageChecker(collections.abc.Mapping): #: Global cache, reused by every instance _paths_cache: Dict[str, Dict[str, os.stat_result]] = {} - def __init__(self, packages_path): + def __init__(self, packages_path: str, package_api: Tuple[int, int]): # The path of the repository managed by this instance self.packages_path = packages_path + self.package_api = package_api # If the cache we need is not there yet, then build it appropriately if packages_path not in self._paths_cache: @@ -379,41 +378,38 @@ def _create_new_cache(self) -> Dict[str, os.stat_result]: # Create a dictionary that will store the mapping between a # package name and its stat info cache: Dict[str, os.stat_result] = {} - for pkg_name in os.listdir(self.packages_path): - # Skip non-directories in the package root. - pkg_dir = os.path.join(self.packages_path, pkg_name) + with os.scandir(self.packages_path) as entries: + for entry in entries: + # Construct the file name from the directory + pkg_file = os.path.join(entry.path, package_file_name) - # Warn about invalid names that look like packages. - if not nm.valid_module_name(pkg_name): - if not pkg_name.startswith(".") and pkg_name != "repo.yaml": + try: + sinfo = os.stat(pkg_file) + except OSError as e: + if e.errno in (errno.ENOENT, errno.ENOTDIR): + # No package.py file here. + continue + elif e.errno == errno.EACCES: + tty.warn(f"Can't read package file {pkg_file}.") + continue + raise e + + # If it's not a file, skip it. + if not stat.S_ISREG(sinfo.st_mode): + continue + + # Only consider package.py files in directories that are valid module names under + # the current package API + if not nm.valid_module_name(entry.name, self.package_api): + x, y = self.package_api tty.warn( - 'Skipping package at {0}. "{1}" is not ' - "a valid Spack module name.".format(pkg_dir, pkg_name) + f"Package {pkg_file} cannot be used because `{entry.name}` is not a valid " + f"Spack package module name for Package API v{x}.{y}." ) - continue - - # Construct the file name from the directory - pkg_file = os.path.join(self.packages_path, pkg_name, package_file_name) - - # Use stat here to avoid lots of calls to the filesystem. - try: - sinfo = os.stat(pkg_file) - except OSError as e: - if e.errno == errno.ENOENT: - # No package.py file here. continue - elif e.errno == errno.EACCES: - tty.warn("Can't read package file %s." % pkg_file) - continue - raise e - # If it's not a file, skip it. - if stat.S_ISDIR(sinfo.st_mode): - continue - - # If it is a file, then save the stats under the - # appropriate key - cache[pkg_name] = sinfo + # Store the stat info by package name. + cache[nm.pkg_dir_to_pkg_name(entry.name, self.package_api)] = sinfo return cache @@ -688,7 +684,7 @@ def put_first(self, repo: "Repo") -> None: return self.repos.insert(0, repo) - self.by_namespace[repo.full_namespace] = repo + self.by_namespace[repo.namespace] = repo def put_last(self, repo): """Add repo last in the search path.""" @@ -700,8 +696,8 @@ def put_last(self, repo): self.repos.append(repo) # don't mask any higher-precedence repos with same namespace - if repo.full_namespace not in self.by_namespace: - self.by_namespace[repo.full_namespace] = repo + if repo.namespace not in self.by_namespace: + self.by_namespace[repo.namespace] = repo def remove(self, repo): """Remove a repo from the search path.""" @@ -710,10 +706,9 @@ def remove(self, repo): def get_repo(self, namespace: str) -> "Repo": """Get a repository by namespace.""" - full_namespace = python_package_for_repo(namespace) - if full_namespace not in self.by_namespace: + if namespace not in self.by_namespace: raise UnknownNamespaceError(namespace) - return self.by_namespace[full_namespace] + return self.by_namespace[namespace] def first_repo(self) -> Optional["Repo"]: """Get the first repo in precedence order.""" @@ -821,10 +816,9 @@ def repo_for_pkg(self, spec: Union[str, "spack.spec.Spec"]) -> "Repo": # If the spec already has a namespace, then return the # corresponding repo if we know about it. if namespace: - fullspace = python_package_for_repo(namespace) - if fullspace not in self.by_namespace: + if namespace not in self.by_namespace: raise UnknownNamespaceError(namespace, name=name) - return self.by_namespace[fullspace] + return self.by_namespace[namespace] # If there's no namespace, search in the RepoPath. for repo in self.repos: @@ -845,8 +839,15 @@ def get(self, spec: "spack.spec.Spec") -> "spack.package_base.PackageBase": assert isinstance(spec, spack.spec.Spec) and spec.concrete, msg return self.repo_for_pkg(spec).get(spec) + def python_paths(self) -> List[str]: + """Return a list of all the Python paths in the repos.""" + return [repo.python_path for repo in self.repos if repo.python_path] + def get_pkg_class(self, pkg_name: str) -> Type["spack.package_base.PackageBase"]: """Find a class for the spec's package and return the class object.""" + for p in self.python_paths(): + if p not in sys.path: + sys.path.insert(0, p) return self.repo_for_pkg(pkg_name).get_pkg_class(pkg_name) @autospec @@ -942,6 +943,30 @@ def _parse_package_api_version( ) +def _validate_and_normalize_subdir(subdir: Any, root: str, package_api: Tuple[int, int]) -> str: + if not isinstance(subdir, str): + raise BadRepoError(f"Invalid subdirectory '{subdir}' in '{root}'. Must be a string") + + if package_api < (2, 0): + return subdir # In v1.x we did not validate subdir names + + if subdir in (".", ""): + raise BadRepoError( + f"Invalid subdirectory '{subdir}' in '{root}'. Use a symlink packages -> . instead" + ) + + # Otherwise we expect a directory name (not path) that can be used as a Python module. + if os.sep in subdir: + raise BadRepoError( + f"Invalid subdirectory '{subdir}' in '{root}'. Expected a directory name, not a path" + ) + if not nm.valid_module_name(subdir, package_api): + raise BadRepoError( + f"Invalid subdirectory '{subdir}' in '{root}'. Must be a valid Python module name" + ) + return subdir + + class Repo: """Class representing a package repository in the filesystem. @@ -962,6 +987,8 @@ class Repo: :py:data:`spack.package_api_version`. """ + namespace: str + def __init__( self, root: str, @@ -991,32 +1018,79 @@ def check(condition, msg): # Read configuration and validate namespace config = self._read_config() + + self.package_api = _parse_package_api_version(config) + self.subdirectory = _validate_and_normalize_subdir( + config.get("subdirectory", packages_dir_name), root, self.package_api + ) + self.packages_path = os.path.join(self.root, self.subdirectory) + check( - "namespace" in config, - f"{os.path.join(root, repo_config_name)} must define a namespace.", + os.path.isdir(self.packages_path), + f"No directory '{self.subdirectory}' found in '{root}'", ) - self.namespace: str = config["namespace"] - check( - re.match(r"[a-zA-Z][a-zA-Z0-9_.]+", self.namespace), - f"Invalid namespace '{self.namespace}' in repo '{self.root}'. " - "Namespaces must be valid python identifiers separated by '.'", - ) + # The parent dir of spack_repo/ which should be added to sys.path for api v2.x + self.python_path: Optional[str] = None + + if self.package_api < (2, 0): + check( + "namespace" in config, + f"{os.path.join(root, repo_config_name)} must define a namespace.", + ) + self.namespace = config["namespace"] + # Note: for Package API v1.x the namespace validation always had bugs, which won't be + # fixed for compatibility reasons. The regex is missing "$" at the end, and it claims + # to test for valid identifiers, but fails to split on `.` first. + check( + isinstance(self.namespace, str) + and re.match(r"[a-zA-Z][a-zA-Z0-9_.]+", self.namespace), + f"Invalid namespace '{self.namespace}' in repo '{self.root}'. " + "Namespaces must be valid python identifiers separated by '.'", + ) + else: + # From Package API v2.0 the namespace follows from the directory structure. + check( + f"{os.sep}spack_repo{os.sep}" in self.root, + f"Invalid repository path '{self.root}'. " + f"Path must contain 'spack_repo{os.sep}'", + ) + derived_namespace = self.root.rpartition(f"spack_repo{os.sep}")[2].replace(os.sep, ".") + if "namespace" in config: + self.namespace = config["namespace"] + + check( + isinstance(self.namespace, str) and self.namespace == derived_namespace, + f"Namespace '{self.namespace}' should be {derived_namespace} or omitted in " + f"{os.path.join(root, repo_config_name)}", + ) + else: + self.namespace = derived_namespace + + # strip the namespace directories from the root path to get the python path + # e.g. /my/pythonpath/spack_repo/x/y/z -> /my/pythonpath + python_path = self.root + for _ in self.namespace.split("."): + python_path = os.path.dirname(python_path) + self.python_path = os.path.dirname(python_path) + + # check that all subdirectories are valid module names + check( + all(nm.valid_module_name(x, self.package_api) for x in self.namespace.split(".")), + f"Invalid namespace '{self.namespace}' in repo '{self.root}'", + ) # Set up 'full_namespace' to include the super-namespace - self.full_namespace = python_package_for_repo(self.namespace) + if self.package_api < (2, 0): + self.full_namespace = f"{PKG_MODULE_PREFIX_V1}{self.namespace}" + elif self.subdirectory == ".": + self.full_namespace = f"{PKG_MODULE_PREFIX_V2}{self.namespace}" + else: + self.full_namespace = f"{PKG_MODULE_PREFIX_V2}{self.namespace}.{self.subdirectory}" # Keep name components around for checking prefixes. self._names = self.full_namespace.split(".") - packages_dir: str = config.get("subdirectory", packages_dir_name) - self.packages_path = os.path.join(self.root, packages_dir) - check( - os.path.isdir(self.packages_path), f"No directory '{packages_dir}' found in '{root}'" - ) - - self.package_api = _parse_package_api_version(config) - # Class attribute overrides by package name self.overrides = overrides or {} @@ -1030,27 +1104,36 @@ def check(condition, msg): self._repo_index: Optional[RepoIndex] = None self._cache = cache + @property + def package_api_str(self) -> str: + return f"v{self.package_api[0]}.{self.package_api[1]}" + def finder(self, value: RepoPath) -> None: self._finder = value def real_name(self, import_name: str) -> Optional[str]: """Allow users to import Spack packages using Python identifiers. - A python identifier might map to many different Spack package - names due to hyphen/underscore ambiguity. + In Package API v1.x, there was no canonical module name for a package, and package's dir + was not necessarily a valid Python module name. For that case we have to guess the actual + package directory. From Package API v2.0 there is a one-to-one mapping between Spack + package names and Python module names, so there is no guessing. - Easy example: - num3proxy -> 3proxy - - Ambiguous: + For Packge API v1.x we support the following one-to-many mappings: + num3proxy -> 3proxy foo_bar -> foo_bar, foo-bar - - More ambiguous: foo_bar_baz -> foo_bar_baz, foo-bar-baz, foo_bar-baz, foo-bar_baz """ + if self.package_api >= (2, 0): + if nm.pkg_dir_to_pkg_name(import_name, package_api=self.package_api) in self: + return import_name + return None + if import_name in self: return import_name + # For v1 generate the possible package names from a module name, and return the first + # package name that exists in this repo. options = nm.possible_spack_module_names(import_name) try: options.remove(import_name) @@ -1183,7 +1266,9 @@ def extensions_for( def dirname_for_package_name(self, pkg_name: str) -> str: """Given a package name, get the directory containing its package.py file.""" _, unqualified_name = self.partition_package_name(pkg_name) - return os.path.join(self.packages_path, unqualified_name) + return os.path.join( + self.packages_path, nm.pkg_name_to_pkg_dir(unqualified_name, self.package_api) + ) def filename_for_package_name(self, pkg_name: str) -> str: """Get the filename for the module we should load for a particular @@ -1200,7 +1285,7 @@ def filename_for_package_name(self, pkg_name: str) -> str: @property def _pkg_checker(self) -> FastPackageChecker: if self._fast_package_checker is None: - self._fast_package_checker = FastPackageChecker(self.packages_path) + self._fast_package_checker = FastPackageChecker(self.packages_path, self.package_api) return self._fast_package_checker def all_package_names(self, include_virtuals: bool = False) -> List[str]: @@ -1212,7 +1297,9 @@ def all_package_names(self, include_virtuals: bool = False) -> List[str]: def package_path(self, name: str) -> str: """Get path to package.py file for this repo.""" - return os.path.join(self.packages_path, name, package_file_name) + return os.path.join( + self.packages_path, nm.pkg_name_to_pkg_dir(name, self.package_api), package_file_name + ) def all_package_paths(self) -> Generator[str, None, None]: for name in self.all_package_names(): @@ -1270,15 +1357,19 @@ def get_pkg_class(self, pkg_name: str) -> Type["spack.package_base.PackageBase"] package. Then extracts the package class from the module according to Spack's naming convention. """ - namespace, pkg_name = self.partition_package_name(pkg_name) - class_name = nm.mod_to_class(pkg_name) - fullname = f"{self.full_namespace}.{pkg_name}" + _, pkg_name = self.partition_package_name(pkg_name) + fullname = f"{self.full_namespace}.{nm.pkg_name_to_pkg_dir(pkg_name, self.package_api)}" + if self.package_api >= (2, 0): + fullname += ".package" + class_name = nm.pkg_name_to_class_name(pkg_name) + if self.python_path and self.python_path not in sys.path: + sys.path.insert(0, self.python_path) try: with REPOS_FINDER.switch_repo(self._finder or self): module = importlib.import_module(fullname) - except ImportError: - raise UnknownPackageError(fullname) + except ImportError as e: + raise UnknownPackageError(fullname) from e except Exception as e: msg = f"cannot load package '{pkg_name}' from the '{self.namespace}' repository: {e}" raise RepoError(msg) from e @@ -1369,46 +1460,71 @@ def partition_package_name(pkg_name: str) -> Tuple[str, str]: return namespace, pkg_name -def create_repo(root, namespace=None, subdir=packages_dir_name): +def get_repo_yaml_dir( + root: str, namespace: Optional[str], package_api: Tuple[int, int] +) -> Tuple[str, str]: + """Returns the directory where repo.yaml is located and the effective namespace.""" + if package_api < (2, 0): + namespace = namespace or os.path.basename(root) + # This ad-hoc regex is left for historical reasons, and should not have a breaking change. + if not re.match(r"\w[\.\w-]*", namespace): + raise InvalidNamespaceError(f"'{namespace}' is not a valid namespace.") + return root, namespace + + # Package API v2 has /spack_repo// structure and requires a namespace + if namespace is None: + raise InvalidNamespaceError("Namespace must be provided.") + + # if namespace has dots those translate to subdirs of further namespace packages. + namespace_components = namespace.split(".") + + if not all(nm.valid_module_name(n, package_api=package_api) for n in namespace_components): + raise InvalidNamespaceError(f"'{namespace}' is not a valid namespace." % namespace) + + return os.path.join(root, "spack_repo", *namespace_components), namespace + + +def create_repo( + root, + namespace: Optional[str] = None, + subdir: str = packages_dir_name, + package_api: Tuple[int, int] = spack.package_api_version, +) -> Tuple[str, str]: """Create a new repository in root with the specified namespace. If the namespace is not provided, use basename of root. Return the canonicalized path and namespace of the created repository. """ root = spack.util.path.canonicalize_path(root) - if not namespace: - namespace = os.path.basename(root) + repo_yaml_dir, namespace = get_repo_yaml_dir(os.path.abspath(root), namespace, package_api) - if not re.match(r"\w[\.\w-]*", namespace): - raise InvalidNamespaceError("'%s' is not a valid namespace." % namespace) + existed = True + try: + dir_entry = next(os.scandir(repo_yaml_dir), None) + except OSError as e: + if e.errno == errno.ENOENT: + existed = False + dir_entry = None + else: + raise BadRepoError(f"Cannot create new repo in {root}: {e}") - existed = False - if os.path.exists(root): - if os.path.isfile(root): - raise BadRepoError("File %s already exists and is not a directory" % root) - elif os.path.isdir(root): - if not os.access(root, os.R_OK | os.W_OK): - raise BadRepoError("Cannot create new repo in %s: cannot access directory." % root) - if os.listdir(root): - raise BadRepoError("Cannot create new repo in %s: directory is not empty." % root) - existed = True + if dir_entry is not None: + raise BadRepoError(f"Cannot create new repo in {root}: directory is not empty.") - full_path = os.path.realpath(root) - parent = os.path.dirname(full_path) - if not os.access(parent, os.R_OK | os.W_OK): - raise BadRepoError("Cannot create repository in %s: can't access parent!" % root) + config_path = os.path.join(repo_yaml_dir, repo_config_name) + + subdir = _validate_and_normalize_subdir(subdir, root, package_api) + + packages_path = os.path.join(repo_yaml_dir, subdir) try: - config_path = os.path.join(root, repo_config_name) - packages_path = os.path.join(root, subdir) - fs.mkdirp(packages_path) with open(config_path, "w", encoding="utf-8") as config: config.write("repo:\n") config.write(f" namespace: '{namespace}'\n") if subdir != packages_dir_name: config.write(f" subdirectory: '{subdir}'\n") - x, y = spack.package_api_version + x, y = package_api config.write(f" api: v{x}.{y}\n") except OSError as e: @@ -1421,22 +1537,27 @@ def create_repo(root, namespace=None, subdir=packages_dir_name): raise BadRepoError( "Failed to create new repository in %s." % root, "Caused by %s: %s" % (type(e), e) - ) + ) from e - return full_path, namespace + return repo_yaml_dir, namespace -def from_path(path: str) -> "Repo": +def from_path(path: str) -> Repo: """Returns a repository from the path passed as input. Injects the global misc cache.""" return Repo(path, cache=spack.caches.MISC_CACHE) -def create_or_construct(path, namespace=None): +def create_or_construct( + root: str, + namespace: Optional[str] = None, + package_api: Tuple[int, int] = spack.package_api_version, +) -> Repo: """Create a repository, or just return a Repo if it already exists.""" - if not os.path.exists(path): - fs.mkdirp(path) - create_repo(path, namespace) - return from_path(path) + repo_yaml_dir, _ = get_repo_yaml_dir(root, namespace, package_api) + if not os.path.exists(repo_yaml_dir): + fs.mkdirp(root) + create_repo(root, namespace=namespace, package_api=package_api) + return from_path(repo_yaml_dir) def _path(configuration=None): @@ -1514,8 +1635,10 @@ class MockRepositoryBuilder: """Build a mock repository in a directory""" def __init__(self, root_directory, namespace=None): - namespace = namespace or "".join(random.choice(string.ascii_uppercase) for _ in range(10)) - self.root, self.namespace = create_repo(str(root_directory), namespace) + namespace = namespace or "".join(random.choice(string.ascii_lowercase) for _ in range(10)) + repo_root = os.path.join(root_directory, namespace) + os.mkdir(repo_root) + self.root, self.namespace = create_repo(repo_root, namespace) def add_package(self, name, dependencies=None): """Create a mock package in the repository, using a Jinja2 template. @@ -1527,7 +1650,7 @@ def add_package(self, name, dependencies=None): ``spack.dependency.default_deptype`` and ``spack.spec.Spec()`` are used. """ dependencies = dependencies or [] - context = {"cls_name": nm.mod_to_class(name), "dependencies": dependencies} + context = {"cls_name": nm.pkg_name_to_class_name(name), "dependencies": dependencies} template = spack.tengine.make_environment().get_template("mock-repository/package.pyt") text = template.render(context) package_py = self.recipe_filename(name) @@ -1539,8 +1662,10 @@ def remove(self, name): package_py = self.recipe_filename(name) shutil.rmtree(os.path.dirname(package_py)) - def recipe_filename(self, name): - return os.path.join(self.root, "packages", name, "package.py") + def recipe_filename(self, name: str): + return os.path.join( + self.root, "packages", nm.pkg_name_to_pkg_dir(name, package_api=(2, 0)), "package.py" + ) class RepoError(spack.error.SpackError): @@ -1590,7 +1715,10 @@ def __init__(self, name, repo=None): # We need to compare the base package name pkg_name = name.rsplit(".", 1)[-1] - similar = difflib.get_close_matches(pkg_name, repo.all_package_names()) + try: + similar = difflib.get_close_matches(pkg_name, repo.all_package_names()) + except Exception: + similar = [] if 1 <= len(similar) <= 5: long_msg += "\n\nDid you mean one of the following packages?\n " diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index b68bbea8c76..ef4eab6bd93 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -1861,9 +1861,7 @@ def add_dependency_edge( @property def fullname(self): return ( - ("%s.%s" % (self.namespace, self.name)) - if self.namespace - else (self.name if self.name else "") + f"{self.namespace}.{self.name}" if self.namespace else (self.name if self.name else "") ) @property diff --git a/lib/spack/spack/test/cmd/ci.py b/lib/spack/spack/test/cmd/ci.py index da1ce50c77e..9f4cf680aca 100644 --- a/lib/spack/spack/test/cmd/ci.py +++ b/lib/spack/spack/test/cmd/ci.py @@ -2032,7 +2032,7 @@ def test_ci_verify_versions_valid( repo, _, commits = mock_git_package_changes spack.repo.PATH.put_first(repo) - monkeypatch.setattr(spack.repo, "packages_path", mock_packages_path(repo.packages_path)) + monkeypatch.setattr(spack.repo, "builtin_repo", lambda: repo) out = ci_cmd("verify-versions", commits[-1], commits[-3]) assert "Validated diff-test@2.1.5" in out @@ -2049,7 +2049,7 @@ def test_ci_verify_versions_standard_invalid( repo, _, commits = mock_git_package_changes spack.repo.PATH.put_first(repo) - monkeypatch.setattr(spack.repo, "packages_path", mock_packages_path(repo.packages_path)) + monkeypatch.setattr(spack.repo, "builtin_repo", lambda: repo) out = ci_cmd("verify-versions", commits[-1], commits[-3], fail_on_error=False) assert "Invalid checksum found diff-test@2.1.5" in out @@ -2060,7 +2060,7 @@ def test_ci_verify_versions_manual_package(monkeypatch, mock_packages, mock_git_ repo, _, commits = mock_git_package_changes spack.repo.PATH.put_first(repo) - monkeypatch.setattr(spack.repo, "packages_path", mock_packages_path(repo.packages_path)) + monkeypatch.setattr(spack.repo, "builtin_repo", lambda: repo) pkg_class = spack.spec.Spec("diff-test").package_class monkeypatch.setattr(pkg_class, "manual_download", True) diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py index a46824310cf..266ebc58330 100644 --- a/lib/spack/spack/test/cmd/env.py +++ b/lib/spack/spack/test/cmd/env.py @@ -1829,7 +1829,7 @@ def test_indirect_build_dep(tmp_path): build-only dep. Make sure this concrete DAG is preserved when writing the environment out and reading it back. """ - builder = spack.repo.MockRepositoryBuilder(tmp_path / "repo") + builder = spack.repo.MockRepositoryBuilder(tmp_path) builder.add_package("z") builder.add_package("y", dependencies=[("z", "build", None)]) builder.add_package("x", dependencies=[("y", None, None)]) @@ -1862,7 +1862,7 @@ def test_store_different_build_deps(tmp_path): z1 """ - builder = spack.repo.MockRepositoryBuilder(tmp_path / "mirror") + builder = spack.repo.MockRepositoryBuilder(tmp_path) builder.add_package("z") builder.add_package("y", dependencies=[("z", "build", None)]) builder.add_package("x", dependencies=[("y", None, None), ("z", "build", None)]) diff --git a/lib/spack/spack/test/cmd/pkg.py b/lib/spack/spack/test/cmd/pkg.py index df977c22952..3e939d8dc3b 100644 --- a/lib/spack/spack/test/cmd/pkg.py +++ b/lib/spack/spack/test/cmd/pkg.py @@ -111,12 +111,12 @@ def split(output): pkg = spack.main.SpackCommand("pkg") -def test_packages_path(): - assert spack.repo.packages_path() == spack.repo.PATH.get_repo("builtin").packages_path +def test_builtin_repo(): + assert spack.repo.builtin_repo() is spack.repo.PATH.get_repo("builtin") -def test_mock_packages_path(mock_packages): - assert spack.repo.packages_path() == spack.repo.PATH.get_repo("builtin.mock").packages_path +def test_mock_builtin_repo(mock_packages): + assert spack.repo.builtin_repo() is spack.repo.PATH.get_repo("builtin.mock") def test_pkg_add(git, mock_pkg_git_repo): diff --git a/lib/spack/spack/test/cmd/repo.py b/lib/spack/spack/test/cmd/repo.py index 86bb09bc81f..56abb8d668d 100644 --- a/lib/spack/spack/test/cmd/repo.py +++ b/lib/spack/spack/test/cmd/repo.py @@ -2,6 +2,7 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) import os +import pathlib import pytest @@ -22,19 +23,19 @@ def test_help_option(): assert repo.returncode in (None, 0) -def test_create_add_list_remove(mutable_config, tmpdir): +def test_create_add_list_remove(mutable_config, tmp_path: pathlib.Path): # Create a new repository and check that the expected # files are there - repo("create", str(tmpdir), "mockrepo") - assert os.path.exists(os.path.join(str(tmpdir), "repo.yaml")) + repo("create", str(tmp_path), "mockrepo") + assert (tmp_path / "spack_repo" / "mockrepo" / "repo.yaml").exists() # Add the new repository and check it appears in the list output - repo("add", "--scope=site", str(tmpdir)) + repo("add", "--scope=site", str(tmp_path / "spack_repo" / "mockrepo")) output = repo("list", "--scope=site", output=str) assert "mockrepo" in output # Then remove it and check it's not there - repo("remove", "--scope=site", str(tmpdir)) + repo("remove", "--scope=site", str(tmp_path / "spack_repo" / "mockrepo")) output = repo("list", "--scope=site", output=str) assert "mockrepo" not in output diff --git a/lib/spack/spack/test/concretization/core.py b/lib/spack/spack/test/concretization/core.py index 6d3b2f57a50..4ee0e25f672 100644 --- a/lib/spack/spack/test/concretization/core.py +++ b/lib/spack/spack/test/concretization/core.py @@ -1719,7 +1719,7 @@ def test_reuse_with_unknown_namespace_dont_raise( @pytest.mark.regression("45538") def test_reuse_from_other_namespace_no_raise(self, tmpdir, temporary_store, monkeypatch): - myrepo = spack.repo.MockRepositoryBuilder(tmpdir.mkdir("mock.repo"), namespace="myrepo") + myrepo = spack.repo.MockRepositoryBuilder(tmpdir, namespace="mock_repo") myrepo.add_package("zlib") builtin = spack.concretize.concretize_one("zlib") @@ -1727,21 +1727,19 @@ def test_reuse_from_other_namespace_no_raise(self, tmpdir, temporary_store, monk with spack.repo.use_repositories(myrepo.root, override=False): with spack.config.override("concretizer:reuse", True): - myrepo = spack.concretize.concretize_one("myrepo.zlib") + myrepo = spack.concretize.concretize_one("mock_repo.zlib") - assert myrepo.namespace == "myrepo" + assert myrepo.namespace == "mock_repo" @pytest.mark.regression("28259") def test_reuse_with_unknown_package_dont_raise(self, tmpdir, temporary_store, monkeypatch): - builder = spack.repo.MockRepositoryBuilder(tmpdir.mkdir("mock.repo"), namespace="myrepo") + builder = spack.repo.MockRepositoryBuilder(str(tmpdir), namespace="myrepo") builder.add_package("pkg-c") with spack.repo.use_repositories(builder.root, override=False): s = spack.concretize.concretize_one("pkg-c") assert s.namespace == "myrepo" PackageInstaller([s.package], fake=True, explicit=True).install() - - del sys.modules["spack.pkg.myrepo.pkg-c"] - del sys.modules["spack.pkg.myrepo"] + del sys.modules["spack_repo.myrepo.packages.pkg_c"] builder.remove("pkg-c") with spack.repo.use_repositories(builder.root, override=False) as repos: # TODO (INJECT CONFIGURATION): unclear why the cache needs to be invalidated explicitly diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index 3e8fc445328..cb7d3f082b1 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -259,9 +259,9 @@ def mock_git_package_changes(git, tmpdir, override_git_repos_cache_path, monkeyp Important attributes of the repo for test coverage are: multiple package versions are added with some coming from a tarball and some from git refs. """ - filename = "diff-test/package.py" + filename = "diff_test/package.py" - repo_path, _ = spack.repo.create_repo(str(tmpdir.mkdir("myrepo"))) + repo_path, _ = spack.repo.create_repo(str(tmpdir), namespace="myrepo") repo_cache = spack.util.file_cache.FileCache(str(tmpdir.mkdir("cache"))) repo = spack.repo.Repo(repo_path, cache=repo_cache) diff --git a/lib/spack/spack/test/package_class.py b/lib/spack/spack/test/package_class.py index 6d4cdcbfe29..5b2abefb34f 100644 --- a/lib/spack/spack/test/package_class.py +++ b/lib/spack/spack/test/package_class.py @@ -246,22 +246,29 @@ class BadDetectablePackage(spack.package.Package): def test_package_url_and_urls(): - class URLsPackage(spack.package.Package): - url = "https://www.example.com/url-package-1.0.tgz" - urls = ["https://www.example.com/archive"] + UrlsPackage = type( + "URLsPackage", + (spack.package.Package,), + { + "__module__": "spack.pkg.builtin.urls_package", + "url": "https://www.example.com/url-package-1.0.tgz", + "urls": ["https://www.example.com/archive"], + }, + ) - s = spack.spec.Spec("pkg-a") + s = spack.spec.Spec("urls-package") with pytest.raises(ValueError, match="defines both"): - URLsPackage(s) + UrlsPackage(s) def test_package_license(): - class LicensedPackage(spack.package.Package): - extendees = None # currently a required attribute for is_extension() - license_files = None + LicensedPackage = type( + "LicensedPackage", + (spack.package.Package,), + {"__module__": "spack.pkg.builtin.licensed_package"}, + ) - s = spack.spec.Spec("pkg-a") - pkg = LicensedPackage(s) + pkg = LicensedPackage(spack.spec.Spec("licensed-package")) assert pkg.global_license_file is None pkg.license_files = ["license.txt"] diff --git a/lib/spack/spack/test/packages.py b/lib/spack/spack/test/packages.py index 2912c689079..52b476e376f 100644 --- a/lib/spack/spack/test/packages.py +++ b/lib/spack/spack/test/packages.py @@ -15,7 +15,7 @@ import spack.repo from spack.paths import mock_packages_path from spack.spec import Spec -from spack.util.naming import mod_to_class +from spack.util.naming import pkg_name_to_class_name from spack.version import VersionChecksumError @@ -47,11 +47,15 @@ def test_nonexisting_package_filename(self): ) def test_package_class_names(self): - assert "Mpich" == mod_to_class("mpich") - assert "PmgrCollective" == mod_to_class("pmgr_collective") - assert "PmgrCollective" == mod_to_class("pmgr-collective") - assert "Pmgrcollective" == mod_to_class("PmgrCollective") - assert "_3db" == mod_to_class("3db") + assert "Mpich" == pkg_name_to_class_name("mpich") + assert "PmgrCollective" == pkg_name_to_class_name("pmgr_collective") + assert "PmgrCollective" == pkg_name_to_class_name("pmgr-collective") + assert "Pmgrcollective" == pkg_name_to_class_name("PmgrCollective") + assert "_3db" == pkg_name_to_class_name("3db") + assert "_True" == pkg_name_to_class_name("true") # reserved keyword + assert "_False" == pkg_name_to_class_name("false") # reserved keyword + assert "_None" == pkg_name_to_class_name("none") # reserved keyword + assert "Finally" == pkg_name_to_class_name("finally") # `Finally` is not reserved # Below tests target direct imports of spack packages from the # spack.pkg namespace @@ -333,3 +337,16 @@ def test_package_can_have_sparse_checkout_properties(mock_packages, mock_fetch, assert isinstance(fetcher, spack.fetch_strategy.GitFetchStrategy) assert hasattr(fetcher, "git_sparse_paths") assert fetcher.git_sparse_paths == pkg_cls.git_sparse_paths + + +def test_pkg_name_can_only_be_derived_when_package_module(): + """When the module prefix is not spack_repo (or legacy spack.pkg) we cannot derive + a package name.""" + ExamplePackage = type( + "ExamplePackage", + (spack.package_base.PackageBase,), + {"__module__": "not.a.spack.repo.packages.example_package.package"}, + ) + + with pytest.raises(ValueError, match="Package ExamplePackage is not a known Spack package"): + ExamplePackage.name diff --git a/lib/spack/spack/test/repo.py b/lib/spack/spack/test/repo.py index 506bef62a71..133c482e52d 100644 --- a/lib/spack/spack/test/repo.py +++ b/lib/spack/spack/test/repo.py @@ -6,11 +6,14 @@ import pytest +import spack import spack.package_base import spack.paths import spack.repo import spack.spec import spack.util.file_cache +import spack.util.naming +from spack.util.naming import valid_module_name @pytest.fixture(params=["packages", "", "foo"]) @@ -184,12 +187,12 @@ def _repo_paths(repos): repo_paths.append(spack.paths.mock_packages_path) namespaces.append("builtin.mock") if entry == "extra": - name = "extra.mock" + name = "extra_mock" repo_dir = tmp_path / name repo_dir.mkdir() - _ = spack.repo.MockRepositoryBuilder(repo_dir, name) - repo_paths.append(str(repo_dir)) - namespaces.append(name) + repo = spack.repo.MockRepositoryBuilder(repo_dir, name) + repo_paths.append(repo.root) + namespaces.append(repo.namespace) return repo_paths, namespaces repo_paths, namespaces = _repo_paths(repos) @@ -310,7 +313,7 @@ def test_creation_from_string(self, mock_test_cache): repo = spack.repo.RepoPath(spack.paths.mock_packages_path, cache=mock_test_cache) assert len(repo.repos) == 1 assert repo.repos[0]._finder is repo - assert repo.by_namespace["spack.pkg.builtin.mock"] is repo.repos[0] + assert repo.by_namespace["builtin.mock"] is repo.repos[0] def test_get_repo(self, mock_test_cache): repo = spack.repo.RepoPath(spack.paths.mock_packages_path, cache=mock_test_cache) @@ -364,3 +367,160 @@ def test_repo_package_api_version(tmp_path: pathlib.Path): ) cache = spack.util.file_cache.FileCache(tmp_path / "cache") assert spack.repo.Repo(str(tmp_path / "example"), cache=cache).package_api == (1, 0) + + +def test_mod_to_pkg_name_and_reverse(): + # In repo v1 the dirname/module name is the package name + assert spack.util.naming.pkg_dir_to_pkg_name("zlib_ng", package_api=(1, 0)) == "zlib_ng" + assert ( + spack.util.naming.pkg_dir_to_pkg_name("_3example_4", package_api=(1, 0)) == "_3example_4" + ) + assert spack.util.naming.pkg_name_to_pkg_dir("zlib_ng", package_api=(1, 0)) == "zlib_ng" + assert ( + spack.util.naming.pkg_name_to_pkg_dir("_3example_4", package_api=(1, 0)) == "_3example_4" + ) + + # In repo v2 there is a 1-1 mapping between module and package names + assert spack.util.naming.pkg_dir_to_pkg_name("_3example_4", package_api=(2, 0)) == "3example-4" + assert spack.util.naming.pkg_dir_to_pkg_name("zlib_ng", package_api=(2, 0)) == "zlib-ng" + assert spack.util.naming.pkg_name_to_pkg_dir("zlib-ng", package_api=(2, 0)) == "zlib_ng" + assert spack.util.naming.pkg_name_to_pkg_dir("3example-4", package_api=(2, 0)) == "_3example_4" + + # reserved names need an underscore + assert spack.util.naming.pkg_dir_to_pkg_name("_finally", package_api=(2, 0)) == "finally" + assert spack.util.naming.pkg_dir_to_pkg_name("_assert", package_api=(2, 0)) == "assert" + assert spack.util.naming.pkg_name_to_pkg_dir("finally", package_api=(2, 0)) == "_finally" + assert spack.util.naming.pkg_name_to_pkg_dir("assert", package_api=(2, 0)) == "_assert" + + # reserved names are case sensitive, so true/false/none are ok + assert spack.util.naming.pkg_dir_to_pkg_name("true", package_api=(2, 0)) == "true" + assert spack.util.naming.pkg_dir_to_pkg_name("none", package_api=(2, 0)) == "none" + assert spack.util.naming.pkg_name_to_pkg_dir("true", package_api=(2, 0)) == "true" + assert spack.util.naming.pkg_name_to_pkg_dir("none", package_api=(2, 0)) == "none" + + +def test_repo_v2_invalid_module_name(tmp_path: pathlib.Path, capsys): + # Create a repo with a v2 structure + root, _ = spack.repo.create_repo(str(tmp_path), namespace="repo_1", package_api=(2, 0)) + repo_dir = pathlib.Path(root) + + # Create two invalid module names + (repo_dir / "packages" / "zlib-ng").mkdir() + (repo_dir / "packages" / "zlib-ng" / "package.py").write_text( + """ +from spack.package import Package + +class ZlibNg(Package): + pass +""" + ) + (repo_dir / "packages" / "UPPERCASE").mkdir() + (repo_dir / "packages" / "UPPERCASE" / "package.py").write_text( + """ +from spack.package import Package + +class Uppercase(Package): + pass +""" + ) + + with spack.repo.use_repositories(str(repo_dir)) as repo: + assert len(repo.all_package_names()) == 0 + + stderr = capsys.readouterr().err + assert "cannot be used because `zlib-ng` is not a valid Spack package module name" in stderr + assert "cannot be used because `UPPERCASE` is not a valid Spack package module name" in stderr + + +def test_repo_v2_module_and_class_to_package_name(tmp_path: pathlib.Path, capsys): + # Create a repo with a v2 structure + root, _ = spack.repo.create_repo(str(tmp_path), namespace="repo_2", package_api=(2, 0)) + repo_dir = pathlib.Path(root) + + # Create an invalid module name + (repo_dir / "packages" / "_1example_2_test").mkdir() + (repo_dir / "packages" / "_1example_2_test" / "package.py").write_text( + """ +from spack.package import Package + +class _1example2Test(Package): + pass +""" + ) + + with spack.repo.use_repositories(str(repo_dir)) as repo: + assert repo.exists("1example-2-test") + pkg_cls = repo.get_pkg_class("1example-2-test") + assert pkg_cls.name == "1example-2-test" + assert pkg_cls.module.__name__ == "spack_repo.repo_2.packages._1example_2_test.package" + + +def test_valid_module_name_v2(): + api = (2, 0) + + # no hyphens + assert not valid_module_name("zlib-ng", api) + + # cannot start with a number + assert not valid_module_name("7zip", api) + + # no consecutive underscores + assert not valid_module_name("zlib__ng", api) + + # reserved names + assert not valid_module_name("finally", api) + assert not valid_module_name("assert", api) + + # cannot contain uppercase + assert not valid_module_name("False", api) + assert not valid_module_name("zlib_NG", api) + + # reserved names are allowed when preceded by underscore + assert valid_module_name("_finally", api) + assert valid_module_name("_assert", api) + + # digits are allowed when preceded by underscore + assert valid_module_name("_1example_2_test", api) + + # underscore is not allowed unless followed by reserved name or digit + assert not valid_module_name("_zlib", api) + assert not valid_module_name("_false", api) + + +def test_namespace_is_optional_in_v2(tmp_path: pathlib.Path): + """Test that a repo without a namespace is valid in v2.""" + repo_yaml_dir = tmp_path / "spack_repo" / "foo" / "bar" / "baz" + (repo_yaml_dir / "packages").mkdir(parents=True) + (repo_yaml_dir / "repo.yaml").write_text( + """\ +repo: + api: v2.0 +""" + ) + + cache = spack.util.file_cache.FileCache(tmp_path / "cache") + repo = spack.repo.Repo(str(repo_yaml_dir), cache=cache) + + assert repo.namespace == "foo.bar.baz" + assert repo.full_namespace == "spack_repo.foo.bar.baz.packages" + assert repo.root == str(repo_yaml_dir) + assert repo.packages_path == str(repo_yaml_dir / "packages") + assert repo.python_path == str(tmp_path) + assert repo.package_api == (2, 0) + + +def test_subdir_in_v2(): + """subdir cannot be . or empty in v2, because otherwise we cannot statically distinguish + between namespace and subdir.""" + with pytest.raises(spack.repo.BadRepoError, match="Use a symlink packages -> . instead"): + spack.repo._validate_and_normalize_subdir(subdir="", root="root", package_api=(2, 0)) + + with pytest.raises(spack.repo.BadRepoError, match="Use a symlink packages -> . instead"): + spack.repo._validate_and_normalize_subdir(subdir=".", root="root", package_api=(2, 0)) + + with pytest.raises(spack.repo.BadRepoError, match="Expected a directory name, not a path"): + subdir = os.path.join("a", "b") + spack.repo._validate_and_normalize_subdir(subdir=subdir, root="root", package_api=(2, 0)) + + with pytest.raises(spack.repo.BadRepoError, match="Must be a valid Python module name"): + spack.repo._validate_and_normalize_subdir(subdir="123", root="root", package_api=(2, 0)) diff --git a/lib/spack/spack/util/naming.py b/lib/spack/spack/util/naming.py index 21b8f7e3c5b..cd8aa9ecfab 100644 --- a/lib/spack/spack/util/naming.py +++ b/lib/spack/spack/util/naming.py @@ -6,72 +6,107 @@ import itertools import re import string - -import spack.error +from typing import List, Tuple __all__ = [ - "mod_to_class", - "spack_module_to_python_module", + "pkg_name_to_class_name", "valid_module_name", - "valid_fully_qualified_module_name", - "validate_fully_qualified_module_name", - "validate_module_name", "possible_spack_module_names", "simplify_name", "NamespaceTrie", ] -# Valid module names can contain '-' but can't start with it. -_valid_module_re = r"^\w[\w-]*$" +#: see keyword.kwlist: https://github.com/python/cpython/blob/main/Lib/keyword.py +RESERVED_NAMES_ONLY_LOWERCASE = frozenset( + ( + "and", + "as", + "assert", + "async", + "await", + "break", + "class", + "continue", + "def", + "del", + "elif", + "else", + "except", + "finally", + "for", + "from", + "global", + "if", + "import", + "in", + "is", + "lambda", + "nonlocal", + "not", + "or", + "pass", + "raise", + "return", + "try", + "while", + "with", + "yield", + ) +) + +RESERVED_NAMES_LIST_MIXED_CASE = ("False", "None", "True") # Valid module names can contain '-' but can't start with it. -_valid_fully_qualified_module_re = r"^(\w[\w-]*)(\.\w[\w-]*)*$" +_VALID_MODULE_RE_V1 = re.compile(r"^\w[\w-]*$") + +_VALID_MODULE_RE_V2 = re.compile(r"^[a-z_][a-z0-9_]*$") -def mod_to_class(mod_name): - """Convert a name from module style to class name style. Spack mostly - follows `PEP-8 `_: +def pkg_name_to_class_name(pkg_name: str): + """Convert a Spack package name to a class name, based on + `PEP-8 `_: * Module and package names use lowercase_with_underscores. * Class names use the CapWords convention. - Regular source code follows these convetions. Spack is a bit - more liberal with its Package names and Compiler names: + Not all package names are valid Python identifiers: - * They can contain '-' as well as '_', but cannot start with '-'. + * They can contain '-', but cannot start with '-'. * They can start with numbers, e.g. "3proxy". - This function converts from the module convention to the class - convention by removing _ and - and converting surrounding - lowercase text to CapWords. If mod_name starts with a number, - the class name returned will be prepended with '_' to make a - valid Python identifier. + This function converts from the package name to the class convention by removing _ and - and + converting surrounding lowercase text to CapWords. If package name starts with a number, the + class name returned will be prepended with '_' to make a valid Python identifier. """ - validate_module_name(mod_name) - - class_name = re.sub(r"[-_]+", "-", mod_name) + class_name = re.sub(r"[-_]+", "-", pkg_name) class_name = string.capwords(class_name, "-") class_name = class_name.replace("-", "") - # If a class starts with a number, prefix it with Number_ to make it - # a valid Python class name. - if re.match(r"^[0-9]", class_name): - class_name = "_%s" % class_name + # Ensure that the class name is a valid Python identifier + if re.match(r"^[0-9]", class_name) or class_name in RESERVED_NAMES_LIST_MIXED_CASE: + class_name = f"_{class_name}" return class_name -def spack_module_to_python_module(mod_name): - """Given a Spack module name, returns the name by which it can be - imported in Python. - """ - if re.match(r"[0-9]", mod_name): - mod_name = "num" + mod_name - - return mod_name.replace("-", "_") +def pkg_dir_to_pkg_name(dirname: str, package_api: Tuple[int, int]) -> str: + """Translate a package dir (pkg_dir/package.py) to its corresponding package name""" + if package_api < (2, 0): + return dirname + return dirname.lstrip("_").replace("_", "-") -def possible_spack_module_names(python_mod_name): +def pkg_name_to_pkg_dir(name: str, package_api: Tuple[int, int]) -> str: + """Translate a package name to its corresponding package dir (pkg_dir/package.py)""" + if package_api < (2, 0): + return name + name = name.replace("-", "_") + if re.match(r"^[0-9]", name) or name in RESERVED_NAMES_ONLY_LOWERCASE: + name = f"_{name}" + return name + + +def possible_spack_module_names(python_mod_name: str) -> List[str]: """Given a Python module name, return a list of all possible spack module names that could correspond to it.""" mod_name = re.sub(r"^num(\d)", r"\1", python_mod_name) @@ -79,7 +114,7 @@ def possible_spack_module_names(python_mod_name): parts = re.split(r"(_)", mod_name) options = [["_", "-"]] * mod_name.count("_") - results = [] + results: List[str] = [] for subs in itertools.product(*options): s = list(parts) s[1::2] = subs @@ -88,7 +123,7 @@ def possible_spack_module_names(python_mod_name): return results -def simplify_name(name): +def simplify_name(name: str) -> str: """Simplify package name to only lowercase, digits, and dashes. Simplifies a name which may include uppercase letters, periods, @@ -136,42 +171,17 @@ def simplify_name(name): return name -def valid_module_name(mod_name): +def valid_module_name(mod_name: str, package_api: Tuple[int, int]) -> bool: """Return whether mod_name is valid for use in Spack.""" - return bool(re.match(_valid_module_re, mod_name)) - - -def valid_fully_qualified_module_name(mod_name): - """Return whether mod_name is a valid namespaced module name.""" - return bool(re.match(_valid_fully_qualified_module_re, mod_name)) - - -def validate_module_name(mod_name): - """Raise an exception if mod_name is not valid.""" - if not valid_module_name(mod_name): - raise InvalidModuleNameError(mod_name) - - -def validate_fully_qualified_module_name(mod_name): - """Raise an exception if mod_name is not a valid namespaced module name.""" - if not valid_fully_qualified_module_name(mod_name): - raise InvalidFullyQualifiedModuleNameError(mod_name) - - -class InvalidModuleNameError(spack.error.SpackError): - """Raised when we encounter a bad module name.""" - - def __init__(self, name): - super().__init__("Invalid module name: " + name) - self.name = name - - -class InvalidFullyQualifiedModuleNameError(spack.error.SpackError): - """Raised when we encounter a bad full package name.""" - - def __init__(self, name): - super().__init__("Invalid fully qualified package name: " + name) - self.name = name + if package_api < (2, 0): + return bool(_VALID_MODULE_RE_V1.match(mod_name)) + elif not _VALID_MODULE_RE_V2.match(mod_name) or "__" in mod_name: + return False + elif mod_name.startswith("_"): + # it can only start with an underscore if followed by digit or reserved name + return mod_name[1:] in RESERVED_NAMES_ONLY_LOWERCASE or mod_name[1].isdigit() + else: + return mod_name not in RESERVED_NAMES_ONLY_LOWERCASE class NamespaceTrie: