fix(vendor): make main() exit non-zero when any crawler target raises (#872)

.github/workflows/cron-vendor.yml

@@ -120,7 +120,66 @@ jobs:
           export PCAPKIT_VENDOR_RETRY=10
           export PCAPKIT_CI_MODE=1
 
-          pcapkit-vendor
+          # `pcapkit-vendor` (#872) exits non-zero if any target raised, and
+          # the owner's ruling on that is: discard a failed sub-vendor's own
+          # changes, keep every zero-exited one's, and never fail the whole
+          # job over it -- the targets that DID succeed still need isort,
+          # diff, commit and push below. `set +e`/`set -e` around the one
+          # command (not a bare `pcapkit-vendor || true`) so $vendor_exit
+          # actually carries the exit code instead of it being thrown away.
+          set +e
+          pcapkit-vendor 2> "$RUNNER_TEMP/vendor-stderr.log"
+          vendor_exit=$?
+          set -e
+
+          # Always replayed into the step log, success or failure: this is
+          # each failing target's qualified name and exception repr (from
+          # __main__.py's run()) plus its VendorRuntimeWarning line, i.e.
+          # exactly what a live, uncaptured `pcapkit-vendor` would have
+          # written to stderr directly. `|| true` deliberately: this line
+          # runs after `set -e` is restored above, and if $RUNNER_TEMP were
+          # ever unset or unwritable the redirect on the previous command
+          # would already have failed to create the log file at all --
+          # `cat` on a missing file returns 1 under `-e` and would abort the
+          # whole step right here, exactly the "one dead crawler kills every
+          # good regeneration" failure mode this PR exists to remove.
+          cat "$RUNNER_TEMP/vendor-stderr.log" >&2 || true
+
+          if [ "$vendor_exit" -ne 0 ]; then
+            # Anchored at the start of the line: the unanchored `failed: `
+            # also matches inside the VendorRuntimeWarning line whenever the
+            # exception's own text happens to contain "failed: " (e.g.
+            # ConnectionError('request failed: 503')), since that line embeds
+            # the same {error!r} __main__.py's run() already put in the
+            # print() line above it -- double-counting one failure as two
+            # ::warning:: annotations. Only the print() line starts with the
+            # bare qualified name; the warning line starts with a file path.
+            failed_targets=$(grep -E '^[A-Za-z_][A-Za-z0-9_.]* failed: ' \
+                                  "$RUNNER_TEMP/vendor-stderr.log" || true)
+
+            {
+              echo "### :warning: \`pcapkit-vendor\` reported failing target(s) (exit $vendor_exit)"
+              echo
+              echo '```'
+              if [ -n "$failed_targets" ]; then
+                echo "$failed_targets"
+              else
+                echo '(no target name could be extracted from stderr; see the step log above)'
+              fi
+              echo '```'
+              echo
+              echo 'Const files from targets that exited zero are still isorted, diffed, committed and pushed below -- only the failing target(s) above were left as they were.'
+            } >> "$GITHUB_STEP_SUMMARY"
+
+            if [ -n "$failed_targets" ]; then
+              while IFS= read -r line; do
+                echo "::warning title=pcapkit-vendor target failed::${line}"
+              done <<< "$failed_targets"
+            else
+              echo "::warning title=pcapkit-vendor exited $vendor_exit::see the step log above for which target(s) failed"
+            fi
+          fi
+
           # `find -mindepth 2`, not a `pcapkit/const/*/*.py` shell glob (#765):
           # this job runs on `macos-latest`, whose default `bash` on PATH is
           # the system's, unlike the Makefile's own `SHELL := $(shell command

pcapkit/vendor/__main__.py

@@ -10,8 +10,12 @@
 """
 
 import argparse
+import contextlib
 import importlib
+import os
+import shutil
 import sys
+import tempfile
 import traceback
 import warnings
 from typing import TYPE_CHECKING
@@ -23,7 +27,7 @@
 
 if TYPE_CHECKING:
     from argparse import ArgumentParser
-    from typing import Type
+    from typing import Iterator, Type
 
     from pcapkit.vendor.default import Vendor
 
@@ -43,28 +47,165 @@ def get_parser() -> 'ArgumentParser':
     return parser
 
 
-def run(vendor: 'Type[Vendor]') -> 'None':
+@contextlib.contextmanager
+def _snapshot_and_restore(vendor: 'Type[Vendor]') -> 'Iterator[None]':
+    """Copy a target's const file aside before it runs; restore it if it raises.
+
+    The owner's ruling on GitHub issue #872, verbatim: *"an easier path is
+    simply keep a copy before running the sub-vendor and revert if anything
+    failed."* This is that -- at the per-target boundary :func:`run` already
+    owns, which is also exactly where the ruling's ``(b)``, "only discard
+    changes made by a non-zero sub-vendor", wants the discarding to happen.
+
+    It is a *wider* guarantee than protecting the single
+    ``open``/``print`` pair :meth:`~pcapkit.vendor.default.Vendor.__init__`
+    happens to write through would give: it covers any way a crawler's own
+    code could end up touching its const file, not only that one write.
+
+    The destination is resolved via
+    :meth:`~pcapkit.vendor.default.Vendor._dest_path` -- a classmethod for
+    exactly this reason -- called on ``vendor`` itself, *before*
+    :meth:`Vendor.__init__` ever runs its fetch, its render and its write.
+    If that resolution fails -- a crawler outside the real
+    :mod:`pcapkit.vendor` tree, such as a test double, or one whose
+    ``_dest_path`` genuinely cannot be resolved without instance state --
+    the snapshot is skipped and ``vendor()`` runs unprotected, exactly as it
+    always did; not being able to *name* the file is not itself a reason to
+    fail the target, and this crawler goes on to fail loudly anyway once
+    :meth:`Vendor.__init__` calls the same ``_dest_path`` itself.
+
+    That is one of two ways this function can decline to protect a target,
+    and the two are not symmetric. The other is :func:`tempfile.mkstemp`
+    failing to create the backup file -- most commonly a destination
+    directory with no write permission, which needs a *directory*-level
+    operation this function performs (creating a new entry) that
+    :meth:`Vendor.__init__`'s own ``open(const_file, 'w')`` never needed (a
+    *file*-level operation on a file that already exists). Unlike a
+    ``_dest_path`` failure, this one is **not** swallowed: it happens before
+    ``vendor()`` is ever called, so it propagates straight out of this
+    function and fails the target outright, with the previous file
+    untouched because nothing was ever attempted. So: a path that cannot be
+    named lets the target proceed unprotected; a path that can be named but
+    not backed up stops the target before it starts. Both leave the
+    previous file exactly as it was, for different reasons.
+
+    A symlinked destination has a related divergence from the pre-#872
+    behaviour -- documented against the write path by rounds 4-8 (deleted
+    along with :meth:`~pcapkit.vendor.default.Vendor._write_atomic`, though
+    the underlying behaviour persists here instead): ``open(const_file,
+    'w')`` writes *through* a symlink, into whatever file it points at,
+    leaving the link itself untouched. ``os.replace(backup, const_file)``
+    on restore instead replaces the *link itself* with a regular file.
+    Measured, for ``link.py`` symlinked to ``real.py``: after a failure and
+    restore, ``real.py`` is left however the crawler's failed write left
+    it (truncated, in this case -- nothing here restores the file a
+    symlink used to point at), ``link.py`` is now a regular file holding
+    the backup's content, and ``os.path.islink(link.py)`` is
+    :data:`False`. ``find pcapkit/const -type l`` is still empty, so this
+    remains latent.
+
+    Nothing is copied at all when the destination does not exist yet: there
+    is no previous file for a failure to discard, so there is nothing this
+    context manager needs to do beyond letting the body run.
+
+    Args:
+        vendor: Subclass of :class:`~pcapkit.vendor.default.Vendor` about to
+            be run.
+
+    Yields:
+        Nothing; the caller's ``vendor()`` call belongs inside the ``with``.
+
+    """
+    try:
+        const_file = vendor._dest_path()  # pylint: disable=protected-access
+    except Exception:
+        yield
+        return
+
+    if not os.path.isfile(const_file):
+        yield
+        return
+
+    fd, backup = tempfile.mkstemp(dir=os.path.dirname(const_file),
+                                  prefix=f'.{os.path.basename(const_file)}.',
+                                  suffix='.bak')
+    os.close(fd)
+    shutil.copy2(const_file, backup)
+    try:  # pylint: disable=no-else-raise
+        yield
+    except BaseException:
+        # os.replace() disposes of the backup by moving it back onto
+        # const_file -- but only once it succeeds. If it does not, the
+        # original exception is masked (it survives only as __context__ on
+        # whatever os.replace raises) and the backup is left orphaned; ``raise``
+        # is unconditional here on purpose, since a failed restore is still a
+        # failure the target must report, not a success to fall through to.
+        #
+        # The `else:` below is deliberate, not a style default pylint would
+        # pick: collapsing it into an unindented statement after this whole
+        # `try` (as an earlier round of this same PR did) makes "cleanup
+        # only runs on success" a fact that has to be re-derived from
+        # "`raise` never falls through", rather than one the structure
+        # states directly -- and it silently stopped being true the moment
+        # someone deleted that `raise`.  A mutation test proved it: with the
+        # `raise` gone, the unindented `os.remove(backup)` still ran, after
+        # `os.replace` had already renamed the backup away, so it raised
+        # FileNotFoundError instead of the crawler's real error -- and every
+        # existing test still passed, because run() catches that too.
+        os.replace(backup, const_file)
+        raise
+    else:
+        # contextlib, per the owner's stated preference over a hand-rolled
+        # try/finally: a failure to remove the backup after a *successful*
+        # run must not itself turn that success into run() -> False.
+        with contextlib.suppress(OSError):
+            os.remove(backup)
+
+
+def run(vendor: 'Type[Vendor]') -> 'bool':
     """Script runner.
 
     Args:
         vendor: Subclass of :class:`~pcapkit.vendor.default.Vendor` from :mod:`pcapkit.vendor`.
 
