Skip to content

fix: invoke Windows copy through cmd in Makefile#8302

Open
ianmohney wants to merge 2 commits into
janhq:mainfrom
ianmohney:codex/fix-windows-make-copy-shell
Open

fix: invoke Windows copy through cmd in Makefile#8302
ianmohney wants to merge 2 commits into
janhq:mainfrom
ianmohney:codex/fix-windows-make-copy-shell

Conversation

@ianmohney

Copy link
Copy Markdown
Contributor

Describe Your Changes

Follow-up to #8293.

That change replaced cp with Windows copy for the jan-cli.exe copy step. This worked in local Windows builds, but the self-hosted Windows CI runners invoke the Make recipe differently: GNU Make tried to launch copy directly via CreateProcess.

On Windows, copy is a cmd.exe built-in, not a standalone executable, so both Windows Linter & Test pipelines failed after the Rust build succeeded:

  • test-on-windows (mcafee)
  • test-on-windows (bit-defender)

The failure was:

process_begin: CreateProcess(NULL, copy src-tauri\target\release\jan-cli.exe src-tauri\resources\bin\jan-cli.exe, ...) failed.
make (e=2): The system cannot find the file specified.
make[1]: *** [Makefile:223: build-cli] Error 2

Fixes Issues

@tokamak-pm

tokamak-pm Bot commented Jun 11, 2026

Copy link
Copy Markdown

PR Review: fix: invoke Windows copy through cmd in Makefile

Recommendation: can merge


Summary

This is a targeted follow-up to #8293. The previous change replaced cp with copy (the Windows shell built-in) in the build-cli and build-cli-dev Makefile targets, but bare copy is not a standalone executable — it is a cmd.exe built-in. GNU Make on Windows calls CreateProcess directly, which cannot resolve built-ins by name alone, causing the CI failure described in the PR.

The fix wraps both copy invocations in cmd /C copy /Y ..., which correctly launches cmd.exe and passes the copy command through the shell, allowing the built-in to be resolved.


Correctness

  • cmd /C tells cmd.exe to run the following string as a command and then exit. This is the standard idiom for invoking cmd.exe built-ins from environments that use CreateProcess (like GNU Make on Windows). Correct.
  • /Y suppresses the overwrite confirmation prompt, which is essential in a non-interactive CI context. Good addition.
  • Quoting: Both source and destination paths are now quoted with double-quotes. Necessary if any path segment ever contains spaces; this is defensive and correct.
  • Both affected call sites (build-cli release path and build-cli-dev debug path) are updated consistently.
  • Non-Windows branches (Darwin, Linux) are untouched, so no regression risk on other platforms.

Risks / Considerations

  • Minor: The build-cli-dev target calls cargo build (without --release) before the ifeq block, so on Windows the debug binary is built at the same relative location (src-tauri\target\debug\jan-cli.exe). The fix correctly references the debug path for build-cli-dev and the release path for build-cli. No copy/paste error.
  • Scope is minimal: Only 2 lines changed in 1 file. Risk of unintended side effects is essentially zero.
  • Alternative approaches (e.g. xcopy, PowerShell Copy-Item, or a $(shell ...) workaround) would all work, but cmd /C copy /Y is the most idiomatic and lightweight solution for this pattern in Makefiles targeting Windows.

Testing

No automated unit tests apply here — CI itself is the test. The PR description documents that the two affected Windows CI jobs (test-on-windows (mcafee) and test-on-windows (bit-defender)) were the failing targets. Verification should come from a passing CI run on this branch.

Verdict

The change is correct, minimal, well-reasoned, and handles both affected call sites consistently. The /Y flag and path quoting are good defensive additions. Ready to merge once CI passes on this PR.

@ianmohney

Copy link
Copy Markdown
Contributor Author

looking into the failures, I've got a couple ideas but need to verify

@ianmohney

Copy link
Copy Markdown
Contributor Author

opting for a different solution. I had options here, but there were undocumented shell requirements. I think this is the most flexible, but happy to discuss if need be.

@tokamak-pm

tokamak-pm Bot commented Jun 12, 2026

Copy link
Copy Markdown

Follow-up review — new commit since last review.

New commit reviewed

cfc377c — "fix: use PowerShell for Windows CLI copy"

The author replaced the cmd /C copy /Y approach (from the first commit) with PowerShell Copy-Item. This was prompted by undocumented shell requirements in the CI environment that made cmd /C unreliable.

Analysis of the new approach

powershell -NoLogo -NoProfile -NonInteractive -Command "Copy-Item -Force -ErrorAction Stop -LiteralPath 'src-tauri/target/release/jan-cli.exe' -Destination 'src-tauri/resources/bin/jan-cli.exe'"

Correctness:

  • -NoLogo -NoProfile -NonInteractive — good CI-friendly flags; -NoProfile avoids loading user profiles that could alter behavior, -NonInteractive prevents prompts.
  • -ErrorAction Stop — ensures PowerShell exits with a non-zero code on failure, so Make correctly detects errors. This is better error handling than cmd /C copy.
  • -LiteralPath — treats the path literally (no wildcard expansion), defensive against special characters.
  • -Force — overwrites without prompting, equivalent to the previous /Y flag.
  • Forward slashes work correctly in PowerShell path resolution.
  • Both build-cli (release) and build-cli-dev (debug) targets are updated consistently, with correct paths for each.

Robustness:
PowerShell is guaranteed on all modern Windows versions (since Windows 7 SP1) and is the preferred shell on Windows CI runners. This is arguably more robust than cmd /C copy because Copy-Item is a first-class cmdlet, not a shell built-in that depends on the parent shell.

CI status: All checks pass — test-on-windows-pr, test-on-macos, test-on-ubuntu, and all build targets succeeded on this commit.

Recommendation: can merge

The PowerShell approach is correct, well-parameterized, and CI-verified. Both affected targets are updated consistently. Ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant