Repository navigation
Modernization Phase 2.6: Part 2 closeout — consolidation memo, NDK r29 modules experiment [androidbuild] - #34
Merged
Merged
Conversation
…s, final trace - MODERNIZATION.md: Phase 2.6 rewritten as a DECISION MEMO. The planned consolidation (code into purview, internal headers deleted) is BLOCKED by the 2.5 Android finding: the NDK's clang cannot build the modules and Android compiles the libraries textually from exactly the headers consolidation would delete. Preconditions documented (NDK clang >= 22 or Android parked; clangd purview-lint maturity; dht partition DAG), along with what consolidation buys (the unmet incremental targets) and the recommended leaf-first measured path when unblocked. The facade stage is a stable resting point. - Final ClangBuildAnalyzer trace recorded: DhtRpc.pb.h fell from #1 (96s, 33x) to #9 (15.7s, 6x); SLogger/Logger left the expensive list entirely; new #1 is gtest.h (stays #include by design); total parse CPU 859s -> 723s. The third success metric is met. - README: C++ named modules developer documentation (import rules, sibling-import rule, direct-link rule, Android exception). - Stale 'No C++ modules in the codebase yet' comments updated in all 9 CMakeLists. - 2.5 section extended with the Android saga (folly supports overlay; STREAMR_MODULES_SUPPORTED fallback; Android workflow lint removal). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (androidbuild) Owner review catch: the 2.5 Android modules failure was on the CI runner's DEFAULT NDK r27.3 (clang 18.0.2), not the latest stable. NDK r29 (Oct 2025) ships clang 21, whose modules support should handle the facades. The Android workflow now selects ANDROID_NDK_LATEST_HOME (r29, preinstalled on macos-26 runners), and STREAMR_MODULES_SUPPORTED on Android is version-gated (NDK clang >= 21 -> modules ON, incl. building the import-using test targets as a compile smoke; older NDKs fall back to the textual build). This PR's androidbuild leg is the validation run; if green, the consolidation memo's Android precondition is already satisfiable today. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
the
ci
ptesavol
changed the title
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (androidbuild) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Phase 2.5 Android failure (import streamr.json -> every imported name "undeclared") was root-caused by reproducing the CI build locally against NDK r27/r28/r29: the import-using test TUs compile with -pthread (inherited from GTest's Threads::Threads) while the module libraries' BMIs were built without it. On Android -pthread toggles a language option clang validates when loading a module file (-Wmodule-file-config-mismatch), so the BMI silently fails to load and every imported name cascades to "use of undeclared identifier". The raw NDK compilers (clang 18/19/21) all build and import the facade modules correctly. - StreamrModules.cmake helpers: make -pthread a PUBLIC compile option on Android (genexpr, no-op elsewhere) so BMIs and importers agree. - Lower the Android STREAMR_MODULES_SUPPORTED floor from clang 21 to clang 18 (= NDK r27, oldest verified); textual fallback below that. - Verified locally: streamr-json + streamr-eventemitter full package builds (tests + examples) green for arm64-android on NDK r27 and r29. - validateandroid.yml: keep the latest-stable-NDK step (hygiene, no longer a modules requirement); comment updated. - MODERNIZATION.md: 2.5 record corrected, 2.6 memo precondition 1 marked RESOLVED; README Android note updated. 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. |
With Android modules enabled, the proto-rpc test/example block now runs on Android builds and exposed protobuf-streamr-plugin: it links protobuf::libprotoc, which vcpkg does not provide for cross targets. The plugin is host tooling (proto.sh codegen) and was only ever skipped on Android as a side effect of the modules gate being OFF there. Give it its own cross-compile guard, independent of modules support. 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. |
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
1 participant
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.
Closeout of Part 2 — and the consolidation decision, which is yours to make. (This PR grew one substantive fix along the way: the Android modules blocker turned out not to be a blocker at all — see below.)
The Android resolution (found while closing out)
The Phase 2.5 Android failure (
import streamr.json→ every imported name "undeclared") was not an NDK-clang capability gap. Reproducing the CI build locally against NDK r27/r28/r29 showed all three raw compilers (clang 18/19/21) build and import the façade modules correctly. The real cause, always truncated out of the CI annotation tail: a-pthreadBMI configuration mismatch — test TUs compile with-pthread(inherited from GTest'sThreads::Threads), the module libraries' BMIs were built without it, and on Android clang validates that language option when loading a module file (-Wmodule-file-config-mismatch), so the BMI silently fails to load and every imported name cascades to "undeclared". Only the thread-less leaf packages (json, eventemitter) could mismatch — the folly-linking packages already inherited-pthreadinto their BMIs.Fixes in this PR:
StreamrModules.cmakehelpers make-pthreada PUBLIC compile option on Android (genexpr; no-op elsewhere) so BMIs and importers always agree.STREAMR_MODULES_SUPPORTEDfloor drops from clang 21 to clang 18 (= NDK r27, oldest verified); textual fallback below that.protobuf-streamr-plugin(host codegen tooling) gets its ownNOT CMAKE_CROSSCOMPILINGguard — it was only ever skipped on Android as a side effect of the modules gate being OFF.Result: the androidbuild leg on this PR builds the full monorepo — all 7 module packages, tests included — for arm64-android with modules ON. Android is no longer a consolidation blocker.
The decision memo (now in MODERNIZATION.md)
The planned Phase 2.6 — move all code into module purview, delete every internal header — remains gated by two preconditions (the third is resolved above):
A modules-capable NDK on AndroidRESOLVED in this PR.Recommendation: defer, and treat the façade stage as the stable resting point — now purely on lint-coverage grounds (2) plus the dht untangling (3). Nothing rots by waiting: headers stay the linted source of truth on every platform, and consolidation resumes leaf-first (eventemitter canary, measured per package) when clangd's modules support matures. If you'd rather trade lint coverage for the incremental-build wins now, say so and I'll re-plan.
Also in this PR
DhtRpc.pb.hfell from First test of CI #1 (96 s, 33 inclusions) to Module1 #9 (15.7 s, 6 inclusions);SLogger.hpp/Logger.hppleft the expensive-headers list entirely; the new First test of CI #1 isgtest.h, which stays#includeby design. Total frontend parse CPU: 859 s → 723 s (−16 %) even with the module units added.-pthreadnote).Part 2 final scorecard
Part 1 (toolchain) + Part 2 façade stage are complete: LLVM 22 + libc++ everywhere, vcpkg 2026.06, iOS 26 device-verified, 7 packages shipping modules on macOS/Linux/iOS and Android, 250+ tests through
import, all four platform gates green, and a measured, honest record of every decision in MODERNIZATION.md.🤖 Generated with Claude Code