+    Returns:
+        :data:`True` if ``vendor`` ran to completion, :data:`False` if it raised --
+        this is what lets :func:`main` turn a crawler failure into a non-zero exit
+        code instead of a silently-ignored no-op.
+
     Warns:
         VendorRuntimeWarning: If failed to initiate the ``vendor`` class.
 
     """
     logger.info(f'{vendor.__module__}.{vendor.__name__}: {vendor.__doc__}')
     try:
-        vendor()
+        with _snapshot_and_restore(vendor):
+            vendor()
     except Exception as error:
         if VERBOSE:
             traceback.print_exc()
+        # Unconditional, unlike the warning below: VendorRuntimeWarning is
+        # filterable and VERBOSE defaults false, so without a plain print a CI
+        # log that ignores warnings would show nothing at all about which
+        # target broke or why -- exactly the failure mode this fix exists for.
+        print(f'{vendor.__module__}.{vendor.__name__} failed: {error!r}', file=sys.stderr)
         warn(f'{vendor.__module__}.{vendor.__name__} <{error!r}>', VendorRuntimeWarning, stacklevel=2)
+        return False
+    return True
 
 
 def main() -> 'int':
     """Entrypoint.
 
+    Every target is attempted regardless of earlier failures -- a crawler that
+    is down should not block regeneration of everything else -- but the exit
+    code reflects whether *all* of them succeeded, so a no-op regeneration
+    (crawler raised, previous const file left untouched) is distinguishable
+    from a real one at the process boundary, which is what CI actually checks.
+
+    Returns:
+        ``0`` if every target ran to completion, ``1`` if any target raised.
+
     Warns:
         InvalidVendorWarning: If vendor target not found in :mod:`pcapkit.vendor` module.
 
