Repository navigation
fix(windows): build CSO with /wd5105 and surface probe diagnostics - #3015
Closed
AlinValentin7 wants to merge 1 commit into
Closed
AlinValentin7 wants to merge 1 commit into
AlinValentin7 wants to merge 1 commit into
Conversation
The CSO Windows build aborted inside a system header: warning C5105 is raised
by the Windows SDK's own winbase.h (observed on SDK 10.0.19041.0) and /W4 /WX
promoted it to error C2220. A trivial `int main(void){return 0;}` compiles fine
with the same flags, which is why the -CheckOnly probe passed while the real
sources did not. Disable that one SDK-internal diagnostic and keep /WX for
warnings the project can act on.
Verified on Windows 11 with VS 2022 Build Tools + Windows SDK 10.0.19041.0:
`./setup --host codex --prefix` now produces bin/gstack-cso-launcher.exe,
bin/gstack-cso-core.exe and .gstack-cso-generation, and finishes with
"gstack ready (codex)." instead of "CSO unavailable".
Separately, the toolchain probe discarded all of its output (>/dev/null 2>&1)
and mapped every failure to `windows-msvc-toolchain`, whose prerequisite text
always says to install Visual Studio 2022 Build Tools. That advice is wrong when
the toolchain is already present and the failure came from a compile step: the
same -CheckOnly probe exits 0 here under both powershell.exe 5.1 and pwsh 7.
Forward the probe's diagnostics to stderr so the reason code is actionable.
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Contributor
|
Thank you, @AlinValentin7. Your fix is folded into v1.91.19.0 (#3025), which is now merged, and you're credited as a co-author on the release commit and in the CHANGELOG. Closing this PR because the change is on main now. |
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.
PR: fix two Windows build/diagnostic bugs in the CSO toolchain
Branch:
fix/windows-cso-build-and-diagnostics(fromf30b7b7/v1.91.13.0)Scope: 2 files, 16 insertions, 4 deletions. No behaviour change on macOS/Linux paths.
Summary
scripts/build-cso-windows.ps1— the CSO Windows build fails inside a systemheader because
warning C5105from the Windows SDK's ownwinbase.hispromoted to
error C2220by/W4 /WX. Added/wd5105to bothclcalls.setup— the CSO toolchain probe discards all output(
>/dev/null 2>&1) and maps every failure towindows-msvc-toolchain, whosemessage tells the user to "install Visual Studio 2022 Build Tools with
Desktop development with C++". That advice is wrong whenever the toolchain is
already present and the failure came from a compile step. The probe's own
diagnostics are now forwarded to stderr.
Side effect worth noting: the same compile failure also broke
test/cso-windows-launcher.test.ts, whose fixture compileslauncher-windows.c.Fixing the build takes that suite from 46 to 64 passing tests (details below).
1.
build-cso-windows.ps1:/wd5105Repro
Windows 11 · VS 2022 Build Tools with
Microsoft.VisualStudio.Component.VC.Tools.x86.x64·Windows SDK 10.0.19041.0 only · Git Bash ·
./setup --host codex --prefixThe diagnostic is raised in a header the project does not own, for a construct the
project cannot change, and
/WXturns it fatal. A trivialint main(void){return 0;}compiles cleanly with the same flags, which is why the-CheckOnlyprobe passes while the real sources do not.Fix
/wd5105on bothclinvocations (probe and build)./WXis kept for everywarning the project can act on; only this one SDK-internal diagnostic is
disabled. Chosen over removing
/WX(which would weaken real checks) and overrequiring a newer Windows SDK (which raises the install bar).
Verification
vswherealready resolvedC:\Program Files (x86)\Microsoft Visual Studio\2022\BuildToolswithVC.Tools.x86.x64, so this was never a missing-toolchain problem. Before andafter on the same machine, same SDK, same MSVC:
Native CSO Windows helper compilation failed (2)bin/gstack-cso-launcher.exebin/gstack-cso-core.exebin/.gstack-cso-generation(+.lock)./setuptailCSO unavailable: its native helper was not builtgstack ready (codex).No source was modified other than the two
clflag lists.2.
setup: stop hiding the CSO probe's diagnosticsWhy this matters
windows-msvc-toolchainis produced by any probe failure, and itsprerequisite text (
setupline 3340) always says "install Visual Studio 2022Build Tools with Desktop development with C++". On this machine that advice was
a dead end: the toolchain was present and the identical
-CheckOnlyprobe exits0 under both
powershell.exe5.1 andpwsh7 when its output is visible.With the output discarded there is no way to tell "toolchain missing" from
"compile step failed", so the user follows the wrong instruction.
Forwarding stderr costs one line and makes the reason code actionable.
3. Test results (before vs after, same machine)
Targeted at the two changed files. Commands run fresh for this PR.
f30b7b7(before)The before-state failure is:
The fixture compiles
launcher-windows.cthrough the samebuild-cso-windows.ps1, so it hit the identical C5105 abort and 18 tests neverexecuted. With
/wd5105the fixture builds and all 67 run.The remaining failure is not introduced by this change, but it is newly
visible because the suite can now reach it. It lives in
CSO native Windows startupand varies between 1 and 2 names across runs (flaky):Both assert runtime behaviour of
launcher-windows.candlib/cso/witness.ts,which this diff does not touch. Reported rather than silently left out of the PR.
Separately,
test/cso-witness.test.tsfails identically before and after(6 pass / 5 fail at both
f30b7b7and this branch), all withCsoError: UNSAFE_PATH "Assertion witness working directory must be private and owned"fromlib/cso/witness.ts:657— a POSIX uid/mode check that cannot holdon NTFS. Pre-existing and Windows-specific, unrelated to this diff.
4. Related, not included here:
--prefixdoes not makenamespec-conformantRaising it as a design question rather than patching it, since it touches
hosts/*andscripts/external-skill-names.ts.The Agent Skills spec requires
nametomatch the parent directory name. After
./setup --host codex --prefix, thedirectories are
gstack-qa/,gstack-autoplan/… but the emitted frontmatterstill carries the upstream short name:
Across a mixed install this is 98 non-conformant skills out of 467. In source the
two agree (
qa/withname: qa), so the mismatch is introduced only by theinstall-time prefixing.
bin/gstack-relinkalready performs exactly the needed rewrite and documents itas "name: line rewritten to NAME, which is how alias and prefixed copies differ" —
but it targets
SKILLS_DIR=$(dirname INSTALL_DIR), not the host's own skill root,and a failing
./setuptail aborts before it runs. Re-running./setupthenresets the patch: the source tree goes back to
name: qaand the generatedcopies follow.
So a
--prefixinstall needs an out-of-tree repair step after every upgrade. Theclean fix is for generation to emit the prefixed name whenever
SKILL_PREFIXison —
externalSkillName()already computes it (gstack-${base}, with anidempotence guard for names already prefixed), so it looks like a question of
where the frontmatter
nameis taken from ingen-skill-docs.tsversushosts/<host>.tsfrontmatter.keepFields.Happy to implement it if the maintainers agree that
--prefixshould implyspec-conformant
namevalues on every host.Checklist
/WXretained for project-relevant warnings./setup --host codex --prefix --no-teamcompletes;/csoartifacts producedbun testrun before and after on the 3 files covering the change: 46 → 64 pass, failure count unchanged at 1test/cso-witness.test.tsconfirmed failing identically at the base commitbun testnot run (the two remaining failures are pre-existing / flaky on Windows and are itemised above)