build: enable the unit/c test suite on Windows - #3806
Conversation
| * if someone happens to have created it on the current drive, so honour | ||
| * the environment's temp directory first and keep /tmp as the fallback. | ||
| */ | ||
| static const char *temp_dir(void) |
There was a problem hiding this comment.
This is something we need in other places as well (the unit test for INI is not running on Windows because of this). Could you add the temp dir handling and also the temp file creation to the shared project (including unit tests)?
Basically get shr_mkstemp working out of the box :)
There was a problem hiding this comment.
Dropped the temp_dir() helper entirely — with shr_mkstemp() now in shared/fs-util.h the test doesn't need any temp-directory handling at all. It creates nvme-cli-conf-test-XXXXXX relative to the CWD (meson runs tests from the build directory), which is the same pattern shared/test/test-fs-util.c and shared/test/test-table.c already use. No /tmp, no TMPDIR/TMP/TEMP probing, and no snprintf() truncation check to get wrong.
On the INI test, two separate things keep it off Windows: shared/test/meson.build gates it behind host_system != 'windows', and test_file() in test-ini.c uses char path[] = "/tmp/nvme-ini-test-XXXXXX" with plain mkstemp(). I have both fixed on a branch — un-gating ini, crypto-util, sig-util and shr-assert, plus the same relative-template shr_mkstemp() change in test-ini.c — and can send it as a follow-up series once this one lands.
| # CLI-driven tests live in ../cli/; device tests live in ../e2e/. | ||
|
|
||
| if host_system == 'linux' | ||
| if host_system == 'linux' or host_system == 'windows' |
There was a problem hiding this comment.
I would remove the gate completely
There was a problem hiding this comment.
Done — the gate is gone entirely, subdir('c') is unconditional now.
|
Glad we get the test coverage increased. BTW, please drop the Co-Authored tag. Thanks! |
| setlocale(LC_NUMERIC, "C"); | ||
| f = freopen("/dev/null", "w", stderr); | ||
| if (!f) | ||
| f = freopen(DEV_NULL, "w", stderr); |
There was a problem hiding this comment.
I think it would make sense also to move the DEV_NULL from common.h to
shared. In fact I'd like to get rid of common.h`. Too many places which do almost the same (common.h, nvme.h, utils.h)
There was a problem hiding this comment.
I've merged the common.h header into shared. There is no shr_dev_null().
There was a problem hiding this comment.
Rebased onto dwsuse's "shared: distribute common.h to common code", so common.h is out of the picture here — this is now freopen(shr_dev_null(), "w", stderr).
While I was in there I also dropped this file's hand-rolled ARRAY_SIZE in favour of <ccan/array_size/array_size.h>, the spelling used elsewhere in the tree. It needs to come from somewhere explicit once the test is built on Windows: the Windows headers provide ARRAYSIZE, not ARRAY_SIZE.
There was a problem hiding this comment.
Thanks — rebased onto that. shr_dev_null() is in shared/fs-util.h, returning "NUL" from fs-util-win.c and "/dev/null" from fs-util-linux.c, so the test just calls it and the common.h include is gone.
Re-verified after the rebase: 52 Ok / 0 Fail / 1 skip on Windows (UCRT64/MinGW, gcc 16.1.0) and 119/119 on Linux, with each of the two patches building standalone.
test-argconfig-parse redirects stderr to the bit bucket so the expected
parse errors don't clutter the test log. Two problems show up when the
test is built for Windows:
- "/dev/null" doesn't exist natively; the spelling is "NUL".
- When freopen() failed the test printed an error and carried on with
test_rc untouched, so it reported success anyway.
Use shr_dev_null(), which already resolves to "NUL" on Windows and
"/dev/null" elsewhere, and make the freopen() failure set test_rc.
Also drop the file's hand-rolled ARRAY_SIZE in favour of ccan's, the
spelling used everywhere else in the tree. Windows' own headers provide
ARRAYSIZE but not ARRAY_SIZE, so the macro has to come from somewhere
explicit once the test is built there.
Signed-off-by: Brandon Busacker <bbusacker@micron.com>
The C unit tests were gated behind host_system == 'linux', but nothing in tests/unit/c is actually Linux-specific: there are no platform conditionals in the directory at all, and both binaries build cleanly with MinGW. Drop the gate rather than widening it, so any host that can build the tests runs them. The one real portability problem was test-global-config.c hardcoding "/tmp" for its throwaway .conf files. On native Windows "/tmp" resolves to the root of the current drive, so it only exists if somebody happened to create it there; on a drive without it mkstemp() returns -1 and the test aborts. Write the files into the current directory instead, which meson sets to the build directory, using the shr_mkstemp() wrapper from shared/ -- the same relative-template pattern shared/test/test-fs-util.c already uses. That drops the dependency on a temp directory entirely. Both tests pass on Windows (UCRT64/MinGW, gcc 16.1.0) and remain green on Linux. Each was also mutation-tested to confirm it is not vacuously passing. Signed-off-by: Brandon Busacker <bbusacker@micron.com>
cf61845 to
caab667
Compare
|
Force-pushed, rebased onto current master ( What changed since the last push:
Net diff is now 3 files, +19/-9 — smaller than the version you reviewed. Verification: Windows (UCRT64/MinGW, gcc 16.1.0) 52 Ok / 0 Fail / 1 skip, Linux 119/119, and each of the two patches builds standalone. |
|
Thanks! |
3 participants
The C unit tests under
tests/unit/care gated behindhost_system == 'linux', but nothing in that directory is actuallyLinux-specific — there are no platform conditionals in it at all, and
both binaries build cleanly with MinGW.
This series fixes the two real portability problems and drops the gate.
tests/unit/c: don't silently pass when stderr can't be redirectedtest-argconfig-parseredirects stderr to the bit bucket so the expectedparse errors don't clutter the log. Two issues on Windows:
"/dev/null"doesn't exist natively; the spelling is"NUL". Fixed byusing the existing
DEV_NULLfromsrc/common.h, which already selectsper-platform, rather than adding another switch.
freopen()failed the test printed an error but lefttest_rcuntouched, so it reported success anyway. That's a latent bug on any
platform, not just Windows.
build: enable the unit/c test suite on Windowstest-global-config.chardcoded"/tmp"for its throwaway.conffiles.On native Windows
"/tmp"resolves to the root of the current drive, so itonly exists if somebody happened to create it there; on a drive without it
mkstemp()returns -1 and the test aborts. Now honoursTMPDIR/TMP/TEMPfirst with
"/tmp"as the fallback, and uses the portableshr_mkstemp()wrapper from
shared/.Testing
Built and run on Windows with MSYS2 UCRT64 / MinGW gcc 16.1.0: full build
clean,
meson testreports 51 passed / 0 failed / 1 skipped, including bothargconfig_parseandglobal-config. Linux behaviour is unchanged — theonly Linux-visible deltas are the
DEV_NULLswap and thetest_rcfix.Each test was also mutation-tested to confirm it isn't inappropriately passing.
checkpatchis clean per patch (0 errors; the one warning is itspreference against
Co-authored-by:, which is already used throughoutthis tree).