fix: Use where command for binary detection on Windows - #358
Merged
Conversation
`which` isn't available by default on Windows CI runners, so detect_binary() silently failed there, falling back to a bare binary name whose subprocess resolution could then fail with a Windows-specific path error. get_extended_path()'s `:`-joined PATH also doesn't apply on Windows, so the Windows branch skips it and queries PATH directly via `where`.
Ziinc
changed the title
where command for binary detection on Windowswhere command for binary detection on Windows
Contributor
|
🚀 Web preview: https://preview-358.treq-9zy.pages.dev |
get_extended_path() injected Homebrew/usr-bin paths and joined with `:`, then pty.rs used it verbatim as the spawned shell's PATH env var. On Windows this replaced the real PATH (System32, PowerShell's own dir, etc.) with a broken `:`-joined string of nonexistent Unix directories, leaving the spawned powershell.exe unable to resolve programs and stalling test PTY sessions before any command output appeared.
The job was hitting its 30-minute timeout mid-suite (cancelled while running jj_home_repo_target_test, with several test binaries still queued after it) rather than hanging on any single test — Windows process/filesystem overhead makes the jj-lib-heavy integration tests run noticeably slower than on Linux/macOS.
PowerShell (via PSReadLine) queries the cursor position at startup using a VT100 Device Status Report (ESC[6n). Under ConPTY there's no real console buffer, so Windows turns that query into an actual DSR request sent through the pty, which blocks until something on the other end answers it. In the app UI, xterm.js answers this automatically; the raw PtyManager reader (used directly by pty_tests.rs and unit tests) had no such responder, so the query just sat in the output buffer and every subsequent write blocked, leaving read output empty on Windows CI. Detect the query in the reader thread and write back a synthetic cursor position through the session's writer to unblock the child process.
PowerShell/cmd submit a line on carriage return; a bare \n with no preceding \r gets inserted into PSReadLine's edit buffer instead of submitting the command, so commands written by callers (including the test suite) never execute and the shell just sits at the prompt. Insert the missing \r before any bare \n on Windows, leaving \r\n untouched.
- e2e_test_helpers: set core.autocrlf=false on test repos. Windows git installs commonly default to core.autocrlf=true, which rewrites LF to CRLF on checkout; tests assert on exact file bytes, so merged/checked-out content came back with \r\n where \n was written and committed. - core_workspaces_sparse_checkout_test: `jj file list` prints platform-native path separators, so the Windows run produced `src\new.rs` where the assertion expected `src/new.rs`. Normalize separators before comparing.
- checks.rs: validate_working_directory only rejected absolute paths via Path::is_absolute(), which doesn't recognize Unix-style /foo paths on Windows (no drive letter). Real validation gap, not just a test bug — now also rejects a leading / or \ explicitly. - core/mod.rs: resolve_app_db_path test asserted a hardcoded /-joined path against a real Path::join result, which uses \ on Windows. Compare PathBufs instead. Its earlier panic (before cleanup ran) also leaked TREQ_APP_DATA_DIR across tests in the same process, which was the actual cause of the two cascading submodules.rs failures. - agent_cli.rs: JSON-escapes backslashes in Windows paths, so a raw substring check against the unescaped skill_dir failed; compare against the JSON-encoded form instead. The temp-prefix-rejection test used /etc/passwd, which doesn't exist on Windows and so was silently skipped instead of exercising the check; use the running test binary's own path instead, which always exists. The cwd-not-writable test relies on Unix's write-permission bit — Windows' read-only directory attribute doesn't block file creation inside it, so it's skipped there. - workspaces.rs: test helper never pinned the initial git branch name, so it followed whatever the runner's git defaulted to (some default to "master"); pin it to "main" to match the bookmark the test sets up. - pty.rs: one test's Windows shell timeout (2s) was too tight for PowerShell's cold-start time on CI; extended to 10s there. Another piped through `yes`/`head`, which don't exist under PowerShell; use an equivalent single Write-Output call on Windows.
- workspaces.rs: symbolic-ref alone didn't help since no git branch ref exists yet at that point (no commit made) — get_default_branch() falls through past the branch-ref check straight to init.defaultBranch config, which the Windows runner's git left at "master". Set that config directly instead. - pty.rs: echo_suppression_releases_output_after_bounded_buffer still timed out at 10s on Windows. Switch to many small Write-Output calls (a single very long line can sit behind console line-wrap handling before ConPTY flushes it) and give it real headroom (30s) — this test runs alongside ~300 others contending for CPU/IO on top of PowerShell's own slower cold start.
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
2 participants
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.
Summary
Updated the
detect_binaryfunction to use the platform-appropriate command for locating executables:whereon Windows andwhichon Unix-like systems.Key Changes
wherecommand instead ofwhich, which is not available by default on Windowsget_extended_path()which uses Unix-style path separatorswhichwith extended PATH configurationImplementation Details
wherecommand output, trimmed and validated to ensure it's not emptyhttps://claude.ai/code/session_01DsPyoEUthBBiCBdyUPbb3C