fix(lazy_deps): only mark a backend active when its signature package is present - #54178
fix(lazy_deps): only mark a backend active when its signature package is present#54178spiky02plateau wants to merge 2 commits into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary\n\nVerdict: LGTM\n\nFixes false-positive active_features() detection by gating on the signature (first) spec only. Previously, shared transitive deps like asyncpg would trigger platform.matrix refresh attempts. Clean regression tests.\n\n---\nReviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Focused fix (54 additions). Gates active_features() on the signature (first) spec only, preventing false positives from shared transitive dependencies. Clear documentation of the regression being fixed. Includes 3 test cases.
Reviewed by Hermes Agent
… is present active_features() flagged a feature as active when *any* of its declared packages was installed. Trailing specs are often generic shared libraries (asyncpg, aiosqlite, aiohttp, qrcode, numpy) pulled in transitively by unrelated features, so a host that merely had asyncpg installed for some other reason was treated as having platform.matrix active. `hermes update` then tried to refresh it and failed building mautrix[encryption] -> python-olm (bundled libolm `make static`) on every run, emitting a "failed to refresh" warning to users who never enabled Matrix. Gate on the signature package (the first spec in each LAZY_DEPS tuple) instead, matching the function's documented intent of detecting backends the user actually enabled. Every LAZY_DEPS entry already leads with its defining package and lists shared libs afterwards. Real users of a backend still have its signature package installed, so genuine refreshes are unaffected. Adds regression tests.
Satisfies the contributor-check CI gate. Appended at the end of the dict to avoid the top-of-map churn that conflicts across concurrent PRs.
8c92324 to
07dddc9
Compare
|
Rebased onto latest The only remaining red is That's the standard fork-PR limitation — a PR from an outside fork can't write to the org's GHCR package. The For context, an earlier run also showed The change itself is isolated to |
|
@teknium1 — flagging this for review whenever you have a moment (no rush); you touched Quick state of the PR:
The change itself is isolated to Happy to reshape |
|
Confirmed this fix against current Environment:
Before the patch, Applying the signature-package gate from this PR produced:
|
|
Fixed on main by #72361 — anchor-gated (first-spec) activation, functionally identical to your signature-package approach. #27878 by @paralegalia was the earliest submission of this fix so it got the salvage; your matrix/python-olm repro was the clearest statement of the impact. Thanks @spiky02plateau — and your no-downgrade analysis from #68008 informed the lockstep invariant that shipped alongside it. |
Problem
tools/lazy_deps.py::active_features()flags a backend as "active" if any of its declared specs is installed:Several
LAZY_DEPSentries list a defining package followed by generic shared libraries, e.g.:When one of those shared libs (
asyncpg,aiosqlite,aiohttp, …) happens to be installed transitively for an unrelated reason, the backend is falsely flagged active.hermes update→_refresh_active_lazy_features()then tries to refresh it and fails buildingmautrix[encryption]→python-olm(bundled libolmmake static):This surfaces on every
hermes updatefor users who never enabled Matrix but haveasyncpg/aiosqlitepresent — a common situation, so the noisy "failed to refresh" warning is widespread.Root cause
The implementation contradicts the function's own docstring, which states active means "backends the user has previously activated." Keying on any spec means an incidental shared transitive dep is enough to mark a backend active.
Fix
Gate on the signature package — the first spec in each feature tuple. Every
LAZY_DEPSentry already leads with its defining package and lists shared libs afterwards, so this matches both the data convention and the documented intent:Real users of a backend still have its signature package installed, so genuine refreshes (the reason this function exists — picking up moved version pins) are unaffected. Only false positives are removed.
Tests
Adds
TestActiveFeaturesSignatureGatingintests/tools/test_lazy_deps.py:Full
tests/tools/test_lazy_deps.pysuite passes locally.Alternative considered
An explicit shared-dep denylist instead of positional
specs[0]. The positional approach is smaller and self-maintaining (no list to keep in sync as backends are added), but happy to switch to a denylist if you prefer that shape.