Added install_llvmpipe script to replace -nographics in Windows builds #706
Conversation
📝 Walkthrough""" WalkthroughThe GitHub Actions workflow for Windows builds was updated to add a new matrix entry that tests the Changes
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
|
Looks good to me, but lets make this based on the That way we're also sure we don't inadvertently ship different behaviour and potentially break other implementers' workflow. |
|
@webbertakken I'm open to that but it seems like we'll need a whole another test matrix row to actually test that, is that ok? Or do we want to add just a single additional windows build target test? |
|
Hmm that seems a bit overkill. Perhaps we can expand the matrix to have a single test for this feature specifically? |
![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)
.github/workflows/build-tests-windows.yml (1)
30-30: Align inline comment casing and clarity
For consistency, update the comment to match the property name and clarify the target platform.- # Additionally test enableGPU build for a standalone windows target + # Additionally test enableGpu build for StandaloneWindows64 with GPU support
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (5)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapdist/platforms/windows/build.ps1is excluded by!**/dist/**dist/platforms/windows/entrypoint.ps1is excluded by!**/dist/**dist/platforms/windows/install_llvmpipe.ps1is excluded by!**/dist/**
📒 Files selected for processing (1)
.github/workflows/build-tests-windows.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/build-tests-windows.yml (1)
29-34: Add GPU-enabled Windows build matrix entry
This include block cleanly adds a single test variation forStandaloneWindows64withenableGpu: true, which aligns with the PR goal of verifying the new graphics flag without expanding the full matrix.
| include: | ||
| # Additionally test enableGPU build for a standalone windows target | ||
| - projectPath: test-project | ||
| unityVersion: 2023.2.2f1 | ||
| targetPlatform: StandaloneWindows64 | ||
| enableGpu: true |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify custom action supports enableGpu input and applies -force-opengl
Ensure that the local Unity build action (referenced via uses: ./) defines an enableGpu input and, when true, replaces -nographics with -force-opengl in its invocation.
🏁 Script executed:
#!/bin/bash
# Description: Confirm that the custom GameCI action recognizes `enableGpu` and uses it to set `-force-opengl`.
# 1. Search action metadata & implementation for enableGpu
rg 'enableGpu' -n .
# 2. Search for force-opengl flag usage
rg 'force-opengl' -n .Length of output: 66784
enableGpu flag not applied in Unity CLI invocation
The custom action defines the enableGpu input (see action.yml:42), but there’s no code handling it to swap out the default -nographics flag for -force-opengl. You’ll need to update the Unity command builder so that when buildParameters.enableGpu is true it:
- removes or omits the
-nographicsargument - adds the
-force-openglargument
And add or extend tests (e.g. in src/model/input.test.ts) to verify that the CLI args include -force-opengl only when enableGpu is true.
🤖 Prompt for AI Agents
In .github/workflows/build-tests-windows.yml around lines 29 to 34, the
enableGpu flag is set but the Unity CLI invocation does not handle it to replace
the default -nographics flag with -force-opengl. To fix this, update the Unity
command builder code to check if buildParameters.enableGpu is true; if so,
remove or omit the -nographics argument and add the -force-opengl argument
instead. Additionally, extend or add tests, such as in src/model/input.test.ts,
to verify that the CLI arguments include -force-opengl only when enableGpu is
true.
|
@webbertakken ok sorry for the long wait. I added enableGpu parameter support and expanded test matrix to include a single windows standalone build with enableGpu. I also pinned the build-mesa version to whatever is latest today, because I talked with the repository maintainer and he suggested doing that. |
Changes
Added
install_llvmpipe.ps1script on windows which executes before activation, downloads latest release ofhttps://github.com/mmozeiko/build-mesaand puts resulting opengl32.dll near the Editor.exe. On success, sets LLVMPIPE_INSTALLED variable to true and replaces-nographicswith-force-openglin the build script.Related Issues
#703
Successful Workflow Run Link
https://github.com/DoctorGester/unity-builder/actions/runs/14870908406
Seems like all main platform builds pass and WSAPlayer is failing in other PRs as well.
Checklist
code of conduct
in the documentation repo)
Summary by CodeRabbit