Features: Improve Spec format strings #10556

becker33 · 2019-02-08T22:07:28Z

The old spec.format method had two separate syntaxes. One was concise but very hard to read ('$_$@$%@+$+$=' would be an example). The other was more verbose, but the formatting names were not aligned with the variable names on the spec, which made it unnecessarily hard on users. Additionally, it had unnecessary $-signs on all of the format specifiers.

This PR replaces the old spec.format method with a new one with a single, self-documenting syntax that revolves around the names of the spec attributes.

From the new spec.format documentation:

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.

Commonly used attributes of the Spec for format strings include:

name
version
compiler
compiler.name
compiler.version
compiler_flags
variants
architecture
architecture.platform
architecture.os
architecture.target
prefix

Some additional special-case properties can be added:

hash[:len]    The DAG hash with optional length argument
spack_root    The spack root directory
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.

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:

@        ``{@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 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. Also use \$ for the literal $ to differentiate from previous, deprecated format string syntax.

The previous format strings are deprecated. They can still be accessed by the old_format method. The format method will call old_format if the character $ appears un-escaped in the format string.

Args:

format_string (str): string containing the format to be expanded

Keyword Args:

color (bool): True if returned string is colored
transform (dict): maps full-string formats to a callable that accepts a string and returns another one

becker33 · 2019-02-11T19:24:10Z

@tgamblin ready for review

becker33 · 2019-04-16T01:55:36Z

@tgamblin ping

Switch to new spec.format method throughout internals

tgamblin

@becker33: two minor change requests, but otherwise I don't think I've been quite as satisfied looking over changes in a PR in recent memory. Every single change in here makes the code more readable. Looks awesome to me.

Can you address the r""" and color-map changes? Then let's get this in.

tgamblin · 2019-04-17T22:37:58Z

lib/spack/docs/config_yaml.rst

@@ -48,7 +48,7 @@ the installation paths. For example
     .. code-block:: yaml

       config:
-         install_path_scheme: '${PACKAGE}/${VERSION}/${HASH:7}'
+         install_path_scheme: '{name}/{version}/{hash:7}'


so much more readable

tgamblin · 2019-04-17T22:39:42Z

lib/spack/spack/architecture.py

@@ -349,16 +349,16 @@ def __init__(self, plat=None, os=None, target=None):
    def concrete(self):
        return all((self.platform is not None,
                    isinstance(self.platform, Platform),
-                    self.platform_os is not None,
-                    isinstance(self.platform_os, OperatingSystem),
+                    self.os is not None,


omg SO satisfying to be rid of platform_

tgamblin · 2019-04-17T22:40:44Z

lib/spack/spack/cmd/__init__.py

        args = ["%s matches multiple packages." % spec,
                "Matching packages:"]
        args += [colorize("  @K{%s} " % s.dag_hash(7)) +
-                 s.cformat('$_$@$%@$=') for s in matching_specs]
+                 s.cformat(format_string) for s in matching_specs]


Yes, ok, it is fairly clear that my original format strings were horrendous. 🙈

tgamblin · 2019-04-17T22:42:00Z

lib/spack/spack/cmd/dependencies.py

@@ -42,7 +42,8 @@ def dependencies(parser, args):
        env = ev.get_env(args, 'dependencies')
        spec = spack.cmd.disambiguate_spec(specs[0], env)

-        tty.msg("Dependencies of %s" % spec.format('$_$@$%@$/', color=True))
+        format_string = '{name}{@version}{%compiler}{/hash:7}'


The code, it does not swear at me anymore. 😄

tgamblin · 2019-04-17T22:46:14Z

lib/spack/spack/cmd/modules/__init__.py

@@ -344,7 +344,9 @@ def modules_cmd(parser, args, module_type, callbacks=callbacks):
    except MultipleSpecsMatch:
        msg = "the constraint '{query}' matches multiple packages:\n"
        for s in specs:
-            msg += '\t' + s.cformat(format_string='$/ $_$@$+$%@+$+$=') + '\n'
+            spec_fmt = '{hash:7} {name}{@version}{%compiler}'


Oh wow. So much regret for creating those prior format strings as I look at this. The horror.

tgamblin · 2019-04-17T22:52:46Z

lib/spack/spack/test/directory_layout.py

    layout_package7 = YamlDirectoryLayout(str(tmpdir),
                                          path_scheme=scheme_package7)
    path_package7 = layout_package7.relative_path_for_spec(spec)

    assert(package7 == path_package7)

    # Test separation of architecture
-    arch_scheme_package = "${PLATFORM}/${TARGET}/${OS}/${PACKAGE}/${VERSION}/${HASH:7}"   # NOQA: ignore=E501
+    arch_scheme_package = "{architecture.platform}/{architecture.target}/{architecture.os}/{name}/{version}/{hash:7}"   # NOQA: ignore=E501


We might consider allowing architecture to be just arch relatively soon here. Or to allow all of these to be top-level attributes of the spec (i.e., just plain old os, platform, target instead of architecture.os, etc.) This format string gets a bit tedious.

Not a change that needs to be made immediately, but I think it would be beneficial.

tgamblin · 2019-04-17T22:56:21Z

lib/spack/spack/test/graph.py

    libelf_hash, libelf_lbl = (
-        s['libelf'].dag_hash(), s['libelf'].format('$_$/'))
+        s['libelf'].dag_hash(), s['libelf'].format('{name}{/hash:7}'))


having some sensible default for, e.g., hash:short might be good eventually -- I don't think it needs to change here, but I don't like the number of places we have to write 7.

tgamblin · 2019-04-17T23:00:12Z

lib/spack/spack/spec.py

@@ -3010,10 +3022,245 @@ def _cmp_key(self):
    def colorized(self):
        return colorize_spec(self)

-    def format(self, format_string='$_$@$%@+$+$=', **kwargs):
+    def format(self, format_string=default_format, **kwargs):


So much yes in this one change.

lib/spack/spack/spec.py

tgamblin · 2019-04-17T23:04:26Z

lib/spack/spack/spec.py

+            # Set color codes for various attributes
+            col = None
+            if 'variants' in parts:
+                col = '+'


It is cleaner at this point to just add extra keys to the dict -- i.e., map 'variants' to the same color as '+' in the map above rather than polluting the already long format() method with this logic.

becker33 requested a review from tgamblin February 11, 2019 19:23

becker33 force-pushed the features/improve-format-strings branch from d9a4c48 to 3fe9f01 Compare February 11, 2019 21:59

becker33 added 3 commits April 16, 2019 13:31

Update spec format to simpler syntax, maintain backwards compatibility

c6567ba

Switch to new spec.format method throughout internals

update package files for new format strings

223da54

documentation and minor code cleanup. removed nonsensical variant sigils

064483a

becker33 force-pushed the features/improve-format-strings branch from 3fe9f01 to 064483a Compare April 16, 2019 20:50

flake

b77fa69

becker33 force-pushed the features/improve-format-strings branch from 0bf4a05 to b77fa69 Compare April 16, 2019 21:28

tgamblin added specs commands configuration and removed commands labels Apr 17, 2019

tgamblin requested changes Apr 17, 2019

View reviewed changes

flake

a4eaf02

tgamblin approved these changes Apr 18, 2019

View reviewed changes

tgamblin merged commit f242f5f into develop Apr 18, 2019

tgamblin deleted the features/improve-format-strings branch April 18, 2019 04:16

becker33 mentioned this pull request Apr 19, 2019

Make module naming schemes backwards compatible #11236

Merged

tgamblin mentioned this pull request Sep 2, 2019

Add machine-readable options to spack find #12677

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features: Improve Spec format strings #10556

Features: Improve Spec format strings #10556

becker33 commented Feb 8, 2019

becker33 commented Feb 11, 2019

becker33 commented Apr 16, 2019

tgamblin left a comment

tgamblin Apr 17, 2019

tgamblin Apr 17, 2019

tgamblin Apr 17, 2019

tgamblin Apr 17, 2019

tgamblin Apr 17, 2019

tgamblin Apr 17, 2019

tgamblin Apr 17, 2019

tgamblin Apr 17, 2019

tgamblin Apr 17, 2019

Features: Improve Spec format strings #10556

Features: Improve Spec format strings #10556

Conversation

becker33 commented Feb 8, 2019

becker33 commented Feb 11, 2019

becker33 commented Apr 16, 2019

tgamblin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment