code review

Author: chouetz

deps/.renovate-untracked.json

@@ -1,5 +1,5 @@
 {
-  "_comment": "Bazel http_archive deps in deps/repos.MODULE.bazel that intentionally have no Renovate customManager. Every entry MUST include a non-empty rationale. The check is enforced by tasks/check_renovate_bazel_coverage.py and the validate-renovate-deps.yml workflow. As deps move from this allowlist to a customManager in renovate.json, remove the entry here in the same PR.",
+  "_comment": "Bazel http_archive and http_file deps in any deps/**/*.MODULE.bazel file that intentionally have no Renovate customManager. Every entry MUST include a non-empty rationale. The check is enforced by tasks/check_renovate_bazel_coverage.py and the validate-renovate-deps.yml workflow. As deps move from this allowlist to a customManager in renovate.json, remove the entry here in the same PR.",
   "intentionally_untracked": {
     "xz": "Pending tracking in Phase 2a (github-releases batch).",
     "zlib": "Pending tracking in Phase 2a (github-releases batch).",
@@ -29,6 +29,31 @@
     "attr": "Hard: hosted on Savannah (savannah.nongnu.org); no native Renovate datasource. Needs a customDatasource. Phase 3.",
     "gpg-error": "Hard: hosted on gnupg.org FTP; no native Renovate datasource. Needs a customDatasource. Phase 3.",
     "gcrypt": "Hard: hosted on gnupg.org FTP; no native Renovate datasource. Needs a customDatasource. Phase 3.",
-    "unixodbc": "Hard: hosted on unixodbc.org; no native Renovate datasource. Needs a customDatasource. Phase 3."
+    "unixodbc": "Hard: hosted on unixodbc.org; no native Renovate datasource. Needs a customDatasource. Phase 3.",
+    "acl": "Pending tracking. Hosted on Savannah (download.savannah.nongnu.org); needs a customDatasource. Phase 3.",
+    "curl": "Pending tracking in Phase 2a (github-releases batch). Tarball is on curl.haxx.se but releases are tagged on github.com/curl/curl.",
+    "cpython": "Pending tracking in Phase 2a (github-releases batch). Released on python.org; corresponding GitHub tags exist at github.com/python/cpython.",
+    "freetds": "Pending tracking in Phase 2a (github-releases batch). Tarballs on freetds.org; releases tagged at github.com/FreeTDS/freetds.",
+    "libpcap": "Pending tracking in Phase 2a (github-releases batch). Tarball on tcpdump.org; releases tagged at github.com/the-tcpdump-group/libpcap.",
+    "nfsiostat": "Pending tracking in Phase 2c. Hosted on kernel.org mirrors (no GitHub mirror with reliable tags). Needs a customDatasource.",
+    "openscap": "Pending tracking in Phase 2a (github-releases batch). Released at github.com/OpenSCAP/openscap.",
+    "setuptools": "Pending tracking in Phase 2a (github-releases batch). Released at github.com/pypa/setuptools.",
+    "compile_policy_x86_64": "Pending tracking in Phase 2a. DataDog/dd-policy-engine GitHub release; will be tracked as one bundle with the arm64 and windows variants since they share VERSION.",
+    "compile_policy_arm64": "Pending tracking in Phase 2a. Shares VERSION with compile_policy_x86_64; tracked together.",
+    "compile_policy_windows_x86_64": "Pending tracking in Phase 2a. Shares VERSION with compile_policy_x86_64; tracked together.",
+    "gstatus_binary": "Pending tracking in Phase 2a (github-releases batch). Released at github.com/gluster/gstatus.",
+    "msodbcsql18_deb_amd64": "Pending tracking. Microsoft .deb package; no native datasource for packages.microsoft.com. Both arches share VERSION and will be tracked as one bundle when a customDatasource is added. Phase 3.",
+    "msodbcsql18_deb_arm64": "Pending tracking. Shares VERSION with msodbcsql18_deb_amd64; tracked together. Phase 3.",
+    "snmp_traps": "Intentionally untracked. Datadog-internal SNMP traps DB published to dd-agent-omnibus S3; no public upstream to watch. Bumped manually when the traps DB is regenerated.",
+    "cacert_pem": "Intentionally untracked. Bumped via a separate Datadog Synthetics-driven workflow (see deps/cacerts/cacerts.MODULE.bazel) that alerts team-agent-build on header changes; auto-PRs from Renovate would race that flow.",
+    "cacert_license": "Intentionally untracked. Mozilla MPL-2.0 license text accompanying cacert_pem; updated together with cacert_pem via the same out-of-band flow.",
+    "sqlite3_license": "Intentionally untracked here — version is pinned via the shared sqlite_ver tuple in deps/repos.MODULE.bazel and will be tracked together with sqlite3 once that refactor lands (see sqlite3 entry).",
+    "sqlite_win": "Intentionally untracked here. Windows-only SQLite amalgamation; pinned to the same release as sqlite3 (manually kept in sync, see TODO in deps/cpython/cpython.MODULE.bazel). Will be folded in with sqlite3 in Phase 3.",
+    "bzip2_win": "Pending tracking. Loop-emitted Windows variant in deps/cpython/cpython.MODULE.bazel; version pinned via the python_src_deps dict alongside the base bzip2. Will be tracked together with bzip2 (Phase 3 customDatasource).",
+    "mpdecimal_win": "Pending tracking. Loop-emitted Windows variant in deps/cpython/cpython.MODULE.bazel; version pinned via the python_src_deps dict. No upstream `mpdecimal` dep exists today — needs its own customManager.",
+    "xz_win": "Pending tracking. Loop-emitted Windows variant in deps/cpython/cpython.MODULE.bazel; version pinned via the python_src_deps dict alongside the base xz. Will be tracked together with xz (Phase 2a).",
+    "zlib_win": "Pending tracking. Loop-emitted Windows variant in deps/cpython/cpython.MODULE.bazel; version pinned via the python_src_deps dict alongside the base zlib. Will be tracked together with zlib (Phase 2a).",
+    "libffi_win": "Pending tracking. Loop-emitted Windows variant in deps/cpython/cpython.MODULE.bazel; version pinned via the python_bin_deps dict. Note: pinned to 3.4.4 (cpython-bin-deps), independent of the base libffi version — must be tracked separately.",
+    "tcltk_win": "Pending tracking. Loop-emitted Windows variant in deps/cpython/cpython.MODULE.bazel; version pinned via the python_bin_deps dict. No upstream `tcltk` dep — needs its own customManager (cpython-bin-deps tags)."
   }
 }

