ci: add hlsl validation#1145
Conversation
WalkthroughThis update modularizes the CI/CD workflow by separating C++ build and shader validation into distinct jobs, introduces workflow inputs for manual control, and adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant CMake
participant PowerShell Script
participant Python/hlslkit
User->>GitHub Actions: Trigger workflow (PR or manual)
GitHub Actions->>check-changes: Analyze file changes
alt C++ build needed
GitHub Actions->>cpp-build: Build C++ plugin/addons
cpp-build->>CMake: Build targets
end
alt Shader validation needed
GitHub Actions->>shader-validation: Run shader validation job
shader-validation->>CMake: Run prepare_shaders target
shader-validation->>Python/hlslkit: Validate shaders
Python/hlslkit-->>shader-validation: Report results
end
User->>PowerShell Script: Run generate-shader-configs.ps1 (manual or CI)
PowerShell Script->>Skyrim Log Files: Detect and validate logs
PowerShell Script->>hlslkit-generate: Generate YAML config files
PowerShell Script-->>User: Output results
GitHub Actions->>prerelease/release: Post-release steps (if applicable)
Suggested reviewers
Poem
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
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds HLSL shader validation support to the CI pipeline.
- Introduces a new CMake custom target ("prepare_shaders") to allow shader preparation independent of the full build.
- Updates the GitHub Actions workflow to include a dedicated shader-validation job with relevant input conditions and caching adjustments.
- Updates documentation for configuration files used in shader validation.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| CMakeLists.txt | Added a custom target for preparing shaders, enabling independent CI validation. |
| .github/workflows/build.yaml | Modified job names, inputs, and conditional triggers; added the shader-validation job. |
| .github/configs/README.md | Added documentation for the CI configuration files related to shader validation. |
Comments suppressed due to low confidence (1)
.github/workflows/build.yaml:155
- Ensure that the check-changes job sets the 'hlsl-should-build' output; otherwise, this condition may not work as intended.
needs.check-changes.outputs.hlsl-should-build == 'true'
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/configs/README.md (1)
5-8: Unify bullet punctuation and fix grammar.The list items are missing terminal punctuation and the third bullet needs an article and semicolon for clarity. For example:
- Defines shader compilation parameters, known warnings, and expected build configurations + Defines shader compilation parameters, known warnings, and expected build configurations. - Used by the GitHub Actions workflow to validate shader compilation during CI builds + Used by the GitHub Actions workflow to validate shader compilation during CI builds. - Not intended for direct user interaction - this is build infrastructure configuration + Not intended for direct user interaction; this is a build infrastructure configuration.🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: Make sure that the noun ‘build’ is correct. Did you mean the past participle “built”?
Context: ...d for direct user interaction - this is build infrastructure configuration ## Files ...(BE_VB_OR_NN)
.github/workflows/build.yaml (2)
29-30: Extend PR trigger to include HLSL shader files.Adding
**.hlsland**.hlsliensures that shader changes correctly trigger CI. Consider using**/*.hlslsyntax for consistency, but the current patterns will match as intended.
211-214: Validate shaders with strict warning policy.Running
hlslkit-compilewith--max-warnings 0and suppressing known non-critical warnings (X1519) enforces shader quality. Consider pinning thehlslkitversion to maintain reproducibility.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/configs/README.md(1 hunks).github/workflows/build.yaml(9 hunks)CMakeLists.txt(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.github/configs/README.md
[grammar] ~7-~7: Make sure that the noun ‘build’ is correct. Did you mean the past participle “built”?
Context: ...d for direct user interaction - this is build infrastructure configuration ## Files ...
(BE_VB_OR_NN)
[uncategorized] ~11-~11: Loose punctuation mark.
Context: ... ## Files - shader-validation.yaml: Configuration for shader compilation va...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (12)
CMakeLists.txt (1)
190-196:Details
✅ Verification successful
Validate shader preparation target dependencies and indentation.
The new
prepare_shaderstarget depends oncopy_shaders.stamp, but the variableHLSL_FILES(used upstream for shader dependencies) must be defined to track.hlsl/.hlsliinputs. Confirm thatHLSL_FILESis globally populated, or introduce:file(GLOB_RECURSE HLSL_FILES "${CMAKE_SOURCE_DIR}/package/Shaders/*.hlsl" "${CMAKE_SOURCE_DIR}/features/*/Shaders/*.hlsl" )Also ensure this block uses the same tab-based indentation as the rest of the file.
🏁 Script executed:
#!/bin/bash # Verify that HLSL_FILES is defined in CMake files rg -n "HLSL_FILES" -R .Length of output: 428
🏁 Script executed:
#!/bin/bash # Properly verify that HLSL_FILES is defined in any CMakeLists or included cmake modules rg -n "HLSL_FILES" .Length of output: 533
🏁 Script executed:
#!/bin/bash # Inspect how copy_shaders.stamp is generated and its dependencies rg -n "copy_shaders.stamp" -C 5 CMakeLists.txtLength of output: 1614
prepare_shaders target dependencies and indentation verified
The
HLSL_FILESvariable is already populated incmake/AddCXXFiles.cmakeand exported viaPARENT_SCOPE, socopy_shaders.stampcorrectly tracks all.hlsl/.hlsliinputs. Theprepare_shaderstarget depends oncopy_shaders.stampas intended, and the block uses the same tab-based indentation as the surrounding CMakeLists. No further changes required..github/workflows/build.yaml (11)
12-21: Add manual dispatch inputs for C++ build and shader validation.Introducing
build-cppandvalidate-shadersworkflow inputs gives precise control over which jobs run on manual trigger. The defaults (true) align with existing behavior. Ensure these inputs are properly documented for contributors.
87-96: Refine execution condition forcpp-buildjob.The
ifnow respects both manual dispatch (build-cpp) and file-change detection for C++ files. This dual gating prevents unnecessary builds while allowing manual overrides.
104-109: Use precise checkout parameters for forked PRs.Specifying
refandrepositoryensures the correct heads are checked out when running onpull_request_target, avoiding permission and context mismatches.
122-126: Improve cache key for invalidation flexibility.Incorporating
inputs.cache-key-suffixinto the cache key allows targeted cache busting. The fallback keys are well-ordered for optimal cache hits.
127-132: Upgraderun-cmakeaction and presets.Bumping to
lukka/run-cmake@v10and using theALLpresets standardizes the build invocation. Confirm that v10 supports the sameconfigurePreset/buildPresetarguments.Please verify that
lukka/run-cmake@v10still accepts theconfigurePresetandbuildPresetsyntax used here.
146-147: Set artifact retention period.Extending artifact retention to 30 days for
dist-artifactsbalances storage costs and troubleshooting needs.
148-157: Introduceshader-validationjob with conditional execution.The new job correctly gates on HLSL file changes and the
validate-shadersinput. This modularization keeps CI performant and flexible.
185-194: Isolate cache per shader validation configuration.Using
${{ matrix.config.name }}in the cache key segregates caches for "Flatrim" and "VR" runs, preventing cross-contamination.
195-201: Prepare shaders with explicit target.Invoking
--target prepare_shadersisolates shader copying from the full build, speeding up validation. Ensure thebuildPresetAdditionalArgssyntax is compatible with the action.
216-225: Upload shader validation logs on failure.Uploading both
new_issues.logand any.logensures comprehensive debugging data. The 7-day retention period is reasonable for ephemeral shader errors.
227-229: Ensure prerelease waits for shader validation.Adding
shader-validationto theneedslist guarantees that pre-release artifacts include validated shaders.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yaml(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (16)
.github/workflows/build.yaml (16)
12-16: Introducebuild-cppworkflow input
Defines a boolean input to manually trigger the C++ build job.Please verify that comparing
github.event.inputs.build-cppto the string'true'behaves as expected whentype: booleanis used.
17-21: Introducevalidate-shadersworkflow input
Adds a boolean toggle for manual shader validation runs.Confirm that
github.event.inputs.validate-shaders == 'true'correctly triggers the shader-validation job withtype: boolean.
29-30: Extend PR trigger patterns to include HLSL files
Includes.hlsland.hlsliinpull_request_targetpaths so shader changes kick off validation.
48-48: Rename and clarify the change-check job
Updating the job name to “Check for changes in PRs” improves readability.
101-101: Addcpp-buildjob for building plugin and addons
Defines the Windows build workflow for C++ components.
103-110: Review cpp-build conditional
Theifcondition gates C++ builds on manual inputs or file-changes detection.
118-123: Use dynamic checkout ref and repository
Switching toref: ${{ github.event.pull_request.head.ref || github.ref }}and dynamicrepositoryensures PR forks are correctly built.
141-146: Invoke CMake build withrun-cmake
Usinglukka/run-cmake@v10is consistent with existing build steps.
160-161: Set retention-days for dist artifacts
Extends artifact availability to 30 days for downstream consumption.
162-170: Addshader-validationjob with change-based gating
New job to validate HLSL shaders on Windows, triggered by manual input or detected.hlslchanges.
209-214: ValidatebuildPresetAdditionalArgsformat
Confirm thatbuildPresetAdditionalArgs: "['--target prepare_shaders']"is parsed correctly bylukka/run-cmake.
242-243: Chainshader-validationinto prerelease step
Updatingneeds: [cpp-build, shader-validation]ensures shaders are validated before prerelease.
298-299: Update prerelease action name and tag
Using dynamic version and PR number in release metadata improves traceability.
311-312: Update auto-generated prerelease action
Maintains consistency with custom-notes path while using GitHub-generated notes.
328-329: Update download link comment message
Points directly to the prerelease tag artifact in the repo.
331-335: Incorporate shader-validation into release job
Ensuring shader checks are part of both prerelease and final release workflows.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/configs/README.md (1)
20-26: Small grammar nitpick in prerequisite sentence“wait for compilation to complete to generate” reads awkwardly. Consider:
-4. **Run the Game**: Launch and wait for compilation to complete to generate shader compilation logs +4. **Run the Game**: Launch and wait for compilation to finish; this will generate the shader-compilation logsPurely cosmetic—feel free to ignore if style differs.
🧰 Tools
🪛 LanguageTool
[style] ~25-~25: The verb ‘complete’ normally expects the gerund. Consider using “generating”.
Context: ...ch and wait for compilation to complete to generate shader compilation logs The required l...(TO_VB_TO_VB)
.github/configs/generate-shader-configs.ps1 (1)
186-202: Superfluous back-tick before colon in error messageWrite-Error "Error generating $OutputName`: $($_.Exception.Message)"The back-tick escapes the following space, not the colon, resulting in a literal back-tick in the output. Remove it:
-Write-Error "Error generating $OutputName`: $($_.Exception.Message)" +Write-Error "Error generating $OutputName: $($_.Exception.Message)"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/configs/README.md(1 hunks).github/configs/generate-shader-configs.ps1(1 hunks)CMakeLists.txt(2 hunks)
🧰 Additional context used
🪛 LanguageTool
.github/configs/README.md
[uncategorized] ~7-~7: Loose punctuation mark.
Context: ... ## Files - shader-validation.yaml: Configuration for shader compilation va...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~25-~25: The verb ‘complete’ normally expects the gerund. Consider using “generating”.
Context: ...ch and wait for compilation to complete to generate shader compilation logs The required l...
(TO_VB_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (1)
CMakeLists.txt (1)
205-210:Details
❓ Verification inconclusive
prepare_shaderstarget is gated behind deployment options—CI may not define them
prepare_shadersis created only whenAUTO_PLUGIN_DEPLOYMENT OR AIO_ZIP_TO_DISTis ON.
If the new GitHub Actions job invokescmake --build … --target prepare_shaderswithout setting either option, the build will error with “Unknown build target”.Either:
- Move the
add_custom_target(prepare_shaders …)outside the conditional block, or- Explicitly pass
-DAIO_ZIP_TO_DIST=ON(or similar) from the workflow.Please verify the workflow behaviour.
🏁 Script executed:
#!/bin/bash # Show the context around the prepare_shaders target grep -R -nC3 "add_custom_target.*prepare_shaders" CMakeLists.txt # Check where the AUTO_PLUGIN_DEPLOYMENT and AIO_ZIP_TO_DIST variables are used grep -R -n "AUTO_PLUGIN_DEPLOYMENT" CMakeLists.txt grep -R -n "AIO_ZIP_TO_DIST" CMakeLists.txtLength of output: 1028
🏁 Script executed:
#!/bin/bash # Display the AUTO_PLUGIN_DEPLOYMENT/AIO_ZIP_TO_DIST block and surrounding lines sed -n '180,260p' CMakeLists.txtLength of output: 3539
Ensure
prepare_shaderstarget is always available in CIThe
prepare_shaderstarget is currently defined inside this block:CMakeLists.txt (around lines 185–210):
if(AUTO_PLUGIN_DEPLOYMENT OR AIO_ZIP_TO_DIST) # … add_custom_target(prepare_shaders DEPENDS copy_shaders.stamp COMMENT "Preparing shaders for validation" ) endif()Note that
AIO_ZIP_TO_DISTdefaults to ON, so by default the target exists—but if your CI workflow ever overrides it (or leaves both flags OFF), runningcmake --build . --target prepare_shaderswill error with “Unknown build target.”
Either
- Move the
add_custom_target(prepare_shaders …)invocation outside theif(...)guard so it’s always defined, or- Explicitly pass
-DAIO_ZIP_TO_DIST=ON(or-DAUTO_PLUGIN_DEPLOYMENT=ON) in your CI jobPlease verify how your GitHub Actions workflow configures these flags and update accordingly.
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/build.yaml (1)
136-140: Cache key context correctly updated
Switching togithub.event.inputs.cache-key-suffixfixes the invalid context issue flagged earlier.
🧹 Nitpick comments (2)
.github/workflows/build.yaml (2)
103-110: Conditional logic forcpp-buildis correct but verbose
The combinedworkflow_dispatchandpull_request_targetconditions work as intended. For readability, consider extracting parts into named expressions or commenting each branch.
221-224: Pinhlslkitfor reproducibility
Installing directly from themainbranch may introduce breakages. Consider locking to a commit hash or version tag.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/configs/generate-shader-configs.ps1(1 hunks).github/workflows/build.yaml(9 hunks)CMakeLists.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CMakeLists.txt
- .github/configs/generate-shader-configs.ps1
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (12)
.github/workflows/build.yaml (12)
12-21: Add manual dispatch inputs for C++ build and shader validation
Providingbuild-cppandvalidate-shadersboolean inputs improves workflow flexibility for manual triggers.
29-30: Include HLSL file patterns in pull_request_target
Adding**.hlsland**.hlsliensures shader changes drive downstream validation.
48-48: Skip: trivial job renaming
Renaming the job to "Check for changes in PRs" is descriptive and needs no further review.
121-123: Explicit checkout of PR head ref and repo
Using bothrefandrepositoryensures the correct branch and fork are checked out in PR contexts.
160-161: Add retention-days on dist artifacts
Settingretention-days: 30ensures build artifacts are kept for a sensible period without manual cleanup.
162-172: Introduce newshader-validationjob
The job is gated by both PR HLSL changes and the newvalidate-shadersinput, aligning validation with code changes and manual triggers.
199-208: Cache CMake build output in shader-validation
Reusing the build cache keyed bymatrix.config.nameandcache-key-suffixwill speed up repeated validation runs.
209-214: VerifybuildPresetAdditionalArgssyntax
The value is passed as a single quoted string. Please confirmluka/run-cmake@v10accepts this format (vs. a YAML list) for--target prepare_shaders.
225-228: Shader compilation step is well configured
Using--max-warnings 0with suppression flagX1519enforces strict validation while allowing known benign warnings.
229-238: Upload shader validation logs with retention
Artifacts are retained for 7 days andif-no-files-found: ignoreavoids failures when no new issues are present.
241-243: Extend prerelease to depend on shader-validation
Ensuring bothcpp-buildandshader-validationcomplete before prerelease avoids shipping unvalidated shaders.
331-336: Extend release to depend on shader-validation
Aligning the final release step with shader validation maintains consistency across manual and tag-based releases.
Summary by CodeRabbit
New Features
Documentation
Chores