From aa0825d642cfa285f5f62761a0e23dc1e511d056 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Tue, 23 Apr 2024 10:52:15 -0700 Subject: [PATCH] Refactor to improve `spec format` speed (#43712) When looking at where we spend our time in solver setup, I noticed a fair bit of time is spent in `Spec.format()`, and `Spec.format()` is a pretty old, slow, convoluted method. This PR does a number of things: - [x] Consolidate most of what was being done manually with a character loop and several regexes into a single regex. - [x] Precompile regexes where we keep them - [x] Remove the `transform=` argument to `Spec.format()` which was only used in one place in the code (modules) to uppercase env var names, but added a lot of complexity - [x] Avoid escaping and colorizing specs unless necessary - [x] Refactor a lot of the colorization logic to avoid unnecessary object construction - [x] Add type hints and remove some spots in the code where we were using nonexistent arguments to `format()`. - [x] Add trivial cases to `__str__` in `VariantMap` and `VersionList` to avoid sorting - [x] Avoid calling `isinstance()` in the main loop of `Spec.format()` - [x] Don't bother constructing a `string` representation for the result of `_prev_version` as it is only used for comparisons. In my timings (on all the specs formatted in a solve of `hdf5`), this is over 2.67x faster than the original `format()`, and it seems to reduce setup time by around a second (for `hdf5`). --- lib/spack/llnl/util/tty/__init__.py | 17 +- lib/spack/llnl/util/tty/color.py | 111 +++++----- lib/spack/spack/cmd/info.py | 17 +- lib/spack/spack/modules/common.py | 23 +- lib/spack/spack/spec.py | 255 ++++++++++------------- lib/spack/spack/test/bindist.py | 4 +- lib/spack/spack/test/config.py | 2 +- lib/spack/spack/test/modules/lmod.py | 3 - lib/spack/spack/test/modules/tcl.py | 3 - lib/spack/spack/test/spec_semantics.py | 38 ++-- lib/spack/spack/variant.py | 3 + lib/spack/spack/version/version_types.py | 20 +- 12 files changed, 226 insertions(+), 270 deletions(-) diff --git a/lib/spack/llnl/util/tty/__init__.py b/lib/spack/llnl/util/tty/__init__.py index 3f3ba1d462..807e50a159 100644 --- a/lib/spack/llnl/util/tty/__init__.py +++ b/lib/spack/llnl/util/tty/__init__.py @@ -12,7 +12,7 @@ import traceback from datetime import datetime from sys import platform as _platform -from typing import NoReturn +from typing import Any, NoReturn if _platform != "win32": import fcntl @@ -158,21 +158,22 @@ def get_timestamp(force=False): return "" -def msg(message, *args, **kwargs): +def msg(message: Any, *args: Any, newline: bool = True) -> None: if not msg_enabled(): return if isinstance(message, Exception): - message = "%s: %s" % (message.__class__.__name__, str(message)) + message = f"{message.__class__.__name__}: {message}" + else: + message = str(message) - newline = kwargs.get("newline", True) st_text = "" if _stacktrace: st_text = process_stacktrace(2) - if newline: - cprint("@*b{%s==>} %s%s" % (st_text, get_timestamp(), cescape(_output_filter(message)))) - else: - cwrite("@*b{%s==>} %s%s" % (st_text, get_timestamp(), cescape(_output_filter(message)))) + + nl = "\n" if newline else "" + cwrite(f"@*b{{{st_text}==>}} {get_timestamp()}{cescape(_output_filter(message))}{nl}") + for arg in args: print(indent + _output_filter(str(arg))) diff --git a/lib/spack/llnl/util/tty/color.py b/lib/spack/llnl/util/tty/color.py index bef7af8e58..a471505d34 100644 --- a/lib/spack/llnl/util/tty/color.py +++ b/lib/spack/llnl/util/tty/color.py @@ -62,6 +62,7 @@ import re import sys from contextlib import contextmanager +from typing import Optional class ColorParseError(Exception): @@ -95,7 +96,7 @@ def __init__(self, message): } # white # Regex to be used for color formatting -color_re = r"@(?:@|\.|([*_])?([a-zA-Z])?(?:{((?:[^}]|}})*)})?)" +COLOR_RE = re.compile(r"@(?:(@)|(\.)|([*_])?([a-zA-Z])?(?:{((?:[^}]|}})*)})?)") # Mapping from color arguments to values for tty.set_color color_when_values = {"always": True, "auto": None, "never": False} @@ -203,77 +204,64 @@ def color_when(value): set_color_when(old_value) -class match_to_ansi: - def __init__(self, color=True, enclose=False, zsh=False): - self.color = _color_when_value(color) - self.enclose = enclose - self.zsh = zsh - - def escape(self, s): - """Returns a TTY escape sequence for a color""" - if self.color: - if self.zsh: - result = rf"\e[0;{s}m" - else: - result = f"\033[{s}m" - - if self.enclose: - result = rf"\[{result}\]" - - return result +def _escape(s: str, color: bool, enclose: bool, zsh: bool) -> str: + """Returns a TTY escape sequence for a color""" + if color: + if zsh: + result = rf"\e[0;{s}m" else: - return "" + result = f"\033[{s}m" - def __call__(self, match): - """Convert a match object generated by ``color_re`` into an ansi - color code. This can be used as a handler in ``re.sub``. - """ - style, color, text = match.groups() - m = match.group(0) + if enclose: + result = rf"\[{result}\]" - if m == "@@": - return "@" - elif m == "@.": - return self.escape(0) - elif m == "@": - raise ColorParseError("Incomplete color format: '%s' in %s" % (m, match.string)) - - string = styles[style] - if color: - if color not in colors: - raise ColorParseError( - "Invalid color specifier: '%s' in '%s'" % (color, match.string) - ) - string += ";" + str(colors[color]) - - colored_text = "" - if text: - colored_text = text + self.escape(0) - - return self.escape(string) + colored_text + return result + else: + return "" -def colorize(string, **kwargs): +def colorize( + string: str, color: Optional[bool] = None, enclose: bool = False, zsh: bool = False +) -> str: """Replace all color expressions in a string with ANSI control codes. Args: - string (str): The string to replace + string: The string to replace Returns: - str: The filtered string + The filtered string Keyword Arguments: - color (bool): If False, output will be plain text without control - codes, for output to non-console devices. - enclose (bool): If True, enclose ansi color sequences with + color: If False, output will be plain text without control codes, for output to + non-console devices (default: automatically choose color or not) + enclose: If True, enclose ansi color sequences with square brackets to prevent misestimation of terminal width. - zsh (bool): If True, use zsh ansi codes instead of bash ones (for variables like PS1) + zsh: If True, use zsh ansi codes instead of bash ones (for variables like PS1) """ - color = _color_when_value(kwargs.get("color", get_color_when())) - zsh = kwargs.get("zsh", False) - string = re.sub(color_re, match_to_ansi(color, kwargs.get("enclose")), string, zsh) - string = string.replace("}}", "}") - return string + color = color if color is not None else get_color_when() + + def match_to_ansi(match): + """Convert a match object generated by ``COLOR_RE`` into an ansi + color code. This can be used as a handler in ``re.sub``. + """ + escaped_at, dot, style, color_code, text = match.groups() + + if escaped_at: + return "@" + elif dot: + return _escape(0, color, enclose, zsh) + elif not (style or color_code): + raise ColorParseError( + f"Incomplete color format: '{match.group(0)}' in '{match.string}'" + ) + + ansi_code = _escape(f"{styles[style]};{colors.get(color_code, '')}", color, enclose, zsh) + if text: + return f"{ansi_code}{text}{_escape(0, color, enclose, zsh)}" + else: + return ansi_code + + return COLOR_RE.sub(match_to_ansi, string).replace("}}", "}") def clen(string): @@ -305,7 +293,7 @@ def cprint(string, stream=None, color=None): cwrite(string + "\n", stream, color) -def cescape(string): +def cescape(string: str) -> str: """Escapes special characters needed for color codes. Replaces the following symbols with their equivalent literal forms: @@ -321,10 +309,7 @@ def cescape(string): Returns: (str): the string with color codes escaped """ - string = str(string) - string = string.replace("@", "@@") - string = string.replace("}", "}}") - return string + return string.replace("@", "@@").replace("}", "}}") class ColorStream: diff --git a/lib/spack/spack/cmd/info.py b/lib/spack/spack/cmd/info.py index b007c60516..3075107d07 100644 --- a/lib/spack/spack/cmd/info.py +++ b/lib/spack/spack/cmd/info.py @@ -263,8 +263,8 @@ def _fmt_name_and_default(variant): return color.colorize(f"@c{{{variant.name}}} @C{{[{_fmt_value(variant.default)}]}}") -def _fmt_when(when, indent): - return color.colorize(f"{indent * ' '}@B{{when}} {color.cescape(when)}") +def _fmt_when(when: "spack.spec.Spec", indent: int): + return color.colorize(f"{indent * ' '}@B{{when}} {color.cescape(str(when))}") def _fmt_variant_description(variant, width, indent): @@ -441,7 +441,7 @@ def get_url(version): return "No URL" url = get_url(preferred) if pkg.has_code else "" - line = version(" {0}".format(pad(preferred))) + color.cescape(url) + line = version(" {0}".format(pad(preferred))) + color.cescape(str(url)) color.cwrite(line) print() @@ -464,7 +464,7 @@ def get_url(version): continue for v, url in vers: - line = version(" {0}".format(pad(v))) + color.cescape(url) + line = version(" {0}".format(pad(v))) + color.cescape(str(url)) color.cprint(line) @@ -475,10 +475,7 @@ def print_virtuals(pkg, args): color.cprint(section_title("Virtual Packages: ")) if pkg.provided: for when, specs in reversed(sorted(pkg.provided.items())): - line = " %s provides %s" % ( - when.colorized(), - ", ".join(s.colorized() for s in specs), - ) + line = " %s provides %s" % (when.cformat(), ", ".join(s.cformat() for s in specs)) print(line) else: @@ -497,7 +494,9 @@ def print_licenses(pkg, args): pad = padder(pkg.licenses, 4) for when_spec in pkg.licenses: license_identifier = pkg.licenses[when_spec] - line = license(" {0}".format(pad(license_identifier))) + color.cescape(when_spec) + line = license(" {0}".format(pad(license_identifier))) + color.cescape( + str(when_spec) + ) color.cprint(line) diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py index 20d75f61c1..5e46ca4c6c 100644 --- a/lib/spack/spack/modules/common.py +++ b/lib/spack/spack/modules/common.py @@ -83,6 +83,17 @@ def configuration(module_set_name): ) +_FORMAT_STRING_RE = re.compile(r"({[^}]*})") + + +def _format_env_var_name(spec, var_name_fmt): + """Format the variable name, but uppercase any formatted fields.""" + fmt_parts = _FORMAT_STRING_RE.split(var_name_fmt) + return "".join( + spec.format(part).upper() if _FORMAT_STRING_RE.match(part) else part for part in fmt_parts + ) + + def _check_tokens_are_valid(format_string, message): """Checks that the tokens used in the format string are valid in the context of module file and environment variable naming. @@ -737,20 +748,12 @@ def environment_modifications(self): exclude = self.conf.exclude_env_vars # We may have tokens to substitute in environment commands - - # Prepare a suitable transformation dictionary for the names - # of the environment variables. This means turn the valid - # tokens uppercase. - transform = {} - for token in _valid_tokens: - transform[token] = lambda s, string: str.upper(string) - for x in env: # Ensure all the tokens are valid in this context msg = "some tokens cannot be expanded in an environment variable name" + _check_tokens_are_valid(x.name, message=msg) - # Transform them - x.name = self.spec.format(x.name, transform=transform) + x.name = _format_env_var_name(self.spec, x.name) if self.modification_needs_formatting(x): try: # Not every command has a value diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index 301510336d..7d8266400e 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -51,7 +51,6 @@ import collections import collections.abc import enum -import io import itertools import os import pathlib @@ -59,7 +58,7 @@ import re import socket import warnings -from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Union +from typing import Any, Callable, Dict, List, Match, Optional, Set, Tuple, Union import llnl.path import llnl.string @@ -121,36 +120,44 @@ "SpecDeprecatedError", ] + +SPEC_FORMAT_RE = re.compile( + r"(?:" # this is one big or, with matches ordered by priority + # OPTION 1: escaped character (needs to be first to catch opening \{) + # Note that an unterminated \ at the end of a string is left untouched + r"(?:\\(.))" + r"|" # or + # OPTION 2: an actual format string + r"{" # non-escaped open brace { + r"([%@/]|arch=)?" # optional sigil (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 + r"(hash\b)(?:\:(\d+))?" # hash followed by : + r"|" # or + r"([^}]*)" # another attribute to format + r")" # end one of + r"(})?" # finish format string with non-escaped close brace }, or missing if not present + r"|" + # OPTION 3: mismatched close brace (option 2 would consume a matched open brace) + r"(})" # brace + r")", + re.IGNORECASE, +) + #: Valid pattern for an identifier in Spack IDENTIFIER_RE = r"\w[\w-]*" +# Coloring of specs when using color output. Fields are printed with +# different colors to enhance readability. +# See llnl.util.tty.color for descriptions of the color codes. COMPILER_COLOR = "@g" #: color for highlighting compilers VERSION_COLOR = "@c" #: color for highlighting versions ARCHITECTURE_COLOR = "@m" #: color for highlighting architectures -ENABLED_VARIANT_COLOR = "@B" #: color for highlighting enabled variants -DISABLED_VARIANT_COLOR = "r" #: color for highlighting disabled varaints -DEPENDENCY_COLOR = "@." #: color for highlighting dependencies +VARIANT_COLOR = "@B" #: color for highlighting variants HASH_COLOR = "@K" #: color for highlighting package hashes -#: This map determines the coloring of specs when using color output. -#: We make the fields different colors to enhance readability. -#: See llnl.util.tty.color for descriptions of the color codes. -COLOR_FORMATS = { - "%": COMPILER_COLOR, - "@": VERSION_COLOR, - "=": ARCHITECTURE_COLOR, - "+": ENABLED_VARIANT_COLOR, - "~": DISABLED_VARIANT_COLOR, - "^": DEPENDENCY_COLOR, - "#": HASH_COLOR, -} - -#: Regex used for splitting by spec field separators. -#: These need to be escaped to avoid metacharacters in -#: ``COLOR_FORMATS.keys()``. -_SEPARATORS = "[\\%s]" % "\\".join(COLOR_FORMATS.keys()) - #: Default format for Spec.format(). This format can be round-tripped, so that: #: Spec(Spec("string").format()) == Spec("string)" DEFAULT_FORMAT = ( @@ -193,26 +200,7 @@ class InstallStatus(enum.Enum): missing = "@r{[-]} " -def colorize_spec(spec): - """Returns a spec colorized according to the colors specified in - COLOR_FORMATS.""" - - class insert_color: - def __init__(self): - self.last = None - - def __call__(self, match): - # ignore compiler versions (color same as compiler) - sep = match.group(0) - if self.last == "%" and sep == "@": - return clr.cescape(sep) - self.last = sep - - return "%s%s" % (COLOR_FORMATS[sep], clr.cescape(sep)) - - return clr.colorize(re.sub(_SEPARATORS, insert_color(), str(spec)) + "@.") - - +# regexes used in spec formatting OLD_STYLE_FMT_RE = re.compile(r"\${[A-Z]+}") @@ -4295,10 +4283,7 @@ def deps(): yield deps - def colorized(self): - return colorize_spec(self) - - def format(self, format_string=DEFAULT_FORMAT, **kwargs): + 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. @@ -4361,79 +4346,65 @@ def format(self, format_string=DEFAULT_FORMAT, **kwargs): literal ``\`` character. 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 - + 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) - color = kwargs.get("color", False) - transform = kwargs.get("transform", {}) - out = io.StringIO() + def safe_color(sigil: str, string: str, color_fmt: Optional[str]) -> str: + # avoid colorizing if there is no color or the string is empty + if (color is False) or not color_fmt or not string: + return sigil + string + # escape and add the sigil here to avoid multiple concatenations + if sigil == "@": + sigil = "@@" + return clr.colorize(f"{color_fmt}{sigil}{clr.cescape(string)}@.", color=color) - def write(s, c=None): - f = clr.cescape(s) - if c is not None: - f = COLOR_FORMATS[c] + f + "@." - clr.cwrite(f, stream=out, color=color) + def format_attribute(match_object: Match) -> str: + (esc, sig, dep, hash, hash_len, attribute, close_brace, unmatched_close_brace) = ( + match_object.groups() + ) + if esc: + return esc + elif unmatched_close_brace: + raise SpecFormatStringError(f"Unmatched close brace: '{format_string}'") + elif not close_brace: + raise SpecFormatStringError(f"Missing close brace: '{format_string}'") - def write_attribute(spec, attribute, color): - attribute = attribute.lower() + current = self if dep is None else self[dep] - sig = "" - if attribute.startswith(("@", "%", "/")): - # color sigils that are inside braces - sig = attribute[0] - attribute = attribute[1:] - elif attribute.startswith("arch="): - sig = " arch=" # include space as separator - attribute = attribute[5:] - - current = spec - if attribute.startswith("^"): - attribute = attribute[1:] - dep, attribute = attribute.split(".", 1) - current = self[dep] + # Hash attributes can return early. + # NOTE: we currently treat abstract_hash like an attribute and ignore + # any length associated with it. We may want to change that. + if hash: + if sig and sig != "/": + raise SpecFormatSigilError(sig, "DAG hashes", hash) + try: + length = int(hash_len) if hash_len else None + except ValueError: + raise SpecFormatStringError(f"Invalid hash length: '{hash_len}'") + return safe_color(sig or "", current.dag_hash(length), HASH_COLOR) if attribute == "": raise SpecFormatStringError("Format string attributes must be non-empty") + attribute = attribute.lower() parts = attribute.split(".") assert parts # check that the sigil is valid for the attribute. - if sig == "@" and parts[-1] not in ("versions", "version"): + if not sig: + sig = "" + elif sig == "@" and parts[-1] not in ("versions", "version"): raise SpecFormatSigilError(sig, "versions", attribute) elif sig == "%" and attribute not in ("compiler", "compiler.name"): raise SpecFormatSigilError(sig, "compilers", attribute) - elif sig == "/" and not re.match(r"(abstract_)?hash(:\d+)?$", attribute): + elif sig == "/" and attribute != "abstract_hash": raise SpecFormatSigilError(sig, "DAG hashes", attribute) - elif sig == " arch=" and attribute not in ("architecture", "arch"): - raise SpecFormatSigilError(sig, "the architecture", attribute) - - # find the morph function for our attribute - morph = transform.get(attribute, lambda s, x: x) - - # Special cases for non-spec attributes and hashes. - # These must be the only non-dep component of the format attribute - if attribute == "spack_root": - write(morph(spec, spack.paths.spack_root)) - return - elif attribute == "spack_install": - write(morph(spec, spack.store.STORE.layout.root)) - return - elif re.match(r"hash(:\d)?", attribute): - col = "#" - if ":" in attribute: - _, length = attribute.split(":") - write(sig + morph(spec, current.dag_hash(int(length))), col) - else: - write(sig + morph(spec, current.dag_hash()), col) - return + 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): @@ -4442,7 +4413,7 @@ def write_attribute(spec, attribute, color): if part.startswith("_"): raise SpecFormatStringError("Attempted to format private attribute") else: - if isinstance(current, vt.VariantMap): + if part == "variants" and isinstance(current, vt.VariantMap): # subscript instead of getattr for variant names current = current[part] else: @@ -4466,62 +4437,31 @@ def write_attribute(spec, attribute, color): raise SpecFormatStringError(m) if isinstance(current, vn.VersionList): if current == vn.any_version: - # We don't print empty version lists - return + # don't print empty version lists + return "" if callable(current): raise SpecFormatStringError("Attempted to format callable object") + if current is None: - # We're not printing anything - return + # not printing anything + return "" # Set color codes for various attributes - col = None + color = None if "variants" in parts: - col = "+" + color = VARIANT_COLOR elif "architecture" in parts: - col = "=" + color = ARCHITECTURE_COLOR elif "compiler" in parts or "compiler_flags" in parts: - col = "%" + color = COMPILER_COLOR elif "version" in parts or "versions" in parts: - col = "@" + color = VERSION_COLOR - # Finally, write the output - write(sig + morph(spec, str(current)), col) + # return colored output + return safe_color(sig, str(current), color) - attribute = "" - in_attribute = False - escape = False - - for c in format_string: - if escape: - out.write(c) - escape = False - elif c == "\\": - escape = True - elif in_attribute: - if c == "}": - write_attribute(self, attribute, color) - attribute = "" - in_attribute = False - else: - attribute += c - else: - if c == "}": - raise SpecFormatStringError( - "Encountered closing } before opening { in %s" % format_string - ) - elif c == "{": - in_attribute = True - else: - out.write(c) - if in_attribute: - raise SpecFormatStringError( - "Format string terminated while reading attribute." "Missing terminating }." - ) - - formatted_spec = out.getvalue() - return formatted_spec.strip() + return SPEC_FORMAT_RE.sub(format_attribute, format_string).strip() def cformat(self, *args, **kwargs): """Same as format, but color defaults to auto instead of False.""" @@ -4529,6 +4469,16 @@ def cformat(self, *args, **kwargs): kwargs.setdefault("color", None) return self.format(*args, **kwargs) + @property + def spack_root(self): + """Special field for using ``{spack_root}`` in Spec.format().""" + return spack.paths.spack_root + + @property + def spack_install(self): + """Special field for using ``{spack_install}`` in Spec.format().""" + return spack.store.STORE.layout.root + def format_path( # self, format_string: str, _path_ctor: Optional[pathlib.PurePath] = None self, @@ -4554,14 +4504,21 @@ def format_path( path_ctor = _path_ctor or pathlib.PurePath format_string_as_path = path_ctor(format_string) - if format_string_as_path.is_absolute(): + if format_string_as_path.is_absolute() or ( + # Paths that begin with a single "\" on windows are relative, but we still + # want to preserve the initial "\\" to be consistent with PureWindowsPath. + # Ensure that this '\' is not passed to polite_filename() so it's not converted to '_' + (os.name == "nt" or path_ctor == pathlib.PureWindowsPath) + and format_string_as_path.parts[0] == "\\" + ): output_path_components = [format_string_as_path.parts[0]] input_path_components = list(format_string_as_path.parts[1:]) else: output_path_components = [] input_path_components = list(format_string_as_path.parts) + output_path_components += [ - fs.polite_filename(self.format(x)) for x in input_path_components + fs.polite_filename(self.format(part)) for part in input_path_components ] return str(path_ctor(*output_path_components)) diff --git a/lib/spack/spack/test/bindist.py b/lib/spack/spack/test/bindist.py index 8afd69e3c5..b30955641f 100644 --- a/lib/spack/spack/test/bindist.py +++ b/lib/spack/spack/test/bindist.py @@ -390,11 +390,11 @@ def test_built_spec_cache(mirror_dir): assert any([r["spec"] == s for r in results]) -def fake_dag_hash(spec): +def fake_dag_hash(spec, length=None): # Generate an arbitrary hash that is intended to be different than # whatever a Spec reported before (to test actions that trigger when # the hash changes) - return "tal4c7h4z0gqmixb1eqa92mjoybxn5l6" + return "tal4c7h4z0gqmixb1eqa92mjoybxn5l6"[:length] @pytest.mark.usefixtures( diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py index 76052eeb38..64fe403daa 100644 --- a/lib/spack/spack/test/config.py +++ b/lib/spack/spack/test/config.py @@ -1276,7 +1276,7 @@ def test_user_config_path_is_default_when_env_var_is_empty(working_env): def test_default_install_tree(monkeypatch, default_config): s = spack.spec.Spec("nonexistent@x.y.z %none@a.b.c arch=foo-bar-baz") - monkeypatch.setattr(s, "dag_hash", lambda: "abc123") + monkeypatch.setattr(s, "dag_hash", lambda length: "abc123") _, _, projections = spack.store.parse_install_tree(spack.config.get("config")) assert s.format(projections["all"]) == "foo-bar-baz/none-a.b.c/nonexistent-x.y.z-abc123" diff --git a/lib/spack/spack/test/modules/lmod.py b/lib/spack/spack/test/modules/lmod.py index 9038d8c9f6..5128518caa 100644 --- a/lib/spack/spack/test/modules/lmod.py +++ b/lib/spack/spack/test/modules/lmod.py @@ -146,9 +146,6 @@ def test_autoload_all(self, modulefile_content, module_configuration): assert len([x for x in content if "depends_on(" in x]) == 5 - @pytest.mark.skipif( - str(archspec.cpu.host().family) != "x86_64", reason="test data is specific for x86_64" - ) def test_alter_environment(self, modulefile_content, module_configuration): """Tests modifications to run-time environment.""" diff --git a/lib/spack/spack/test/modules/tcl.py b/lib/spack/spack/test/modules/tcl.py index 6630065373..76e854ced9 100644 --- a/lib/spack/spack/test/modules/tcl.py +++ b/lib/spack/spack/test/modules/tcl.py @@ -114,9 +114,6 @@ def test_prerequisites_all( assert len([x for x in content if "prereq" in x]) == 5 - @pytest.mark.skipif( - str(archspec.cpu.host().family) != "x86_64", reason="test data is specific for x86_64" - ) def test_alter_environment(self, modulefile_content, module_configuration): """Tests modifications to run-time environment.""" diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py index 2f4e7f43a4..171c194dc0 100644 --- a/lib/spack/spack/test/spec_semantics.py +++ b/lib/spack/spack/test/spec_semantics.py @@ -703,22 +703,25 @@ def check_prop(check_spec, fmt_str, prop, getter): actual = spec.format(named_str) assert expected == actual - def test_spec_formatting_escapes(self, default_mock_concretization): - spec = default_mock_concretization("multivalue-variant cflags=-O2") - - sigil_mismatches = [ + @pytest.mark.parametrize( + "fmt_str", + [ "{@name}", "{@version.concrete}", "{%compiler.version}", "{/hashd}", "{arch=architecture.os}", - ] + ], + ) + def test_spec_formatting_sigil_mismatches(self, default_mock_concretization, fmt_str): + spec = default_mock_concretization("multivalue-variant cflags=-O2") - for fmt_str in sigil_mismatches: - with pytest.raises(SpecFormatSigilError): - spec.format(fmt_str) + with pytest.raises(SpecFormatSigilError): + spec.format(fmt_str) - bad_formats = [ + @pytest.mark.parametrize( + "fmt_str", + [ r"{}", r"name}", r"\{name}", @@ -728,11 +731,12 @@ def test_spec_formatting_escapes(self, default_mock_concretization): r"{dag_hash}", r"{foo}", r"{+variants.debug}", - ] - - for fmt_str in bad_formats: - with pytest.raises(SpecFormatStringError): - spec.format(fmt_str) + ], + ) + def test_spec_formatting_bad_formats(self, default_mock_concretization, fmt_str): + spec = default_mock_concretization("multivalue-variant cflags=-O2") + with pytest.raises(SpecFormatStringError): + spec.format(fmt_str) def test_combination_of_wildcard_or_none(self): # Test that using 'none' and another value raises @@ -1138,12 +1142,12 @@ def _check_spec_format_path(spec_str, format_str, expected, path_ctor=None): r"\\hostname\sharename\{name}\{version}", r"\\hostname\sharename\git-test\git.foo_bar", ), - # Windows doesn't attribute any significance to a leading - # "/" so it is discarded + # leading '/' is preserved on windows but converted to '\' + # note that it's still not "absolute" -- absolute windows paths start with a drive. ( "git-test@git.foo/bar", r"/installroot/{name}/{version}", - r"installroot\git-test\git.foo_bar", + r"\installroot\git-test\git.foo_bar", ), ], ) diff --git a/lib/spack/spack/variant.py b/lib/spack/spack/variant.py index 07aafe8945..bb4138116f 100644 --- a/lib/spack/spack/variant.py +++ b/lib/spack/spack/variant.py @@ -638,6 +638,9 @@ def copy(self): return clone def __str__(self): + if not self: + return "" + # print keys in order sorted_keys = sorted(self.keys()) diff --git a/lib/spack/spack/version/version_types.py b/lib/spack/spack/version/version_types.py index b772cd9d90..f35192192d 100644 --- a/lib/spack/spack/version/version_types.py +++ b/lib/spack/spack/version/version_types.py @@ -146,13 +146,11 @@ def from_string(string: str): @staticmethod def typemin(): - return StandardVersion("", ((), (ALPHA,)), ("",)) + return _STANDARD_VERSION_TYPEMIN @staticmethod def typemax(): - return StandardVersion( - "infinity", ((VersionStrComponent(len(infinity_versions)),), (FINAL,)), ("",) - ) + return _STANDARD_VERSION_TYPEMAX def __bool__(self): return True @@ -390,6 +388,13 @@ def up_to(self, index): return self[:index] +_STANDARD_VERSION_TYPEMIN = StandardVersion("", ((), (ALPHA,)), ("",)) + +_STANDARD_VERSION_TYPEMAX = StandardVersion( + "infinity", ((VersionStrComponent(len(infinity_versions)),), (FINAL,)), ("",) +) + + class GitVersion(ConcreteVersion): """Class to represent versions interpreted from git refs. @@ -1019,6 +1024,9 @@ def __hash__(self): return hash(tuple(self.versions)) def __str__(self): + if not self.versions: + return "" + return ",".join( f"={v}" if isinstance(v, StandardVersion) else str(v) for v in self.versions ) @@ -1127,7 +1135,9 @@ def _prev_version(v: StandardVersion) -> StandardVersion: components[1::2] = separators[: len(release)] if prerelease_type != FINAL: components.extend((PRERELEASE_TO_STRING[prerelease_type], *prerelease[1:])) - return StandardVersion("".join(str(c) for c in components), (release, prerelease), separators) + + # this is only used for comparison functions, so don't bother making a string + return StandardVersion(None, (release, prerelease), separators) def Version(string: Union[str, int]) -> Union[GitVersion, StandardVersion]: