Skip to content

Build cleanly on Windows/Linux/macOS; replace vendored ogg/vorbis/flac with single-header decoders - #561

Merged
bibendovsky merged 17 commits into
bibendovsky:wipfrom
AdamCoulterOz:wip-crossplatform-build
Jul 28, 2026
Merged

Build cleanly on Windows/Linux/macOS; replace vendored ogg/vorbis/flac with single-header decoders#561
bibendovsky merged 17 commits into
bibendovsky:wipfrom
AdamCoulterOz:wip-crossplatform-build

Conversation

@AdamCoulterOz

@AdamCoulterOz AdamCoulterOz commented Jul 27, 2026

Copy link
Copy Markdown

Summary

This makes the wip branch build cleanly on Windows (MSVC), Linux (GCC) and
macOS (Clang) from a single cmake -DBSTONE_SDL_BUNDLED=ON configure, adds CI that
proves it, and gets the existing test suite running again. Along the way it replaces the
vendored Xiph audio libraries with two zero-configuration single-header decoders.

All commits are small and self-contained. CI is green on all six jobs (three platforms ×
Debug/Release), and the unit tests pass in every one.

What's here

Toolchain / build-portability fixes

  • Add missing <algorithm> includes — modern libc++/libstdc++ no longer pull these in
    transitively, so std::max / std::copy_n and friends failed to compile.
  • Guard bstone_win32_utility.{cpp,h} with #ifdef _WIN32 so the Win32-only UTF helpers
    are not compiled on other platforms, matching its three already-guarded siblings. The
    file self-guards rather than the build carving out a Windows-only source list.
  • Use if(MSVC) for the Windows manifest source instead of a $<CXX_COMPILER_ID>
    generator expression, which is not valid in a source-file list on the VS generator.

Fix: sys::File reported closed-file operations by asserting

sys::File is a return-code primitive — FileStream calls it and turns a negative/false
result into an exception, and its own tests require that operating on a closed file fails
that way. Every method instead asserted is_open() and then passed the null handle to SDL
regardless. So a debug build aborted rather than failing, and a release build depended on
SDL's own null-handle parameter checks to produce the right answer.

Each operation now returns its documented failure value when the file is not open.
Likewise an unknown FileMode or FileOrigin is a rejected request the return value
already expresses, not a programming error, so those two asserts are gone as well; genuine
preconditions (non-null buffer, non-negative size) keep theirs.

This was aborting the test suite partway through — 238 of 430 tests ran before the process
died on SIGABRT
. The whole suite passes now.

CI

  • Add .github/workflows/build.yml: a full cross-product of the three platforms and
    Debug/Release, building every target (game, tools, bundled libraries, tests) and
    running the unit tests in each configuration.
  • Both configurations are run on purpose: a Release build defines NDEBUG, which compiles
    BSTONE_ASSERT away, so only the Debug run exercises the assertion-guarded failure paths
    — which is exactly how the sys::File problem above went unnoticed.
  • Install libXtst dev headers on Linux (the bundled SDL3 X11 backend treats XTEST as a
    hard dependency).

Audio: replace vendored ogg/vorbis/flac with stb_vorbis + dr_flac