tasks/BUILD.bazel

@@ -22,5 +22,6 @@ exports_files(["core_checks.py"])
 py_binary(
     name = "check_renovate_bazel_coverage",
     srcs = ["renovate.py"],
+    main = "renovate.py",
     deps = ["@py_dev_requirements//invoke"],
 )

tasks/renovate.py

@@ -43,13 +43,18 @@ def main():
 @task
 def check_bazel_coverage(_: Context, root: str | None = None) -> None:
     """
-    Fail if any http_archive or http_file in deps/ lacks a Renovate customManager.
+    Fail if any http_archive or http_file in deps/ lacks a Renovate customManager,
+    or if the allowlist contains stale entries that no longer match any dep.
 
     Scans every ``*.MODULE.bazel`` file under ``deps/`` for ``http_archive``
     and ``http_file`` calls. A dep is considered covered when either:
       * its name appears as ``depNameTemplate`` in one of ``renovate.json``'s customManagers, or
       * it is listed in ``deps/.renovate-untracked.json`` with a non-empty rationale.
 
+    Also fails when ``deps/.renovate-untracked.json`` lists a dep that no longer
+    exists in ``deps/**/*.MODULE.bazel`` — without this, renamed or removed deps
+    leave dead entries behind and the file rots.
+
     Writes a markdown report to ``$GITHUB_STEP_SUMMARY`` when running in GitHub Actions.
     """
     root_path = Path(root) if root is not None else Path(REPO_ROOT)
