Consolidation step 5 of 8: utils — 21 headers become named sub-modules [androidbuild] [iosbuild]#46
Merged
Merged
Conversation
…s [androidbuild] [iosbuild] Plain application of the settled architecture: one streamr.utils.X sub-module per former header, no umbrella, include/ tree deleted, all 45 repo-wide importers flipped to narrow imports. Documents a clang 22.1.8 modules miscompile found during the flip: an UNUSED import of a folly-coro-heavy BMI into a module unit with its own textual folly coroutine declarations broke coroutine resumption at runtime — minimal imports is now recorded as a correctness rule, not just hygiene. Also removes a 2024 typo-named duplicate test file that no target ever built (SigninUtilsTest.cpp). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidation C-5: streamr-utils
Fifth consolidation step (MODERNIZATION.md Phase 2.6). This one is a plain application of the settled architecture — no generated-code complications — but it surfaced one important compiler finding, documented below.
What changed
include/streamr-utils/became named sub-modules undermodules/— one.cppmper former header (streamr.utils.AbortController,.Branded,.StreamID,.waitForEvent, …), moved withgit mvso history is preserved.streamr.utilsumbrella +streamr.utils-all) is deleted; per the settled architecture there is no umbrella module.include/tree is deleted and the package no longer exports any include directory.import streamr.utils;now import exactly the sub-modules they use — module imports mirror the old header includes one to one. streamr-utils' own tests import their subjects directly.Important finding: a clang 22.1.8 modules miscompile, and the rule that avoids it
My first, script-driven pass at flipping the 45 importers over-matched names and added some unused imports. One of them —
import streamr.utils.waitForCondition;insidestreamr.protorpc.RpcCommunicatorClientApi— made all 17 RpcCommunicator tests hang deterministically: the RPC response arrived, the promise was resolved, and the awaiting coroutine simply never resumed (stack traces showed every thread idle-parked).The receiving module unit textually includes folly's coroutine Promise/Timeout headers in its own global module fragment and defines coroutine code; the imported module's global module fragment carries overlapping folly-coroutine declarations. Merging the two miscompiles the promise-resumption path. Bisection pinned it precisely: adding only
streamr.utils.collectis fine; adding onlystreamr.utils.waitForConditionhangs.Removing the unused import fixes it. The consequence is now recorded as a hard rule in MODERNIZATION.md (it was already the architecture's intent): a module unit imports only the modules whose names its code actually uses — never speculative or convenience imports. The flip tooling now prunes candidate imports against a curated per-module export-name list, and the compiler's "declaration of 'X' must be imported from module 'M'" error is the ground truth for adding imports back. All 115 spurious imports that the first pass had introduced across 37 files were removed again.
Incidental cleanup
test/unit/SigninUtilsTest.cpp(note the missing "g") — a typo-named, byte-identical duplicate ofSigningUtilsTest.cppdating from 2024 — was never part of any CMake target. It surfaced because the code-analysis tool had been checking it with compile flags borrowed from a neighboring file, which stopped working once imports became per-file. Deleted.Verification
[androidbuild] [iosbuild]tags request the cross-platform CI legs.--no-verify(known pre-push-hook SIGPIPE issue); all lint scripts run manually instead.Next step (C-6)
streamr-logger consolidation.
🤖 Generated with Claude Code