dev: generated checklist of DF defect mitigations for release review

.github/workflows/watch-df-release.yml

@@ -77,6 +77,15 @@ jobs:
       with:
         webhook-url: ${{ secrets.DISCORD_TEAM_PRIVATE_WEBHOOK_URL }}
         content: "<@&${{ secrets.DISCORD_TEAM_ROLE_ID }}> Steam ${{ matrix.df_steam_branch }} branch updated (build id: ${{ env.BUILDID }})"
+    - name: Check out dfhack develop for mitigation checklist
+      uses: actions/checkout@v4
+      if: env.BUILDID
+      with:
+        ref: develop
+        submodules: true
+    - name: Emit DF mitigation checklist to job summary
+      if: env.BUILDID
+      run: python3 ci/list-df-mitigations.py >> "$GITHUB_STEP_SUMMARY"
     - name: Launch symbol generation workflow
       if: env.BUILDID && matrix.dfhack_ref
       env:

ci/list-df-mitigations.py

@@ -0,0 +1,166 @@
+#!/usr/bin/env python3
+"""List DF defect mitigations that should be re-reviewed on a new DF release.
+
+DFHack works around a number of defects in Dwarf Fortress itself. When Bay 12
+releases a new version of DF, some of those mitigations may need to be adjusted
+or removed. This script generates the review checklist so the release
+coordinator does not have to track the mitigations by hand.
+
+Mitigations are marked where they live in the code with a comment containing
+the ``DF-MITIGATION:`` marker, e.g.::
+
+    // DF-MITIGATION: site_id is not assigned on reclaim until the first save
+
+In addition, every ``fix/*`` script in the scripts repo is a mitigation by
+definition, so they are listed automatically without needing a marker. Their
+descriptions come from the ``:summary:`` field of their documentation.
+
+The checklist is printed to stdout. If the GITHUB_STEP_SUMMARY environment
+variable is set (i.e. when running in a GitHub Actions job), it is also
+appended to the job summary so it is visible on the workflow run page.
+"""
+
+import os
+import re
+
+REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+SELF = relpath_self = os.path.relpath(os.path.abspath(__file__),
+                                      REPO_ROOT).replace(os.sep, '/')
+
+MARKER_RE = re.compile(r'DF-MITIGATION:?\s*(.*?)\s*(?:\*+/)?\s*$')
+SUMMARY_RE = re.compile(r'^\s*:summary:\s*(.*)$')
+
+# directories that never contain our own code
+SKIP_DIRS = {'.git', 'build', 'depends', 'package'}
+
+SOURCE_EXTS = {
+    '.c', '.cc', '.cpp', '.cxx', '.h', '.hh', '.hpp',
+    '.lua', '.py', '.rb', '.js', '.ts', '.sh', '.ps1',
+}
+
+
+def iter_source_files(root):
+    for dirpath, dirnames, filenames in os.walk(root):
+        dirnames[:] = [d for d in dirnames
+                       if d not in SKIP_DIRS and not d.startswith('.')]
+        for name in filenames:
+            if os.path.splitext(name)[1].lower() in SOURCE_EXTS:
+                yield os.path.join(dirpath, name)
+
+
+def relpath(path):
+    return os.path.relpath(path, REPO_ROOT).replace(os.sep, '/')
+
+
+def collect_markers():
+    entries = []
+    for path in iter_source_files(REPO_ROOT):
+        if relpath(path) == SELF:
+            continue
+        try:
+            with open(path, encoding='utf-8', errors='replace') as f:
+                for lineno, line in enumerate(f, 1):
+                    match = MARKER_RE.search(line)
+                    if match:
+                        entries.append((relpath(path), lineno,
+                                        match.group(1) or '(no description)'))
+        except OSError:
+            continue
+    return sorted(entries)
+
+
+def doc_summary(script_path):
+    """Look up the :summary: field in the script's documentation."""
+    rel = relpath(script_path)
+    name = os.path.splitext(os.path.basename(rel))[0]
+    for doc in (os.path.join(REPO_ROOT, 'scripts', 'docs', 'fix',
+                             name + '.rst'),
+                os.path.join(REPO_ROOT, 'scripts', 'docs', name + '.rst')):
+        try:
+            with open(doc, encoding='utf-8', errors='replace') as f:
+                for line in f:
+                    match = SUMMARY_RE.match(line)
+                    if match:
+                        return match.group(1).strip()
+        except OSError:
+            continue
+    return None
+
+
+def script_description(path):
+    """Fall back to the first substantive comment line of the script."""
+    try:
+        with open(path, encoding='utf-8', errors='replace') as f:
+            for line in f:
+                line = line.strip()
+                if line.startswith('--'):
+                    desc = line.lstrip('-').strip()
+                    if desc and not desc.startswith('@'):
+                        return desc
+                elif line:
+                    return None
+    except OSError:
+        pass
+    return None
+
+
+def collect_fix_scripts():
+    """fix/* scripts (and the top-level fix* scripts) are all DF bug
+    mitigations."""
+    entries = []
+    scripts_dir = os.path.join(REPO_ROOT, 'scripts')
+    if not os.path.isdir(scripts_dir):
+        return entries
+    for dirpath, _dirnames, filenames in os.walk(scripts_dir):
+        if relpath(dirpath) not in ('scripts', 'scripts/fix'):
+            continue
+        for name in filenames:
+            if not name.endswith('.lua'):
+                continue
+            if relpath(dirpath) == 'scripts' and not name.startswith('fix'):
+                continue
+            path = os.path.join(dirpath, name)
+            desc = doc_summary(path) or script_description(path) \
+                or '(no description)'
+            entries.append((relpath(path), desc))
+    return sorted(entries)
+
+
+def main():
+    lines = [
+        '## DF defect mitigations',
+        '',
+        'A new DF release may have fixed some of the defects that DFHack works',
+        'around. Review each entry and adjust or remove mitigations (and their',
+        '`DF-MITIGATION` markers) that are no longer needed.',
+        '',
+        '### Code sites',
+        '',
+    ]
+    markers = collect_markers()
+    if markers:
+        for path, lineno, desc in markers:
+            lines.append(f'- [ ] `{path}:{lineno}`: {desc}')
+    else:
+        lines.append('- (none found)')
+
+    lines += ['', '### `fix/*` scripts', '']
+    fixes = collect_fix_scripts()
+    if fixes:
+        for path, desc in fixes:
+            lines.append(f'- [ ] `{path}`: {desc}')
+    else:
+        lines.append('- (scripts repo not checked out)')
+
+    lines.append('')
+    output = '\n'.join(lines)
+    print(output)
+
+    summary_path = os.environ.get('GITHUB_STEP_SUMMARY')
+    if summary_path:
+        with open(summary_path, 'a', encoding='utf-8') as f:
+            f.write(output)
+
+
+if __name__ == '__main__':
+    main()

