Conversation
WalkthroughThis pull request adds Linux/Mac platform support to the Walnut framework while updating build configurations. Changes include implementing a Linux/Mac entry point in EntryPoint.h, adding platform-specific build filters for Windows and Linux, updating GLFW dependency paths across build files, and updating the GLFW submodule reference. Changes
Sequence DiagramsequenceDiagram
participant Platform as Platform (Linux/Mac)
participant Main as Walnut::Main()
participant Factory as CreateApplication()
participant App as Application
Platform->>Main: main(argc, argv)
loop while g_ApplicationRunning
Main->>Factory: CreateApplication(argc, argv)
Factory-->>App: new Application()
Main->>App: Run()
App-->>Main: (completes)
Main->>App: delete
end
Main-->>Platform: return status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Walnut/src/Walnut/EntryPoint.h (1)
43-53: Consider applying the namespace wrapper refactoring consistently across both platforms.Both the Windows (line 5) and Linux/Mac (line 43) branches use identical qualified declarator syntax for
CreateApplication. While this works in practice due to include ordering in consuming code (Walnut/Application.hincluded beforeEntryPoint.h), wrapping the declaration innamespace Walnut { ... }blocks—as suggested in the proposed diff—is cleaner and removes reliance on include order. Apply this refactoring to both branches for consistency, or keep both as-is if include order is already established as a requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Walnut/src/Walnut/EntryPoint.h` around lines 43 - 53, The CreateApplication declaration is currently written with a qualified declarator (extern Walnut::Application* Walnut::CreateApplication(...)) which relies on include ordering; update both platform branches to use a namespace wrapper for clarity and consistency by moving the declaration inside a namespace Walnut { extern Application* CreateApplication(int argc, char** argv); } and similarly ensure the Main function and g_ApplicationRunning usage remain in the Walnut namespace (e.g., int Main(int argc, char** argv) and bool g_ApplicationRunning) so both Windows and Linux/Mac branches consistently declare CreateApplication inside namespace Walnut rather than using the fully-qualified declarator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@premake5.lua`:
- Around line 12-18: The global links entry in Walnut/premake5.lua is adding
%{Library.Vulkan} (a Windows-only vulkan-1.lib) unconditionally, breaking Linux
builds; move the Vulkan linking into the system filters instead: remove or stop
linking %{Library.Vulkan} from the global links block and add links {
"%{Library.Vulkan}" } inside the filter "system:windows", and ensure filter
"system:linux" uses links { "vulkan" } (update the existing filters
"system:windows" and "system:linux" in premake5.lua accordingly).
---
Nitpick comments:
In `@Walnut/src/Walnut/EntryPoint.h`:
- Around line 43-53: The CreateApplication declaration is currently written with
a qualified declarator (extern Walnut::Application*
Walnut::CreateApplication(...)) which relies on include ordering; update both
platform branches to use a namespace wrapper for clarity and consistency by
moving the declaration inside a namespace Walnut { extern Application*
CreateApplication(int argc, char** argv); } and similarly ensure the Main
function and g_ApplicationRunning usage remain in the Walnut namespace (e.g.,
int Main(int argc, char** argv) and bool g_ApplicationRunning) so both Windows
and Linux/Mac branches consistently declare CreateApplication inside namespace
Walnut rather than using the fully-qualified declarator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3aab22c0-5d8f-4dae-8d3c-319b2511c0f5
📒 Files selected for processing (5)
Walnut/premake5.luaWalnut/src/Walnut/EntryPoint.hWalnutExternal.luapremake5.luavendor/GLFW
| filter "system:windows" | ||
| systemversion "latest" | ||
| defines { "WL_PLATFORM_WINDOWS" } | ||
| links { "%{Library.Vulkan}" } | ||
|
|
||
| filter "system:linux" | ||
| links { "vulkan" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Vulkan link token definition and usage across premake files.
# Expected: detect Windows-specific token definition and unconditional usage sites.
rg -n -C2 'Library\["Vulkan"\]|vulkan-1\.lib' --type lua
rg -n -C2 '%{Library\.Vulkan}' --type lua
rg -n -C2 'filter\s*"system:(windows|linux)"|links\s*\{[^}]*"vulkan"[^}]*\}' --type luaRepository: StudioCherno/Walnut
Length of output: 1280
🏁 Script executed:
# Search for Vulkan-related content in Walnut/premake5.lua specifically
fd -n "premake5.lua" | while read f; do
echo "=== File: $f ==="
rg -n "Vulkan|vulkan|Library" "$f" -C1
doneRepository: StudioCherno/Walnut
Length of output: 293
🏁 Script executed:
# Read the full Walnut/premake5.lua to see the complete context
cat -n Walnut/premake5.luaRepository: StudioCherno/Walnut
Length of output: 1434
Move Vulkan linking to system-specific filters in Walnut/premake5.lua.
Walnut/premake5.lua unconditionally links %{Library.Vulkan} (line 27), which resolves to vulkan-1.lib from WalnutExternal.lua—a Windows-only library. This breaks Linux builds even though the root premake5.lua correctly filters Vulkan by system. Move the Vulkan link from the global links block to per-system filters:
Proposed fix
--- a/Walnut/premake5.lua
+++ b/Walnut/premake5.lua
@@ -24,8 +24,6 @@
{
"ImGui",
"GLFW",
-
- "%{Library.Vulkan}",
}
targetdir ("bin/" .. outputdir .. "/%{prj.name}")
@@ -33,6 +31,7 @@
filter "system:windows"
systemversion "latest"
defines { "WL_PLATFORM_WINDOWS" }
+ links { "%{Library.Vulkan}" }
+
+ filter "system:linux"
+ links { "vulkan" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@premake5.lua` around lines 12 - 18, The global links entry in
Walnut/premake5.lua is adding %{Library.Vulkan} (a Windows-only vulkan-1.lib)
unconditionally, breaking Linux builds; move the Vulkan linking into the system
filters instead: remove or stop linking %{Library.Vulkan} from the global links
block and add links { "%{Library.Vulkan}" } inside the filter "system:windows",
and ensure filter "system:linux" uses links { "vulkan" } (update the existing
filters "system:windows" and "system:linux" in premake5.lua accordingly).
1 participant
No description provided.