@@ -88,9 +229,11 @@ def main() -> 'int':
 
     # with multiprocessing.Pool() as pool:
     #     pool.map(run, target_list)
+    success = True
     for vendor in target_list:
-        run(vendor)
-    return 0
+        if not run(vendor):
+            success = False
+    return 0 if success else 1
 
 
 if __name__ == '__main__':

pcapkit/vendor/default.py

@@ -430,7 +430,8 @@ def __init__(self) -> 'None':
         with open(const_file, 'w') as file:  # pylint: disable=unspecified-encoding
             print(context, file=file)
 
-    def _dest_path(self) -> 'str':
+    @classmethod
+    def _dest_path(cls) -> 'str':
         """Resolve the ``const/`` file this crawler's module mirrors.
 
         :attr:`~Vendor.__module__` sits somewhere under the :mod:`pcapkit.vendor`
@@ -470,12 +471,23 @@ def _dest_path(self) -> 'str':
         ``sys.exit(SomeCrawler())``, so running one from a second checkout, or
         from a module that never sits under ``vendor/`` at all, resolves
         :mod:`pcapkit.vendor` from wherever it is installed while
-        ``inspect.getfile(type(self))`` names a file elsewhere entirely --  and
+        ``inspect.getfile(cls)`` names a file elsewhere entirely --  and
         ``Vendor.__init__`` then opens that path with ``'w'``, silently
         truncating whatever :attr:`~Vendor.__module__` actually names instead of
         raising. So the escape is rejected outright, rather than trusted to
         produce a harmless-looking wrong path.
 
+        This is a classmethod, not an instance method, deliberately: nothing
+        in the body ever needed instance state -- it only ever consulted
+        ``type(self)``, so ``cls`` is exactly that, with no instance required
+        to obtain it. That lets a caller resolve a crawler's destination
+        *without* instantiating it, and so without triggering
+        :meth:`Vendor.__init__`'s network fetch, render and write --
+        :func:`pcapkit.vendor.__main__.run`'s pre-run snapshot (GitHub issue
+        #872) is exactly such a caller. Every crawler defined in this
+        codebase inherits this implementation unchanged; none overrides
+        ``_dest_path``, so the classmethod is authoritative for all of them.
+
         Returns:
             Absolute path of the constant module this crawler should write.
 
@@ -488,7 +500,7 @@ def _dest_path(self) -> 'str':
         import pcapkit.vendor  # pylint: disable=import-outside-toplevel
 
         vendor_root = os.path.dirname(os.path.abspath(pcapkit.vendor.__file__))
-        module_file = os.path.abspath(inspect.getfile(type(self)))
+        module_file = os.path.abspath(inspect.getfile(cls))
 
         rel_path = os.path.relpath(module_file, vendor_root)
         if rel_path.split(os.sep, 1)[0] == os.pardir: