Skip to content

tests: include compat.h directly in the converted tests - #1440

Merged
JustVugg merged 1 commit into
JustVugg:devfrom
texasich:fix/compat-direct-includes
Sep 12, 2026
Merged

JustVugg merged 1 commit into
JustVugg:devfrom
texasich:fix/compat-direct-includes

Conversation

@texasich

Copy link
Copy Markdown
Contributor

Summary

Direct follow-up to #1432, as raised in review: the nine converted test sources call setenv()/unsetenv() but relied on compat.h arriving transitively (through st.h for the engine tests, through route_trace.h for test_route_trace). They now include ../compat.h directly, so a future reorder of the engine headers cannot silently drop the shims from these Windows C tests.

No functional change: each direct include sits after the file's primary include, and the COMPAT_H guard is already set by then, so preprocessing is unchanged apart from shifted diagnostic line numbers. One adjacent comment in test_route_trace.c is updated to match ("...and compat.h..."), and the two tier tests place the include where their sibling test_qwen36_tier_int8.c already does.

Validation

  • make -C c check (see notes)
  • CUDA changes were tested with make -C c cuda-test (not applicable, no CUDA changes)
  • Performance claims include hardware, commands, and repeatable measurements (not applicable, no performance claims)

Notes (isolated Linux sandbox, GCC 15.3.0):

  • All nine targets build with zero warnings, and the runnable suites pass: test_qwen36_ctx, test_qwen36_dense_batch, test_qwen36_tier_int8_decode, test_qwen36_tier_int8_engine, test_route_trace, test_stops, bench_qwen36_dense_batch.
  • test_inkling_shared_batch and bench_inkling_shared_batch keep their pre-existing results, identical to unmodified dev in this sandbox (bf16 scalar-vs-batch bit-exactness; int4-g64: 24077 values differ, worst=4.76837e-07).
  • c/tests/test_makefile_deps.py passes (2 tests).
  • Windows behavior is exercised by the UCRT64 CI job, the authoritative run for these paths.

Compatibility

  • The default CPU build remains dependency-free
  • No model files, generated binaries, or benchmark artifacts are included

Follow-up to the per-test _putenv_s cleanup: the nine converted files call
setenv()/unsetenv() directly but relied on compat.h arriving transitively
(via st.h / route_trace.h). Include it explicitly so a future reorder of
the engine headers cannot silently break these Windows C tests.

No functional change: by the time these direct includes are reached the
COMPAT_H guard is already set, so preprocessing is identical apart from
the updated diagnostic line numbers.

Signed-off-by: texasich <101962694+texasich@users.noreply.github.com>
@JustVugg
JustVugg merged commit e74d0d7 into JustVugg:dev Sep 12, 2026
27 checks passed
trigger2k20 pushed a commit to trigger2k20/colibri that referenced this pull request Sep 13, 2026
dev went red on `Windows UCRT64` / `make check` right after JustVugg#1390 landed:

    tests/test_qwen36_tier_fill_wait.c:42:5: error: implicit declaration of
    function 'setenv'; did you mean 'getenv'?

MinGW has no setenv. The tests that include an engine .c inherit compat.h
through it, but this one includes qwen36_tier.c, which does not pull it in,
so the declaration was never there. Linux never noticed because glibc has
setenv, and modern GCC turns an implicit declaration into an error rather
than a warning, so the Windows leg is where it surfaced. Same shape as
JustVugg#1440, which added the include to the six tests that already had it.

Reproduced and fixed against mingw-w64 with the Makefile's own Windows flags
plus -Werror=implicit-function-declaration, then every other gated test was
cross-compiled the same way: this file is the only one affected. The three
that still fail that sweep (test_uring, test_deepseek_v4, test_v4_ownership)
are in TEST_EXCLUDE and are not built on the Windows leg at all.

compat.h joins the rule's prerequisites too, so editing the shim relinks the
test instead of leaving a stale one.
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