docs: schema validation and fixes (spack#51324)

Signed-off-by: Harmen Stoppels <[email protected]>

Author: haampie

.github/workflows/bin/format-rst.py

@@ -7,6 +7,7 @@
 whitespace. It exits with a non-zero status if any files were modified."""
 
 import difflib
+import importlib
 import io
 import json
 import os
@@ -21,6 +22,25 @@
 from docutils.parsers.rst import Directive, directives
 from ruamel.yaml import YAML
 
+from spack.vendor import jsonschema
+
+import spack.schema
+
+#: Map Spack config sections to their corresponding JSON schema
+SECTION_AND_SCHEMA = [
+    # The first property's key is the config section name
+    (next(iter(m.schema["properties"])), m.schema)
+    # Dynamically load all modules in spack.schema to be future-proof
+    for m in (
+        importlib.import_module(f"spack.schema.{f[:-3]}")
+        for f in os.listdir(os.path.dirname(spack.schema.__file__))
+        if f.endswith(".py") and f != "__init__.py"
+    )
+    if hasattr(m, "schema") and len(m.schema.get("properties", {})) == 1
+]
+
+assert SECTION_AND_SCHEMA, "no schemas found"
+
 END_OF_SENTENCE = re.compile(
     r"""
 (
@@ -39,6 +59,32 @@
 DOCUTILS_SETTING = {"report_level": 5, "raw_enabled": False, "file_insertion_enabled": False}
 
 
+def _warning(msg: str) -> str:
+    return f"\033[33mwarning:\033[0m {msg}"
+
+
+class Warning:
+    def __init__(self, path: str, line: int, message: str) -> None:
+        self.path = path
+        self.line = line
+        self.message = message
+
+    def __str__(self) -> str:
+        return _warning(f"{self.path}:{self.line}: {self.message}")
+
+
+class CodeBlockWarning(Warning):
+    def __init__(self, path: str, line: int, message: str, diff: str):
+        super().__init__(path, line, f"{message}\n{diff}")
+
+    def __str__(self) -> str:
+        return _warning(f"{self.path}:{self.line}: {self.message}")
+
+
+class ValidationWarning(Warning):
+    pass
+
+
 class SphinxCodeBlock(Directive):
     """Defines a code-block directive with the options Sphinx supports."""
 
@@ -89,15 +135,25 @@ def _is_node_in_table(node: nodes.Node) -> bool:
     return False
 
 
-def _format_code_blocks(document: nodes.document, path: str) -> None:
+def _validate_schema(data: object) -> None:
+    if not isinstance(data, dict):
+        return
+    for section, schema in SECTION_AND_SCHEMA:
+        if section in data:
+            jsonschema.validate(data, schema)
+
+
+def _format_code_blocks(document: nodes.document, path: str) -> List[Warning]:
     """Try to parse and format Python, YAML, and JSON code blocks. This does *not* update the
-    sources, but merely warns. That's because not all code examples are meant to be valid."""
+    sources, but collects issues for later reporting. Returns a list of warnings."""
+    issues: List[Warning] = []
     for code_block in document.findall(nodes.literal_block):
         language = code_block.attributes.get("language", "")
         if language not in ("python", "yaml", "json"):
             continue
         original = code_block.astext()
         line = code_block.line if code_block.line else 0
+        possible_config_data = None
 
         try:
             if language == "python":
@@ -107,17 +163,22 @@ def _format_code_blocks(document: nodes.document, path: str) -> None:
                 yaml.width = 10000  # do not wrap lines
                 yaml.preserve_quotes = True  # do not force particular quotes
                 buf = io.BytesIO()
-                yaml.dump(yaml.load(original), buf)
+                possible_config_data = yaml.load(original)
+                yaml.dump(possible_config_data, buf)
                 formatted = buf.getvalue().decode("utf-8")
             elif language == "json":
                 formatted = json.dumps(json.loads(original), indent=2)
             else:
                 assert False
         except Exception as e:
-            print(
-                f"{path}:{line}: formatting failed: {e}: {original!r}", flush=True, file=sys.stderr
-            )
+            issues.append(Warning(path, line, f"formatting failed: {e}: {original!r}"))
             continue
+
+        try:
+            _validate_schema(possible_config_data)
+        except jsonschema.ValidationError as e:
+            issues.append(ValidationWarning(path, line, f"schema validation failed: {e.message}"))
+
         if formatted == original:
             continue
         diff = "\n".join(
@@ -130,7 +191,8 @@ def _format_code_blocks(document: nodes.document, path: str) -> None:
             )
         )
         if diff:
-            print(diff, flush=True, file=sys.stderr)
+            issues.append(CodeBlockWarning(path, line, "formatting suggested:", diff))
+    return issues
 
 
 def _format_paragraphs(document: nodes.document, path: str, src_lines: List[str]) -> bool:
@@ -171,15 +233,15 @@ def _format_paragraphs(document: nodes.document, path: str, src_lines: List[str]
     return modified
 
 
-def reformat_rst_file(path: str) -> bool:
+def reformat_rst_file(path: str, warnings: List[Warning]) -> bool:
     """Reformat a reStructuredText file "in-place". Returns True if modified, False otherwise."""
     with open(path, "r", encoding="utf-8") as f:
         src = f.read()
 
     src_lines = src.splitlines()
     document: nodes.document = publish_doctree(src, settings_overrides=DOCUTILS_SETTING)
 
-    _format_code_blocks(document, path)
+    warnings.extend(_format_code_blocks(document, path))
 
     if not _format_paragraphs(document, path, src_lines):
         return False
@@ -194,10 +256,22 @@ def reformat_rst_file(path: str) -> bool:
 
 def main(*files: str) -> None:
     modified = False
+    warnings: List[Warning] = []
     for f in files:
-        modified |= reformat_rst_file(f)
+        modified |= reformat_rst_file(f, warnings)
+
     if modified:
         subprocess.run(["git", "--no-pager", "diff", "--color=always", "--", *files])
+
+    for warning in sorted(warnings, key=lambda w: isinstance(w, ValidationWarning)):
+        print(warning, flush=True, file=sys.stderr)
+
+    if warnings:
+        print(
+            _warning(f"completed with {len(warnings)} potential issues"),
+            flush=True,
+            file=sys.stderr,
+        )
     sys.exit(1 if modified else 0)
 
 

.github/workflows/prechecks.yml

@@ -57,7 +57,7 @@ jobs:
         bin/spack license verify
         pylint -j $(nproc) --disable=all --enable=unspecified-encoding --ignore-paths=lib/spack/spack/vendor lib
     - name: Run style tests (docs)
-      run: .github/workflows/bin/format-rst.py $(git ls-files 'lib/spack/docs/*.rst')
+      run: bin/spack python .github/workflows/bin/format-rst.py $(git ls-files 'lib/spack/docs/*.rst')
 
 
   # Check that spack can bootstrap the development environment on Python 3.6 - RHEL8

lib/spack/docs/configuration.rst

@@ -36,7 +36,8 @@ Here is an example ``config.yaml`` file:
 .. code-block:: yaml
 
    config:
-     install_tree: $spack/opt/spack
+     install_tree:
+       root: $spack/opt/spack
      build_stage:
      - $tempdir/$user/spack-stage
      - ~/.spack/stage
@@ -295,7 +296,8 @@ If your configurations look like this:
    :name: code-example-defaults-config-yaml
 
    config:
-     install_tree: $spack/opt/spack
+     install_tree:
+       root: $spack/opt/spack
      build_stage:
      - $tempdir/$user/spack-stage
      - ~/.spack/stage
@@ -306,7 +308,8 @@ If your configurations look like this:
    :name: code-example-user-config-yaml
 
    config:
-     install_tree: /some/other/directory
+     install_tree:
+       root: /some/other/directory
 
 
 Spack will only override ``install_tree`` in the ``config`` section, and will take the site preferences for other settings.
@@ -317,7 +320,8 @@ You can see the final, combined configuration with the ``spack config get <confi
 
    $ spack config get config
    config:
-     install_tree: /some/other/directory
+     install_tree:
+       root: /some/other/directory
      build_stage:
      - $tempdir/$user/spack-stage
      - ~/.spack/stage
@@ -339,15 +343,17 @@ For example:
    :name: code-example-append-install-tree
 
    config:
-     install_tree-: /my/custom/suffix/
+     install_tree:
+       root-: /my/custom/suffix/
 
-Spack will then append to the lower-precedence configuration under the ``install_tree-:`` section:
+Spack will then append to the lower-precedence configuration under the ``root`` key:
 
 .. code-block:: console
 
    $ spack config get config
    config:
-     install_tree: /some/other/directory/my/custom/suffix
+     install_tree:
+       root: /some/other/directory/my/custom/suffix
      build_stage:
      - $tempdir/$user/spack-stage
      - ~/.spack/stage
@@ -361,7 +367,8 @@ Similarly, ``+:`` can be used to *prepend* to a path or name:
    :name: code-example-prepend-install-tree
 
    config:
-     install_tree+: /my/custom/suffix/
+     install_tree:
+       root+: /my/custom/suffix/
 
 
 .. _config-overrides:
@@ -380,15 +387,17 @@ For example:
    :name: code-example-override-config-section
 
    config::
-     install_tree: /some/other/directory
+     install_tree:
+       root: /some/other/directory
 
 Spack will ignore all lower-precedence configuration under the ``config::`` section:
 
 .. code-block:: console
 
    $ spack config get config
    config:
-     install_tree: /some/other/directory
+     install_tree:
+       root: /some/other/directory
 
 
 List-valued settings
@@ -428,7 +437,8 @@ The list in the higher-precedence scope is *prepended* to the defaults.
 
    $ spack config get config
    config:
-     install_tree: /some/other/directory
+     install_tree:
+       root: /some/other/directory
      build_stage:
      - /lustre-scratch/$user/spack
      - ~/mystage
@@ -457,7 +467,8 @@ The merged configuration would look like this:
 
    $ spack config get config
    config:
-     install_tree: /some/other/directory
+     install_tree:
+       root: /some/other/directory
      build_stage:
        - /lustre-scratch/$user/spack
        - ~/mystage
@@ -565,7 +576,8 @@ For example, to see the fully merged ``config.yaml``, you can type:
      verify_ssl: true
      dirty: false
      build_jobs: 8
-     install_tree: $spack/opt/spack
+     install_tree:
+       root: $spack/opt/spack
      template_dirs:
      - $spack/templates
      directory_layout: {architecture}/{compiler.name}-{compiler.version}/{name}-{version}-{hash}

lib/spack/docs/containers.rst

@@ -121,7 +121,8 @@ The ``Dockerfile`` that gets created uses multi-stage builds and other technique
    &&   echo "  concretizer:" \
    &&   echo "    unify: true" \
    &&   echo "  config:" \
-   &&   echo "    install_tree: /opt/software" \
+   &&   echo "    install_tree: \
+   &&   echo "      root: /opt/software" \
    &&   echo "  view: /opt/view") > /opt/spack-environment/spack.yaml
 
    # Install the software, remove unnecessary deps
@@ -311,7 +312,8 @@ uses ``spack/almalinux9:0.22.0`` and ``almalinux:9`` for the stages where the so
    &&   echo '  concretizer:' \
    &&   echo '    unify: true' \
    &&   echo '  config:' \
-   &&   echo '    install_tree: /opt/software' \
+   &&   echo '    install_tree: ' \
+   &&   echo '      root: /opt/software' \
    &&   echo '  view: /opt/views/view') > /opt/spack-environment/spack.yaml
    [ ... ]
    # Bare OS image to run the installed executables
@@ -420,7 +422,8 @@ produces, for instance, the following ``Dockerfile``:
    &&   echo "  concretizer:" \
    &&   echo "    unify: true" \
    &&   echo "  config:" \
-   &&   echo "    install_tree: /opt/software" \
+   &&   echo "    install_tree: " \
+   &&   echo "      root: /opt/software" \
    &&   echo "  view: /opt/view") > /opt/spack-environment/spack.yaml
 
    # Install the software, remove unnecessary deps
@@ -559,7 +562,8 @@ The recipe that gets generated contains the two extra instructions that we added
    &&   echo "  config:" \
    &&   echo "    template_dirs:" \
    &&   echo "    - /tmp/environment/templates" \
-   &&   echo "    install_tree: /opt/software" \
+   &&   echo "    install_tree: " \
+   &&   echo "      root: /opt/software" \
    &&   echo "  view: /opt/view") > /opt/spack-environment/spack.yaml
 
    # Install the software, remove unnecessary deps

lib/spack/docs/env_vars_yaml.rst

@@ -23,7 +23,7 @@ For example:
     set:
       ENVAR_TO_SET_IN_ENV_LOAD: "FOO"
     unset:
-      ENVAR_TO_UNSET_IN_ENV_LOAD:
+    - ENVAR_TO_UNSET_IN_ENV_LOAD
     prepend_path:
       PATH_LIST: "path/to/prepend"
     append_path:

lib/spack/docs/environments.rst

@@ -862,7 +862,7 @@ The environment can be configured to set, unset, prepend, or append using ``env_
       set:
         ENVAR_TO_SET_IN_ENV_LOAD: "FOO"
       unset:
-        ENVAR_TO_UNSET_IN_ENV_LOAD:
+      - ENVAR_TO_UNSET_IN_ENV_LOAD
       prepend_path:
         PATH_LIST: "path/to/prepend"
       append_path:

lib/spack/docs/frequently_asked_questions.rst

@@ -70,7 +70,7 @@ The following set of criteria (from lowest to highest precedence) explains commo
         foo:
           require:
           - "@1.2: +mpi"
-          conflicts:
+          conflict:
           - "@1.4"
 
 Requirements and constraints restrict the set of possible solutions, while reuse behavior and preferences influence what an optimal solution looks like.

lib/spack/docs/module_file_support.rst

@@ -537,8 +537,8 @@ If the ``use_view`` value is set in the config, then the prefix inspections for
          - PATH
      view:
        my_view:
+         root: /path/to/my/view
          projections:
-           root: /path/to/my/view
            all: "{name}-{hash}"
 
 The ``spack`` key is relevant to :ref:`environment <environments>` configuration, and the view key is discussed in detail in the section on :ref:`Configuring environment views <configuring_environment_views>`.