sparkles:shaders — Adversarial review of sections 4–8
This review is the adversarial review the Stage 0 gate requires: sections 4–8 of SPEC.md — the vocabulary, device modules, fragment entry points, the build step and build integration — read against the implementation at revision 5966597c7, with the four worked traces the gate names. It also checks every verified row of the evidence ledger by opening the test it cites.
Each requirement was attacked with a concrete input, and the input was run wherever that was cheap: the build step against sparkles:ui, and against small probe packages written outside the repository for the purpose (their sources are in the appendix). Every claim below cites a file and line at 5966597c7 or a command and its output. Section 9 was read only where sections 4–8 depend on it.
The outcome: 28 findings, two of them blockers. The two blockers are a contradiction between the per-entry interface rule (SHF2) and the unit model (SHP3), which the pinned compiler resolves silently and wrongly for a two-entry unit, and a freshness stamp whose input set misses files the device compilation reads, so --if-stale vouches for stale output. Fourteen major findings follow, most of them a requirement the code or the pinned compiler breaks with no diagnostic, or a ledger row that claims more than its test asserts. This review proposes dispositions; it accepts nothing.
Method
The review used the dev shell's toolchain: ldc2-vulkan (LDC 1.43.0 on LLVM 24.0.0git, at the SHT1 pins, which the dlang-nix input's pkgs/default.nix:193 and pkgs/llvm-spirv/default.nix:30 confirm), SPIR-V Tools and SPIRV-Cross 1.4.357.0, glslang 16.4.0, and dub 1.42.0-beta.1. The build step was run as apps/shader-compile/build/shader-compile (built by dub run :shader-compile) so that its own exit status, not dub's, is reported. dub test :shader-compile passed all seven tests (Summary: 7 passed, 0 failed). Scratch output went to /tmp; nothing in the repository was modified except this file.
Worked traces
Each trace lists its steps with the requirement that governs each, what the specification says, what the code does, and whether the two agree.
Trace A: a successful build of a two-entry unit
The unit is sparkles:ui's. It has ten entry points; the trace follows two, bloomComposite (two Sampler2D parameters, libs/ui/shaders/effects.d:120–125) and crtTube (two Sampler2D parameters and 24 uniforms, libs/ui/shaders/effects.d:197–208). Command: dub run :shader-compile -- --package=libs/ui --out=/tmp/…/a --keep; exit 0, 20 files written, 8.4 s including dub's up-to-date check of the tool.
| Step | Governs | Specification | Implementation | Agree |
|---|---|---|---|---|
| 1. The consumer opts in | SHB5, SHB7 | A separate configuration runs the step with --if-stale before the build, adds the output to the string-import paths, defines a version | gpu-effects does all three (libs/ui/dub.sdl:75–80); the default library configuration does not (libs/ui/dub.sdl:55–62) | yes |
| 2. Arguments | SHP9, §7 opening | The step takes a package directory and an output directory and "learns everything else from the package's build recipe" | It also takes --config (default shaders), --ldc, --if-stale, --keep, --quiet (apps/shader-compile/src/app.d:206–219) | partly; see R21 |
| 3. The compiler exists | SHP9, SHP10 | --ldc, else ldc2-vulkan on PATH; exit 3 when missing | ldc --version is run; status 127 → exit 3 (app.d:242–248) | yes |
| 4. Describe the device configuration | SHP1, SHP2 | dub describe of the configuration, with DFLAGS removed | dub describe --config=shaders --compiler=ldc2-vulkan under an environment without DFLAGS (app.d:253–255, app.d:631–635). Running with DFLAGS='--threads=3 -fsanitize=address' gave byte-identical GLSL | yes |
| 5. Read target, paths, versions, flags | SHP1 | Use the configuration's settings as reported | Root target's settings, deduplicated; the target flag split off (app.d:93–109); here vulkan-130 from libs/ui/dub.sdl:119 | yes |
| 6. Pick the unit | SHP3, SHM5 | Every source of the package and its dependencies that SHM5 classifies as a device module; at least one device-only | unitOf (app.d:176–197): effects.d device-only; crt_shaders.d, effect_shaders.d, math.d, types.d host-and-device (the stamp's input lines). attributes.d is not @compute and is not compiled | yes — but the test cited for SHP3 omits crt_shaders.d (R16) |
| 7. Compile once | SHP3, SHM3 | One compiler invocation | One ldc2-vulkan -O2 -c -m64 -mdcompute-targets=… over the whole unit (app.d:303–308) | yes |
| 8. The fragment interface | SHF1–SHF3 | @input locations 0, 1, … in parameter order; Sampler2D bindings 1, 2, … ; the sampler at binding 0; finalColor at location 0 | Disassembly of the kept module: fragTexCoord Location 0, fragColor Location 1, texture0 Binding 1, texture1 Binding 2, sampler Binding 0, finalColor Location 0, all in set 0 | yes for both entries — only because every entry in this unit spells the same names in the same order; a unit that does not is Trace A′ |
| 9. Validate | SHP4 | spirv-val under SPIR-V 1.4 universal rules before anything reads it | spirv-val --target-env spv1.4 (app.d:326); the module header reads Version: 1.4 | yes |
| 10. Optimise | SHP4 | spirv-opt -O | app.d:332 | yes |
| 11. Find the entries | SHP5 | Each fragment entry point of the optimised module | OpEntryPoint Fragment lines of the disassembly, in order (app.d:336–347, app.d:586–598): ten entries, bloomComposite ninth, crtTube tenth | yes |
| 12. Translate | SHP5 | <entry>.frag (GLSL 3.30) and <entry>.es.frag (ES 1.00); each combined sampler named after its Sampler2D parameter; unused variables removed | spirv-cross --remove-unused-variables per entry and dialect (app.d:353–364); the rename (app.d:617–618) yields uniform sampler2D texture0; … texture1; (bloomComposite.frag:18–19). Unused globals survive: vec4 _44; vec3 _46; … (bloomComposite.frag:8–14) | names yes; "unused variables are removed" no (R25) |
| 13. ES precision | SHV7, Q4 | The ES output "declares precision mediump float" | It declares that, then qualifies every variable highp (bloomComposite.es.frag:15–21; 174 highp in crtTube.es.frag) and declares precision highp int | no (R8) |
| 14. Provenance | SHP7 | Names the package, the device-only modules relative to the package, the entry, and that the file is generated | // Generated by shader-compile from ui's shaders/effects.d (entry point `bloomComposite`). (app.d:564–572) | yes, though the package is named ui, not sparkles:ui (R23) |
| 15. Validate each file | SHP6 | glslangValidator before it is written to the output directory | Run on the scratch copy (app.d:369–372); the output directory is written only after every file passed | yes |
| 16. Write | SHP8, SHB1 | Temp file and rename, stamp last; remove files no entry produces; the stamp lists inputs and outputs | writeAtomically (app.d:399–408), stale *.frag sweep (app.d:389–391), stamp last (app.d:392): shader-compile stamp 1, seven inputs (five modules, two recipes), 20 outputs | yes, with R3 and R19 |
| 17. Exit | SHP10 | 0 on success | 0. The output is byte-identical to libs/ui/generated/shaders/ (diff -r excluding .gitignore: no difference) | yes |
Trace A′: two entries that disagree on ordinals. Step 8 holds for sparkles:ui by convention only. Probe P2 has two entries, first(@input vec4 fragColor, Sampler2D texB, @uniform float u) and second(@input vec2 fragTexCoord, @input vec4 fragColor, Sampler2D texA, Sampler2D texB, @uniform float u). The build exits 0. Its disassembly has one variable per name, shared by both entries, each with one decoration:
OpDecorate %fragColor Location 0
OpDecorate %texB Binding 1
OpDecorate %fragTexCoord Location 0
OpDecorate %texA Binding 1For second, SHF2 requires fragColor at location 1 and texB at binding 2. Instead two inputs share location 0 and two images share binding 1, and neither spirv-val nor glslangValidator objects. This is R1.
Trace B: a stale stamp
| Step | Governs | Specification | Implementation | Agree |
|---|---|---|---|---|
1. Fresh output, --if-stale | SHB2 | Exit 0 without dub or the compiler | isFresh runs before the compiler check (app.d:239–240, app.d:472–500). On sparkles:ui's output with --ldc=/nonexistent/ldc: exit 0 | yes |
| 2. A recorded input changes | SHB1, SHB2 | Regenerate | A hash mismatch fails freshness (app.d:491–493); shaderCompile.stamp.freshUntilAnInputChanges (app.d:529–535) passes | yes |
| 3. A recorded output is deleted | SHB2 | Regenerate | app.d:496–497; the same test, app.d:538–539 | yes |
4. A non-@compute module the unit imports changes | SHB1, contract item 6 | Item 6: "a build regenerates it exactly when an input changed" | Probe P7: fx.d reads gain from consts.d, which has no @compute. After a build (finalColor = fragColor * vec4(2.0);), gain changed from 2.0 to 3.0; --if-stale --ldc=/nonexistent/ldc exits 0 and the file still reads vec4(2.0), while a fresh build writes vec4(3.0). consts.d is not a stamp input (app.d:438–453). In the repository, libs/shaders/src/sparkles/shaders/attributes.d is such a module: the unit imports it, and the stamp does not record it | no (R2) |
| 5. A new device module, imported by nothing | SHB4 (deferred) | Freshness must fail | Probe P7 plus hx.d with @fragment halve: --if-stale exits 0 and writes nothing; a forced run writes halve.frag and halve.es.frag. The comment at libs/ui/shaders/effects.d:136–139 records that this already happened while crtTube was written | no, as the ledger states (testing.md:144) |
| 6. A device module leaves the unit | SHB4 | Freshness must fail | Already holds: a recorded input that no longer exists fails freshness (app.d:492) | yes (R17) |
| 7. An input changes during generation | SHB1, SHP8 rationale | The stamp never vouches for output it does not describe | Inputs are hashed when the stamp is written (app.d:392, app.d:431), after compilation. Probe P10, edited 0.15 s and 0.2 s into a 0.25 s run: the output says finalColor = fragColor;, the source says fragColor * 0.25, and --if-stale exits 0 | no (R3) |
Trace C: a configuration without a target
Probe P10's shaders configuration lists src and shaders but no -mdcompute-targets.
| Step | Governs | Specification | Implementation | Agree |
|---|---|---|---|---|
1. Arguments parsed, no --if-stale | SHP10 | — | app.d:226–235 | — |
| 2. The compiler check | SHP10 | No precedence is stated between exit 2 and exit 3 | The compiler check comes first (app.d:242), so with --ldc=/nonexistent the same configuration exits 3 | unspecified (R14) |
| 3. Describe | SHP1 | — | succeeds | — |
| 4. No target | SHP1, SHP10 | A configuration error: exit 2, printing the cause | shader-compile: p10's configuration `shaders` names no dcompute target (`-mdcompute-targets=`), so it is not a device build; exit 2 (app.d:264–269); the output directory is not created | yes |
5. With --if-stale and a stamp that was fresh | SHB2, SHP1 | — | Not run; by reading: removing the flag edits the recipe, which is a stamp input whenever the package contributes a unit module (app.d:441–450), so freshness fails and step 4 runs. A flag removed from a recipe that is not an input would not be seen | yes for the package's own recipe |
Variations on the same theme, run against probes P2, P9, P10 and P11:
| Input | Expected (SHP10) | Observed |
|---|---|---|
--config=nope (no such configuration) | 2 | 1, Unknown build configuration: nope |
--package=/nonexistent | 2 | 1, No valid root package found |
The default --config=shaders on a package without one (P9) | 2 | 1, Unknown build configuration: shaders |
A configuration with the flag but no source files (P9 gpu) | 2, with the true cause | 2, but the message says the configuration "names no dcompute target": dub describe reports no targets, so the flag is never read |
An empty configuration without the flag (P2 library) | 2 | 1, after dub itself fails an assertion (core.exception.AssertError@source/dub/compilers/ldc.d(210): No target name set.) |
A flag naming an invalid target, vulkan-130,ocl-300 (P11) | 2 | 3, `ldc2-vulkan` is not a dcompute-enabled LDC with the `@fragment` stage |
The flag, but only host modules (P10 hostonly) | not stated; testing.md:85 expects 1 | 1 |
Trace D: a leaked selection
A scratch graph reproduces the evidence of D8: app depends on host and selects its tui configuration; host depends on lib at top level and adds backend only in its gui configuration; backend depends on lib. lib has configurations default and opt, and app asserts at compile time that lib is not built as opt.
| Step | Governs | Specification | Observed | Agree |
|---|---|---|---|---|
1. backend selects lib's opt at its top level | SHB6 rationale | dub applies it through the unselected gui | dub describe --root=app: {'app': 'application', 'host': 'tui', 'backend': 'library', 'lib': 'opt'}; dub build fails on the assertion | yes: the leak is real on dub 1.42.0-beta.1 |
2. The selection moves inside host's gui | SHB6 | Selecting beside the dependency does not leak | lib: 'default'; dub build exits 0 | yes |
| 3. The repository's backend | SHB6 | The package that needs the opt-in selects it in each configuration that adds the dependency | sparkles:ui-raylib adds sparkles:ui at top level and selects gpu-effects only in unittest (libs/ui-raylib/dub.sdl:14–34); its own recipe explains why it cannot select it anywhere else | no: the rule as written cannot be met by the package it names (R13) |
| 4. What the repository does instead | SHB6 | — | Each dependent of the backend selects sparkles:ui's configuration beside the backend: apps/terminal/dub.sdl:44–45 and 54–55, apps/hue/dub.sdl:89–90 and 124–125, libs/ui-app/dub.sdl:41–44, 51–52 and 60–61, libs/doc-view/dub.sdl:43–44 | consistent with the rationale, not with the text |
| 5. A build without the selection | SHB6 | "should fail to compile, with a message naming the configuration" | static assert(hasGpuEffects, "sparkles:ui-raylib needs sparkles:ui's `gpu-effects` configuration: …") (libs/ui-raylib/src/sparkles/ui_raylib/effect_gpu.d:41–43) | yes |
Findings
Severity: a blocker must be resolved in the specification before Stage 0 can be accepted; a major finding is a contract the code or the pinned compiler breaks without a diagnostic, a missing failure contract, or evidence that claims more than exists; a minor finding is an ambiguity or a narrow divergence; an editorial one changes wording only.
| ID | Severity | Requirements | Finding | Evidence | Proposed disposition |
|---|---|---|---|---|---|
| R1 | blocker | SHF2, SHF3, SHP3 | SHF2 assigns locations and bindings per entry point, but SHP3 compiles the whole unit into one module in which an interface name denotes one variable with one decoration. When two entries put the same name at different ordinals, the pinned compiler keeps the first decoration and assigns colliding ones to the rest — two inputs at location 0, two images at binding 1 — and nothing fails. It does diagnose a type conflict (Error: shader uniform `u` declared with two types, probe P3), so the rule exists for types but not for ordinals. | Trace A′ (probe P2); probe P3 | Amend spec: state that an interface name is one variable across the unit, so every entry that uses it must give it the same kind, type, location or binding; require a diagnostic naming both entries when they differ. Amend the compiler to emit it, and add the P2 shape to the SHF2 scenarios. |
| R2 | blocker | SHB1, SHB2, contract item 6 | The stamp's inputs are the unit's modules and their recipes. The device compilation also reads non-@compute modules the unit imports, the compiler and its druntime, and the step's own pipeline; none is recorded, so --if-stale reports fresh after any of them changes. The repository's own attributes.d is such an import. A toolchain pin change (SHT1, SHT2, whose rationale calls the pin "part of the output's definition") leaves every existing stamp fresh; only a hand-bumped stampVersion (app.d:413–415) invalidates them. | Trace B step 4 (probe P7: output vec4(2.0) kept fresh after the source said 3.0); app.d:438–453; stamp input lines of libs/ui/generated/shaders/.stamp | Amend spec: the inputs are every file the device compiler read (as the compiler reports its dependencies) plus the identity of the compiler, the SPIR-V tools and the step. Amend code accordingly. SHB4's deferral does not cover this. |
| R3 | major | SHB1, SHB2, SHP8 | The stamp hashes its inputs when it is written, after compilation, so an input edited during a run is recorded with its new hash beside output generated from its old content. The SHP8 rationale ("never a stamp that vouches for output not yet written") is met, but the stamp still vouches for output it does not describe; two concurrent runs over different content can produce the same state. | Trace B step 7 (probe P10, delays 0.15 s and 0.2 s); app.d:392, app.d:431 | Amend code: hash the inputs before compiling and write those hashes. Amend SHB1: the hashes are of the bytes the compilation read. Add the edit-during-run case to the SHP8 scenarios. |
| R4 | major | SHF1, SHP5 | Two device-only modules that each define @fragment vec4 tint produce entry points tint and tint.1, and files tint.frag and tint.1.frag; the build exits 0. Which module became which name is not specified, and tint.1 is not "named after the D function". | Probe P4 | Amend spec: an entry point name must be unique across the unit, or the compilation (or the step, exit 1) fails naming both definitions. Amend compiler or step to match. |
| R5 | major | SHF2, SHP5 | Interface names that are GLSL reserved words are renamed by translation without a diagnostic: parameters smooth, filter and active become _smooth, _filter and _active in both dialects, and the build exits 0. A consumer that binds by the D name finds nothing. A parameter named texture instead fails glslangValidator (exit 1). SHP5 protects only sampler names. | Probe P5; generated kw.frag and kw.es.frag | Amend spec: every interface name in each generated file must equal its D parameter name, or the step exits 1 naming the parameter. Amend code: compare the translated declarations with the entry's interface after spirv-cross. |
| R6 | major | SHF5, SHF2 | SHF5 forbids a vec3 at the interface but gives no failure contract, and the pinned compiler accepts what it forbids: @input vec3 c becomes in vec3 c;, @uniform int n becomes uniform uint n; (not "the parameter's type", as SHF2 requires), and @uniform Big big (a struct of float[8]) becomes a GLSL struct whose member is renamed _m0. All exit 0. | Probes P6 and Q2; generated bad.frag | Amend spec: a disallowed interface type fails the compilation with a diagnostic naming the parameter, as SHF2 does for an unmarked one. Amend the compiler. Correct testing.md:132: the gap is not only a missing test; the requirement is not implemented. |
| R7 | major | SHF2 | "Each parameter must be exactly one of these", but @input @uniform vec2 uv compiles to an input and exits 0. (An unmarked parameter is diagnosed as SHF2 requires, probe Q1.) | Probe Q3 | Amend the compiler to reject a parameter with more than one marker, or a marker on a Sampler2D; add both to the SHF2 scenarios. |
| R8 | major | SHV7, SHP5, Q4 | SHV7 measures the ES output at mediump because it "declares precision mediump float", and Q4 asks whether to request highp. The output already requests it: every variable is qualified highp and precision highp int is declared. In GLSL ES 1.00, highp in a fragment shader is optional (available only where GL_FRAGMENT_PRECISION_HIGH is defined), so these shaders can fail to compile on a conforming device, and glslangValidator does not catch it. | libs/ui/generated/shaders/bloomComposite.es.frag:4–5, 15–21; 174 occurrences of the highp qualifier in crtTube.es.frag | Amend spec: state the precision the ES output uses, and resolve Q4 now rather than at M3. Amend code if mediump is intended (the translation's precision options), or require a GL_FRAGMENT_PRECISION_HIGH guard if highp is. |
| R9 | major | SHV7 | "For every finite input" includes inputs where GLSL defines no result: sqrt of a negative number, pow with a negative base or with a zero base and a non-positive exponent, mod by zero, smoothstep with e0 >= e1, clamp with lo > hi. On those the obligation cannot be met or tested. | GLSL 4.60 §8 definitions; libs/shaders/src/sparkles/shaders/math.d:140–167 | Amend spec: restrict SHV7 to inputs on which GLSL defines the result, and fold the remaining tolerance question into Q3. |
| R10 | major | SHV1 | Equality differs by compiler and length. Under LDC, vec2 == vec2 and vec4 == vec4 are __vector(uint[N]) masks, while vec3 == vec3 is bool; under DMD all three are bool. bool same(vec2 a, vec2 b) => a == b; compiles with DMD and fails with LDC. GLSL's == on vectors is one bool. The ledger marks SHV1 verified, but its tests compare .array, never two vectors. | pragma(msg) probe under DMD and LDC; libs/shaders/src/sparkles/shaders/types.d:26–36; libs/shaders/src/sparkles/shaders/testing.d:14–43; testing.md:120 | Amend spec: define vector equality (a bool-valued function such as GLSL's all(equal(a, b)), or drop == from SHV1). Amend code to provide it on both representations, add a test, and downgrade the ledger row to partial. |
| R11 | major | SHV5, SHV6 | SHV5 requires each built-in "where GLSL defines them per component, for each vector type". step and smoothstep exist only for float; mix takes only a scalar t; there is no vector–scalar form of min, max, clamp or mod, all of which GLSL defines. Code works around it (max(d, v2(0.0f)), libs/ui/shaders/effects.d:145). exp is provided and used but not listed. The ledger's gap names only sin, cos and pow. | math.d:74–167; testing.md:122 | Amend code (vector step, smoothstep and mix, and the scalar-argument forms), or amend spec to list exactly the signatures provided. Add exp to SHV5. Correct the ledger gap. |
| R12 | major | SHV8 | SHV8 promises that a shader module "compiles as ordinary D under any compiler". The host stand-in Sampler2D has no sample, so any module that samples a texture does not: DMD reports no property `sample` for `texture0` . The design says as much — the device-only module is never host-compiled (libs/ui/shaders/effects.d:5–9). The ledger's evidence (ui.effect.builtins.areHonouredByACellGrid) runs a host-and-device module that uses neither @fragment, the parameter markers nor Sampler2D. | libs/shaders/src/sparkles/shaders/attributes.d:14–70; dmd -o- on probe H; libs/ui/src/sparkles/ui/effect.d:780; testing.md:124 | Amend spec: limit SHV8 to host-and-device modules, and state that device-only modules are never host-compiled. Downgrade the row, or add a test that host-compiles a module using every stand-in. |
| R13 | major | SHB6, D8 | SHB6 requires "a package that needs the opt-in configuration of a dependency" to select it in each of its own configurations that adds that dependency. The package that needs it, the backend, adds sparkles:ui at top level and cannot select it without the leak SHB6 exists to prevent. The repository follows a different rule: every dependent of the backend selects sparkles:ui's configuration beside the backend. Selections of intermediate configurations that carry the opt-in (sparkles:ui-app's gui and full) are made at top level by apps/hue/dub.sdl:55 and apps/ui-gallery/dub.sdl:41, and SHB6 does not say whether that is allowed. | Trace D steps 3–4 | Amend spec: state the rule practised — the opt-in is selected beside every dependency through which it is reached, by every package whose configurations add that dependency, and applications may select it at top level. Keep the compile-time check. |
| R14 | major | SHP10, SHP1, SHP3 | Configuration and usage errors exit 1, not 2: an unknown --config, a nonexistent package, a package without a shaders configuration, and an empty configuration on which dub itself crashes. A configuration that carries the flag but has no sources exits 2 with a false cause. The order of the checks is unspecified, and a missing compiler (exit 3) masks a configuration error. Whether "no device-only module" is a configuration error (2) or a failure (1) is not stated; testing.md:85 expects 1. | Trace C variations; app.d:242–276 | Amend code: report a failed dub describe as exit 2 with dub's message, and read the target from the recipe's configuration rather than from a target dub may omit. Amend spec: state the order of the checks, and the code for "no device-only module". Add a test for each exit code (M3 already plans it). |
| R15 | major | SHP10, SHP9 | Exit 3 is detected by matching three compiler messages (app.d:681–684), and the match misses in both directions. A stock LDC 1.42 passed as --ldc=ldc2 fails with module `ldc.dcompute` import `fragment` not found, which no pattern matches: exit 1, with no instructions. An invalid target value in the recipe matches Unrecognised or invalid DCompute targets: exit 3, telling the user that ldc2-vulkan is not a dcompute-enabled LDC. | Trace C; --ldc=ldc2 run; probe P11 | Amend code: probe the compiler's capability with a small known module before compiling the unit, so exit 3 does not depend on the unit's diagnostics, and validate the target value from the recipe (exit 2). |
| R16 | major | ledger: SHP3, SHP5, SHP7, SHB1–SHB3 | Rows marked verified cite tests that assert less than the row claims. shaderCompile.unitOf.repositoryUi (SHP3) feeds a hand-written source list that "is exactly what the unit must pick out", but it omits crt_shaders.d, which the real unit contains; it never runs dub describe and does not check the single invocation or the device-only minimum. The SHP5 tests check the sampler rename and the entry-point parser, not the two dialect files or the removal of unused variables. The SHP7 test uses sparkles:ui, a package name the real step never prints (it prints ui), and does not test directory independence. The stamp test writes the stamp with stampFor instead of running the step, and does not change a dependency's recipe. | app.d:686–712, app.d:575–583, app.d:502–546; testing.md:137, 139, 140, 143 | Downgrade the four rows to partial with the gaps named. Replace the hand-written list with a test of dub describe output, or derive the list from the repository. |
| R17 | minor | SHB4, D11, contract item 6 | SHB4 digests the contents of every file in every dependency's source directories. Every edit to, say, sparkles:base would then regenerate the GLSL and require the device compiler, although membership can change only when a file's classification does — which contradicts "regenerates it exactly when an input changed" and D6's cost argument. The "leaves the unit" half of SHB4 already holds (Trace B step 6). | SPEC.md:406–414; app.d:492 | Defer to M2, amending SHB4 first: digest each candidate file's relative path and its SHM5 classification rather than its contents. |
| R18 | minor | SHM5, SHM1 | The lexical classifier and the compiler disagree on spellings SHM5 does not exclude: @compute(cast(CompileFor) 1) is classified device-only (the compiler compiles it for both); @compute(CompileFor.hostAndDevice - 1) is classified host-and-device (the compiler: device-only); a file starting with a UTF-8 byte-order mark is classified host code. "Any qualified name ending in compute" can also be read as a string suffix; the code compares the last component. | A dmd -run probe of computeModeOf; libs/shaders/src/sparkles/shaders/compute_mode.d:44–61, 112–118 | Amend spec: allow only CompileFor.<member> (optionally qualified) as the argument, say "whose last component is compute", and state the byte-order mark. Amend code to skip it. |
| R19 | minor | SHP8 | "Files in the output directory that no entry point produces must be removed", but the step removes only *.frag (app.d:389–391), and the repository keeps a tracked .gitignore there (libs/ui/generated/shaders/.gitignore). A temporary *.tmp-<pid> left by a killed run is never removed. | app.d:389–391, app.d:405 | Amend spec to "every *.frag file"; amend code to remove stale temporaries. |
| R20 | minor | SHB2, SHB3 | Lines that are neither input nor output are ignored, so a stamp holding only the version line is fresh. prebuilt followed by an empty line is not prebuilt: it is then stale, and a sandbox without the compiler exits 3. SHB3 does not say whether prebuilt applies without --if-stale; the code honours it only with it. | app.d:480–499 | Amend spec: a malformed stamp is stale, and SHB3 applies under --if-stale. Amend code to reject unknown lines. |
| R21 | minor | §7 opening, SHP1, D5 | D5 says the device configuration is "recognised by its dcompute target flag rather than its name", but the step selects it by name (--config, default shaders, app.d:207, 254) and only checks the flag; §7 does not mention --config. SHP1 does not restrict the flag to a single Vulkan target: vulkan-130,ocl-200 gets as far as compiling the unit for OpenCL, and fails there (exit 1). | app.d:206–219; probe P11 | Amend spec: document --config and require exactly one Vulkan target (otherwise exit 2), or amend code to find the configuration by its flag. |
| R22 | minor | SHF7 | No other stage attribute can be written: the pinned compiler's ShaderStage has only fragment, and the _shader struct that carries a stage is private. The obligation is vacuous and cannot be tested from D source. | ldc/dcompute.d:162–174 in the pinned compiler's druntime | Amend spec: drop SHF7, or restate it as "the compiler declares no other stage"; mark the ledger row accordingly. |
| R23 | minor | SHP7 | "Names the package": the step prints the name of the package recipe it was pointed at (ui), not the name its dependents use (sparkles:ui). The header lists every device-only module, not the one that defines the entry, so adding any device module rewrites every file's first line. | Trace A step 14; probe P7 (from p7's shaders/fx.d, shaders/hx.d (entry point `tint`)) | Amend spec: say which name and which modules; consider naming the defining module. |
| R24 | minor | SHT1, ledger | SHT1 requires a pin change to "pass the acceptance suite of testing.md", but the parts of that suite that check device behaviour (SHV7, SHF4, the disassembly checks) are planned; the ledger row SHT1, SHT2: verified covers only that the flake pins the components. | testing.md:123, 131, 148 | Amend the ledger: split the pinning (verified) from the pin-change gate (planned, M3). |
| R25 | editorial | SHP5 | "Unused variables are removed" is broader than what happens: --remove-unused-variables removes unused interface variables; unused global temporaries remain. | bloomComposite.frag:8–14 | Amend spec to "unused interface variables". |
| R26 | editorial | SHT3, SHP7 | "Byte-identical output … in every directory" holds for the GLSL (P10 built from two directories and working directories: identical) but not for the stamp, whose input paths are relative to the package and therefore differ when a dependency lives elsewhere. | diff -r of the two outputs: only .stamp lines 3, 4 and 6 differ | Amend spec: say "generated GLSL". |
| R27 | editorial | SHB7 | The requirement attributes a "lock-protected build cache" to dub run --temp-build, whose help text says only "Builds the project in the temp folder if possible". The property may hold, but nothing in the tree supports it. | dub run --help; a warm run took 0.54 s after a 4.94 s cold one | Cite the dub behaviour relied on, or state the obligation (parallel dependents do not race on the tool's build) without naming the mechanism. |
| R28 | editorial | libs/ui/dub.sdl | A comment has lost its first line: // artifact uses. Neither `debug` … directly follows the shaders configuration. | libs/ui/dub.sdl:121–122 | Amend code (restore the first line). |
Not covered
This review did not examine:
- Section 9 beyond the pin revisions: the CI legs that
SHT4cites (Windows, aarch64 Linux and macOS) were not inspected, and no cross-platform output was compared (SHT3). - Host/device numeric agreement (
SHV7,SHF4): no generated shader was executed on a GPU or a software rasterizer. SHF6: no disassembly was searched for images in composites or in Function storage.SHM3andSHM4: no device-rule violation was seeded, and the editor analysis's mirror of the rules (dmd_lsp.testing.dcompute.*,TGT7) was not compared with the compiler.- Concurrency beyond one process: several simultaneous runs against one output directory (
SHP8) and parallel dependents (SHB7) were not exercised. - The Nix packaging of prebuilt output (
nix/packages/ui-shaders.nix:52–58) was read, not built. The claim attesting.md:146that CI builds the terminal-only examples in a sandbox withoutldc2-vulkanwas not traced to a specific job. - Sections 1–3 and the cold read of the opening, which the gate lists separately.
- Publication validation: this page has not been added to
docs/.vitepress/sidebar.jsonor to thesrcExcludelist indocs/.vitepress/docs-config.json(where the fuzzy review is listed), soci --check-docs-sidebarwill report it until one of them is.
Proposed dispositions
Proposed by the follow-up work, one commit per concern; each awaits the owner's acceptance.
| ID | Proposed disposition | Evidence |
|---|---|---|
| R1 | Fixed. SHF2 states that an interface name is one variable across the unit; the compiler rejects an entry that gives it another kind, location or binding, naming both entries. | shaderCompile.cli.interfaceContractIsEnforced (the P2 shape); the fork's lit test |
| R2 | Amended specification. The stamp is restated as an edit-loop shortcut over the unit's modules and recipes; freshness of output that ships is Nix's, whose inputs include the imports and the toolchain (D14, section 8, contract item 6). | nix/packages/ui-shaders.nix |
| R3 | Fixed. The inputs are hashed before compiling (SHB1). | shaderCompile.cli.anInputEditedDuringTheRunLeavesItStale, failing before the fix |
| R6 | Fixed. SHF5 gains its failure contract, and the compiler enforces it, naming the parameter and the reason. What was rejected because it miscompiled (int as uint, members as _m0) is now translated correctly; a vec3 may cross, as it occupies the same bytes on both sides (SHV2). | shaderCompile.cli.interfaceContractIsEnforced; probeUniforms in ui_raylib.shader_readback.hostAndDeviceAgree |
| R8 | Amended specification. The ES output keeps highp, at the project's VP_ANDROID_baseline_2022 floor (D15); SHV7 names it. Q4 is closed. | the phone captures recorded in D15 |
| R14 | Fixed. SHP10 states the order of the checks; every configuration error, dub's included, exits 2, and a configuration without a device-only module is one. | shaderCompile.cli.exitCodes |
| R15 | Fixed. Exit 3 comes from what the compiler can compile (a minimal @fragment shader for the target), not from its messages; the target must be exactly one Vulkan target (exit 2). | shaderCompile.cli.exitCodes, shaderCompile.cli.stockCompilerIsAToolchainError |
| R21 | Fixed in part: exactly one Vulkan target is required (exit 2). The configuration is still selected by name with --config. | shaderCompile.cli.exitCodes |
Outside the findings, SHP6 gained its negative case (shaderCompile.cli.aRejectedTranslationWritesNothing). The other findings await dispositions.
Acceptance
This review is a draft request for dispositions, not a disposition. Each finding needs one of the dispositions of Acceptance Review and Closure — fixed, rejected or deferred — recorded with its evidence. Acceptance of the specification, and of each disposition, is the project owner's to record. It is not implied by this review, by its merging, or by the dispositions it proposes.
Appendix: probe packages
Every probe is a dub package outside the repository with this recipe, where <checkout> is the repository root and <name> the probe's name; P7 adds importPaths "shaders" to the shaders configuration. Each was run as shader-compile --package=<probe> --out=<dir>.
name "<name>"
targetType "library"
dependency "sparkles:shaders" path="<checkout>"
configuration "library" {
}
configuration "shaders" {
sourcePaths "shaders"
dflags "-mdcompute-targets=vulkan-130"
}Each module below starts with @compute(CompileFor.deviceOnly), module <m>; and import sparkles.shaders;, omitted here.
| Probe | Module(s) under shaders/ | Result |
|---|---|---|
| P2 | @fragment vec4 first(@input vec4 fragColor, Sampler2D texB, @uniform float u) => texB.sample(v2(0.5f)) * fragColor * u; and @fragment vec4 second(@input vec2 fragTexCoord, @input vec4 fragColor, Sampler2D texA, Sampler2D texB, @uniform float u) => texA.sample(fragTexCoord) * texB.sample(fragTexCoord) * fragColor * u; | exit 0; colliding locations and bindings (R1) |
| P3 | first(@input vec2 uv, @uniform float u) and second(@input vec2 uv, @uniform vec2 u) | exit 1, shader uniform `u` declared with two types |
| P4 | fx.d and gx.d, each @fragment vec4 tint(@input vec4 fragColor) | exit 0; tint.frag and tint.1.frag (R4) |
| P5 | @fragment vec4 kw(@input vec2 smooth, Sampler2D tex, @uniform float filter, @uniform float active) | exit 0; _smooth, _filter, _active (R5). With the sampler named texture: exit 1 at glslangValidator |
| P6 | @fragment vec4 bad(@input vec3 c, @uniform int n) => v4(c, 1.0f) * cast(float) n; | exit 0; in vec3 c;, uniform uint n; (R6) |
| P7 | fx.d: import consts : gain; @fragment vec4 tint(@input vec4 fragColor) => fragColor * gain;; consts.d (no @compute): enum float gain = 2.0f;; later hx.d: @fragment vec4 halve(@input vec4 fragColor) => fragColor * 0.5f; | Trace B steps 4–5 (R2) |
| P9 | No sources; configurations library and gpu (the latter with the flag) | Trace C variations (R14) |
| P10 | Configurations library, shaders (sources src and shaders, without the flag) and hostonly (with it); src/host.d holds a host function; shaders/fx.d holds @fragment vec4 tint(@input vec4 fragColor) => fragColor; | Trace C; with the flag added, Trace B step 7 and R26 |
| P11 | P10, with -mdcompute-targets=vulkan-130,ocl-300, then …,ocl-200 | exit 3 with a false cause, then exit 1 (R15, R21) |
| Q2 | struct Big { float[8] a; } and @fragment vec4 bad(@input vec2 uv, @uniform Big big) => v4(big.a[0]); | exit 0; uniform fx_Big big; with member _m0 (R6) |
| Q3 | @fragment void nothing(@input vec2 uv) {} and @fragment vec4 both(@input @uniform vec2 uv) => v4(uv.x); | exit 0; both takes uv as an input (R7) |
| Q1 | @fragment vec4 bad(@input vec2 uv, float loose) => v4(loose); | exit 1, `@fragment` parameter `loose` must be marked `@input` or `@uniform`, or be a `Sampler2D` (SHF2 holds) |
| H | @fragment vec4 tint(@input vec2 uv, Sampler2D texture0) => texture0.sample(uv);, compiled by dmd -o- -Ilibs/shaders/src without a device target | no property `sample` for `texture0` (R12) |