docs/dev/Contributing.rst

@@ -79,6 +79,24 @@ General C++ code guidelines
 * Prefer range for loops to traditional for loops when iterating over a container.
 * Avoid macros when possible; prefer ``constexpr`` variables for constants and functions or templates for code generation.
 
+Marking mitigations for DF bugs
+-------------------------------
+DFHack works around a number of defects in Dwarf Fortress itself. When Bay 12
+fixes one of these defects in a new DF release, the corresponding mitigation
+may need to be adjusted or removed. To keep track of these mitigations, mark
+them with a ``DF-MITIGATION:`` comment where they live in the code, e.g.::
+
+    // DF-MITIGATION: site_id is not assigned on reclaim until the first save
+
+The marker is also appropriate for code whose behavior depends on a DF defect
+without working around it, e.g. comments explaining why a field we set has no
+effect. Briefly describe the defect and include a reference to the DF bug or
+DFHack issue/PR when one exists. The markers are collected by
+:source:`ci/list-df-mitigations.py` into a checklist that is reviewed on every
+new DF release; see `release-process-df-mitigations`. The ``fix/*`` scripts are
+all DF bug mitigations by definition and are listed automatically, so they do
+not need markers.
+
 .. _contributing-pr-guidelines:
 
 Pull request guidelines

docs/dev/github-workflows.rst

@@ -107,7 +107,9 @@ Watch DF Releases
 This workflow runs every 8 minutes and checks the Steam metadata, the Itch
 website, and the Bay 12 website for evidence of new releases. If a new release
 is found, it generates an announcement in a private channel on the DFHack
-Discord server.
+Discord server and appends the `DF defect mitigation checklist
+<release-process-df-mitigations>`_ to the job summary for the release
+coordinator to review.
 
 Inside the ``watch-df-releases.yml`` workflow, there are separate jobs for
 watching Steam branches and watching the websites. For the Steam watcher, it

docs/dev/release-process.rst

@@ -6,6 +6,30 @@ This page details the process we follow for beta and stable releases.
 For documentation on the related GitHub workflows, see
 `workflows-release-automation`.
 
+.. _release-process-df-mitigations:
+
+New DF releases
+---------------
+
+When Bay 12 releases a new version of DF, the mitigations we maintain for
+defects in DF itself may need to be adjusted or removed. These mitigations are
+marked with ``DF-MITIGATION:`` comments in the code (see
+`contributing` for the convention) and the ``fix/*`` scripts in
+the scripts repo are all mitigations by definition.
+
+To review them:
+
+1. Run ``ci/list-df-mitigations.py`` in a DFHack checkout (with submodules) to
+   generate the checklist. The Watch DF Releases workflow also appends the
+   checklist to its job summary when it detects a new release.
+
+2. For each entry, determine whether the new DF version still exhibits the
+   defect. Some entries can be checked by code inspection; others need a save
+   that reproduces the defect.
+
+3. Remove or adjust mitigations that are no longer needed, and remove their
+   ``DF-MITIGATION`` markers. Keep entries that still apply.
+
 Beta release
 ------------
 

library/modules/Buildings.cpp

@@ -128,7 +128,8 @@ static df::building_extents_type *getExtentTile(const df::building::T_room &room
 }
 
 /*
- * A monitor to work around this bug, in its application to buildings:
+ * DF-MITIGATION: monitor works around DF bug 1416 in its application to
+ * buildings:
  *
  * http://www.bay12games.com/dwarves/mantisbt/view.php?id=1416
  */

