Skip to content

fix(test): make test_stops build on Windows (mkdtemp compat shim)#352

Merged
JustVugg merged 1 commit into
JustVugg:devfrom
woolcoxm:fix/test-stops-mkdtemp-dev
Jul 19, 2026
Merged

fix(test): make test_stops build on Windows (mkdtemp compat shim)#352
JustVugg merged 1 commit into
JustVugg:devfrom
woolcoxm:fix/test-stops-mkdtemp-dev

Conversation

@woolcoxm

Copy link
Copy Markdown
Contributor

What

make test halted on the Windows job: tests/test_stops.c uses POSIX mkdtemp(), which MinGW-w64 doesn't declare, so the test failed to compile on Windows (and only there).

tests/test_stops.c:73:9: error: implicit declaration of function 'mkdtemp';
   did you mean 'mktemp'? [-Wimplicit-function-declaration]
make: *** [Makefile:318: tests/test_stops.exe] Error 1

This blocked the entire C-test phase on Windows. (test_uring is correctly gated to Linux via ifneq (,$(LINUX)), so test_stops was the only Windows blocker.)

Fix

Added a compat_mkdtemp shim to c/compat.h, following the file's stated convention β€” "every platform difference lives here; the .c files stay clean." POSIX mkdtemp takes a char[] template ending in XXXXXX, replaces the X's randomly, mkdirs 0700, and returns the pointer (or NULL). On Windows:

static inline char *compat_mkdtemp(char *tmpl){
    if(!tmpl) return NULL;
    if(!_mktemp(tmpl)) return NULL;       /* fills the trailing X's in place */
    if(_mkdir(tmpl) != 0) return NULL;    /* EEXIST is impossible post-_mktemp */
    return tmpl;
}
#define mkdtemp(tmpl) compat_mkdtemp(tmpl)

_mktemp (CRT, <io.h>) has the same XXXXXX contract; _mkdir needed <direct.h>, added. On Linux compat.h is a complete no-op β€” POSIX mkdtemp is untouched.

Why a shim and not a test-local fix

The earlier fix in test_stops.c (commit on this file) already moved off /tmp/... to a CWD-relative path to fix the Windows job, but mkdtemp itself was still POSIX-only. A compat.h shim is the project's established pattern for exactly this (see getline, setenv, posix_fadvise, pread, rename...) and keeps the test portable without #ifdef _WIN32 inside the test source.

Verification

On MinGW:

make tests/test_stops.exe   # builds clean (the mkdtemp error is gone)
./tests/test_stops.exe      # all 5 sub-cases pass

The two unrelated -Wall warnings that still show in this branch's build (compat.h:241 pragma, glm.c:1210 g_numa_nodes) are addressed by #350 β€” this PR is based on origin/dev and is independent of it.

test_stops.c uses POSIX mkdtemp() to make a scratch dir in the CWD, but
MinGW-w64 does not declare mkdtemp, so the test failed to compile on the
Windows job - and only there:

  tests/test_stops.c:73:9: error: implicit declaration of function 'mkdtemp';
     did you mean 'mktemp'? [-Wimplicit-function-declaration]
  make: *** [Makefile:318: tests/test_stops.exe] Error 1

That halted `make test` at the C-test stage on Windows (test_uring is
correctly Linux-only, so test_stops was the only blocker).

Added a compat_mkdtemp shim to compat.h, following the file's existing
convention (every platform difference lives there; the .c stays clean):
_mktemp fills the trailing X's in place (same contract as mkdtemp), then
_mkdir creates the directory. Also added <direct.h> for _mkdir. On Linux
compat.h is a complete no-op, so POSIX mkdtemp is untouched there.

Verified: test_stops builds clean on MinGW and all 5 sub-cases pass:
  tokenizer special-flag parsing, config/generation_config eos union,
  no-generation_config fallback, both-configs-mutilated tokenizer sweep,
  and the T=NULL validation path.
JustVugg pushed a commit that referenced this pull request Jul 17, 2026
… not O(V log V) qsort (#335)

dist_build() sorted the entire 151936-entry vocab by probability (qsort) on every
sampled token whenever 0 < g_nuc < 1 β€” the serve default β€” and again per draft
position under rejection sampling. Measured cost: 5.6-8.0 ms/call; the actual work
is finding the few-hundred-token head whose cumulative mass reaches g_nuc.

Replace the full qsort + linear scan with a max-heap partial select:
  - Floyd heapify g_pidx over V by descending g_pbuf prob  (O(V), cache-friendly)
  - pop winners to the array's high end until cum >= g_nuc  (k * O(log V))
  - the remaining heap prefix IS the tail -> zero it, renormalize the head

Winners land in g_pidx[out..V-1] in descending order, so s2 accumulates in the
same order as before -> head is unchanged on tie-free shapes (ties were already
unspecified under the unstable qsort). All four dist_build/dist_sample contract
properties hold: g_pbuf stays id-indexed, g_pidx stays internal, the tail is
fully zeroed, the head renormalizes to 1.

No API change, no caller change, no new globals.

c/tests/test_topp.c (new): drives the REAL dist_build via the include-glm.c
pattern against an independent double-precision reimplementation of the OLD
algorithm. 123 cases: 6 sizes (1..1519) x 5 shapes (uniform/peaked/geometric/
plateau/sharptail) x 4 nuc values, plus the g_nuc>=1 guard-off paths and V=1.
Tie-free shapes compare head values within 1e-6 rel (float vs double renorm
noise); tie shapes compare value-multisets. No scratch files -> builds clean on
Windows MinGW without the unmerged mkdtemp shim (#352).
@JustVugg
JustVugg merged commit d94c681 into JustVugg:dev Jul 19, 2026
8 checks passed
@JustVugg JustVugg mentioned this pull request Jul 19, 2026
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