Add a maintainers directive (#35083)

fixes #34879

This commit adds a new maintainer directive,
which by default extend the list of maintainers
for a given package.

The directive is backward compatible with the current
practice of having a "maintainers" list declared at
the class level.

lib/spack/docs/features.rst

@@ -116,7 +116,7 @@ creates a simple python file:
 
        # FIXME: Add a list of GitHub accounts to
        # notify when the package is updated.
-       # maintainers = ["github_user1", "github_user2"]
+       # maintainers("github_user1", "github_user2")
 
        version("0.8.13", sha256="591a9b4ec81c1f2042a97aa60564e0cb79d041c52faa7416acb38bc95bd2c76d")
 

lib/spack/docs/packaging_guide.rst

@@ -268,7 +268,7 @@ generates a boilerplate template for your package, and opens up the new
 
        # FIXME: Add a list of GitHub accounts to
        # notify when the package is updated.
-       # maintainers = ["github_user1", "github_user2"]
+       # maintainers("github_user1", "github_user2")
 
        version("6.2.1", sha256="eae9326beb4158c386e39a356818031bd28f3124cf915f8c5b1dc4c7a36b4d7c")
 
@@ -319,14 +319,8 @@ The rest of the tasks you need to do are as follows:
 
 #. Add a comma-separated list of maintainers.
 
-   The ``maintainers`` field is a list of GitHub accounts of people
+   Add a list of Github accounts of people who want to be notified
-   who want to be notified any time the package is modified. When a
+   any time the package is modified. See :ref:`package_maintainers`.
-   pull request is submitted that updates the package, these people
-   will be requested to review the PR. This is useful for developers
-   who maintain a Spack package for their own software, as well as
-   users who rely on a piece of software and want to ensure that the
-   package doesn't break. It also gives users a list of people to
-   contact for help when someone reports a build error with the package.
 
 #. Add ``depends_on()`` calls for the package's dependencies.
 
@@ -497,6 +491,31 @@ some examples:
 In general, you won't have to remember this naming convention because
 :ref:`cmd-spack-create` and :ref:`cmd-spack-edit` handle the details for you.
 
+.. _package_maintainers:
+
+-----------
+Maintainers
+-----------
+
+Each package in Spack may have one or more maintainers, i.e. one or more
+GitHub accounts of people who want to be notified any time the package is
+modified.
+
+When a pull request is submitted that updates the package, these people will
+be requested to review the PR. This is useful for developers who maintain a
+Spack package for their own software, as well as users who rely on a piece of
+software and want to ensure that the package doesn't break. It also gives users
+a list of people to contact for help when someone reports a build error with
+the package.
+
+To add maintainers to a package, simply declare them with the ``maintainers`` directive:
+
+.. code-block:: python
+
+   maintainers("user1", "user2")
+
+The list of maintainers is additive, and includes all the accounts eventually declared in base classes.
+
 -----------------
 Trusted Downloads
 -----------------

lib/spack/spack/cmd/create.py

@@ -70,7 +70,7 @@ class {class_name}({base_class_name}):
 
     # FIXME: Add a list of GitHub accounts to
     # notify when the package is updated.
-    # maintainers = ["github_user1", "github_user2"]
+    # maintainers("github_user1", "github_user2")
 
 {versions}
 

lib/spack/spack/directives.py

@@ -54,6 +54,7 @@ class OpenMpi(Package):
     "conflicts",
     "depends_on",
     "extends",
+    "maintainers",
     "provides",
     "patch",
     "variant",
@@ -767,6 +768,22 @@ def build_system(*values, **kwargs):
     )
 
 
+@directive(dicts=())
+def maintainers(*names: str):
+    """Add a new maintainer directive, to specify maintainers in a declarative way.
+
+    Args:
+        names: GitHub username for the maintainer
+    """
+
+    def _execute_maintainer(pkg):
+        maintainers_from_base = getattr(pkg, "maintainers", [])
+        # Here it is essential to copy, otherwise we might add to an empty list in the parent
+        pkg.maintainers = list(sorted(set(maintainers_from_base + list(names))))
+
+    return _execute_maintainer
+
+
 class DirectiveError(spack.error.SpackError):
     """This is raised when something is wrong with a package directive."""
 

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

@@ -14,6 +14,8 @@
 
 maintainers = spack.main.SpackCommand("maintainers")
 
+MAINTAINED_PACKAGES = ["maintainers-1", "maintainers-2", "maintainers-3", "py-extension1"]
+
 
 def split(output):
     """Split command line output into an array."""
@@ -23,14 +25,12 @@ def split(output):
 
 def test_maintained(mock_packages):
     out = split(maintainers("--maintained"))
-    assert out == ["maintainers-1", "maintainers-2"]
+    assert out == MAINTAINED_PACKAGES
 
 
 def test_unmaintained(mock_packages):
     out = split(maintainers("--unmaintained"))
