Shader::Check ignores included files and compile options, so stale .spv files are kept #35

New issue

Open

opened 2026-10-06 13:25:47 +00:00 by jorijnvdgraaf · 0 comments

jorijnvdgraaf commented

2026-10-06 13:25:47 +00:00

Owner

Summary

Shader::Check decides whether a shader needs recompiling from one comparison: the shader source's mtime against the output's (implementations/Crafter.Build-Shader.cpp:40-43 at c6b0bba).

bool Shader::Check(const fs::path& outputDir) const {
    fs::path spv = outputDir / path.filename().replace_extension("spv");
    return fs::exists(spv) && fs::last_write_time(path) < fs::last_write_time(spv);
}

Anything else that changes the SPIR-V is invisible to it, so a build can ship stale shaders without any message. Found by reading the code.

What is not tracked

Included files. Include directories come from buildFiles (implementations/Crafter.Build-Clang.cpp:483-498), and glslang reads the includes from there, but they take no part in the staleness check (:506).
Concrete case: Crafter.Graphics's five UI kernels all #include "ui-shared.glsl", which holds the binding declarations and the shared item structs. Editing only ui-shared.glsl rebuilds none of them.
Compile options. entrypoint and type today, and the per-shader defines / variant added for the descriptor-set shader builds (pending): changing a define without touching the source keeps the old output.

Suggested fix

Use a stamp, like the std PCM does (HashStamp over the command and the source bytes): hash the source, every file the includer resolved during the compile, the entry point, the stage and the defines, and store it next to the .spv. The includer is called for every include glslang resolves, so a thin wrapper around DirStackFileIncluder can record the list at compile time without a separate scan.

## Summary `Shader::Check` decides whether a shader needs recompiling from one comparison: the shader source's mtime against the output's (`implementations/Crafter.Build-Shader.cpp:40-43` at `c6b0bba`). ```cpp bool Shader::Check(const fs::path& outputDir) const { fs::path spv = outputDir / path.filename().replace_extension("spv"); return fs::exists(spv) && fs::last_write_time(path) < fs::last_write_time(spv); } ``` Anything else that changes the SPIR-V is invisible to it, so a build can ship stale shaders without any message. Found by reading the code. ## What is not tracked - **Included files.** Include directories come from `buildFiles` (`implementations/Crafter.Build-Clang.cpp:483-498`), and glslang reads the includes from there, but they take no part in the staleness check (`:506`). Concrete case: Crafter.Graphics's five UI kernels all `#include "ui-shared.glsl"`, which holds the binding declarations and the shared item structs. Editing only `ui-shared.glsl` rebuilds none of them. - **Compile options.** `entrypoint` and `type` today, and the per-shader `defines` / `variant` added for the descriptor-set shader builds (pending): changing a define without touching the source keeps the old output. ## Suggested fix Use a stamp, like the std PCM does (`HashStamp` over the command and the source bytes): hash the source, every file the includer resolved during the compile, the entry point, the stage and the defines, and store it next to the `.spv`. The includer is called for every include glslang resolves, so a thin wrapper around `DirStackFileIncluder` can record the list at compile time without a separate scan.

jorijnvdgraaf referenced this issue from a commit

2026-10-06 14:51:11 +00:00

feat(shader): per-entry defines and a named output variant

jorijnvdgraaf referenced this issue

2026-10-06 14:51:14 +00:00

Shader variants, sysroot libc++ headers, and a fix for the project.so stamp #37