Skip to content

fix(build): restore Windows compile and add windows CI job#410

Merged
jdx merged 1 commit into
mainfrom
fix/windows-build
May 3, 2026
Merged

fix(build): restore Windows compile and add windows CI job#410
jdx merged 1 commit into
mainfrom
fix/windows-build

Conversation

@jdx

@jdx jdx commented May 3, 2026

Copy link
Copy Markdown
Owner

Summary

The v2.9.0 release pipeline failed on both Windows targets (run 25281536062) due to two recently-merged PRs that didn't cfg-gate Unix-only code:

This PR:

  • Gates mod pty; behind cfg(unix).
  • Replaces libc::SIG* lookups in StopSignal with platform-portable constants — libc on Unix (where signal numbers vary across BSD/Linux), hardcoded POSIX-typical values on Windows. The Windows constants are only used for parsing/Display; procs::kill ignores stop_signal on Windows and uses TerminateProcess via sysinfo.
  • Cleans up the incidental Windows-only warnings in the same touched files (unused imports, unused vars, dead-code variant, needless return).
  • Adds a windows-build CI job running cargo check --lib --bins --all-features on windows-latest. Integration tests under tests/ use lsof/pkill and are Unix-only, so we limit the check to lib + bins. This catches the regression in PR CI rather than the release pipeline.

A follow-up patch release will be needed since v2.9.0 shipped with no Windows binaries.

Test plan

  • cargo check --all-targets --all-features (Linux) clean
  • cargo check --target x86_64-pc-windows-gnu --all-features clean
  • cargo clippy --all-targets --all-features -- -D warnings (Linux) clean
  • cargo clippy --lib --bins --target x86_64-pc-windows-gnu -- -D warnings clean
  • New windows-build CI job passes on this PR

🤖 Generated with Claude Code


Note

Low Risk
Low risk: changes are primarily platform cfg gating and CI configuration to catch Windows-only compile issues; runtime behavior is unchanged except for stop-signal parsing/display constants on Windows.

Overview
Restores Windows compatibility by cfg-gating Unix-only modules/imports (e.g. supervisor::pty, Unix signal handling) and fixing Windows-only warnings/unused code paths.

Makes StopSignal platform-portable by using libc signal numbers on Unix and POSIX-typical constants on Windows, and adjusts Windows process-kill code to ignore unused stop-signal parameters.

Adds a new GitHub Actions windows-build job that runs cargo check --lib --bins --all-features on windows-latest to catch Windows compile regressions in PR CI.

Reviewed by Cursor Bugbot for commit d3c3830. Bugbot is set up for automated code reviews on this repo. Configure here.

Two recent PRs broke the Windows release build:

- PR #408 added `src/supervisor/pty.rs` (uses `std::os::fd`) and a bare
  `mod pty;` declaration in `supervisor/mod.rs` — Windows lacks `std::os::fd`.
- PR #406 referenced `libc::SIG*` constants in `src/config_types.rs`, but
  `libc` is gated to `[target.'cfg(unix)']`.

Gate `mod pty;` to `cfg(unix)` and use platform-portable signal constants
in `StopSignal` (libc on Unix, hardcoded POSIX-typical values on Windows
where `procs::kill` ignores the signal anyway). Clean up incidental
Windows-only warnings in the same touched files.

Add a `windows-build` CI job running `cargo check --lib --bins` on
windows-latest so this regression cannot reach the release pipeline again.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR restores Windows compilation by #[cfg(unix)]-gating mod pty and replacing unconditional libc::SIG* references with platform-conditional constants, and adds a windows-build CI job to catch such regressions in PR CI instead of the release pipeline. The remaining changes are incidental warning cleanups (unused imports, dead-code variants, unused variables) in the files that were touched.

Confidence Score: 4/5

Safe to merge; all changes are compile-fix and warning-cleanup with no logic changes on Unix paths.

All findings are P2 (style/best-practice). The Windows CI job lacks an explicit toolchain pin, and the cfg_attr-on-let pattern in logs.rs is unconventional, but neither affects correctness. No P0 or P1 issues found.

