Fix Codex P1: wire Android accessibility + Windows SDF include - #132
Merged
Conversation
…clude 1. Wire accessibility_android.cpp into pulp-view Android target (PulpAndroid.cmake) — the JNI exports existed but were never compiled into the Android build (#129 review) 2. Fix Windows SDF atlas include: use SkTypeface_win.h (contains SkFontMgr_New_DirectWrite) instead of SkFontMgr_directory.h (#131 review) PR #130 P1 (WindowHost API) and P2 (use_gpu) were already fixed in PR #131.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86e888f237
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
danielraffel
added a commit
that referenced
this pull request
Apr 13, 2026
…vive PR #132 P1 (never actually resolved): accessibility_android.cpp provides the TalkBack JNI entry points (Java_com_pulp_accessibility_nativeGetAccessibilityNodeCount, nativePerformAction, nativeGetNodeRole, etc.) inside pulp-view. Nothing inside pulp-view or libpulp.so references these symbols — the callers are on the Kotlin side (PulpAccessibility.kt) — so the Android linker is free to strip them under the default dead-code elimination. That manifests at runtime as UnsatisfiedLinkError the first time TalkBack queries the node tree. pulp-jni's target_link_options already wraps pulp-platform, pulp-audio, and (when present) pulp-render in --whole-archive for the same reason. Add pulp-view to the same block. Fixes the only P1 from PR #132 that was merged unresolved.
2 tasks
danielraffel
added a commit
that referenced
this pull request
Apr 13, 2026
…pulp-view PR #132's P1 review never actually landed on main. accessibility_android.cpp defines all Java_com_pulp_accessibility_PulpAccessibilityDelegate_native* entry points inside pulp-view (static lib). Nothing inside libpulp.so references them — the only callers are on the Kotlin side (PulpAccessibility.kt) — so the Android linker is free to strip them under default dead-code elimination. At runtime TalkBack's first node-tree walk then hits UnsatisfiedLinkError. PR #148 tried to fix this by whole-archiving pulp-view. That pulled SDL3's libSDL3.a in transitively, and SDL3 defines its own JNI_OnLoad, colliding with core/platform/src/android/jni_bridge.cpp:41. #148 was closed. This PR uses the narrower `-Wl,--undefined=<symbol>` approach: force- reference each of the nine TalkBack JNI exports in pulp-jni's link options. The linker then keeps the accessibility_android.cpp object file alive without dragging any other sections of pulp-view (or its SDL3 transitive closure) into libpulp.so. No behaviour change on non-Android builds (the whole pulp-jni target is gated on ANDROID). On Android, the nine forced references grow libpulp.so by the single accessibility_android.cpp object file — same footprint PR #127 intended when it first landed TalkBack.
3 tasks
danielraffel
added a commit
that referenced
this pull request
Apr 14, 2026
…pulp-view PR #132's P1 review never actually landed on main. accessibility_android.cpp defines all Java_com_pulp_accessibility_PulpAccessibilityDelegate_native* entry points inside pulp-view (static lib). Nothing inside libpulp.so references them — the only callers are on the Kotlin side (PulpAccessibility.kt) — so the Android linker is free to strip them under default dead-code elimination. At runtime TalkBack's first node-tree walk then hits UnsatisfiedLinkError. PR #148 tried to fix this by whole-archiving pulp-view. That pulled SDL3's libSDL3.a in transitively, and SDL3 defines its own JNI_OnLoad, colliding with core/platform/src/android/jni_bridge.cpp:41. #148 was closed. This PR uses the narrower `-Wl,--undefined=<symbol>` approach: force- reference each of the nine TalkBack JNI exports in pulp-jni's link options. The linker then keeps the accessibility_android.cpp object file alive without dragging any other sections of pulp-view (or its SDL3 transitive closure) into libpulp.so. No behaviour change on non-Android builds (the whole pulp-jni target is gated on ANDROID). On Android, the nine forced references grow libpulp.so by the single accessibility_android.cpp object file — same footprint PR #127 intended when it first landed TalkBack.
danielraffel
added a commit
that referenced
this pull request
Apr 14, 2026
…pulp-view (#151) * Fix Android: force-keep TalkBack JNI symbols without whole-archiving pulp-view PR #132's P1 review never actually landed on main. accessibility_android.cpp defines all Java_com_pulp_accessibility_PulpAccessibilityDelegate_native* entry points inside pulp-view (static lib). Nothing inside libpulp.so references them — the only callers are on the Kotlin side (PulpAccessibility.kt) — so the Android linker is free to strip them under default dead-code elimination. At runtime TalkBack's first node-tree walk then hits UnsatisfiedLinkError. PR #148 tried to fix this by whole-archiving pulp-view. That pulled SDL3's libSDL3.a in transitively, and SDL3 defines its own JNI_OnLoad, colliding with core/platform/src/android/jni_bridge.cpp:41. #148 was closed. This PR uses the narrower `-Wl,--undefined=<symbol>` approach: force- reference each of the nine TalkBack JNI exports in pulp-jni's link options. The linker then keeps the accessibility_android.cpp object file alive without dragging any other sections of pulp-view (or its SDL3 transitive closure) into libpulp.so. No behaviour change on non-Android builds (the whole pulp-jni target is gated on ANDROID). On Android, the nine forced references grow libpulp.so by the single accessibility_android.cpp object file — same footprint PR #127 intended when it first landed TalkBack. * Fix macOS AU v2 plugin bundles: pin CXX_STANDARD=23 on ${target}_AU ViewBridge Phase 3 (#146) added au_v2_cocoa_view.mm to each plugin's ${target}_AU MODULE target via PulpUtils.cmake's pulp_add_plugin helper. The .mm compiles inside that AU target, not pulp-format, so it does NOT inherit pulp-format's `target_compile_features(... PUBLIC cxx_std_23)` PUBLIC setting — CMake only propagates compile features when the consumer links against the target, not when its sources are merely `add_library(... src1 src2 ...)`-ed in alongside. AUUtility.h in AudioUnitSDK 1.4 uses std::expected / std::unexpected (C++23). Apple clang only exposes those at -std=c++23. Result on github-hosted macos-14 runners (clang 17): every plugin AU target fails to build with 'no template named unexpected in namespace std'. Pin CXX_STANDARD=23 + CXX_STANDARD_REQUIRED=ON on ${target}_AU directly. Same rationale documented at core/format/CMakeLists.txt L76 for the pulp-format target. * Fix macOS AU v2 (follow-up): .mm files need OBJCXX_STANDARD=23 too CXX_STANDARD=23 alone doesn't apply to .mm sources — CMake treats them as Objective-C++ (OBJCXX), a separate language with its own OBJCXX_STANDARD property. Previous commit 1e3b4f9 only set the C++ standard, so au_v2_cocoa_view.mm still compiled at the default C++17 and AUUtility.h's std::expected still failed. Add OBJCXX_STANDARD=23 + OBJCXX_STANDARD_REQUIRED=ON alongside the existing CXX_STANDARD so both C++ and Objective-C++ TUs in ${target}_AU pick up the required standard.
Collaborator
Author
|
Resolved by PR #151 (commit edb8dbb): |
7 tasks
danielraffel
added a commit
that referenced
this pull request
Apr 21, 2026
* coverage: expose full code surface to llvm-cov + switch to LCOV→Cobertura User called out that the Codecov dashboard was only tracking 188 of 695 lines (0.6% of Pulp's ~113k-line coverage-bearing surface). Codex sanity-check confirmed two real bugs, both fixed here: 1. `-object` list was too narrow. `scripts/run_coverage.sh` passed only test binaries to llvm-cov, which means llvm-cov only reported translation units LINKED INTO a test binary. Production code in first-party libraries that no test transitively depended on was invisible end-to-end. This silently false-negatived the diff-cover gate (#132) on any PR touching code outside the test-linked slice. Fix: also pass every `libpulp-*.a` static archive and non-test first-party executable (CLI, standalone, inspector). LLVM docs confirm `.a` archives are valid `-object` inputs. 2. gcovr 8.6 drops ~99% of the coverage data once the `-object` set widens. Direct `llvm-cov report` saw 110k tracked lines across 577 source files; the same profdata piped through `gcovr --llvm-cov-binary` per test binary emitted a Cobertura XML with 150 lines across 4 files. Fix: bypass gcovr. Pipeline is now `llvm-cov export --format=lcov` → vendored `tools/scripts/lcov_cobertura.py` → Cobertura XML. gcovr is no longer installed in CI. Honest baseline after both fixes (local macOS run): - 53,528 lines tracked (was 695, ~77× expansion) - 14,494 covered (27.08% line coverage) - core/ 32.3%, tools/ 16.8%, ship/ 20.7%, inspect/ 3.2% The headline rate doesn't change much (27% → 27%) because the denominator grew while the numerator stayed about the same — which is exactly the honest outcome. Per-subsystem slicing is now meaningful and diff-cover can actually score PRs that touch code outside the former test-linked slice. The `apple/` tree is still a blind spot: PULP_ENABLE_COVERAGE is Clang C/C++ only, so the Swift subsystems aren't instrumented. Tracked as #615. Also: - `.claude/commands/codex-consult.md` — new Pulp-plugin slash command wrapping `codex exec` with the right invocation pattern (stdin, not argv; `-c model_reasoning_effort="medium"`; `timeout` wrapper). Long argv prompts hang `codex exec` silently (0% CPU, no TCP); this command codifies the safe path so the next agent doesn't rediscover the gotcha. Paired with gotchas section in `~/.claude/skills/codex/`. - `DEPENDENCIES.md` + `NOTICE.md` — Apache-2.0 attribution for the vendored lcov_cobertura.py (eriwen/lcov-to-cobertura-xml 2.1.2). - `.github/workflows/coverage.yml` — gcovr install step removed; structural-empty error message retargeted at the llvm-cov path. Codex consult 2026-04-21 (Q1/Q3/Q5) confirmed approach; see `docs/guides/coverage.md` "Why not gcovr" section for the full rationale. Skill-Update: skip skill=ci reason="remove vestigial gcovr install step from coverage.yml — pipeline is now llvm-cov export → lcov_cobertura.py; not a skill-contract change" Skill-Update: skip skill=cli-maintenance reason="add .claude/commands/codex-consult.md slash command that wraps codex exec with the stdin/timeout/reasoning gotchas; command docs live in the command file itself" Skill-Update: skip skill=packages reason="DEPENDENCIES/NOTICE entries for vendored Apache-2.0 lcov_cobertura.py; standard attribution, no policy change" * coverage: pre-flight probe on -object list; skip malformed archives Linux CI on PR #616 failed with: error: failed to load coverage: 'libpulp-ship.a': malformed coverage data: coverage mapping header section is larger than buffer size `llvm-cov report` refuses the entire run if a single `-object` entry has malformed coverage mapping — one bad archive blackholes the whole pipeline. Seen only on Linux in CI for libpulp-ship.a; running the same script locally on macOS surfaces the same failure for libpulp-platform.a. Different archives go bad on different platforms, presumably because of linker-specific quirks with how ar/llvm-ar concatenates instrumented objects. Rather than guessing which archive will be bad where, add a pre-flight probe: for each binary in the `-object` list, run a minimal `llvm-cov report -object=X -instr-profile=PROFDATA >/dev/null 2>&1`. Any binary that fails to load gets dropped with a warning to stderr and excluded from the real report/export. The pipeline proceeds with the remaining entries. Measured locally: 508 initial -object entries, 1 dropped (libpulp-platform.a), 506 kept. Cobertura XML generation and llvm-cov report/show both succeed. Line coverage on main: 46.7% of ~52k lines. The dropped archive represents a small surface (ship/ or platform/) that other test binaries and executables typically still reach through their own -object entries, so effective coverage loss from a single dropped archive is small. Cost: O(N) llvm-cov invocations in the pre-flight, each reading profdata once and emitting no output. Empirically ~1s per probe on the local build; CI should see similar. * coverage: lcov_cobertura — tolerate cross-drive paths on Windows Windows CI on PR #616 failed the reporting step with: ValueError: path is on mount 'C:', start on mount 'D:' `os.path.relpath` raises ValueError when `file_name` and `self.base_dir` are on different Windows drives. The runner workspace lives on D: but FetchContent deps land on C: (GitHub-hosted windows-latest default), so absolute LCOV paths from llvm-cov straddle two drives and the converter blows up. Fix: catch the ValueError and fall back to the absolute path. Codecov ingests absolute paths fine — the relative-path step is purely an aesthetic (shorter file names in the Cobertura XML). Any entry that can't be made relative gets its absolute path kept; the rest still get shortened as before. No-op on macOS and Linux where all paths share a single root. Skill-Update: skip skill=ci reason="vendored converter tweak; no skill content change"
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.
Addresses Codex P1 comments from PRs #129 and #131.