make version(...) kwargs explicit (#36998)

- [x] Replace `version(ver, checksum=None, **kwargs)` signature with
      `version(ver, checksum=None, *, sha256=..., ...)` explicitly listing all arguments.
- [x] Fix various issues in packages:
  - `tags` instead of `tag`
  - `default` instead of `preferred`
  - `sha26` instead of `sha256`
  - etc

Also, use `sha256=...` consistently.

Note: setting `sha256` currently doesn't validate the checksum length, so you could do
`sha256="a"*32` and it would get checked as `md5`... but that's something for another PR.

4 files changed, 97 insertions, 31 deletions

diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst
index d7d63f634e..969da6de20 100644
--- a/lib/spack/docs/packaging_guide.rst
+++ b/lib/spack/docs/packaging_guide.rst
@@ -604,9 +604,9 @@ add a line like this in the package class:
 
        url = "http://example.com/foo-1.0.tar.gz"
 
-       version("8.2.1", "4136d7b4c04df68b686570afa26988ac")
-       version("8.2.0", "1c9f62f0778697a09d36121ead88e08e")
-       version("8.1.2", "d47dd09ed7ae6e7fd6f9a816d7f5fdf6")
+       version("8.2.1", md5="4136d7b4c04df68b686570afa26988ac")
+       version("8.2.0", md5="1c9f62f0778697a09d36121ead88e08e")
+       version("8.1.2", md5="d47dd09ed7ae6e7fd6f9a816d7f5fdf6")
 
 Versions should be listed in descending order, from newest to oldest.
 
@@ -695,7 +695,7 @@ of its versions, you can add an explicit URL for a particular version:
 
 .. code-block:: python
 
-   version("8.2.1", "4136d7b4c04df68b686570afa26988ac",
+   version("8.2.1", md5="4136d7b4c04df68b686570afa26988ac",
            url="http://example.com/foo-8.2.1-special-version.tar.gz")
 
 
@@ -757,7 +757,7 @@ executables and other custom archive types), you can add ``expand=False`` to a
 
 .. code-block:: python
 
-   version("8.2.1", "4136d7b4c04df68b686570afa26988ac",
+   version("8.2.1", md5="4136d7b4c04df68b686570afa26988ac",
            url="http://example.com/foo-8.2.1-special-version.sh", expand=False)
 
 When ``expand`` is set to ``False``, Spack sets the current working
@@ -955,10 +955,10 @@ file:
 .. code-block:: console
 
    ==> Checksummed new versions of libelf:
-       version("0.8.13", "4136d7b4c04df68b686570afa26988ac")
-       version("0.8.12", "e21f8273d9f5f6d43a59878dc274fec7")
-       version("0.8.11", "e931910b6d100f6caa32239849947fbf")
-       version("0.8.10", "9db4d36c283d9790d8fa7df1f4d7b4d9")
+       version("0.8.13", md5="4136d7b4c04df68b686570afa26988ac")
+       version("0.8.12", md5="e21f8273d9f5f6d43a59878dc274fec7")
+       version("0.8.11", md5="e931910b6d100f6caa32239849947fbf")
+       version("0.8.10", md5="9db4d36c283d9790d8fa7df1f4d7b4d9")
 
 By default, Spack will search for new tarball downloads by scraping
 the parent directory of the tarball you gave it.  So, if your tarball
@@ -1112,8 +1112,8 @@ class-level tarball URL and VCS. For example:
 
        version("develop", branch="develop")
        version("master",  branch="master")
-       version("12.12.1", "ecd4606fa332212433c98bf950a69cc7")
-       version("12.10.1", "667333dbd7c0f031d47d7c5511fd0810")
+       version("12.12.1", md5="ecd4606fa332212433c98bf950a69cc7")
+       version("12.10.1", md5="667333dbd7c0f031d47d7c5511fd0810")
        version("12.8.1",  "9f37f683ee2b427b5540db8a20ed6b15")
 
 If a package contains both a ``url`` and ``git`` class-level attribute,
@@ -1276,7 +1276,7 @@ checksum.
 
 .. code-block:: python
 
-       version("1.9.5.1.1", "d035e4bc704d136db79b43ab371b27d2",
+       version("1.9.5.1.1", md5="d035e4bc704d136db79b43ab371b27d2",
                url="https://www.github.com/jswhit/pyproj/tarball/0be612cc9f972e38b50a90c946a9b353e2ab140f")
 
 .. _hg-fetch:
@@ -2212,7 +2212,7 @@ looks like this:
        homepage = "http://www.openssl.org"
        url      = "http://www.openssl.org/source/openssl-1.0.1h.tar.gz"
 
-       version("1.0.1h", "8d6d684a9430d5cc98a62a5d8fbda8cf")
+       version("1.0.1h", md5="8d6d684a9430d5cc98a62a5d8fbda8cf")
        depends_on("zlib")
 
        parallel = False
@@ -2309,7 +2309,7 @@ Spack makes this relatively easy.  Let's take a look at the
        url      = "http://www.prevanders.net/libdwarf-20130729.tar.gz"
        list_url = homepage
 
-       version("20130729", "4cc5e48693f7b93b7aa0261e63c0e21d")
+       version("20130729", md5="4cc5e48693f7b93b7aa0261e63c0e21d")
        ...
 
        depends_on("libelf")
@@ -2808,7 +2808,7 @@ supplying a ``depends_on`` call in the package definition.  For example:
        homepage = "https://github.com/hpc/mpileaks"
        url = "https://github.com/hpc/mpileaks/releases/download/v1.0/mpileaks-1.0.tar.gz"
 
-       version("1.0", "8838c574b39202a57d7c2d68692718aa")
+       version("1.0", md5="8838c574b39202a57d7c2d68692718aa")
 
        depends_on("mpi")
        depends_on("adept-utils")
diff --git a/lib/spack/spack/cmd/create.py b/lib/spack/spack/cmd/create.py
index 8ee259c52d..3582877ee1 100644
--- a/lib/spack/spack/cmd/create.py
+++ b/lib/spack/spack/cmd/create.py
@@ -807,7 +807,7 @@ def get_versions(args, name):
     # Default version with hash
     hashed_versions = """\
     # FIXME: Add proper versions and checksums here.
-    # version("1.2.3", "0123456789abcdef0123456789abcdef")"""
+    # version("1.2.3", md5="0123456789abcdef0123456789abcdef")"""
 
     # Default version without hash
     unhashed_versions = """\
diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index c4d5865d9e..341b8b2c1f 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -32,7 +32,7 @@ import collections.abc
 import functools
 import os.path
 import re
-from typing import List, Set
+from typing import List, Optional, Set
 
 import llnl.util.lang
 import llnl.util.tty.color
@@ -317,34 +317,100 @@ directive = DirectiveMeta.directive
 
 
 @directive("versions")
-def version(ver, checksum=None, **kwargs):
+def version(
+    ver: str,
+    # this positional argument is deprecated, use sha256=... instead
+    checksum: Optional[str] = None,
+    *,
+    # generic version options
+    preferred: Optional[bool] = None,
+    deprecated: Optional[bool] = None,
+    no_cache: Optional[bool] = None,
+    # url fetch options
+    url: Optional[str] = None,
+    extension: Optional[str] = None,
+    expand: Optional[bool] = None,
+    fetch_options: Optional[dict] = None,
+    # url archive verification options
+    md5: Optional[str] = None,
+    sha1: Optional[str] = None,
+    sha224: Optional[str] = None,
+    sha256: Optional[str] = None,
+    sha384: Optional[str] = None,
+    sha512: Optional[str] = None,
+    # git fetch options
+    git: Optional[str] = None,
+    commit: Optional[str] = None,
+    tag: Optional[str] = None,
+    branch: Optional[str] = None,
+    get_full_repo: Optional[bool] = None,
+    submodules: Optional[bool] = None,
+    submodules_delete: Optional[bool] = None,
+    # other version control
+    svn: Optional[str] = None,
+    hg: Optional[str] = None,
+    cvs: Optional[str] = None,
+    revision: Optional[str] = None,
+    date: Optional[str] = None,
+):
     """Adds a version and, if appropriate, metadata for fetching its code.
 
     The ``version`` directives are aggregated into a ``versions`` dictionary
     attribute with ``Version`` keys and metadata values, where the metadata
     is stored as a dictionary of ``kwargs``.
 
-    The ``dict`` of arguments is turned into a valid fetch strategy for
+    The (keyword) arguments are turned into a valid fetch strategy for
     code packages later. See ``spack.fetch_strategy.for_package_version()``.
-
-    Keyword Arguments:
-        deprecated (bool): whether or not this version is deprecated
     """
 
     def _execute_version(pkg):
-        if checksum is not None:
-            if hasattr(pkg, "has_code") and not pkg.has_code:
-                raise VersionChecksumError(
-                    "{0}: Checksums not allowed in no-code packages"
-                    "(see '{1}' version).".format(pkg.name, ver)
-                )
+        if (
+            any((sha256, sha384, sha512, md5, sha1, sha224, checksum))
+            and hasattr(pkg, "has_code")
+            and not pkg.has_code
+        ):
+            raise VersionChecksumError(
+                "{0}: Checksums not allowed in no-code packages "
+                "(see '{1}' version).".format(pkg.name, ver)
+            )
 
-            kwargs["checksum"] = checksum
+        kwargs = {
+            key: value
+            for key, value in (
+                ("sha256", sha256),
+                ("sha384", sha384),
+                ("sha512", sha512),
+                ("preferred", preferred),
+                ("deprecated", deprecated),
+                ("expand", expand),
+                ("url", url),
+                ("extension", extension),
+                ("no_cache", no_cache),
+                ("fetch_options", fetch_options),
+                ("git", git),
+                ("svn", svn),
+                ("hg", hg),
+                ("cvs", cvs),
+                ("get_full_repo", get_full_repo),
+                ("branch", branch),
+                ("submodules", submodules),
+                ("submodules_delete", submodules_delete),
+                ("commit", commit),
+                ("tag", tag),
+                ("revision", revision),
+                ("date", date),
+                ("md5", md5),
+                ("sha1", sha1),
+                ("sha224", sha224),
+                ("checksum", checksum),
+            )
+            if value is not None
+        }
 
         # Store kwargs for the package to later with a fetch_strategy.
         version = Version(ver)
         if isinstance(version, GitVersion):
-            if not hasattr(pkg, "git") and "git" not in kwargs:
+            if git is None and not hasattr(pkg, "git"):
                 msg = "Spack version directives cannot include git hashes fetched from"
                 msg += " URLs. Error in package '%s'\n" % pkg.name
                 msg += "    version('%s', " % version.string
diff --git a/lib/spack/spack/test/cmd/pkg.py b/lib/spack/spack/test/cmd/pkg.py
index 4ae8b516c0..7c5aae590e 100644
--- a/lib/spack/spack/test/cmd/pkg.py
+++ b/lib/spack/spack/test/cmd/pkg.py
@@ -25,7 +25,7 @@ class {name}(Package):
     homepage = "http://www.example.com"
     url      = "http://www.example.com/test-1.0.tar.gz"
 
-    version('1.0', '0123456789abcdef0123456789abcdef')
+    version("1.0", md5="0123456789abcdef0123456789abcdef")
 
     def install(self, spec, prefix):
         pass