@@ -58,8 +63,10 @@ def check_bazel_coverage(_: Context, root: str | None = None) -> None:
     allowlist = _parse_allowlist(root_path / "deps" / ".renovate-untracked.json")
 
     untracked = dep_names - tracked_names - set(allowlist)
-    if untracked:
-        report = _emit_failure_report(untracked, allowlist)
+    stale = set(allowlist) - dep_names
+
+    if untracked or stale:
+        report = _emit_failure_report(untracked, stale, allowlist)
         summary_path = os.environ.get("GITHUB_STEP_SUMMARY")
         if summary_path:
             # GITHUB_STEP_SUMMARY is a shared file for the whole step; append
@@ -80,12 +87,18 @@ def _parse_deps_dir(deps_dir: Path) -> set[str]:
     for path in deps_dir.rglob("*.MODULE.bazel"):
         text = path.read_text()
         for call in ("http_archive", "http_file"):
-            names |= _extract_call_names(text, call)
+            names |= _extract_call_names(text, call, path)
     return names
 
 
-def _extract_call_names(text: str, call_name: str) -> set[str]:
-    """Extract name = "..." from all call_name(...) blocks, regardless of arg order or comments."""
+def _extract_call_names(text: str, call_name: str, path: Path) -> set[str]:
+    """Extract name = "..." from all call_name(...) blocks, regardless of arg order or comments.
+
+    When the name is a Starlark template (e.g. `name = "{}_win".format(name)`),
+    resolve it against the enclosing list comprehension and the dict literal it
+    iterates. Fails loudly on any template we can't resolve — silent skips
+    would let loop-emitted deps escape the coverage check.
+    """
     names: set[str] = set()
     name_re = re.compile(r'\bname\s*=\s*"([^"]+)"')
     marker = call_name + "("
@@ -113,11 +126,84 @@ def _extract_call_names(text: str, call_name: str) -> set[str]:
         uncommented = "\n".join(line for line in block.splitlines() if not line.lstrip().startswith("#"))
         m = name_re.search(uncommented)
         if m:
-            names.add(m.group(1))
+            literal = m.group(1)
+            if "{" in literal:
+                names |= _resolve_templated_name(text, pos, literal, path)
+            else:
+                names.add(literal)
         start = i
     return names
 
 
+_DICT_LITERAL_RE = re.compile(
+    r'^(\w+)\s*=\s*\{(.*?)\n\}',
+    re.DOTALL | re.MULTILINE,
+)
+_DICT_KEY_RE = re.compile(r'^\s*"([^"]+)"\s*:', re.MULTILINE)
+# `for <iter_var>, ... in <dict_var>.items()` — first target name, optional
+# tuple-destructuring tail (which may contain parentheses for `(a, b)`), then
+# the source dict. We anchor on the closing ` in ` token so the destructuring
+# tail can be anything.
+_COMPREHENSION_RE = re.compile(
+    r'\bfor\s+(\w+)[^\n]*?\s+in\s+(\w+)\.items\(\)',
+)
+
+
+def _resolve_templated_name(text: str, call_pos: int, template: str, path: Path) -> set[str]:
+    """Resolve a templated name like "{}_win" by finding the enclosing
+    comprehension and the dict it iterates.
+
+    Supported pattern (the only one in-tree today):
+
+        SOME_DICT = {"keyA": (...), "keyB": (...)}
+        [
+            http_archive(
+                name = "{}_suffix".format(VAR),
+                ...
+            )
+            for VAR, (...) in SOME_DICT.items()
+        ]
+
+    Raises Exit if the template can't be resolved — a silent skip would defeat
+    the coverage guarantee.
+    """
+    after = text[call_pos:]
+    comp = _COMPREHENSION_RE.search(after)
+    if not comp:
+        raise Exit(
+            f"{path}: templated name {template!r} has no enclosing "
+            "`for VAR, ... in DICT.items()` comprehension. Refactor to literal "
+            "names, or extend tasks/renovate.py to handle this pattern."
+        )
+    iter_var, dict_var = comp.group(1), comp.group(2)
+    keys = _lookup_dict_keys(text, dict_var)
+    if keys is None:
+        raise Exit(
+            f"{path}: templated name {template!r} iterates {dict_var}.items() "
+            f"but no top-level `{dict_var} = {{...}}` literal was found. "
+            "Refactor to literal names, or extend tasks/renovate.py."
+        )
+    # Substitute each key for the loop variable. Both `"{}".format(VAR)` and
+    # `"{name}".format(name = VAR)` reduce to a single `{...}` placeholder
+    # that we replace with the literal key.
+    if "{}" in template:
+        return {template.replace("{}", k) for k in keys}
+    placeholder = "{" + iter_var + "}"
+    if placeholder in template:
+        return {template.replace(placeholder, k) for k in keys}
+    raise Exit(
+        f"{path}: templated name {template!r} doesn't reference loop var "
+        f"{iter_var!r} via `{{}}` or `{{{iter_var}}}`. Refactor or extend the parser."
+    )
+
+
+def _lookup_dict_keys(text: str, dict_var: str) -> list[str] | None:
+    for m in _DICT_LITERAL_RE.finditer(text):
+        if m.group(1) == dict_var:
+            return _DICT_KEY_RE.findall(m.group(2))
+    return None
+
+
 def _parse_renovate_json(path: Path) -> set[str]:
     # Assumption: renovate.json is plain JSON plus trailing commas only — no
     # // or /* */ comments, no single-quoted strings. Renovate accepts the full
