Conversation
📝 WalkthroughWalkthroughLinux platform support added: public header types for Linux, embedded runtime wiring to carry a Linux GL area through the C ABI, and renderer updates to initialize and gate OpenGL behavior for embedded/Linux contexts. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Runtime
participant CABI as C ABI (ghostty_platform_u)
participant Renderer as OpenGL Renderer
participant GLArea as GtkGLArea
App->>CABI: read PlatformTag (.linux) and linux.gl_area
CABI-->>App: return linux.gl_area pointer
App->>Renderer: init with platform.linux (gl_area)
Renderer->>GLArea: prepareContext(null) to load GL functions
Renderer-->>App: surfaceInit / displayRealized complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
The current failing |
Greptile SummaryThis PR adds Linux embedded host support for Key changes:
Issue found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Host as GtkGLArea Host (C)
participant CABI as ghostty.h (C ABI)
participant Embedded as apprt/embedded.zig
participant OGL as renderer/OpenGL.zig
participant Thread as renderer/Thread.zig
Host->>CABI: ghostty_surface_new(platform_tag=LINUX, platform.linux.gl_area)
CABI->>Embedded: Platform.init(.linux, C{gl_area})
Embedded->>Embedded: validate gl_area != null
Embedded-->>CABI: Platform{ .linux = { gl_area } }
Host->>OGL: surfaceInit(surface)
Note over OGL: Host has already made GL context current
OGL->>OGL: prepareContext(null) — load GL fn ptrs
OGL->>OGL: threadEnter() — no-op (host owns context)
Note over Thread: must_draw_from_app_thread = true on Linux
Thread->>Host: app_mailbox.push(redraw_surface) instead of direct drawFrame
Host->>OGL: displayRealized()
OGL->>OGL: prepareContext(null) — re-load GL fn ptrs
Last reviewed commit: 4d5e3bf |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| typedef union { | ||
| ghostty_platform_macos_s macos; | ||
| ghostty_platform_ios_s ios; | ||
| ghostty_platform_linux_s linux; |
There was a problem hiding this comment.
linux field name conflicts with predefined GCC/Clang macro
On Linux, GCC and Clang in GNU-extensions mode (e.g. -std=gnu11, which is the default and common when building GTK apps) predefine linux as a macro that expands to 1. Any C translation unit that #includes this header on Linux will fail to compile because the field declaration expands to:
ghostty_platform_linux_s 1; // preprocessor expansion of `linux` → compile errorThe macOS and iOS fields (macos, ios) do not have this problem because macos and ios are not predefined macros on those platforms.
The fix is to use a name that doesn't collide with the predefined macro. Options include:
typedef union {
ghostty_platform_macos_s macos;
ghostty_platform_ios_s ios;
ghostty_platform_linux_s linux_platform; // or linux_host, gl_area_platform, etc.
} ghostty_platform_u;Or, alternatively, #undef linux at the top of the header (before the union) and restore it afterward with #ifdef __linux__ / #define linux 1. However, renaming the field is cleaner and avoids unexpected side-effects for downstream consumers. The Zig-side Platform.C.linux and Platform.linux variant names would need to be updated to match.
| .linux => if (Linux != void) linux: { | ||
| const config = c_platform.linux; | ||
| const gl_area = config.gl_area orelse | ||
| break :linux error.UnsupportedPlatform; | ||
| break :linux .{ .linux = .{ .gl_area = gl_area } }; | ||
| } else error.UnsupportedPlatform, |
There was a problem hiding this comment.
Null gl_area returns misleading error
When gl_area is null, the code returns error.UnsupportedPlatform, but that error is also used for the cross-platform case (building for non-Linux and passing GHOSTTY_PLATFORM_LINUX). A caller on Linux who simply forgets to set gl_area will receive an error that suggests the entire platform is unsupported, which will be confusing to debug.
The iOS branch uses the more descriptive error.UIViewMustBeSet for the analogous case. Consider using a similarly specific error here:
| .linux => if (Linux != void) linux: { | |
| const config = c_platform.linux; | |
| const gl_area = config.gl_area orelse | |
| break :linux error.UnsupportedPlatform; | |
| break :linux .{ .linux = .{ .gl_area = gl_area } }; | |
| } else error.UnsupportedPlatform, | |
| .linux => if (Linux != void) linux: { | |
| const config = c_platform.linux; | |
| const gl_area = config.gl_area orelse | |
| break :linux error.GlAreaMustBeSet; | |
| break :linux .{ .linux = .{ .gl_area = gl_area } }; | |
| } else error.UnsupportedPlatform, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/apprt/embedded.zig (1)
401-407: Consider using a more specific error for nullgl_area.When
gl_areais null, returningerror.UnsupportedPlatformis inconsistent with the macOS/iOS paths which return dedicated errors (error.NSViewMustBeSet,error.UIViewMustBeSet). A specific error likeerror.GLAreaMustBeSetwould provide better diagnostics for host developers.Proposed change
.linux => if (Linux != void) linux: { const config = c_platform.linux; const gl_area = config.gl_area orelse - break :linux error.UnsupportedPlatform; + break :linux error.GLAreaMustBeSet; break :linux .{ .linux = .{ .gl_area = gl_area } }; } else error.UnsupportedPlatform,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/apprt/embedded.zig` around lines 401 - 407, The linux path currently maps a null gl_area to error.UnsupportedPlatform; change this to a specific error (e.g., error.GLAreaMustBeSet) for clarity: in the .linux branch that reads const gl_area = config.gl_area orelse break :linux error.UnsupportedPlatform, replace the error with break :linux error.GLAreaMustBeSet and add the corresponding error declaration (error.GLAreaMustBeSet) to the error set used by the surrounding function/enum so callers see a distinct, descriptive failure instead of UnsupportedPlatform.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/apprt/embedded.zig`:
- Around line 401-407: The linux path currently maps a null gl_area to
error.UnsupportedPlatform; change this to a specific error (e.g.,
error.GLAreaMustBeSet) for clarity: in the .linux branch that reads const
gl_area = config.gl_area orelse break :linux error.UnsupportedPlatform, replace
the error with break :linux error.GLAreaMustBeSet and add the corresponding
error declaration (error.GLAreaMustBeSet) to the error set used by the
surrounding function/enum so callers see a distinct, descriptive failure instead
of UnsupportedPlatform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92680cbf-43f4-45c0-8747-a54a10a135ae
📒 Files selected for processing (3)
include/ghostty.hsrc/apprt/embedded.zigsrc/renderer/OpenGL.zig
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/generic.zig (1)
1503-1506: Prefer a capability flag here instead ofbuiltin.target.os.tag.This makes shared renderer policy depend on a specific OS rather than the surface/runtime contract that actually drives the resize behavior. If another embedded host needs different sync-resize handling later,
generic.zigwill need more platform branching again. Pushing this behind anapprt/GraphicsAPIcapability would keep the shared renderer generic.As per coding guidelines, "Maintain shared Zig core in the
src/directory".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/generic.zig` around lines 1503 - 1506, The current logic for use_sync_stale_frame_guard uses builtin.target.os.tag which ties shared renderer behavior to a specific OS; instead add a capability flag on the apprt (or the GraphicsAPI/runtime capability struct) such as supports_sync_stale_frame_guard and use that in the switch that computes use_sync_stale_frame_guard (replace builtin.target.os.tag check with apprt.supports_sync_stale_frame_guard); update the embedded host initializations that set apprt.runtime to also set this capability appropriately for Linux vs non‑Linux embedded hosts so the platform-specific decision is owned by the host capability rather than by generic.zig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/generic.zig`:
- Around line 1503-1506: The current logic for use_sync_stale_frame_guard uses
builtin.target.os.tag which ties shared renderer behavior to a specific OS;
instead add a capability flag on the apprt (or the GraphicsAPI/runtime
capability struct) such as supports_sync_stale_frame_guard and use that in the
switch that computes use_sync_stale_frame_guard (replace builtin.target.os.tag
check with apprt.supports_sync_stale_frame_guard); update the embedded host
initializations that set apprt.runtime to also set this capability appropriately
for Linux vs non‑Linux embedded hosts so the platform-specific decision is owned
by the host capability rather than by generic.zig.
shuhei0866
force-pushed
the
draft/linux-embedded-host-support
branch
from
10cb29c to
9b0febb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/apprt/embedded.zig`:
- Around line 402-406: The Linux branch currently returns
error.UnsupportedPlatform when config.gl_area is missing; change that to a
distinct error (e.g., error.MissingLinuxGLArea) so missing caller configuration
is distinguishable from an unsupported build. Update the break at the gl_area
check (the line using "const gl_area = config.gl_area orelse break :linux
error.UnsupportedPlatform;") to return the new error variant instead, and add
the new error symbol to the module's error union/enum where other errors are
declared so the code compiles and callers can handle this specific
missing-configuration case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d9c933e-2e7b-492b-b300-86ed7677b6ff
📒 Files selected for processing (4)
include/ghostty.hsrc/apprt/embedded.zigsrc/renderer/OpenGL.zigsrc/renderer/generic.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/OpenGL.zig
| .linux => if (Linux != void) linux: { | ||
| const config = c_platform.linux; | ||
| const gl_area = config.gl_area orelse | ||
| break :linux error.UnsupportedPlatform; | ||
| break :linux .{ .linux = .{ .gl_area = gl_area } }; |
There was a problem hiding this comment.
Use a distinct error when gl_area is missing.
Returning error.UnsupportedPlatform here makes “Linux embedding isn't built in” indistinguishable from “the caller forgot to populate platform.linux.gl_area”, which makes host-side init failures harder to diagnose.
🩹 Proposed fix
- const gl_area = config.gl_area orelse
- break :linux error.UnsupportedPlatform;
+ const gl_area = config.gl_area orelse
+ break :linux error.GtkGLAreaMustBeSet;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/apprt/embedded.zig` around lines 402 - 406, The Linux branch currently
returns error.UnsupportedPlatform when config.gl_area is missing; change that to
a distinct error (e.g., error.MissingLinuxGLArea) so missing caller
configuration is distinguishable from an unsupported build. Update the break at
the gl_area check (the line using "const gl_area = config.gl_area orelse break
:linux error.UnsupportedPlatform;") to return the new error variant instead, and
add the new error symbol to the module's error union/enum where other errors are
declared so the code compiles and callers can handle this specific
missing-configuration case.
1 participant
Summary
Add the Linux-specific embedded host support needed for a
GtkGLArea-based libghostty host.Problem
The embedded runtime already works for the macOS host, and
#8addresses the renderer-side stale-frame behavior during resize.That still leaves a separate gap for the Ubuntu/cmux host:
apprt.embeddeddoes not recognize a Linux host surfaceWithout this, the Ubuntu host is relying on local-only Ghostty changes for basic surface creation and draw-thread behavior.
Change
GHOSTTY_PLATFORM_LINUXandghostty_platform_linux_stoghostty.hsrc/apprt/embedded.zigto accept a Linuxgl_areaplatform payloadmust_draw_from_app_threadsrc/renderer/OpenGL.ziginitialize from the current embedded GL context and treatdisplayRealizedlike GTKWhy this layer
This is platform bring-up for the embedded runtime, not renderer resize policy.
#8keeps resize presentation policy correct forapprt.embeddedKeeping them separate makes review clearer:
#8: renderer policyValidation
Validated through the cmux Ubuntu host integration path:
GtkGLAreapayload into libghosttyzig build -Dapp-runtime=nonepasses in this branchNote: full
zig buildin this environment still stops earlier on missing local build tools (msgfmt,blueprint-compiler), which is unrelated to this diff.Notes
#8.Related draft PRs
This PR is one part of the Ghostty-side dependency chain for the Ubuntu/cmux MVP.
Scope split:
#8: renderer policy forapprt.embeddedresize presentation#9: Linux embedded host support forGtkGLArea/ OpenGL bring-upSummary by cubic
Add Linux embedded host support for
GtkGLAreaso Linux hosts can create a surface and render via the embedded runtime. OpenGL now initializes from the host-owned context, draws on the app thread, and the resize stale-frame guard override is limited to Linux to avoid stale visuals during interactive resize.New Features
GHOSTTY_PLATFORM_LINUXandghostty_platform_linux_s { gl_area }toghostty.h.gl_areapayload; setsmust_draw_from_app_threadon Linux.displayRealizedlike GTK; no thread setup for embedded.Bug Fixes
Written for commit 9b0febb. Summary will update on new commits.
Summary by CodeRabbit