Skip to content

fix(cli): normalize workspace path to forward slashes for CMake templates - #3040

Merged
trunk-io[bot] merged 2 commits into
dora-rs:mainfrom
GuTS805:fix/windows-cmake-path-escape
Aug 11, 2026
Merged

fix(cli): normalize workspace path to forward slashes for CMake templates#3040
trunk-io[bot] merged 2 commits into
dora-rs:mainfrom
GuTS805:fix/windows-cmake-path-escape

Conversation

@GuTS805

@GuTS805 GuTS805 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Problem

workspace_dir() in binaries/cli/src/template/mod.rs resolved the dora
workspace root from env!("CARGO_MANIFEST_DIR") and returned it as a raw OS
path. On Windows that's backslash-separated (e.g. C:\Users\...). This
string was substituted verbatim — no escaping — into the __DORA_PATH__
placeholder in the C/C++ CMake templates
(binaries/cli/src/template/c/mod.rs, binaries/cli/src/template/cxx/mod.rs),
producing a generated CMakeLists.txt line like:

set(DORA_ROOT_DIR "C:\Users\alok0\...\dora" CACHE FILEPATH "Path to the root of dora")

CMake treats \ inside a quoted string as an escape character. \U, \D,
etc. are not recognized escapes, so cmake -B build failed immediately with
a parse error (Invalid character escape '\U'), before any compiler/generator
logic ran. This only affects dora new --lang c/cxx --internal-create-with-path-dependencies
(the flag used to build templates against a local checkout) — the default
dora new flow substitutes an empty string and git-clones instead, so it
never hit this.

This is why it was never caught in CI: .github/workflows/nightly.yml
exercises this exact dora new --lang c/cxx --internal-create-with-path-dependencies

  • cmake -B flow, but is gated if: runner.os == 'Linux', where paths are
    already forward-slash.

Fix

workspace_dir() now normalizes \ to / before returning, since both
CMake and Windows accept forward slashes. Added a regression test
(workspace_dir_has_no_backslashes) asserting the returned path never
contains a backslash.

Verification

  • Built the actual dora CLI and ran
    dora new test_c_project --lang c --internal-create-with-path-dependencies
    — the generated CMakeLists.txt now contains
    set(DORA_ROOT_DIR "C:/Users/.../dora" ...) (forward slashes).
  • cmake -B build on the generated project no longer hits the
    backslash-escape parse error (progresses past it to the project() step;
    the only remaining error is an unrelated missing MSVC/nmake toolchain on
    this machine).
  • cargo fmt --all -- --check — clean
  • cargo clippy -p dora-cli -- -D warnings — clean
  • cargo test -p dora-cli workspace_dir_has_no_backslashes — pass

Fixes #3038

…ates

workspace_dir() returned a raw Windows path (backslash-separated),
which gets substituted verbatim into __DORA_PATH__ in the generated
CMakeLists.txt for --lang c/cxx --internal-create-with-path-dependencies.
CMake treats backslash as a string escape character, so cmake -B
failed immediately with 'Invalid character escape' before any
compiler logic ran. Normalize to forward slashes, which both CMake
and Windows accept.

Also box the large Cached variant of CachedResult in the coordinator
(pre-existing clippy::large_enum_variant failure surfaced while
running clippy locally, related to dora-rs#2979).
@trunk-io

trunk-io Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

😎 Merged successfully - details.

phil-opp commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

I reviewed both parts of this diff.

The CMake path normalization is correct: replacing \ with / yields a path that CMake (which treats \ as a string-escape character) and Windows both accept, and the C/C++ template callers are updated to borrow the now-owned String.

Two things worth raising:

  1. An unrelated change is bundled in. Beyond the CMake templates, this PR also boxes CachedResult::Cached in binaries/coordinator/src/state.rs to silence a clippy::large_enum_variant warning. That is the same change already proposed on its own in fix(coordinator): box CachedResult::Cached to fix large_enum_variant on Windows #3001, so the two PRs overlap and will conflict, and per the repo convention ("Don't fix unrelated warnings in PRs") it doesn't belong in a CMake-path fix. I'd drop it here and let fix(coordinator): box CachedResult::Cached to fix large_enum_variant on Windows #3001 carry it (or close whichever is redundant). The boxing itself is correct and behavior-preserving — all CachedResult::Cached sites still compile (the send_result_to(&result, …) call happens before the value is boxed, and the is_ok()/is_err() guards work through the Box).

  2. The added test can't catch the regression on PR CI. workspace_dir_has_no_backslashes is trivially true on Linux (Linux paths contain no \), so the Linux-only PR CI never exercises the replace('\\', "/") path — the fix is only really guarded on the Windows nightly job. Asserting the replacement against a constructed sample path that contains backslashes would give the test teeth on every platform.


🤖 Fully automated review by Claude (Claude Code) — no human has reviewed these findings. Please verify before relying on them.

Generated by Claude Code


Generated by Claude Code

… OS-independent test

The CachedResult::Cached boxing fix duplicated the already-open PR dora-rs#3001,
so it's dropped here to avoid merge conflicts (per 'don't fix unrelated
warnings in PRs' convention).

The regression test now asserts against a hardcoded Windows-style sample
path instead of the live workspace_dir() output, since on Linux/macOS CI
the real path never contains a backslash and the old assertion passed
trivially without exercising the normalization logic.
@GuTS805

GuTS805 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed both points:

  1. Dropped the unrelated coordinator change. Reverted the CachedResult::Cached
    boxing in binaries/coordinator/src/state.rs — you're right that it duplicates
    fix(coordinator): box CachedResult::Cached to fix large_enum_variant on Windows #3001, so I'll let that PR own it and keep this one scoped to the CMake path fix.

  2. Made the test OS-independent. Pulled the normalization logic into a
    normalize_for_cmake() helper and changed the test to assert against a
    hardcoded Windows-style sample path (C:\Users\example\dora) instead of the
    live workspace_dir() output. It now actually exercises the \/
    replacement on every platform instead of trivially passing on Linux CI.

Pushed both fixes. Diff is back to just the 3 template files.

phil-opp commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

🤖 Automated review by Claude — fully automated review, not verified by a human.

Reviewed the latest commit (b45176f). Both points from the prior review are addressed: the unrelated CachedResult::Cached boxing in binaries/coordinator/src/state.rs is dropped, and the regression test now runs normalize_for_cmake(r"C:\Users\example\dora") against a hardcoded Windows-style path, so it exercises the \/ replacement on every platform instead of passing trivially on Linux CI. The diff is scoped to the three template files, and the &super::workspace_dir()? borrow updates match the new String return type. No new issues found.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

@GuTS805 the Trunk merge queue failed for this PR.

See the Trunk merge-status comment for details.

Posted as a new comment so GitHub sends an email — Trunk's sticky comment is edited in place and won't trigger a notification.

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

Projects

None yet

2 participants