@@ -143,29 +229,46 @@ def _parse_allowlist(path: Path) -> dict[str, str]:
     return entries
 
 
-def _emit_failure_report(untracked: set[str], allowlist: dict[str, str]) -> str:
-    lines = [
-        "## ❌ Renovate coverage check failed",
-        "",
-        "The following native deps (http_archive / http_file) in `deps/` have no "
-        "matching `customManager` in `renovate.json`:",
-        "",
-        "| dep | suggested fix |",
-        "|---|---|",
-    ]
-    for dep in sorted(untracked):
-        lines.append(
-            f"| `{dep}` | Add a `customManagers` entry with "
-            f'`depNameTemplate: "{dep}"`, or add to '
-            "`deps/.renovate-untracked.json` with a rationale. |"
-        )
-    lines += [
-        "",
-        "See `renovate.json` for existing patterns (linux-images, windows-images, ...).",
-        "",
+def _emit_failure_report(untracked: set[str], stale: set[str], allowlist: dict[str, str]) -> str:
+    lines = ["## ❌ Renovate coverage check failed", ""]
+    if untracked:
+        lines += [
+            "The following native deps (http_archive / http_file) in `deps/` have no "
+            "matching `customManager` in `renovate.json`:",
+            "",
+            "| dep | suggested fix |",
+            "|---|---|",
+        ]
+        for dep in sorted(untracked):
+            lines.append(
+                f"| `{dep}` | Add a `customManagers` entry with "
+                f'`depNameTemplate: "{dep}"`, or add to '
+                "`deps/.renovate-untracked.json` with a rationale. |"
+            )
+        lines += [
+            "",
+            "See `renovate.json` for existing patterns (linux-images, windows-images, ...).",
+            "",
+        ]
+    if stale:
+        lines += [
+            "The following entries in `deps/.renovate-untracked.json` no longer match "
+            "any `http_archive` / `http_file` name in `deps/**/*.MODULE.bazel` and "
+            "must be removed:",
+            "",
+        ]
+        for dep in sorted(stale):
+            lines.append(f"- `{dep}`")
+        lines += [
+            "",
+            "If the dep was renamed, replace the old entry with the new name. If it "
+            "was removed, delete the entry. Stale allowlist entries silently weaken the check.",
+            "",
+        ]
+    lines.append(
         f"Currently allowlisted ({len(allowlist)}): "
-        + (", ".join(f"`{k}`" for k in sorted(allowlist)) if allowlist else "_none_"),
-    ]
+        + (", ".join(f"`{k}`" for k in sorted(allowlist)) if allowlist else "_none_")
+    )
     return "\n".join(lines)