library/modules/Gui.cpp

@@ -2040,8 +2040,12 @@ void Gui::showPopupAnnouncement(std::string message, int color, bool bright)
 {
     df::popup_message *popup = new df::popup_message();
     popup->text = message;
-    popup->color = color; // Doesn't do anything anymore? Popups are always [C:7:0:0] gray text
-    popup->bright = bright; // See: https://dwarffortressbugtracker.com/view.php?id=12672
+    // DF-MITIGATION: DF ignores popup color/bright fields (bug 12672)
+    // Popups always render as [C:7:0:0] gray text; keep setting the fields so
+    // they take effect again if DF is fixed:
+    // https://dwarffortressbugtracker.com/view.php?id=12672
+    popup->color = color;
+    popup->bright = bright;
 
     auto &popups = world->status.popups;
     popups.push_back(popup);

library/modules/MapCache.cpp

@@ -531,7 +531,8 @@ void MapExtras::Block::ParseTiles(TileInfo *tiles)
 
                     tt = con->original_tile;
 
-                    // Ice under construction is buggy:
+                    // DF-MITIGATION: DF bug 6330 makes ice under constructions
+                    // behave incorrectly:
                     // http://www.bay12games.com/dwarves/mantisbt/view.php?id=6330
                     // Therefore we just pretend it wasn't there (if it isn't too late),
                     // and overwrite it if/when we write the base layer.

library/modules/Screen.cpp

@@ -845,6 +845,7 @@ void dfhack_viewscreen::logic()
     // Various stuff works poorly unless always repainting
     Screen::invalidate();
 
+    // DF-MITIGATION: DF can get stuck when a dismissed screen stays buried
     // if the DF screen immediately beneath the DFHack viewscreens is waiting to
     // be dismissed, raise it to the top so DF never gets stuck
     auto *p = parent;

library/modules/World.cpp

@@ -218,8 +218,8 @@ int32_t World::GetCurrentSiteId() {
     if (!plotinfo)
         return -1;
     if (isFortressMode()) {
-        // on a reclaimed fortress, site_id isn't assigned until the first
-        // save; fortress_site is set at embark, so use it as a fallback
+        // DF-MITIGATION: reclaimed forts lack site_id until first save (#5716)
+        // fortress_site is set at embark, so use it as a fallback
         if (plotinfo->site_id >= 0)
             return plotinfo->site_id;
         if (auto site = plotinfo->main.fortress_site)

plugins/lua/buildingplan/unlink_mechanisms.lua

@@ -224,7 +224,9 @@ function MechLinkOverlay:init()
             on_activate = self:callback("ask_unlink_all"),
             enabled = function() return next(self.links) end,
         },
-        widgets.Scrollbar --Work around for https://dwarffortressbugtracker.com/view.php?id=12721
+        -- DF-MITIGATION: extra scrollbar works around DF bug 12721
+        -- https://dwarffortressbugtracker.com/view.php?id=12721
+        widgets.Scrollbar
         {
             view_id = "scroll",
             frame = {t=0, r=0, h=24},

plugins/lua/sort/slab.lua

@@ -68,6 +68,7 @@ function SlabOverlay:onInput(keys)
 end
 
 function SlabOverlay:get_key()
+    -- DF-MITIGATION: building.category not reset when no units memorializable
     -- DF fails to set building.category back to NONE if there are no units that
     -- can be memorialized, so we have to manually check for a populated button vector
     if #building.button > 0 and

plugins/reveal.cpp

@@ -371,7 +371,8 @@ static void unhideFlood_internal(const df::coord &xy) {
         if(!des || !des->bits.hidden)
             continue;
 
-        // we don't want constructions or ice to restrict vision (to avoid bug #1871)
+        // DF-MITIGATION: we don't want constructions or ice to restrict vision
+        // (to avoid bug #1871)
         df::tiletype *tt = Maps::getTileType(current);
         if (!tt)
             continue;

plugins/tweak/tweaks/reaction-gloves.h

@@ -1,4 +1,4 @@
-// Workaround for DF bug #6273 - adjust all custom reactions to produce GLOVES items in sets with correct handedness
+// DF-MITIGATION: workaround for DF bug #6273 - adjust all custom reactions to produce GLOVES items in sets with correct handedness
 // It also analyzes the body plan of the unit performing the reaction, so Antmen will get 4 gloves instead of 2
 
 // If a reaction tries to produce either 1 glove or 2 gloves, it will produce a single set

plugins/zone.cpp

@@ -539,7 +539,9 @@ static command_result assignUnitToCage(color_ostream& out, df::unit* unit, df::b
         return CR_WRONG_USAGE;
     }
 
-    // don't assign owned pets to a cage. the owner will release them, resulting into infinite hauling (df bug)
+    // DF-MITIGATION: caging owned pets causes infinite hauling (df bug)
+    // don't assign owned pets to a cage. the owner will release them, resulting
+    // into infinite hauling
     if(unit->relationship_ids[df::unit_relationship_type::PetOwner] != -1)
         return CR_OK;