From a43807647328819c7e3b5281760ded78e282c7f1 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Thu, 19 Jan 2023 16:56:24 -0600 Subject: [PATCH] Add a new `disinherit` directive for extending packages When you inherit from a package in Spack, you inherit all the metadata from its directives, including `version`, `provides`, `depends_on`, `conflicts`, etc. In some cases, you may not want this metadata. This PR adds a new `disinherit` directive that allows you to throw out things inherited from the base class. For example: ```python from spack.pkg.builtin.mpich import Mpich class MyMpich(Mpich): disinherit("versions") # don't inherit any versions from builtin Mpich version("5.0", "08721a102fefcea2ae4add8c9cc548df77e9224f5385ad0872a9150fdd26a415") version("6.0", "9cc39dd33dd4227bb82301d285437588d705290846d22ab6b8791c7e631ce385") ``` Without the `disinherit("versions")` directive, this package would have versions `5.0`, `6.0`, *and* anything inherited from `Mpich`. With it, this package has only versions `5.0` and `6.0`. You can `disinherit` any of: `conflicts`, `dependencies`, `extendees`, `patches`, `provided`, `resources`, `variants`, or `versions`. - [x] add new `disinherit directive` - [x] Two packages were modifying their `versions` dictionary in their constructors to achieve this, but this causes `spack url stats` to fail with a concurrent modification exception as it iterates over all packages. Fixed these to use `disinherit` instead. - [x] Update documentation - [x] Add tests --- lib/spack/docs/packaging_guide.rst | 66 ++++++++++++++++++- lib/spack/docs/repositories.rst | 35 +--------- lib/spack/spack/directives.py | 40 +++++++++-- lib/spack/spack/test/directives.py | 18 +++++ .../packages/disinherit/package.py | 15 +++++ .../gromacs-chain-coordinate/package.py | 18 +---- .../builtin/packages/gromacs-swaxs/package.py | 16 +---- 7 files changed, 137 insertions(+), 71 deletions(-) create mode 100644 var/spack/repos/builtin.mock/packages/disinherit/package.py diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst index 45f1533e66a..21b9b0230b8 100644 --- a/lib/spack/docs/packaging_guide.rst +++ b/lib/spack/docs/packaging_guide.rst @@ -2410,7 +2410,7 @@ package, and a `canonical hash `_ of the ``package.py`` recipes). ``test`` dependencies do not affect the package hash, as they are only used to construct a test environment *after* building and installing a given package installation. Older versions of Spack did not include -build dependencies in the hash, but this has been +build dependencies in the hash, but this has been `fixed `_ as of |Spack v0.18|_. .. |Spack v0.18| replace:: Spack ``v0.18`` @@ -3604,6 +3604,70 @@ In the example above ``Cp2k`` inherits all the conflicts and variants that ``Cud .. _install-environment: +-------------------------------- +Package Inheritance +-------------------------------- + +Spack packages are Python classes, and you can use inheritance with them just as you can +with any Python class. This is common when you have your own package :ref:`repository +` with packages that extend Spack's ``builtin`` packages. + +You can extend a ``builtin`` package like this: + +.. code-block:: python + + from spack.pkg.builtin.mpich import Mpich + + class MyPackage(Mpich): + version("1.0", "0209444070d9c8af9b62c94095a217e3bc6843692d1e3fdc1ff5371e03aac47c") + version("2.0", "5dda192154047d6296ba14a4ab2d869c6926fd7f44dce8ce94f63aae2e359c5b") + +Every repository registered with Spack ends up in a submodule of ``spack.pkg`` with a +name corresponding to its :ref:`namespace `. So, if you have a different +repository with namespace ``myrepo`` you want to import packages from , you might write: + +.. code-block:: python + + from spack.pkg.myrepo.my_package import MyPackage + + class NewPackage(MyPackage): + version("3.0", "08721a102fefcea2ae4add8c9cc548df77e9224f5385ad0872a9150fdd26a415") + version("4.0", "9cc39dd33dd4227bb82301d285437588d705290846d22ab6b8791c7e631ce385") + +^^^^^^^^^^^^^^^^^^^^^^^^ +``disinherit`` +^^^^^^^^^^^^^^^^^^^^^^^^ + +When you inherit from a package in Spack, you inherit all the metadata from its +directives, including ``version``, ``provides``, ``depends_on``, ``conflicts``, etc. For +example, ``NewPackage`` above will have four versions: ``1.0`` and ``2.0`` inherited +from ``MyPackage``, as well as, ``3.0``, and ``4.0`` defined in ``NewPackage``. + +If you do not want your package to define all the same things as its base class, you can +use the ``disinherit`` directive to start fresh in your subclass: + +.. code-block:: python + + from spack.pkg.myrepo.my_package import MyPackage + + class NewerPackage(MyPackage): + disinherit("versions") # don't inherit any versions from MyPackage + version("5.0", "08721a102fefcea2ae4add8c9cc548df77e9224f5385ad0872a9150fdd26a415") + version("6.0", "9cc39dd33dd4227bb82301d285437588d705290846d22ab6b8791c7e631ce385") + +Now, ``NewerPackage`` will have **only** versions ``5.0`` and ``6.0``, and will not +inherit ``1.0`` or ``2.0`` from ``MyPackage``. You can ``disinherit`` many different +properties from base packages. The full list of options is: + +* ``conflicts`` +* ``dependencies`` +* ``extendees`` +* ``patches`` +* ``provided`` +* ``resources`` +* ``variants`` +* ``versions`` + ----------------------- The build environment ----------------------- diff --git a/lib/spack/docs/repositories.rst b/lib/spack/docs/repositories.rst index 3691bdfebb6..8ce5d426139 100644 --- a/lib/spack/docs/repositories.rst +++ b/lib/spack/docs/repositories.rst @@ -91,6 +91,8 @@ packages and use the first valid file: to eventually support URLs in ``repos.yaml``, so that you can easily point to remote package repositories, but that is not yet implemented. +.. _namespaces: + --------------------- Namespaces --------------------- @@ -426,36 +428,3 @@ By path: $ spack repo list ==> 1 package repository. builtin ~/spack/var/spack/repos/builtin - --------------------------------- -Repo namespaces and Python --------------------------------- - -You may have noticed that namespace notation for repositories is similar -to the notation for namespaces in Python. As it turns out, you *can* -treat Spack repositories like Python packages; this is how they are -implemented. - -You could, for example, extend a ``builtin`` package in your own -repository: - -.. code-block:: python - - from spack.pkg.builtin.mpich import Mpich - - class MyPackage(Mpich): - ... - -Spack repo namespaces are actually Python namespaces tacked on under -``spack.pkg``. The search semantics of ``repos.yaml`` are actually -implemented using Python's built-in `sys.path -`_ search. The -:py:mod:`spack.repo` module implements a custom `Python importer -`_. - -.. warning:: - - The mechanism for extending packages is not yet extensively tested, - and extending packages across repositories imposes inter-repo - dependencies, which may be hard to manage. Use this feature at your - own risk, but let us know if you have a use case for it. diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py index 6e9dd819bbd..e7b2a656437 100644 --- a/lib/spack/spack/directives.py +++ b/lib/spack/spack/directives.py @@ -53,6 +53,7 @@ class OpenMpi(Package): "version", "conflicts", "depends_on", + "disinherit", "extends", "provides", "patch", @@ -235,12 +236,12 @@ class Foo(Package): 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))) + if dicts is not None: + if not isinstance(dicts, collections.abc.Sequence): + raise TypeError(f"dicts arg must be list, tuple, or string. Found {type(dicts)}") - # Add the dictionary names if not already there - DirectiveMeta._directive_dict_names |= set(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): @@ -767,6 +768,35 @@ def build_system(*values, **kwargs): ) +@directive() +def disinherit(dict_name: str): + """Clear all values in a dict inherited from base packages. + + You can use this to, e.g., remove all inherited versions from a base package: + + disinherit("versions") # this package doesn't share any versions w/parents + version("2.0", ...) # new versions specific to this package + version("3.0", ...) + + The dictionary name is checked, so you can't call this on somethign that's not a + valid directive dictonary. + + Arguments: + dict_name: name of directive dictionary to clear. + + """ + if dict_name not in DirectiveMeta._directive_dict_names: + names = ", ".join(DirectiveMeta._directive_dict_names) + raise DirectiveError(f"Can't disinherit '{dict_name}'. Options are: {names}") + + def _execute_disinherit(pkg): + dictionary = getattr(pkg, dict_name, None) + if dictionary: + dictionary.clear() + + return _execute_disinherit + + class DirectiveError(spack.error.SpackError): """This is raised when something is wrong with a package directive.""" diff --git a/lib/spack/spack/test/directives.py b/lib/spack/spack/test/directives.py index d1fc31d09b6..1d5e5b351cb 100644 --- a/lib/spack/spack/test/directives.py +++ b/lib/spack/spack/test/directives.py @@ -68,3 +68,21 @@ def test_error_on_anonymous_dependency(config, mock_packages): pkg = spack.repo.path.get_pkg_class("a") with pytest.raises(spack.directives.DependencyError): spack.directives._depends_on(pkg, "@4.5") + + +def test_disinherited_version(config, mock_packages): + pkg = spack.repo.path.get_pkg_class("disinherit") + + # these would be inherited from A if disinherit failed + assert "1.0" not in pkg.versions + assert "2.0" not in pkg.versions + + # these are defined in dininherit + assert "3.0" not in pkg.versions + assert "4.0" not in pkg.versions + + +def test_disinherit(config, mock_packages): + with pytest.raises(spack.directives.DirectiveError): + # This is not a valid thing to disinherit + spack.directives.disinherit("foobarbaz") diff --git a/var/spack/repos/builtin.mock/packages/disinherit/package.py b/var/spack/repos/builtin.mock/packages/disinherit/package.py new file mode 100644 index 00000000000..fd32f93631d --- /dev/null +++ b/var/spack/repos/builtin.mock/packages/disinherit/package.py @@ -0,0 +1,15 @@ +# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other +# Spack Project Developers. See the top-level COPYRIGHT file for details. +# +# SPDX-License-Identifier: (Apache-2.0 OR MIT) + +from spack.package import * +from spack.pkg.builtin.mock.a import A + + +class Disinherit(A): + """Disinherit from A and add our own versions.""" + + disinherit("versions") + version("4.0", "abcdef0123456789abcdef0123456789") + version("3.0", "0123456789abcdef0123456789abcdef") diff --git a/var/spack/repos/builtin/packages/gromacs-chain-coordinate/package.py b/var/spack/repos/builtin/packages/gromacs-chain-coordinate/package.py index 8efdf457c13..469989d9853 100644 --- a/var/spack/repos/builtin/packages/gromacs-chain-coordinate/package.py +++ b/var/spack/repos/builtin/packages/gromacs-chain-coordinate/package.py @@ -18,14 +18,13 @@ class GromacsChainCoordinate(Gromacs): git = "https://gitlab.com/cbjh/gromacs-chain-coordinate.git" maintainers = ["w8jcik"] + disinherit("versions") version("main", branch="main") - version( "2021.5-0.2", sha256="33dda1e39cd47c5ae32b5455af8534225d3888fd7e4968f499b8483620fa770a", url="https://gitlab.com/cbjh/gromacs-chain-coordinate/-/archive/release-2021.chaincoord-0.2/gromacs-chain-coordinate-release-2021.chaincoord-0.2.tar.bz2", ) - version( "2021.2-0.1", sha256="879fdd04662370a76408b72c9fbc4aff60a6387b459322ac2700d27359d0dd87", @@ -34,21 +33,6 @@ class GromacsChainCoordinate(Gromacs): conflicts("+plumed") - def remove_parent_versions(self): - """ - By inheriting GROMACS package we also inherit versions. - They are not valid, so we are removing them. - """ - - for version_key in Gromacs.versions.keys(): - if version_key in self.versions: - del self.versions[version_key] - - def __init__(self, spec): - super(GromacsChainCoordinate, self).__init__(spec) - - self.remove_parent_versions() - def check(self): """The default 'test' targets does not compile the test programs""" with working_dir(self.build_directory): diff --git a/var/spack/repos/builtin/packages/gromacs-swaxs/package.py b/var/spack/repos/builtin/packages/gromacs-swaxs/package.py index f8074430863..c745fefb6c1 100644 --- a/var/spack/repos/builtin/packages/gromacs-swaxs/package.py +++ b/var/spack/repos/builtin/packages/gromacs-swaxs/package.py @@ -15,6 +15,7 @@ class GromacsSwaxs(Gromacs): git = "https://gitlab.com/cbjh/gromacs-swaxs.git" maintainers = ["w8jcik"] + disinherit("versions") version( "2021.5-0.4", sha256="9f8ed6d448a04789d45e847cbbc706a07130377f578388220a9d5357fae9d1c3", @@ -136,18 +137,3 @@ class GromacsSwaxs(Gromacs): conflicts("+plumed") conflicts("+opencl") conflicts("+sycl") - - def remove_parent_versions(self): - """ - By inheriting GROMACS package we also inherit versions. - They are not valid, so we are removing them. - """ - - for version_key in Gromacs.versions.keys(): - if version_key in self.versions: - del self.versions[version_key] - - def __init__(self, spec): - super(GromacsSwaxs, self).__init__(spec) - - self.remove_parent_versions()