The vendored Xiph libraries (libogg, libvorbis, libFLAC — 35 C source files) carry
autotools/CMake-era configuration that has to be detected correctly per platform
(HAVE_ALLOCA_H, SIZE_MAX / <stdint.h>, ogg's ogg_int*_t typedefs, lround).
Getting those right across GCC/MSVC/Clang was most of the porting friction.

They are replaced with two self-contained public-domain single-header decoders that need no
build configuration and compile identically everywhere:

  • stb_vorbis.c (nothings/stb @ 1ee679ca2ef7) → bstone::lib::stb_vorbis
  • dr_flac.h (mackron/dr_libs @ e1f125c1e03b) → bstone::lib::dr_flac

Each is vendored at a pinned commit and built as an isolated C static library, so
third-party code never sees bstone's stricter C++ flags. The Vorbis and FLAC
AudioDecoder implementations are rewritten onto the new APIs while preserving the
existing interface, factory functions and callers exactly
— same whole-stream-to-memory
decode, interleaved float output, nearest-neighbour resampler, 1–2 channel /
8000–96000 Hz validation, and rewind. dr_flac's normalised f32 output additionally
removes the per-bit-depth sample converters and the libFLAC callback machinery.

One small correctness improvement: rewind() now also resets the resampler's phase
accumulator, so a looped, resampled track restarts cleanly.

Repo hygiene

  • Ignore build-*/ output directories and data/, so out-of-source build trees and the
    copyrighted game assets used for local testing cannot be committed by accident.

Testing

  • CI green on all six jobs: Linux/Windows/macOS × Debug/Release.
  • Unit tests run in every job — 380 tests on Linux/macOS, 402 on Windows (the extra 22 are
    the _WIN32-gated registry tests), 0 failures.
  • Built and ran the game locally on macOS.
  • The decoder rewrites keep the AudioDecoder contract identical; validation and
    resampling logic are carried over unchanged.

I'm happy to split this if you'd prefer to review the pieces independently — the build/CI
stabilisation and the sys::File fix stand on their own, and the decoder swap is the
opinionated part.


🤖 Generated with Claude Code

AdamCoulterOz and others added 9 commits July 27, 2026 05:15
bstone_win32_utility.{h,cpp} include <windows.h>/use Win32 APIs unconditionally,
unlike bstone_win32_advapi32_symbols/os_version/registry_key which already wrap
themselves in `#ifdef _WIN32`. That made non-Windows builds fail. Add the same
internal guard (plus WIN32_LEAN_AND_MEAN/NOMINMAX) so it compiles to nothing off
Windows; its callers already guard their win32:: use.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
These sources use std::max/min/copy/copy_n/copy_backward/etc. but relied on a
transitive include that newer libc++/libstdc++ no longer provide, breaking the
build on current toolchains. Include <algorithm> explicitly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GitHub Actions matrix that configures and builds the bundled-SDL3 target on
ubuntu-latest (GCC), windows-latest (MSVC) and macos-latest (Clang), so the build
is verified green on all three toolchains on every push/PR.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The MSVC application manifest was added to the resource source list via
$<$<CXX_COMPILER_ID:MSVC>:...bstone_win32_msvc.manifest>. The Visual Studio
generator rejects a $<CXX_COMPILER_ID> genex in that context ("$<CXX_COMPILER_ID>
may only be used with binary targets"), failing configure on Windows/MSVC. Add the
manifest with a plain if(MSVC) block instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SDL3's CheckX11 treats a missing XTEST dependency as fatal, so the Linux configure
failed. Add libxtst-dev to the apt install list.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The vendored Xiph libraries (libogg, libvorbis, libFLAC) carry 35 C source
files plus autotools/CMake-era configuration that had to be coaxed into
building on each platform (HAVE_ALLOCA_H, SIZE_MAX/stdint, ogg int typedefs,
lround). Replace them with two self-contained public-domain single-header
decoders that need no build configuration and compile identically on
Windows/Linux/macOS:

  - stb_vorbis.c  (nothings/stb @ 1ee679ca2ef7) -> bstone::lib::stb_vorbis
  - dr_flac.h     (mackron/dr_libs @ e1f125c1e03b) -> bstone::lib::dr_flac

Each is built as an isolated C static library so its code never sees bstone's
stricter C++ flags. The Vorbis and FLAC AudioDecoder implementations are
rewritten onto the new APIs while preserving the existing interface exactly:
read the whole stream into memory, interleaved float output, the same
nearest-neighbour resampler, 1-2 channel / 8000-96000 Hz validation, and
rewind. dr_flac's normalised f32 output removes the per-bit-depth sample
converters and the libFLAC callback machinery entirely.

rewind() now also resets the resampler's phase accumulator so a looped,
resampled track restarts cleanly (the previous decoders left it stale).

Public headers, factory functions, and callers are unchanged; this is
internal to the two decoders plus the build wiring.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The build/ pattern misses out-of-source dirs like build-host/ and build-sdl3/,
and the copyrighted game data (*.BS6 etc.) that gets dropped into data/ for
local testing must never be committed. Ignore both.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sys::File is a return-code primitive: FileStream calls it and converts a
negative/false result into an exception, and its tests require that operating
on a closed file fails that way. Every method instead asserted is_open() and
then passed the null handle to SDL regardless, so a debug build aborted rather
than failing, and a release build depended on SDL's own null-handle parameter
checks to produce the right answer.

Guard each operation and return its documented failure value when the file is
not open. Likewise, an unknown FileMode or FileOrigin is a rejected request
that the return value already expresses, not a programming error, so drop
those two asserts as well. Genuine preconditions (non-null buffer,
non-negative size) keep their asserts.

This aborted the test suite partway through: 238 of 430 tests ran before the
process died on SIGABRT. All 380 non-Windows tests now pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The workflow only built the `bstone` target in Release, so it never compiled
the tools or the test suite and never ran a single test. That is how an
assertion that aborted the test suite partway through reached the branch
unnoticed.

Drop the --target filter so the build covers everything (game, tools, bundled
libraries, tests), enable BSTONE_TESTS, and run the suite. The matrix is now a
full cross-product of the three platforms and Debug/Release: Release defines
NDEBUG, which compiles BSTONE_ASSERT away, so only the Debug run exercises the
assertion-guarded failure paths.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment thread src/bstone/src/bstone_sys_file_sdl.cpp
AdamCoulterOz and others added 3 commits July 28, 2026 14:28
Review feedback on the closed-stream checks: SDL already handles this.
Verified in the bundled source - every SDL_*IO entry point validates its
context in the default build, and the wrapper already translates those
errors into failure returns, so the runtime checks added earlier were
redundant. The original BSTONE_ASSERT(is_open()) calls do not come back
either: the FileStream suite deliberately operates on closed streams
(the "Closed." cases), so the asserts abort any debug run now that the
debug suite is part of CI.

Keep returning failure for FileMode::none, which those same tests reach
through a default-constructed stream.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every job warns that checkout v4 targets the deprecated Node.js 20
runtime; v5 is the same action on Node.js 24.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The FileStream tests leave test.data behind in the working directory.
Delete it once the run is over, and ignore it for anyone who has
already run the tests from a checkout.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread src/bstone/src/bstone_sys_file_sdl.cpp Outdated
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@bibendovsky

Copy link
Copy Markdown
Owner

There is a common problem with modified audio decoders.
Reading the whole audio file into memory from a file is unacceptable.
The audio caching (whole or partial) already done in appropriate audio mixer.

AdamCoulterOz and others added 3 commits July 28, 2026 19:11
read_exactly is the interface's only out-of-line member, which makes
the whole VFS implementation a link dependency of anything that
implements the interface. Move it into the header.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both decoders read the entire file into memory before decoding, while
the mixers already decide what gets cached. Decode from the stream
instead: dr_flac pulls through its callbacks, with seeking emulated
over the VFS stream's rewind-and-read contract, and stb_vorbis works
in push mode over a bounded window that grows only if an Ogg page
outsizes it. Rewinding reopens the push-mode stream since it cannot
seek.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Decode an encoder-generated sine fixture through a fake VFS stream:
full decode, rewind, resampling, and reads that arrive a few bytes at
a time - the case that proves nothing preloads the whole file.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@AdamCoulterOz

Copy link
Copy Markdown
Author

Reworked both decoders to stream (be5efea..62f1205): dr_flac now pulls through read/seek/tell callbacks, with seeking emulated over the VFS stream's rewind-and-read contract, and stb_vorbis runs in push mode over a bounded window (32 KiB, growing only if an Ogg page outsizes it); rewind reopens the push stream since it cannot seek. Nothing reads the file up front any more — WAV already streamed. Added decoder tests with encoder-generated fixtures, including a reads-arrive-seven-bytes-at-a-time case to prove it.

🤖 Addressed by Claude Code

@bibendovsky

Copy link
Copy Markdown
Owner

One last thing.
The curly braces for controlled one-line statement was ommited.

Before:

for (int i = 0; i < 10; ++i)
{
    if (cond)
        do_this();
}
if (cond2)
    do_that();
else
    return;

After:

for (int i = 0; i < 10; ++i)
{
    if (cond)
    {
        do_this();
    }
}
if (cond2)
{
    do_that();
}
else
{
    return;
}

@bibendovsky bibendovsky added this to the v1.4.0 milestone Jul 28, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@AdamCoulterOz

Copy link
Copy Markdown
Author

Braced it (5dc71d8) — a sweep of the PR's added lines found exactly one offender, the break in the read_exactly that moved into bstone_vfs.h; the other brace-less bodies the sweep surfaced are all inside the vendored dr_flac.h, which I left in its upstream style.

🤖 Addressed by Claude Code

@bibendovsky
bibendovsky merged commit ca9ef72 into bibendovsky:wip Jul 28, 2026
6 checks passed
@bibendovsky

Copy link
Copy Markdown
Owner

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants