fix(build): run offline bake with portable zstd and Clang 18 #1954

Merged

Arkex-ai merged 2 commits from fix/bake-portability into main

2026-11-11 03:11:40 +00:00

Arkex-ai commented

2026-11-10 21:36:37 +00:00

(Migrated from github.com)

Summary

Offline shader bake tools could fail on a clean Windows setup when they could not find a system Zstd library. They now use the project's existing Python dependency. The compiler check also accepts Clang 18 when it supports the constexpr assertion code the translator uses.

Closes #1931.

Technical changes

Use the pinned Python Zstandard package for dictionary fixtures and decoding, while preserving the existing standalone fallbacks.
Size unknown-size frames with bounded reads, and reject truncated frames and corrupt trailing bytes.
Probe the actual C++23 constexpr assertion path. Compiler discovery and cache identity are tracked in #1872.
Keep main's translator reuse from #2025 and its current Windows bake-test settings.

Validation

At b5890847 on d47206ed, all 5 portability checks passed on Windows LLVM and Ubuntu Clang 18. The checks cover real dictionary frames, corrupt and truncated input, native-reader fallback and the compiler probe.
Hosted source checks passed with 330 passed / 0 failed / 24 skipped on Windows and 344 / 0 / 10 on Ubuntu. The portability fixture ran on both. The scope is CPU-side shader baking.

Current-head check	Result
Linux AppImage	Passed
Windows build	Passed
Source suite	Passed

## Summary Offline shader bake tools could fail on a clean Windows setup when they could not find a system Zstd library. They now use the project's existing Python dependency. The compiler check also accepts Clang 18 when it supports the constexpr assertion code the translator uses. Closes #1931. ## Technical changes - Use the pinned Python Zstandard package for dictionary fixtures and decoding, while preserving the existing standalone fallbacks. - Size unknown-size frames with bounded reads, and reject truncated frames and corrupt trailing bytes. - Probe the actual C++23 constexpr assertion path. Compiler discovery and cache identity are tracked in #1872. - Keep main's translator reuse from #2025 and its current Windows bake-test settings. ## Validation - At `b5890847` on `d47206ed`, all **5 portability checks passed** on Windows LLVM and Ubuntu Clang 18. The checks cover real dictionary frames, corrupt and truncated input, native-reader fallback and the compiler probe. - Hosted source checks passed with **330 passed / 0 failed / 24 skipped on Windows** and **344 / 0 / 10 on Ubuntu**. The portability fixture ran on both. The scope is CPU-side shader baking.  | Current-head check | Result | |---|---| | [Linux AppImage](https://github.com/NotRexed/Hookshot/actions/runs/42938152510) | Passed | | [Windows build](https://github.com/NotRexed/Hookshot/actions/runs/42938152766) | Passed | | [Source suite](https://github.com/NotRexed/Hookshot/actions/runs/42938152528) | Passed |

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 02:18:58 +00:00

hckrman101 (Migrated from github.com) left a comment

Nits, none blocking:

The unknown-size path decodes each frame twice (count pass, then the exact-size decode). Fine for the shader archives; just worth knowing if a very large container shows up.
The over-limit check is cap > 1 << 31 here and cap >= (1 << 31) in _Zstd; harmless, but the two readers could agree.
cxx_std still caches the result without keying it by compiler, and a stale -std=c++2a left in an old work/bake/cxx_std would still be returned. You note this is tracked in #1872, so fine to leave.

Looks good. I read the diff against the description and it matches: the zstandard path is tried first with the libzstd/CLI fallbacks kept, unknown-size frames are counted in bounded chunks and then validated with `allow_extra_data=False` (so truncated and trailing-garbage frames are rejected, and the tests cover both), the dictionary fixture now fails loudly instead of writing `stats["dict"] = "zstd -D failed"`, and the new constexpr probe mirrors the assert.h failure path. `zstandard==0.25.0` is already pinned in requirements.txt, `RUN_ALL_ARGS = ["--cxx", "{cxx}"]` is a known placeholder, and the new test is declared the way run_all expects. All CI checks are green on this head, including the Windows and Ubuntu source suites. No earlier discussion on the PR to reconcile. Nits, none blocking: - The unknown-size path decodes each frame twice (count pass, then the exact-size decode). Fine for the shader archives; just worth knowing if a very large container shows up. - The over-limit check is `cap > 1 << 31` here and `cap >= (1 << 31)` in `_Zstd`; harmless, but the two readers could agree. - `cxx_std` still caches the result without keying it by compiler, and a stale `-std=c++2a` left in an old `work/bake/cxx_std` would still be returned. You note this is tracked in #1872, so fine to leave.