Skip to content

Forward native standard streams to platform diagnostics - #233

Merged
bkaradzic-microsoft merged 7 commits into
BabylonJS:mainfrom
bkaradzic-microsoft:jsruntime-console-logger
Aug 31, 2026
Merged

Forward native standard streams to platform diagnostics#233
bkaradzic-microsoft merged 7 commits into
BabylonJS:mainfrom
bkaradzic-microsoft:jsruntime-console-logger

Conversation

@bkaradzic-microsoft

@bkaradzic-microsoftbkaradzic-microsoft commented Aug 27, 2026

Copy link
Copy Markdown
Member

Captures process-level stdout and stderr so native and third-party diagnostics reach each platform's normal diagnostic channel without routing through JavaScript.

What

  • Adds an opt-in, N-API-independent Babylon::StandardStreamLogger in Core/Foundation.
  • Windows tees both streams to their original destinations and OutputDebugString.
  • Android tees both streams to their original destinations and logcat.
  • iOS and macOS tee both streams to their original destinations and unified logging.
  • Linux and other Unix platforms leave standard streams unchanged because terminals, CI pipes, journald, and containers already consume them.
  • Replaces the Android unit-test host's AndroidExtensions::StdoutLogger with the shared implementation.

Start() and Stop() are idempotent. The host opts in before initializing third-party libraries, and Stop() restores the original destinations and drains pending output.

Why

The original JsConsoleLogger design only captured migrated call sites, required a live Napi::Env, and could not capture libraries writing directly through printf or fprintf. Redirecting the process streams at the application boundary covers both first-party and third-party native output while preserving existing terminal, file, parent-process, and CI destinations.

Tests

  • Win32/Chakra: 12 C++ tests and 225 JavaScript assertions passing.
  • New lifecycle test covers repeated start/stop calls, exact tee behavior, and a final line without a trailing newline.
  • The Foundation source also compiles through its non-Windows Unix path.

@bkaradzic
Add cross-platform standard stream forwarding
76ef338
Add an opt-in Foundation utility that tees process stdout and stderr to
platform diagnostics while preserving the original stream destinations.
Use it in the Android unit-test host and cover lifecycle and tee behavior.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
CopilotAI lite review requested due to automatic review settings August 27, 2026 17:37
@bkaradzic-microsoftbkaradzic-microsoft mentioned this pull request Aug 27, 2026
Closed
CopilotAI reviewed Aug 27, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a cross-platform Babylon::StandardStreamLogger in Foundation to forward native stdout/stderr output into platform diagnostic channels (Windows OutputDebugString, Android logcat, Apple unified logging) while preserving the original stream destinations, and wires it into the Android unit-test host plus new unit coverage.

Changes:

  • Introduces Babylon::StandardStreamLogger API + implementation with start/stop lifecycle and per-platform forwarding.
  • Adds a new lifecycle unit test and builds it into the shared unit-test targets (including Android JNI test library).
  • Replaces the Android unit-test host’s prior stdout logger with the shared StandardStreamLogger.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
FileDescription
Core/Foundation/Source/StandardStreamLogger.cppImplements platform-specific stream redirection, background draining, and forwarding to platform diagnostics.
Core/Foundation/Include/Babylon/StandardStreamLogger.hDeclares the opt-in Start/Stop/IsStarted API contract.
Core/Foundation/CMakeLists.txtAdds the new logger sources to Foundation and links Android’s log library when needed.
Tests/UnitTests/Shared/StandardStreamLogger.cppAdds a lifecycle unit test plus stdout capture helper to validate start/stop behavior.
Tests/UnitTests/CMakeLists.txtIncludes the new shared test source in the unit test build.
Tests/UnitTests/Android/app/src/main/cpp/JNI.cppStarts/stops the StandardStreamLogger around Android unit test execution.
Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txtBuilds the new shared test into the Android JNI unit-test library.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadCore/Foundation/Source/StandardStreamLogger.cpp Outdated
Comment threadTests/UnitTests/Shared/StandardStreamLogger.cpp
bkaradzicand others added 2 commits August 27, 2026 10:58
@bkaradzic
Fix StandardStreamLogger UWP and iOS CI failures
f8b8619
Use CreatePipe/_open_osfhandle and _sopen_s so the Windows path builds on
UWP, and stop writing to stderr in the lifecycle test because iOS CI captures
simctl launch stderr as the process exit code.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
@bkaradzic
Mark both POSIX pipe ends CLOEXEC in StandardStreamLogger
227d45a
Avoid a concurrent exec inheriting the write end and delaying Drain EOF
on Stop. Windows already creates non-inheritable pipe ends.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
bkaradzic-microsoft
bkaradzic-microsoft commented Aug 27, 2026
View reviewed changes

