Skip to content

[build] Fix Windows build, run Windows test CI job in release configuration#6452

Merged
fhanau merged 1 commit intomainfrom
felix/032726-windows-ci
Mar 28, 2026
Merged

[build] Fix Windows build, run Windows test CI job in release configuration#6452
fhanau merged 1 commit intomainfrom
felix/032726-windows-ci

Conversation

@fhanau
Copy link
Copy Markdown
Contributor

@fhanau fhanau commented Mar 28, 2026

This should avoid Windows release CI failures from now on.
Since I don't have a Windows system available and only found this workaround through many rounds of manually bisecting changes, I don't have a good understanding of which commit introduced these issues or what's causing the test failures that happen when worker-interface gets optimized.
It turns out the capnp/rpc.c++ workaround is no longer needed with clang-cl 20, so we can drop it.

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm Bonk, and I've done a quick review of your PR.

This PR switches Windows CI from the windows_no_dbg configuration to the release_windows configuration, and adds -fno-inline workarounds for two files to work around test failures caused by optimized builds.

Issues (ranked by severity):

  1. [MEDIUM] The -fno-inline workaround is merged without understanding the root cause. A TODO: Why does this cause test failures? in production build config is concerning — if the test failures are caused by undefined behavior that only surfaces with inlining, disabling inlining masks the real bug rather than fixing it. The comment should at least describe which tests fail and how they fail, to aid future investigation.
  2. [LOW] The commented-out old config line adds noise. If the intent is to switch permanently, remove it.
  3. [LOW] The per_file_copt regex patterns are broader than necessary — "src/workerd/server/server" also matches server-test.c++ and server.h.

These comments were generated by an AI code review assistant and may not be perfect.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 28, 2026

I've posted a review on PR #6452 with 4 inline comments covering 3 findings:

  1. [MEDIUM] The -fno-inline workaround is shipped with a "TODO: Why does this cause test failures?" comment — this masks a potentially real bug without root-cause understanding. Recommended documenting which tests fail and how.

  2. [LOW] The commented-out old windows_no_dbg config line should be removed rather than kept as dead code.

  3. [LOW] The per_file_copt regex patterns are overly broad, matching test files and headers in addition to the intended .c++ files. Suggested narrowing with \.c\+\+ suffix patterns.

github run

This should avoid Windows release CI failures in the future
@fhanau fhanau force-pushed the felix/032726-windows-ci branch from 17bf0e2 to 9442534 Compare March 28, 2026 22:16
@fhanau fhanau changed the title [build] Run Windows test CI job in release configuration [build] Fix Windows build, run Windows test CI job in release configuration Mar 28, 2026
@fhanau fhanau marked this pull request as ready for review March 28, 2026 22:32
@fhanau fhanau requested review from a team as code owners March 28, 2026 22:32
@fhanau
Copy link
Copy Markdown
Contributor Author

fhanau commented Mar 28, 2026

I've posted a review on PR #6452 with 4 inline comments covering 3 findings:

  1. [MEDIUM] The -fno-inline workaround is shipped with a "TODO: Why does this cause test failures?" comment — this masks a potentially real bug without root-cause understanding. Recommended documenting which tests fail and how.
  2. [LOW] The commented-out old windows_no_dbg config line should be removed rather than kept as dead code.
  3. [LOW] The per_file_copt regex patterns are overly broad, matching test files and headers in addition to the intended .c++ files. Suggested narrowing with \.c\+\+ suffix patterns.

github run

I disagree with the LLM suggestions, the overrides without file extension are easier to read and we don't care all that much if we disable optimizations slightly more widely than we'd need to on Windows.

@fhanau fhanau merged commit 7283000 into main Mar 28, 2026
37 of 38 checks passed
@fhanau fhanau deleted the felix/032726-windows-ci branch March 28, 2026 22:55
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.

3 participants