Skip to content

Forward native standard streams to platform diagnostics - #233

Open
bkaradzic-microsoft wants to merge 7 commits into
BabylonJS:mainfrom
bkaradzic-microsoft:jsruntime-console-logger
Open

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

Conversation

@bkaradzic-microsoft

@bkaradzic-microsoft bkaradzic-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
Copilot AI lite review requested due to automatic review settings August 27, 2026 17:37
@bkaradzic-microsoft bkaradzic-microsoft mentioned this pull request Aug 27, 2026
Closed
Copilot AI reviewed Aug 27, 2026
View reviewed changes

Copilot AI 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
File Description
Core/Foundation/Source/StandardStreamLogger.cpp Implements platform-specific stream redirection, background draining, and forwarding to platform diagnostics.
Core/Foundation/Include/Babylon/StandardStreamLogger.h Declares the opt-in Start/Stop/IsStarted API contract.
Core/Foundation/CMakeLists.txt Adds the new logger sources to Foundation and links Android’s log library when needed.
Tests/UnitTests/Shared/StandardStreamLogger.cpp Adds a lifecycle unit test plus stdout capture helper to validate start/stop behavior.
Tests/UnitTests/CMakeLists.txt Includes the new shared test source in the unit test build.
Tests/UnitTests/Android/app/src/main/cpp/JNI.cpp Starts/stops the StandardStreamLogger around Android unit test execution.
Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt Builds 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 thread Core/Foundation/Source/StandardStreamLogger.cpp Outdated
Comment thread Tests/UnitTests/Shared/StandardStreamLogger.cpp
bkaradzic and 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-microsoft bkaradzic-microsoft left a comment

Copy link
Copy Markdown
Member Author

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.

@bkaradzic-microsoft
bkaradzic-microsoft requested a lite review from Copilot August 27, 2026 18:53
Copilot AI reviewed Aug 27, 2026
View reviewed changes

Copilot AI 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 thread Core/Foundation/Source/StandardStreamLogger.cpp Outdated
Comment thread Core/Foundation/Source/StandardStreamLogger.cpp Outdated
Comment thread Core/Foundation/Source/StandardStreamLogger.cpp Outdated
Comment thread Tests/UnitTests/Android/app/src/main/cpp/JNI.cpp Outdated
Comment thread Core/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 thread Core/Foundation/Include/Babylon/StandardStreamLogger.h Outdated
Comment thread Core/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 thread Core/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 thread Core/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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@ryantrem ryantrem ryantrem approved these changes
@bghgary bghgary Awaiting requested review from bghgary
@CedricGuillemet CedricGuillemet Awaiting 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