fix(ENG-RELEASE-WINDOWS): M_PI is POSIX, not C++ — and the grep that found it undercounted both ways (#720) - #732
Merged
Merged
Conversation
…found it undercounted both ways (#720) `M_PI` is a POSIX extension. MSVC's `<cmath>` defines it only when `_USE_MATH_DEFINES` precedes the include, so `windows-msvc-cpu` and `windows-msvc-vulkan` hard-error C2065 at `ltx2.cpp` on every open PR. This replaces every `M_PI` in `src/` and `tests/` with `std::numbers::pi_v<double>`, the C++20 spelling already used at four sites here. `_USE_MATH_DEFINES` was rejected: it is order-dependent across transitive includes and re-declares the extension instead of removing the dependency. THE INTAKE'S "THREE TUs" IS WRONG IN BOTH DIRECTIONS, and measurement rather than `git grep -l` is what separates them. `ltx2_audio_vae.cpp` and `ltx2_video_vae.cpp` carry their own `#ifndef M_PI` guard and compile fine under MSVC — they are two extra spellings of pi (#687's disease), and the video one has ZERO uses, so its define is dead. Meanwhile `tests/vllm/models/test_vocoder1d.cpp` has two unguarded uses and IS a hard break: `git grep -- src include` never looked at `tests/`, and the CI log never named it because the library build stops at `ltx2.cpp` first. RED is a test that was ALREADY COMMITTED AND ALREADY FAILING. `test_check_windows_portability.py`'s `test_real_tree_uses_portable_windows_allocation_and_math` asserts no `\bM_PI\b` under `src/` or `tests/`, and named all four files at the base SHA. No checker semantics change here. That suite runs in no workflow (#646, #680), which is how the assertion went unheld while the LTX-2.5 lane landed on top of it. The MSVC condition also reproduces on Linux: `-U_GNU_SOURCE -D_ISOC99_SOURCE` removes glibc's `M_PI` from `<cmath>`, and g++ then reports `ltx2.cpp` 72:58, 557:69, 572:76 — the same line AND column as the hosted MSVC log — plus `test_vocoder1d.cpp` 106:21 and 121:21, which no MSVC log had reached yet. It reports nothing for the two VAEs, confirming their guards by measurement. VALUE IDENTITY IS ASSERTED, NOT ASSUMED, three ways. `M_PI`, `std::numbers::pi_v<double>` and the hand-rolled `3.14159265358979323846` are the same double, `400921fb54442d18`. A token-sequence comparison over all four files (41,191 tokens, both pi spellings unified) reports pure token substitution: the only deltas are the added `<numbers>` includes and the two removed defines, so no operator order, cast or operand moved. The seven LTX-2.5 and vocoder golden suites report identical counts before and after — 185 cases, 17,378 assertions, all green in both arms. That third arm is not vacuous: perturbing pi to `3.14159` builds clean and reds 60 assertions in `test_ltx2` and 5 in `test_ltx2_pipeline`. Reverting the constant to `M_PI` still BUILDS CLEAN on Linux (exit 0) while re-reding the portability gate and the MSVC probe — which is the whole shape of this bug: the Linux build is structurally blind to it. Full ctest 440/441. The one failure, `test_op_parity`, throws the identical `json.exception.type_error.302` at the base SHA and is not from this change. `ctest -N` is 441 against the 429 last recorded; this diff touches no `CMakeLists.txt` and registers no test, so the drift is main's. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
FOLLOWING_AGENTS_PROTOCOL main advanced five commits (indextts2 S2Mel/talker, MUSIC3 records) while this row was gating. Merged rather than rebased; `docs/FEATURES.md` was co-edited by both sides and auto-merged with this row's scoped line-30 amendment intact. Re-gated on the merged tree, because a clean merge is not a built merge: no `M_PI` survives in `src/`, `include/` or `tests/`; the token-equivalence proof still reports pure substitution across all four files; the portability suite is 71 cases / 1 pre-existing failure (#680); full build exit 0 with no ENOSPC; and ctest is 442/443 with only the pre-existing `test_op_parity` throw. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
This was referenced Aug 14, 2026
localai-bot
added a commit
that referenced
this pull request
Aug 14, 2026
…s with POSIX stat FOLLOWING_AGENTS_PROTOCOL `src/vllm/multimodal/video_engine.cpp` reached Windows with unguarded POSIX `stat` -- `:21` `#include <sys/stat.h>`, `:57-60` `IsDir`, `:62-65` `Exists` -- which `check-windows-portability.py:1675-1688` flags under `full_source_posix`, i.e. every scanned source and not only the platform-boundary set. It reddened `windows-msvc-cpu` and `windows-msvc-vulkan` on EVERY open pull request in the repository, across five unrelated lanes. It hid because those jobs are `skipped` on `main` pushes (#584), so a green main carried no information. Repaired at the SOURCE, never the checker: `IsDir`/`Exists` now take the `std::error_code` <filesystem> overloads through a file-local path helper, preserving `::stat`'s return-false-for-an-uninspectable-path behaviour that the THROWING overloads would have turned into a `filesystem_error` escaping a registry query. The helper adopts `fs_io.cpp:31`'s spelling byte-for-byte and deliberately NOT `minimax_h3_sharded.cpp:55`'s, which throws on malformed UTF-8 under _WIN32 -- that would reintroduce on Windows the exception this removes on POSIX. The wider divergence is #687. RED first, and the honest RED is worth recording: a missing-path test would have passed with and without the guarantee, because ENOENT does not throw. The real probe is a 300-character path component (ENAMETOOLONG). Reviewed PASS by a fresh reviewer who reproduced the premise from another lane's CI log, confirmed by sha256 that the checker was untouched, and REJECTED the implementer's probe for the one surviving mutation -- "fails to refute is not unreachability" -- then established unreachability from the library mechanism instead: `IsDir` is TU-local with one call site gated by `Exists`, and libstdc++ implements both via `f(status(p))`, so `exists()` can only return true when the stat already succeeded. Operator gate at the merged tree: WINPORT_EXIT=0, BUILD_EXIT=0, 0 errors, 0 ENOSPC, REGISTERED=444, test_video_engine 12 cases / 260 assertions exit 0. Two ctest failures both attributed by measurement, not assumption: `test_op_parity` is #737 (a null in the Music3 golden added today -- proven by moving that one golden aside: 10/10 pass, assertions 70 -> 123), and `test_openai_conformance` is a load artifact at load average 115 that passes alone, exit 0, 48 assertions. This does NOT turn the Windows lane green on its own. Two independent defects were stacked; with this one gone the build reaches `M_PI`, which MSVC does not define -- #720, fixed by #732. Closes #664. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5[1m] [claude-code]
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
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.
Closes #720.
M_PIis a POSIX extension, not standard C++. MSVC's<cmath>defines it onlywhen
_USE_MATH_DEFINESprecedes the include, sowindows-msvc-cpuandwindows-msvc-vulkanhard-error C2065 atltx2.cppon every open PR. Thisreplaces every
M_PIundersrc/andtests/withstd::numbers::pi_v<double>— the C++20 spelling this repo already uses at
mla_attention.cpp:82,deepseek_v4.cpp:602,minimax_h3_video_vae.cpp:258andvocoder1d.cpp:35._USE_MATH_DEFINESwas rejected: it is order-dependent across transitiveincludes, and it re-declares the POSIX extension rather than removing the
dependency on it. No fifth spelling of pi is introduced (cf. #687).
Spec:
.agents/specs/windows-msvc-m-pi.md.This is the second of the two stacked Windows defects; the first is #664 / PR #677.
The TU list, and why the issue title undercounts both ways
git grep -l M_PI -- src includereturns three files. Measurement disagreeswith that in both directions:
M_PIuses#defineguard?src/.../ltx2.cpptests/vllm/models/test_vocoder1d.cppsrc/.../ltx2_audio_vae.cppsrc/.../ltx2_video_vae.cppThe two VAEs carry
#ifndef M_PI / #define M_PI 3.14159265358979323846andcompile fine under MSVC — they are two extra hand-rolled spellings of pi, and
the video one's guard has zero uses. Meanwhile
test_vocoder1d.cppis a genuinehard break that
git grep -- src includenever looked at and no CI log evernamed, because the library build stops at
ltx2.cppfirst. All four are fixed.RED — an already-committed, already-failing test
No checker semantics change.
tests/scripts/test_check_windows_portability.py'stest_real_tree_uses_portable_windows_allocation_and_mathalready assertsthat nothing under
src/ortests/contains\bM_PI\b, and it was alreadyfailing at the base SHA naming all four files:
That suite runs in no workflow (#646, #680), which is how the assertion went
unheld while the LTX-2.5 lane landed on top of it. GREEN is the same 71 cases
with failures 2 → 1; the survivor is
test_real_unsupported_tier_helper_is_structurally_scoped, pre-existing andowned by #680 (captured failing on the pristine base before any edit).
The MSVC diagnostic also reproduces on Linux.
-U_GNU_SOURCE -D_ISOC99_SOURCEremoves glibc's
M_PIfrom<cmath>, recreating MSVC's header condition, andg++ then reports the same line and column as the hosted MSVC log:
It emits nothing for the two VAEs, confirming their guards by measurement rather
than by reading. This is g++/glibc, not MSVC — the hosted jobs remain the
authoritative compiler gate.
No golden moved — asserted three ways, not assumed
M_PI,std::numbers::pi_v<double>and the hand-rolled3.14159265358979323846are the same double,400921fb54442d18; the threederived quantities (
sqrt(2/pi)as float,pi/2as float and as double) arebit-equal.
(41,191 tokens, both pi spellings unified) reports no difference beyond the
added
<numbers>includes and the two removed defines — so no operatororder, cast or operand moved.
17,378 assertions, all passing.
test_vocoder1dtest_ltx2test_ltx2_vaetest_ltx2_devicetest_ltx2_loadertest_ltx2_text_encodertest_ltx2_pipelineMutation
Every mutation was restored from the index and verified by
sha256sum -c, neverby
git status. Compile status is printed beside each result, because amutation that fails to build reads as a passing test.
M_PIinltx2.cpp—M1_BUILD_EXIT=0, i.e. itstill builds clean on Linux, while the portability gate goes red and the
MSVC probe reproduces the error. That is the whole shape of this bug: the
Linux build is structurally blind to it.
3.14159—M2_BUILD_EXIT=0, and 60 assertions red intest_ltx2plus 5 intest_ltx2_pipeline. So arm 3 above is not a vacuousgreen. (
test_ltx2_vaestays green under this mutation — it does not reachthese sites, and its unchanged result is therefore evidence of nothing.)
goes red, so arm 2 is not vacuous either.
Gate
BUILD_EXIT=0,grep -cE 'No space left|BFD assertion'= 0,df -h /88%.ctest -N= 443. The 429 last recorded is main's drift: this diff touchesno
CMakeLists.txtand registers no test.ctest -j4: 442/443. The one failure,test_op_parity, throws anidentical
json.exception.type_error.302when rebuilt and run at the baseSHA — pre-existing, not from this change.
scripts/agent-preflight.sh --stagedand post-commit: all gates green atthe pre-merge head. On the merged head the only failures are
audit-live-rows/
test_audit_live_rows, which are main red: audit-live-rows reports the Music3 ACTIVE row abandoned, so preflight is red on a clean main #731 (mainred on a clean checkout fromMUSIC3's ACTIVE row) and arrived with the merge, not with this change.
Owed
Native
windows-msvc-cpuandwindows-msvc-vulkanon this head are theauthoritative
/W4 /WXvalidation. Linux cannot substitute for the MSVCcompiler gate. Note these jobs are PR-only, so they cannot go green on this
branch until #677 lands too — the two defects are stacked.
🤖 Generated with Claude Code