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.

6 files changed, 95 insertions, 24 deletions

diff --git a/lib/spack/docs/features.rst b/lib/spack/docs/features.rst
index bfc484f518..c39075fe10 100644
--- a/lib/spack/docs/features.rst
+++ b/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")
 
diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst
index 93a049e69a..4ebb62da07 100644
--- a/lib/spack/docs/packaging_guide.rst
+++ b/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
-   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.
+   Add a list of Github accounts of people who want to be notified
+   any time the package is modified. See :ref:`package_maintainers`.
 
 #. 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
 -----------------
diff --git a/lib/spack/spack/cmd/create.py b/lib/spack/spack/cmd/create.py
index b62daa22d3..bf796e47dc 100644
--- a/lib/spack/spack/cmd/create.py
+++ b/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}
 
diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index 8c247b7aff..c4d5865d9e 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -54,6 +54,7 @@ __all__ = [
     "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."""
 
diff --git a/lib/spack/spack/test/cmd/maintainers.py b/lib/spack/spack/test/cmd/maintainers.py
index 11560a6ce5..fba42bfc8e 100644
--- a/lib/spack/spack/test/cmd/maintainers.py
+++ b/lib/spack/spack/test/cmd/maintainers.py
@@ -14,6 +14,8 @@ import spack.repo
 
 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(
-        set(spack.repo.all_package_names()) - set(["maintainers-1", "maintainers-2"])
-    )
+    assert out == sorted(set(spack.repo.all_package_names()) - set(MAINTAINED_PACKAGES))
 
 
 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"]
diff --git a/lib/spack/spack/test/directives.py b/lib/spack/spack/test/directives.py
index 841e820083..18242feb67 100644
--- a/lib/spack/spack/test/directives.py
+++ b/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