Auto-compile Metal shaders via SwiftPM build plugin - #1
Conversation
- Plugins/MetalCompilerPlugin: buildCommand globs Sources/SwiftRender/Shaders, compiles via xcrun metal, output metallib auto-embeds in the module bundle - checked-in Resources/default.metallib removed; Shaders/ excluded from resources (plugin owns them); incremental: editing a .metal recompiles - kills the silently-stale-shader footgun for good Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 52 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces automatic Metal shader compilation at build time using a SwiftPM build tool plugin. A new ChangesMetal Shader Compilation Plugin
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (3)
144-144:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale documentation about CLI metallib freshness warning.
This sentence describes a runtime warning from the old manual workflow (when users ran
tools/build_shaders.shseparately). With the MetalCompilerPlugin handling incremental compilation automatically duringswift build, this warning is no longer relevant. The plugin ensures the metallib is always up-to-date with source changes.📝 Suggested removal
- -(If a `.metal` file is newer than the compiled metallib, the CLI warns you loudly — no silently-stale shaders.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 144, Delete the outdated sentence about a CLI warning that a ".metal" file is newer than the compiled metallib from the README, since the MetalCompilerPlugin now performs incremental compilation during swift build; update any nearby context referencing manual shader build steps to avoid implying the old runtime warning behavior and ensure references to MetalCompilerPlugin remain as the source of up-to-date metallib handling.
218-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove completed roadmap item.
The first roadmap bullet "SwiftPM build plugin for automatic metallib compilation" is implemented by this PR. It should be removed from the roadmap to avoid confusion about what features are completed versus planned.
📝 Suggested removal
## Roadmap -- SwiftPM build plugin for automatic metallib compilation - `contact <Scene>` grid-sheet export · transparent ProRes 4444 · 10-bit masters - Audio-reactive FFT improvements (configurable bands, onset detection) - Linux? No — this is proudly the native-Apple lane.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 218, Remove the completed roadmap item text "SwiftPM build plugin for automatic metallib compilation" from the README roadmap section by deleting that bullet entry (the exact string appears in the roadmap list) so the roadmap only contains planned items; ensure any trailing punctuation or an empty list marker is cleaned up to keep formatting consistent.
213-213:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify Xcode requirement for the build plugin.
The parenthetical "
xcrun metalneeded only when editing shaders" is misleading with the new plugin workflow. The Metal toolchain (xcrun metal) is now invoked automatically duringswift buildby the MetalCompilerPlugin, not just when manually editing shaders. CONTRIBUTING.md more accurately describes this as "requires full Xcode for the metal toolchain."Consider rephrasing to align with CONTRIBUTING.md and clarify that the toolchain is required for builds, not just manual shader compilation.
📝 Suggested fix
-- Xcode 15+ toolchain (`xcrun metal` needed only when editing shaders) +- Xcode 15+ toolchain (Metal compiler required for builds)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 213, Update the README line "Xcode 15+ toolchain (`xcrun metal` needed only when editing shaders)" to clarify that the Metal toolchain is required for builds and is invoked automatically by the MetalCompilerPlugin during swift build; reference the same wording/intent used in CONTRIBUTING.md (e.g., "requires full Xcode for the metal toolchain") and remove the parenthetical that implies manual invocation is the only case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Package.swift`:
- Line 19: The Package.swift target's plugin declaration is using invalid
syntax; update the target's plugins entry (the one that currently reads plugins:
["MetalCompilerPlugin"]) to use the SwiftPM plugin API by replacing the string
array with a plugin specification such as plugins: [.plugin(name:
"MetalCompilerPlugin")] (if the plugin is in another package use .plugin(name:
"MetalCompilerPlugin", package: "PackageName") instead) so the target's plugin
is declared with .plugin(name: ...) rather than a plain string.
In `@Plugins/MetalCompilerPlugin/plugin.swift`:
- Around line 12-15: The plugin currently swallows failures and empties by
returning [] when directory reading fails or no .metal files are found; change
this to fail fast by propagating an error (or calling fatalError) with a clear
message including shaderDir and the underlying error when
contentsOfDirectory(atPath:) fails, and when metalFiles.isEmpty throw/raise an
error stating "no .metal files found in \(shaderDir)" so the build fails instead
of producing no default.metallib; update the code paths around entries,
metalFiles and shaderDir to return/throw an appropriate Error type (or
fatalError) rather than returning an empty array.
---
Outside diff comments:
In `@README.md`:
- Line 144: Delete the outdated sentence about a CLI warning that a ".metal"
file is newer than the compiled metallib from the README, since the
MetalCompilerPlugin now performs incremental compilation during swift build;
update any nearby context referencing manual shader build steps to avoid
implying the old runtime warning behavior and ensure references to
MetalCompilerPlugin remain as the source of up-to-date metallib handling.
- Line 218: Remove the completed roadmap item text "SwiftPM build plugin for
automatic metallib compilation" from the README roadmap section by deleting that
bullet entry (the exact string appears in the roadmap list) so the roadmap only
contains planned items; ensure any trailing punctuation or an empty list marker
is cleaned up to keep formatting consistent.
- Line 213: Update the README line "Xcode 15+ toolchain (`xcrun metal` needed
only when editing shaders)" to clarify that the Metal toolchain is required for
builds and is invoked automatically by the MetalCompilerPlugin during swift
build; reference the same wording/intent used in CONTRIBUTING.md (e.g.,
"requires full Xcode for the metal toolchain") and remove the parenthetical that
implies manual invocation is the only case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: add58148-15f3-4817-acc0-67140f3f5c5a
📒 Files selected for processing (6)
CHANGELOG.mdCONTRIBUTING.mdPackage.swiftPlugins/MetalCompilerPlugin/plugin.swiftREADME.mdSources/SwiftRender/Resources/default.metallib
| .process("Shaders"), | ||
| ] | ||
| ], | ||
| plugins: ["MetalCompilerPlugin"] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For // swift-tools-version: 5.10, is this valid in Package.swift`?
.target(name: "SwiftRender", plugins: ["MetalCompilerPlugin"])
Or must plugin usage be declared as:
.target(name: "SwiftRender", plugins: [.plugin(name: "MetalCompilerPlugin")])?`
💡 Result:
Yes, for // swift-tools-version: 5.10, the following is valid: .target(name: "SwiftRender", plugins: ["MetalCompilerPlugin"]) You must only use the explicit structured form when you need to specify the plugin’s package (i.e., the plugin is provided by another package). In that case the syntax is: .target(name: "SwiftRender", plugins: [.plugin(name: "MetalCompilerPlugin")]) If the plugin is from a dependency, you’d use the package-qualified form: .target(name: "SwiftRender", plugins: [.plugin(name: "MetalCompilerPlugin", package: "<package-name>")]) Evidence: Apple’s plugin product/target API documents the .plugin(name:capability:dependencies:path:exclude:sources:) and shows plugin usage in target declarations using .plugin(name: ...) (i.e., structured entries), and SwiftPM’s manifest/plugin usage is defined as plugins: [PluginUsage] where PluginUsage provides plugin(_ name: String, package: String? = nil). [1][2] Also, the SwiftPM evolution/proposal docs show the modern/typed syntax for target plugin usage as plugins: [ .plugin(name: "OpenAPIGenerator", package: "swift-openapi-generator") ], and it notes that when the plugin is within the same package you omit --package (so no package parameter is needed). [3]
Citations:
- 1: https://developer.apple.com/documentation/packagedescription/target/plugin(name:capability:dependencies:path:exclude:sources:)
- 2: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0303-swiftpm-extensible-build-tools.md
- 3: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0511-swiftpm-add-target-plugin.md
🌐 Web query:
SwiftPM 5.10 Package.swift target plugins: ["PluginName"] syntax valid?
💡 Result:
Yes, that specific syntax is valid (for SwiftPM 5.10 and the modern manifest API): in a target declaration you can use the plugins: parameter with an array of Target.PluginUsage, written like plugins: [.plugin(name: "PluginName")] (optionally with package: for plugins coming from another package) [1][2]. Example: let package = Package( name: "Example", targets: [.target( name: "MyTarget", plugins: [.plugin(name: "PluginName") ]) ]) Important constraints / common gotchas: 1) The plugins: parameter applies only to targets like .target(...), .executableTarget(...), and .testTarget(...) that use the manifest API supporting plugin usage [2]. 2) The plugin must be declared as a plugin target in the same Package.swift (or otherwise present in the dependency closure) and must have a compatible capability (e.g., if it’s used via plugins:, SwiftPM expects the plugin to support the buildTool capability, since plugins are “applied to a target” before the build system creates its dependency graph) [2]. 3) If you declare a plugin capability that doesn’t match what SwiftPM expects, you can get build errors complaining about mismatched capabilities (for instance, a declared command plugin being treated inconsistently) [3].
Citations:
- 1: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0511-swiftpm-add-target-plugin.md
- 2: https://github.com/apple/swift-evolution/blob/main/proposals/0303-swiftpm-extensible-build-tools.md
- 3: Vague errors for conflicting plugin declarations swiftlang/swift-package-manager#6328
🌐 Web query:
SwiftPM 5.10 plugins: ["PluginName"] in Package.swift target declaration valid or must use .plugin(name:)
💡 Result:
In SwiftPM 5.10, a target’s plugin usage must be declared with.plugin(name:) (optionally with package:), inside the target’s plugins: array. You cannot just list plugin names like ["PluginName"] in Package.swift’s target declaration. Authoritative syntax: - Define a plugin target in targets with.plugin(name:..., capability:...). [1][2] - Attach a plugin to a regular target using the plugins: parameter on.target /.executableTarget /.testTarget, with plugins: [.plugin(name: "OpenAPIGenerator", package: "...") ]. [3][4] So if your target looks like: - Wrong (not supported):.target(name: "X", plugins: ["PluginName"]) - Right (supported):.target(name: "X", plugins: [.plugin(name: "PluginName")]) (add package: only if the plugin comes from another package). [3][4]
Citations:
- 1: https://github.com/swiftlang/swift-package-manager/blob/main/Sources/PackageManagerDocs/Documentation.docc/Plugins/WritingBuildToolPlugin.md
- 2: https://developer.apple.com/documentation/packagedescription/target/plugin(name:capability:dependencies:path:exclude:sources:)
- 3: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0303-swiftpm-extensible-build-tools.md
- 4: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0511-swiftpm-add-target-plugin.md
Fix SwiftPM target plugin declaration syntax in Package.swift (line 19).
plugins: ["MetalCompilerPlugin"] isn’t valid; target plugin usage must be declared as plugins: [.plugin(name: "MetalCompilerPlugin")] (add package: only if the plugin comes from another package).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Package.swift` at line 19, The Package.swift target's plugin declaration is
using invalid syntax; update the target's plugins entry (the one that currently
reads plugins: ["MetalCompilerPlugin"]) to use the SwiftPM plugin API by
replacing the string array with a plugin specification such as plugins:
[.plugin(name: "MetalCompilerPlugin")] (if the plugin is in another package use
.plugin(name: "MetalCompilerPlugin", package: "PackageName") instead) so the
target's plugin is declared with .plugin(name: ...) rather than a plain string.
| guard let entries = try? fm.contentsOfDirectory(atPath: shaderDir.string) else { return [] } | ||
| let metalFiles = entries.filter { $0.hasSuffix(".metal") }.sorted() | ||
| .map { shaderDir.appending($0) } | ||
| guard !metalFiles.isEmpty else { return [] } |
There was a problem hiding this comment.
Fail fast when shader discovery fails (don’t silently no-op).
Line 12 and Line 15 return [] on read failure / empty shader set, which can let build pass without producing default.metallib and defer the breakage to runtime.
Suggested fix
+enum MetalCompilerPluginError: Error {
+ case shaderDirectoryUnreadable(String)
+ case noMetalShadersFound(String)
+}
+
`@main`
struct MetalCompilerPlugin: BuildToolPlugin {
func createBuildCommands(context: PluginContext, target: Target) async throws -> [Command] {
guard let target = target as? SourceModuleTarget else { return [] }
let shaderDir = target.directory.appending("Shaders")
let fm = FileManager.default
- guard let entries = try? fm.contentsOfDirectory(atPath: shaderDir.string) else { return [] }
+ let entries: [String]
+ do {
+ entries = try fm.contentsOfDirectory(atPath: shaderDir.string)
+ } catch {
+ throw MetalCompilerPluginError.shaderDirectoryUnreadable(shaderDir.string)
+ }
let metalFiles = entries.filter { $0.hasSuffix(".metal") }.sorted()
.map { shaderDir.appending($0) }
- guard !metalFiles.isEmpty else { return [] }
+ guard !metalFiles.isEmpty else {
+ throw MetalCompilerPluginError.noMetalShadersFound(shaderDir.string)
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Plugins/MetalCompilerPlugin/plugin.swift` around lines 12 - 15, The plugin
currently swallows failures and empties by returning [] when directory reading
fails or no .metal files are found; change this to fail fast by propagating an
error (or calling fatalError) with a clear message including shaderDir and the
underlying error when contentsOfDirectory(atPath:) fails, and when
metalFiles.isEmpty throw/raise an error stating "no .metal files found in
\(shaderDir)" so the build fails instead of producing no default.metallib;
update the code paths around entries, metalFiles and shaderDir to return/throw
an appropriate Error type (or fatalError) rather than returning an empty array.
…ry (sandbox-safe on cold builds) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Replaces the checked-in metallib + manual build_shaders.sh step with a build-tool plugin. Verified: bundle embedding, runtime ShaderLibrary load, incremental recompile on .metal edit, 24/24 tests green locally.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
swift buildvia a new plugin, eliminating manual compilation steps.Documentation
Chores