format: allow spaces in format specifiers (#45487)

* format: allow spaces in format specifiers

Key-value pair format specifiers can now contain spaces in the key. This allows us to
add spaces to format strings that are *only* present when the attribute formatted is not
``None``. Instead of writing:

```
    {arch=architecture}
```

and special casing `arch=` like a sigil in `Spec.format()`, we can now write:

```
    { arch=architecture}
```

And the space is *only* printed when `architecture` is not `None`. This allows us to
remove the special case in `Spec.format()` for `arch=`.

Previously the only `key=` prefix allowed in format specifiers was `arch=`, but this PR
removes that requirement, and the `key=` part of a key-value specifier can be any name.
It does *not* have to correspond to the formatted attribute.

- [x] modify `SPEC_FORMAT_RE` to allow arbitrary keys in key-value pairs.
- [x] remove special case for `arch=` from `Spec.format()`.
- [x] modify format strings using `{arch=architecture}` to use `{ arch=architecture}`
- [x] add more tests for formatting

This PR saves other more complex attributes like compiler flags and their spacing for later.

Signed-off-by: Todd Gamblin <tgamblin@llnl.gov>

4 files changed, 88 insertions, 47 deletions

diff --git a/lib/spack/spack/ci.py b/lib/spack/spack/ci.py
index 528fa45063..db8e8f1a35 100644
--- a/lib/spack/spack/ci.py
+++ b/lib/spack/spack/ci.py
@@ -71,7 +71,7 @@ SPACK_RESERVED_TAGS = ["public", "protected", "notary"]
 # TODO: Remove this in Spack 0.23
 SHARED_PR_MIRROR_URL = "s3://spack-binaries-prs/shared_pr_mirror"
 JOB_NAME_FORMAT = (
-    "{name}{@version} {/hash:7} {%compiler.name}{@compiler.version}{arch=architecture}"
+    "{name}{@version} {/hash:7} {%compiler.name}{@compiler.version}{ arch=architecture}"
 )
 IS_WINDOWS = sys.platform == "win32"
 spack_gpg = spack.main.SpackCommand("gpg")
diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py
index 00e30a551d..48f8b5b9c1 100644
--- a/lib/spack/spack/cmd/__init__.py
+++ b/lib/spack/spack/cmd/__init__.py
@@ -237,7 +237,7 @@ def ensure_single_spec_or_die(spec, matching_specs):
     if len(matching_specs) <= 1:
         return
 
-    format_string = "{name}{@version}{%compiler.name}{@compiler.version}{arch=architecture}"
+    format_string = "{name}{@version}{%compiler.name}{@compiler.version}{ arch=architecture}"
     args = ["%s matches multiple packages." % spec, "Matching packages:"]
     args += [
         colorize("  @K{%s} " % s.dag_hash(7)) + s.cformat(format_string) for s in matching_specs
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index d35163c638..d3ec7d7157 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -129,7 +129,7 @@ SPEC_FORMAT_RE = re.compile(
     r"|"  # or
     # OPTION 2: an actual format string
     r"{"  # non-escaped open brace {
-    r"([%@/]|arch=)?"  # optional sigil (to print sigil in color)
+    r"([%@/]|[\w ][\w -]*=)?"  # optional sigil (or identifier or space) to print sigil in color
     r"(?:\^([^}\.]+)\.)?"  # optional ^depname. (to get attr from dependency)
     # after the sigil or depname, we can have a hash expression or another attribute
     r"(?:"  # one of
@@ -163,14 +163,14 @@ HASH_COLOR = "@K"  #: color for highlighting package hashes
 DEFAULT_FORMAT = (
     "{name}{@versions}"
     "{%compiler.name}{@compiler.versions}{compiler_flags}"
-    "{variants}{arch=architecture}{/abstract_hash}"
+    "{variants}{ arch=architecture}{/abstract_hash}"
 )
 
 #: Display format, which eliminates extra `@=` in the output, for readability.
 DISPLAY_FORMAT = (
     "{name}{@version}"
     "{%compiler.name}{@compiler.version}{compiler_flags}"
-    "{variants}{arch=architecture}{/abstract_hash}"
+    "{variants}{ arch=architecture}{/abstract_hash}"
 )
 
 #: Regular expression to pull spec contents out of clearsigned signature
@@ -1894,14 +1894,14 @@ class Spec:
         """Returns a version of the spec with the dependencies hashed
         instead of completely enumerated."""
         spec_format = "{name}{@version}{%compiler.name}{@compiler.version}"
-        spec_format += "{variants}{arch=architecture}{/hash:7}"
+        spec_format += "{variants}{ arch=architecture}{/hash:7}"
         return self.format(spec_format)
 
     @property
     def cshort_spec(self):
         """Returns an auto-colorized version of ``self.short_spec``."""
         spec_format = "{name}{@version}{%compiler.name}{@compiler.version}"
-        spec_format += "{variants}{arch=architecture}{/hash:7}"
+        spec_format += "{variants}{ arch=architecture}{/hash:7}"
         return self.cformat(spec_format)
 
     @property
@@ -4387,13 +4387,14 @@ class Spec:
         yield deps
 
     def format(self, format_string: str = DEFAULT_FORMAT, color: Optional[bool] = False) -> str:
-        r"""Prints out particular pieces of a spec, depending on what is
-        in the format string.
+        r"""Prints out attributes of a spec according to a format string.
 
-        Using the ``{attribute}`` syntax, any field of the spec can be
-        selected.  Those attributes can be recursive. For example,
-        ``s.format({compiler.version})`` will print the version of the
-        compiler.
+        Using an ``{attribute}`` format specifier, any field of the spec can be
+        selected. Those attributes can be recursive. For example,
+        ``s.format({compiler.version})`` will print the version of the compiler.
+
+        If the attribute in a format specifier evaluates to ``None``, then the format
+        specifier will evaluate to the empty string, ``""``.
 
         Commonly used attributes of the Spec for format strings include::
 
@@ -4409,6 +4410,7 @@ class Spec:
             architecture.os
             architecture.target
             prefix
+            namespace
 
         Some additional special-case properties can be added::
 
@@ -4417,40 +4419,51 @@ class Spec:
             spack_install The spack install directory
 
         The ``^`` sigil can be used to access dependencies by name.
-        ``s.format({^mpi.name})`` will print the name of the MPI
-        implementation in the spec.
+        ``s.format({^mpi.name})`` will print the name of the MPI implementation in the
+        spec.
 
-        The ``@``, ``%``, ``arch=``, and ``/`` sigils
-        can be used to include the sigil with the printed
-        string. These sigils may only be used with the appropriate
-        attributes, listed below::
+        The ``@``, ``%``, and ``/`` sigils can be used to include the sigil with the
+        printed string. These sigils may only be used with the appropriate attributes,
+        listed below::
 
             @        ``{@version}``, ``{@compiler.version}``
             %        ``{%compiler}``, ``{%compiler.name}``
-            arch=    ``{arch=architecture}``
             /        ``{/hash}``, ``{/hash:7}``, etc
 
-        The ``@`` sigil may also be used for any other property named
-        ``version``. Sigils printed with the attribute string are only
-        printed if the attribute string is non-empty, and are colored
-        according to the color of the attribute.
-
-        Sigils are not used for printing variants. Variants listed by
-        name naturally print with their sigil. For example,
-        ``spec.format('{variants.debug}')`` would print either
-        ``+debug`` or ``~debug`` depending on the name of the
-        variant. Non-boolean variants print as ``name=value``. To
-        print variant names or values independently, use
+        The ``@`` sigil may also be used for any other property named ``version``.
+        Sigils printed with the attribute string are only printed if the attribute
+        string is non-empty, and are colored according to the color of the attribute.
+
+        Variants listed by name naturally print with their sigil. For example,
+        ``spec.format('{variants.debug}')`` prints either ``+debug`` or ``~debug``
+        depending on the name of the variant. Non-boolean variants print as
+        ``name=value``. To print variant names or values independently, use
         ``spec.format('{variants.<name>.name}')`` or
         ``spec.format('{variants.<name>.value}')``.
 
-        Spec format strings use ``\`` as the escape character. Use
-        ``\{`` and ``\}`` for literal braces, and ``\\`` for the
-        literal ``\`` character.
+        There are a few attributes on specs that can be specified as key-value pairs
+        that are *not* variants, e.g.: ``os``, ``arch``, ``architecture``, ``target``,
+        ``namespace``, etc. You can format these with an optional ``key=`` prefix, e.g.
+        ``{namespace=namespace}`` or ``{arch=architecture}``, etc. The ``key=`` prefix
+        will be colorized along with the value.
+
+        When formatting specs, key-value pairs are separated from preceding parts of the
+        spec by whitespace. To avoid printing extra whitespace when the formatted
+        attribute is not set, you can add whitespace to the key *inside* the braces of
+        the format string, e.g.:
+
+            { namespace=namespace}
+
+        This evaluates to `` namespace=builtin`` if ``namespace`` is set to ``builtin``,
+        and to ``""`` if ``namespace`` is ``None``.
+
+        Spec format strings use ``\`` as the escape character. Use ``\{`` and ``\}`` for
+        literal braces, and ``\\`` for the literal ``\`` character.
 
         Args:
             format_string: string containing the format to be expanded
             color: True for colorized result; False for no color; None for auto color.
+
         """
         ensure_modern_format_string(format_string)
 
@@ -4504,10 +4517,6 @@ class Spec:
                 raise SpecFormatSigilError(sig, "compilers", attribute)
             elif sig == "/" and attribute != "abstract_hash":
                 raise SpecFormatSigilError(sig, "DAG hashes", attribute)
-            elif sig == "arch=":
-                if attribute not in ("architecture", "arch"):
-                    raise SpecFormatSigilError(sig, "the architecture", attribute)
-                sig = " arch="  # include space as separator
 
             # Iterate over components using getattr to get next element
             for idx, part in enumerate(parts):
@@ -4552,15 +4561,19 @@ class Spec:
 
             # Set color codes for various attributes
             color = None
-            if "variants" in parts:
-                color = VARIANT_COLOR
-            elif "architecture" in parts:
+            if "architecture" in parts:
                 color = ARCHITECTURE_COLOR
+            elif "variants" in parts or sig.endswith("="):
+                color = VARIANT_COLOR
             elif "compiler" in parts or "compiler_flags" in parts:
                 color = COMPILER_COLOR
             elif "version" in parts or "versions" in parts:
                 color = VERSION_COLOR
 
+            # return empty string if the value of the attribute is None.
+            if current is None:
+                return ""
+
             # return colored output
             return safe_color(sig, str(current), color)
 
@@ -5523,7 +5536,7 @@ class UnconstrainableDependencySpecError(spack.error.SpecError):
 class AmbiguousHashError(spack.error.SpecError):
     def __init__(self, msg, *specs):
         spec_fmt = "{namespace}.{name}{@version}{%compiler}{compiler_flags}"
-        spec_fmt += "{variants}{arch=architecture}{/hash:7}"
+        spec_fmt += "{variants}{ arch=architecture}{/hash:7}"
         specs_str = "\n  " + "\n  ".join(spec.format(spec_fmt) for spec in specs)
         super().__init__(msg + specs_str)
 
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index b21f0e7ac9..faca9fad9a 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -656,6 +656,7 @@ class TestSpecSemantics:
             ("{@VERSIONS}", "@", "versions", lambda spec: spec),
             ("{%compiler}", "%", "compiler", lambda spec: spec),
             ("{arch=architecture}", "arch=", "architecture", lambda spec: spec),
+            ("{namespace=namespace}", "namespace=", "namespace", lambda spec: spec),
             ("{compiler.name}", "", "name", lambda spec: spec.compiler),
             ("{compiler.version}", "", "version", lambda spec: spec.compiler),
             ("{%compiler.name}", "%", "name", lambda spec: spec.compiler),
@@ -706,13 +707,40 @@ class TestSpecSemantics:
     @pytest.mark.parametrize(
         "fmt_str",
         [
-            "{@name}",
-            "{@version.concrete}",
-            "{%compiler.version}",
-            "{/hashd}",
-            "{arch=architecture.os}",
+            "{name}",
+            "{version}",
+            "{@version}",
+            "{%compiler}",
+            "{namespace}",
+            "{ namespace=namespace}",
+            "{ namespace =namespace}",
+            "{ name space =namespace}",
+            "{arch}",
+            "{architecture}",
+            "{arch=architecture}",
+            "{  arch=architecture}",
+            "{  arch =architecture}",
         ],
     )
+    def test_spec_format_null_attributes(self, fmt_str):
+        """Ensure that attributes format to empty strings when their values are null."""
+        spec = spack.spec.Spec()
+        assert spec.format(fmt_str) == ""
+
+    def test_spec_formatting_spaces_in_key(self, default_mock_concretization):
+        spec = default_mock_concretization("multivalue-variant cflags=-O2")
+
+        # test that spaces are preserved, if they come after some other text, otherwise
+        # they are trimmed.
+        # TODO: should we be trimming whitespace from formats? Probably not.
+        assert spec.format("x{ arch=architecture}") == f"x arch={spec.architecture}"
+        assert spec.format("x{ namespace=namespace}") == f"x namespace={spec.namespace}"
+        assert spec.format("x{ name space =namespace}") == f"x name space ={spec.namespace}"
+        assert spec.format("x{ os =os}") == f"x os ={spec.os}"
+
+    @pytest.mark.parametrize(
+        "fmt_str", ["{@name}", "{@version.concrete}", "{%compiler.version}", "{/hashd}"]
+    )
     def test_spec_formatting_sigil_mismatches(self, default_mock_concretization, fmt_str):
         spec = default_mock_concretization("multivalue-variant cflags=-O2")