Repository navigation
[M0-CORE-08] Fix Windows CI: <algorithm> missing from budget_harness.h (MSVC C2039/C3861 on std::sort) - #11
Merged
Conversation
…h (MSVC C2039/C3861 on std::sort) Histogram::stats() calls std::copy_n and std::sort in budget_harness.h, but the header included only <chrono>/<cmath>/<cstddef>/<cstdint>/<string>/ <string_view>/<vector>. GCC and Clang pull <algorithm> in transitively (via <vector> and friends), so the linux lanes and local builds pass, but MSVC standard headers do not: the windows-msvc job died with C2039 (sort: is not a member of std) / C3861 while compiling budget_harness.cpp. - Add #include <algorithm> to the header (self-contained per CPP-010; alphabetically first in the include block). - Regenerate the checked-in laige-api.json with laige-api-scanner: the manifest records per-symbol line numbers, and the added include shifts every budget_harness.h entry by one line (58 lines; signatures and summaries unchanged). Without it the api-real-tree drift check fails in every P0 job, including windows-msvc, right after the build fix. Verified: full build + all 22 CTest entries pass in build; include-lint passes (17 source files scanned, 78 system headers, 1 vendored dependency); audited every CI-built TU (src/tests/tools) for std symbols used without a direct include - no other gaps.
…r /WX) After the <algorithm> include fix, the windows-msvc build proceeds past laige-core and fails on the remaining unguarded C4996 deprecations (fatal under the engine /WX policy, NFR-8.10): - tools/bench/laige-bench.cpp: getenv (LAIGE_BENCH_MACHINE, LAIGE_BUDGETS_PATH) and fopen (report file, mode "a") - tests/laige-core/budget_harness_tests.cpp: getenv (LAIGE_BUDGETS_PATH) Same CPP-009 platform-boundary pattern the repo already uses in logging.cpp / budget_harness.cpp / logging_tests.cpp: #if defined(_MSC_VER) switches to the CRT replacements (getenv_s, _fsopen with _SH_DENYNO) with the same lookup/open semantics; other compilers keep the standard calls. - envValue() returns the value as std::string (empty when unset); a value beyond the named kEnvValueMaxBytes (4096) is treated as unset, and the documented fallback applies (CORE-005 named bound, CORE-008 loud default instead of a truncated value). - BudgetsFilePath() now returns std::string (single call site, passed into loadBudgets(std::string_view) within one full expression - the temporary outlives the call). Audited every CI-built TU (src/tests/tools) for the C4996-prone CRT calls (getenv, fopen, tmpnam, tmpfile, strcpy, strcat, sprintf, gets, getwd, mkstemps, scandir): these were the only unguarded sites. Verified: full build + all 22 CTest entries pass; laige-bench smoke test with LAIGE_BENCH_MACHINE/LAIGE_BUDGETS_PATH set and --report append writes the report file correctly.
…under multi-config layout
The six api-* CTest tests drive laige-api-scanner from generated
cmake -P check scripts with a configure-time path
(${CMAKE_BINARY_DIR}/bin/laige-api-scanner). That layout only matches
single-config generators: under the Windows VS multi-config generator
the executable lives in bin/<Config>/laige-api-scanner.exe, so
execute_process dies with "no such file or directory" (and
api-fixture-scan additionally fails its file(READ) on the manifest that
the never-running scanner was supposed to write).
Fix (verified by experiment on CMake 4.4.3, Ninja Multi-Config and
Unix Makefiles):
- The scanner path is carried in each test's ENVIRONMENT property as
$<TARGET_FILE:laige-api-scanner>; CMake expands it per configuration
in the generated CTestTestfile.cmake (Debug -> bin/Debug/..., Release
-> bin/Release/..., single-config -> bin/...).
- The check script reads it at test time via $ENV{LAIGE_API_SCANNER}
(configure_file copies the reference verbatim; a configure-time
expansion would bake in an empty/wrong value) and fails loudly when
unset (CORE-008).
- kEnvValueMaxBytes moved inside the MSVC #if branches in laige-bench.cpp
and budget_harness_tests.cpp: outside them it is an unused
namespace-scope constant, which -Wunused-const-variable makes fatal
under -Werror on Clang (NFR-8.10, caught in the ASan tree).
Verified: all 22 CTest entries pass in build, build-asan, build-tsan,
build-clang, build-shared, and build-clang-shared; the multi-config
experiment confirmed per-config resolution and single-config parity.
…ormalize CRLF to LF in --check) After the scanner-path fix, the Windows windows-msvc job's ctest left one failure: api-check-fresh reported "stale (0 difference(s))". Root cause: the canonical manifest form is LF (the serializer emits LF; .gitattributes pins eol=lf on every checkout). But CMake's file(WRITE) opens a text-mode stream on Windows (cmFileCommand: ofstream without ios::binary), so the CMake-generated test fixtures (expected-fixture-api.json, ...) land on disk with CRLF endings. The api-fixture-scan test survived because CMake's file(READ) normalizes CRLF->LF on read; the scanner's --check reads the file in binary mode, so its byte compare saw CRLF vs LF, while the symbol-level diff was 0 (CRLF is JSON whitespace) - hence "stale (0 difference(s))". Fix: in runCheck, fold CRLF pairs to LF in the read file before the byte compare (a lone \r is NOT folded, so a genuinely corrupted file still fails, CORE-008). The regeneration itself is unchanged and stays byte-identical LF (NFR-13.4); contract text (header comment, --help) updated in the same change (CORE-006). Verified locally (CMake 4.4.3): LF copy -> exit 0, CRLF copy -> exit 0, lone-CR copy -> exit 1 "stale", stale fixture -> exit 1 with the 2 symbol diffs, missing file -> exit 2; all 22 CTest entries pass in build, build-asan, build-tsan, build-clang, build-shared, and build-clang-shared.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The windows-msvc job (
cmake --build build --config Debug -j) failed with:budget_harness.h(242,10): error C2039: 'sort': is not a member of 'std'That was only the first of four distinct Windows-only defects; each was
unmasked by fixing the previous one (the build halts at the first failure).
All four are fixed here, verified on real
windows-2022runs.Fixes (one commit each)
fe0f5a5— missing<algorithm>include (the reported error).Histogram::stats()callsstd::copy_n/std::sort, butbudget_harness.hincluded only<chrono>/<cmath>/<cstddef>/<cstdint>/ <string>/<string_view>/<vector>. GCC/Clang pull<algorithm>intransitively (linux lanes + local builds pass); MSVC standard headers do
not. Added
#include <algorithm>(self-contained per CPP-010) andregenerated the checked-in
laige-api.json(the manifest recordsper-symbol line numbers; 58 entries shift by one line, signatures
unchanged — the api-real-tree drift check would otherwise fail).
6b3d7fe— unguardedgetenv/fopen(C4996, fatal under /WX).laige-bench.cpp(LAIGE_BENCH_MACHINE,LAIGE_BUDGETS_PATH, reportfile) and
budget_harness_tests.cpp(LAIGE_BUDGETS_PATH) used plainstd::getenv/std::fopen. Fixed with the repo's established CPP-009platform-boundary pattern (
#if defined(_MSC_VER)→getenv_s/_fsopen(_SH_DENYNO), same semantics as on other compilers).kEnvValueMaxByteslives inside the MSVC branches — outside them it is anunused constant, fatal under
-Wunused-const-variableon Clang (-Werror).89eae70— api CTest tests cannot find the scanner (multi-config layout).The six
api-*CTest tests drove the scanner from generatedcmake -Pscripts with a configure-time path
${CMAKE_BINARY_DIR}/bin/laige-api-scanner.Under the VS multi-config generator the executable lives in
bin/<Config>/laige-api-scanner.exe, soexecute_processdied with"no such file or directory". Fixed by carrying the scanner path in each
test's
ENVIRONMENTproperty as$<TARGET_FILE:laige-api-scanner>—CMake expands it per configuration in the generated CTestTestfile.cmake —
and reading it at test time via
$ENV{LAIGE_API_SCANNER}(configure_filecopies the reference verbatim; a configure-time expansion bakes in a wrong
value). Verified by experiment on CMake 4.4.3 (Ninja Multi-Config +
Unix Makefiles).
b5bbacb—api-check-freshstale on CRLF fixture (0 differences).The canonical manifest form is LF (serializer emits LF;
.gitattributespins
eol=lf). But CMake'sfile(WRITE)uses a text-mode stream onWindows, so the CMake-generated test fixtures land on disk with CRLF
endings. The scanner's
--checkreads in binary mode → byte compare seesCRLF vs LF while the symbol diff is 0 (CRLF is JSON whitespace) →
"stale (0 difference(s))".
runChecknow folds CRLF pairs to LF beforethe byte compare (a lone
\ris NOT folded, so a genuinely corrupted filestill fails, CORE-008); regeneration stays byte-identical LF (NFR-13.4);
contract text updated in the same change (CORE-006).
Verification
on the final commit (run of
b5bbacb, withci:windowslabel).(
ci:macoslabel run).fe0f5a5; for the final commit all sixcanonical local trees pass 22/22 (build, build-asan, build-tsan,
build-clang, build-shared, build-clang-shared).
--checkcase matrix verified locally: LF → 0, CRLF → 0,lone-CR → 1 (stale), stale fixture → 1 with the 2 symbol diffs, missing
file → 2.
laige-benchsmoke-tested withLAIGE_BENCH_MACHINE/LAIGE_BUDGETS_PATHset and
--reportappend (report file written correctly).Note: this branch has two empty trigger commits (the PR workflow is
label-gated:
ci:windows/ci:macosselect which OS lanes run per PR); asquash merge collapses them.