ExampleShaders compiles with the glslang on PATH, not the one crafter-build links, so it misses the regressions it was written for #170

New issue

Open

opened 2026-10-31 14:57:15 +00:00 by jorijnvdgraaf · 0 comments

jorijnvdgraaf commented

2026-10-31 14:57:15 +00:00

Owner

Summary

tests/ExampleShaders (#164) exists because crafter-build compiles the examples' shaders with the glslang it links, unpinned, and a change there broke Sponza without anything noticing. The test compiles them with the glslang on PATH instead (tests/ExampleShaders/main.cpp:113-117), which its header says "need not be the glslang crafter-build links" (:16-17). In practice it never is, so a shader that only crafter-build's glslang rejects still passes. Line numbers refer to 670df31.

Details

Under the Flatpak, the test's glslang is the runtime's /usr/bin/glslang (16.4.0, on the runtime's libglslang.so.16), while /app/bin/crafter-build links no glslang library, so it carries its own. Natively it is the distro's (16.6.0 here).
Even with the same glslang, the CLI decides differently from crafter-build's Shader::Compile (implementations/Crafter.Build-Shader.cpp in Crafter.Build, lines as of b26807f): a define without a value is #define NAME 1 there (:159) and an empty macro with the CLI's -DNAME; and Shader::Compile fails when GlslangToSpv logs anything (:195-206), while the test only looks at the CLI's exit status (main.cpp:117).
It finds shaders by listing examples/<Name>/*.glsl and takes the stage from the file name (:48-59, :89-94), not from the examples' registrations. That covers every example today, but a shader in a subdirectory would be left out silently, which the header says cannot happen (:13-14).
It writes into one fixed directory, temp_directory_path()/crafter-example-shaders-test (:83), ignores create_directories' error (:85) and uses the throwing directory_iterator (:89, :91), so a stale unwritable directory or an unreadable entry shows up as failures or an abort instead of a clear message.
It is registered through LibraryTest (project.cpp:564), which builds every library source into it, though it imports only std.

Suggested fix

Let crafter-build compile them, as HeapASRead has its shaders compiled: register each example's shaders on the test's config.shaders with the ShaderType, defines and variant its example uses, so the linked glslang and Shader::Compile's rules decide, and a shader that no longer compiles fails the test's build. main then only checks that the .spv files are there. That drops the CLI, the temp directory and the stage guessing; the cost is keeping the registrations in step with the examples' project.cpp files.

## Summary `tests/ExampleShaders` (#164) exists because crafter-build compiles the examples' shaders with the glslang it links, unpinned, and a change there broke Sponza without anything noticing. The test compiles them with the `glslang` on `PATH` instead (`tests/ExampleShaders/main.cpp:113-117`), which its header says "need not be the glslang crafter-build links" (`:16-17`). In practice it never is, so a shader that only crafter-build's glslang rejects still passes. Line numbers refer to `670df31`. ## Details - Under the Flatpak, the test's `glslang` is the runtime's `/usr/bin/glslang` (16.4.0, on the runtime's `libglslang.so.16`), while `/app/bin/crafter-build` links no glslang library, so it carries its own. Natively it is the distro's (16.6.0 here). - Even with the same glslang, the CLI decides differently from crafter-build's `Shader::Compile` (`implementations/Crafter.Build-Shader.cpp` in Crafter.Build, lines as of `b26807f`): a define without a value is `#define NAME 1` there (`:159`) and an empty macro with the CLI's `-DNAME`; and `Shader::Compile` fails when GlslangToSpv logs anything (`:195-206`), while the test only looks at the CLI's exit status (`main.cpp:117`). - It finds shaders by listing `examples/<Name>/*.glsl` and takes the stage from the file name (`:48-59`, `:89-94`), not from the examples' registrations. That covers every example today, but a shader in a subdirectory would be left out silently, which the header says cannot happen (`:13-14`). - It writes into one fixed directory, `temp_directory_path()/crafter-example-shaders-test` (`:83`), ignores `create_directories`' error (`:85`) and uses the throwing `directory_iterator` (`:89`, `:91`), so a stale unwritable directory or an unreadable entry shows up as failures or an abort instead of a clear message. - It is registered through `LibraryTest` (`project.cpp:564`), which builds every library source into it, though it imports only `std`. ## Suggested fix Let crafter-build compile them, as `HeapASRead` has its shaders compiled: register each example's shaders on the test's `config.shaders` with the `ShaderType`, defines and variant its example uses, so the linked glslang and `Shader::Compile`'s rules decide, and a shader that no longer compiles fails the test's build. `main` then only checks that the `.spv` files are there. That drops the CLI, the temp directory and the stage guessing; the cost is keeping the registrations in step with the examples' `project.cpp` files.