@bkaradzic-microsoftbkaradzic-microsoft left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-requesting Balanced Copilot review after CLOEXEC fix and replies.

CopilotAI reviewed Aug 27, 2026
View reviewed changes

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Comment threadCore/Foundation/Source/StandardStreamLogger.cpp Outdated
Comment threadCore/Foundation/Source/StandardStreamLogger.cpp Outdated
Comment threadCore/Foundation/Source/StandardStreamLogger.cpp Outdated
Comment threadTests/UnitTests/Android/app/src/main/cpp/JNI.cpp Outdated
Comment threadCore/Foundation/Include/Babylon/StandardStreamLogger.h
@bkaradzic
Address Copilot follow-ups on StandardStreamLogger
e29f716
Keep timed-out drain futures so Start() cannot install a second tee while a
detached drain is still alive. Consume pending lines with a start index to
avoid quadratic erase, document the 3800-byte platform line cap and IsStarted()
contract, and log Android Stop() failures to logcat.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
ryantrem
ryantrem reviewed Aug 28, 2026
View reviewed changes
Comment threadCore/Foundation/Include/Babylon/StandardStreamLogger.h Outdated
Comment threadCore/Foundation/Source/StandardStreamLogger.cpp
@bkaradzic
Split StandardStreamLogger into platform TUs
ac36b14
Match AppRuntime: shared API + one JSRUNTIMEHOST_PLATFORM implementation.
Windows (Win32/UWP), Android, Apple, and Unix each get their own source file;
Android/Apple share the POSIX tee body via an .inl. Also fix IsStarted() doc
comment indentation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
ryantrem
ryantrem reviewed Aug 28, 2026
View reviewed changes
Comment threadCore/Foundation/Source/StandardStreamLogger_Windows.cpp
@bkaradzic
Share StandardStreamLogger tee body across platforms
1c4519a
Pull Drain/StartChannel/StopChannel into StandardStreamLogger_Shared.inl and
leave each platform TU as thin OS ops (fd/pipe/null, diagnostic sink, and
Windows StdHandle hooks). Android/Apple share POSIX fd helpers via
StandardStreamLogger_PosixOps.inl.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
ryantrem
ryantrem reviewed Aug 28, 2026
View reviewed changes
Comment threadCore/Foundation/CMakeLists.txt Outdated
@bkaradzic
Clarify StandardStreamLogger .inl CMake listing
60c6b53
Android/Apple already #include PosixOps.inl; the CMake entries are IDE-only.
Mark .inl files HEADER_FILE_ONLY and document that so they are not mistaken
for separate translation units.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
@bkaradzic-microsoft
bkaradzic-microsoft merged commit a6b98ea into BabylonJS:mainAug 31, 2026
25 checks passed
@matthargettmatthargett mentioned this pull request Sep 1, 2026
Draft
@bkaradzic-microsoftbkaradzic-microsoft mentioned this pull request Sep 2, 2026
Merged
1 task
bkaradzic-microsoft added a commit to BabylonJS/BabylonNative that referenced this pull request Sep 2, 2026
@bkaradzic-microsoft
Bump JsRuntimeHost to a6b98eaa (forward std streams) (#1861)
f37edf3
## Summary
Bump `JsRuntimeHost` from `0d263627` to `a6b98eaa` (current `main`).
## What's included
-
[JsRuntimeHost#233](BabylonJS/JsRuntimeHost#233)
— Forward native standard streams to platform diagnostics
Master already had through
[JsRuntimeHost#220](BabylonJS/JsRuntimeHost#220)
(setInterval queue fix). This is the single remaining commit on `main`.
## Test plan
- [ ] CI green across engines/platforms
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@ryantremryantremryantrem approved these changes
@bghgarybghgaryAwaiting requested review from bghgary
@CedricGuillemetCedricGuillemetAwaiting requested review from CedricGuillemet

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bkaradzic-microsoft@ryantrem@bkaradzic