Skip to content

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

Open
woolcoxm wants to merge 1 commit into
JustVugg:devfrom
woolcoxm:fix/test-stops-mkdtemp-dev
Open

fix(test): make test_stops build on Windows (mkdtemp compat shim)#352
woolcoxm wants to merge 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.
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.

1 participant