-    assert out == sorted(
+    assert out == sorted(set(spack.repo.all_package_names()) - set(MAINTAINED_PACKAGES))
-        set(spack.repo.all_package_names()) - set(["maintainers-1", "maintainers-2"])
-    )
 
 
 def test_all(mock_packages, capfd):
@@ -43,6 +43,14 @@ def test_all(mock_packages, capfd):
         "maintainers-2:",
         "user2,",
         "user3",
+        "maintainers-3:",
+        "user0,",
+        "user1,",
+        "user2,",
+        "user3",
+        "py-extension1:",
+        "user1,",
+        "user2",
     ]
 
     with capfd.disabled():
@@ -58,23 +66,34 @@ def test_all_by_user(mock_packages, capfd):
     with capfd.disabled():
         out = split(maintainers("--all", "--by-user"))
     assert out == [
+        "user0:",
+        "maintainers-3",
         "user1:",
-        "maintainers-1",
+        "maintainers-1,",
+        "maintainers-3,",
+        "py-extension1",
         "user2:",
         "maintainers-1,",
-        "maintainers-2",
+        "maintainers-2,",
+        "maintainers-3,",
+        "py-extension1",
         "user3:",
-        "maintainers-2",
+        "maintainers-2,",
+        "maintainers-3",
     ]
 
     with capfd.disabled():
         out = split(maintainers("--all", "--by-user", "user1", "user2"))
     assert out == [
         "user1:",
-        "maintainers-1",
+        "maintainers-1,",
+        "maintainers-3,",
+        "py-extension1",
         "user2:",
         "maintainers-1,",
-        "maintainers-2",
+        "maintainers-2,",
+        "maintainers-3,",
+        "py-extension1",
     ]
 
 
@@ -116,16 +135,16 @@ def test_maintainers_list_fails(mock_packages, capfd):
 def test_maintainers_list_by_user(mock_packages, capfd):
     with capfd.disabled():
         out = split(maintainers("--by-user", "user1"))
-    assert out == ["maintainers-1"]
+    assert out == ["maintainers-1", "maintainers-3", "py-extension1"]
 
     with capfd.disabled():
         out = split(maintainers("--by-user", "user1", "user2"))
-    assert out == ["maintainers-1", "maintainers-2"]
+    assert out == ["maintainers-1", "maintainers-2", "maintainers-3", "py-extension1"]
 
     with capfd.disabled():
         out = split(maintainers("--by-user", "user2"))
-    assert out == ["maintainers-1", "maintainers-2"]
+    assert out == ["maintainers-1", "maintainers-2", "maintainers-3", "py-extension1"]
 
     with capfd.disabled():
         out = split(maintainers("--by-user", "user3"))
-    assert out == ["maintainers-2"]
+    assert out == ["maintainers-2", "maintainers-3"]

lib/spack/spack/test/directives.py

@@ -68,3 +68,19 @@ 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")
+
+
+@pytest.mark.regression("34879")
+@pytest.mark.parametrize(
+    "package_name,expected_maintainers",
+    [
+        ("maintainers-1", ["user1", "user2"]),
+        # Reset from PythonPackage
+        ("py-extension1", ["user1", "user2"]),
+        # Extends maintainers-1
+        ("maintainers-3", ["user0", "user1", "user2", "user3"]),
+    ],
+)
+def test_maintainer_directive(config, mock_packages, package_name, expected_maintainers):
+    pkg_cls = spack.repo.path.get_pkg_class(package_name)
+    assert pkg_cls.maintainers == expected_maintainers

var/spack/repos/builtin.mock/packages/maintainers-1/package.py

@@ -12,6 +12,6 @@ class Maintainers1(Package):
     homepage = "http://www.example.com"
     url = "http://www.example.com/maintainers-1.0.tar.gz"
 
-    maintainers = ["user1", "user2"]
+    maintainers("user1", "user2")
 
     version("1.0", "0123456789abcdef0123456789abcdef")

var/spack/repos/builtin.mock/packages/maintainers-3/package.py

@@ -0,0 +1,17 @@
+# Copyright 2013-2023 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.maintainers_1 import Maintainers1
+
+
+class Maintainers3(Maintainers1):
+    """A second package with a maintainers field."""
+
+    homepage = "http://www.example.com"
+    url = "http://www.example.com/maintainers2-1.0.tar.gz"
+
+    maintainers("user0", "user3")
+
+    version("1.0", "0123456789abcdef0123456789abcdef")

var/spack/repos/builtin.mock/packages/py-extension1/package.py

@@ -13,8 +13,7 @@ class PyExtension1(PythonPackage):
     homepage = "http://www.example.com"
     url = "http://www.example.com/extension1-1.0.tar.gz"
 
-    # Override settings in base class
+    maintainers = ["user1", "user2"]
-    maintainers = []
 
     version("1.0", "00000000000000000000000000000110")
     version("2.0", "00000000000000000000000000000120")