.github/workflows/ci.yml — consider adding an explicit Rust toolchain step to match rust-version = 1.87 declared in Cargo.toml.

Important Files Changed

Filename Overview
.github/workflows/ci.yml Adds windows-build CI job using cargo check --lib --bins --all-features; no explicit Rust toolchain step to match declared rust-version = "1.87"
src/config_types.rs Replaces unconditional libc::SIG* references with #[cfg(unix)] imports and #[cfg(windows)] hardcoded POSIX constants; logic is sound since Windows uses TerminateProcess and ignores stop_signal at runtime
src/supervisor/mod.rs Gates mod pty and use std::collections::HashSet behind #[cfg(unix)]; tokio::signal correctly left un-gated since the Windows signals() impl uses signal::ctrl_c()
src/supervisor/pty.rs Removes redundant per-function #[cfg(unix)] on openpty(); the whole module is already gated in mod.rs, so this is correct cleanup with no functional change
src/procs.rs Gates use crate::settings::settings to Unix; uses let _ = (stop_signal, stop_timeout) to suppress unused-variable warnings in the Windows branch; removes unreachable return
src/proxy/server.rs Moves use std::io::Write inside #[cfg(unix)] blocks at two call sites, eliminating unused-import warnings on Windows; correct and low-risk
src/web/routes/logs.rs Suppresses dead-code and unused-variable warnings for non-Unix targets via #[allow(dead_code)] and #[cfg_attr]; works but _-prefix idiom would be simpler
src/main.rs Gates use tokio::signal to #[cfg(unix)]; correct since the only usage in this file is signal::unix::signal() which is Unix-only
src/deps.rs Removes unused Retry import from test module
src/state_file.rs Removes unused Retry import from test module

Fix All in Claude Code

Reviews (1): Last reviewed commit: "fix(build): restore Windows compile and ..." | Re-trigger Greptile

Comment thread .github/workflows/ci.yml
Comment on lines +38 to +43
- uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2
# `cargo check` (debug) catches the compile errors that otherwise only
# surface in the release pipeline, without paying for an optimized link.
# Integration tests under tests/ are Unix-only (lsof, pkill, etc.) so
# we limit the check to lib + bins.
- run: cargo check --lib --bins --all-features

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Missing Rust toolchain setup step

The windows-build job relies on the Rust version that happens to be pre-installed on the windows-latest runner. Cargo.toml declares rust-version = "1.87", but GitHub's hosted runners ship whatever Rust stable was current when the image was built — that may lag behind. Adding an explicit dtolnay/rust-toolchain action (or similar) ensures the Windows job uses the same pinned version managed by mise on the Linux job and prevents silent version drift.

Fix in Claude Code

Comment thread src/web/routes/logs.rs
Comment on lines +421 to 425
#[cfg_attr(not(unix), allow(unused_variables))]
let poll_count = poll_count;
tokio::task::spawn_blocking(move || {
#[cfg_attr(not(unix), allow(unused_variables))]
let opened_fresh = fh.is_none();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Prefer _-prefix over cfg_attr for unused-variable suppression

#[cfg_attr(not(unix), allow(unused_variables))] on a let statement is uncommon and easy to misread. Since both poll_count and opened_fresh are only consumed inside #[cfg(unix)] blocks, the simpler and more idiomatic way to silence the warning on non-Unix targets is to use a leading underscore in the binding itself (let _poll_count = poll_count;), which requires no attribute at all.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

@jdx jdx merged commit b33aa4e into main May 3, 2026
5 checks passed
@jdx jdx deleted the fix/windows-build branch May 3, 2026 14:51
@mise-en-dev mise-en-dev mentioned this pull request May 3, 2026
jdx pushed a commit that referenced this pull request May 3, 2026
## 🤖 New release

* `pitchfork-cli`: 2.9.0 -> 2.9.1

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

## [2.9.1](v2.9.0...v2.9.1)
- 2026-05-03

### Fixed

- *(build)* restore Windows compile and add windows CI job
([#410](#410))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).
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.

1 participant