fix(build): embed the Windows app manifest via the linker so cargo test can load#499
Merged
Merged
Conversation
…st can load The default tauri-build app manifest — whose entire content is the Common-Controls v6 side-by-side dependency — is embedded as a compiled resource that embed-resource links into bins only. The `cargo test` harness for the lib target is not a bin: it gets no manifest, binds legacy comctl32 v5, and fails to load with STATUS_ENTRYPOINT_NOT_FOUND (0xc0000139) on the comctl6-only export `TaskDialogIndirect` (imported transitively via tao/muda/dialog code) before any test runs — making the Rust suite unrunnable on Windows. CI runs it on ubuntu only, so this never surfaced there. Cargo has no directive scoped to the unit-test harness (rustc-link-arg-tests covers only tests/*.rs targets), so switch mechanisms: tauri_build no longer embeds the resource manifest (WindowsAttributes::new_without_app_manifest) and the linker embeds an identical one (/MANIFEST:EMBED + /MANIFESTDEPENDENCY) into every product it links — app binary and test harnesses alike. No-op on non-Windows targets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Tauri build script (build.rs) to properly embed the Common-Controls v6 manifest dependency on Windows (MSVC) targets, ensuring that the unit-test harness (cargo test) can load without failing. The reviewer identified a compilation issue where comparing Result directly with Ok(...) fails because std::env::VarError does not implement PartialEq, and provided a code suggestion to use .ok() == Some(...) instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Owner
|
Looks good to merge. The fix is well-scoped, technically sound, and preserves existing behavior outside Windows/MSVC. Please track the 38 remaining Windows test failures in a separate issue, since merging this PR will close #498. |
This was referenced Jul 13, 2026
Xoshbin
added a commit
that referenced
this pull request
Jul 14, 2026
…503) * fix(runtimes): make path guards and binary lookup correct on Windows Two of the guards used Path::is_absolute(), which on Windows requires a drive prefix AND a root — so unix-absolute paths slipped through: - validate_runtime_name accepted "/etc" (and drive-prefixed names like "C:evil", which Path::join lets replace the runtimes root entirely). Reject on has_root() or any Prefix component instead. - extract_tar_gz's tar-slip guard accepted entries like "/tmp/evil" on Windows. Replace the ParentDir+is_absolute check with a single components guard: any component that is not Normal/CurDir (ParentDir, RootDir, or a drive Prefix) rejects the entry. "./"-prefixed entries stay accepted. Also fix the ensure_reports_needs_download_again_after_remove fixture to write binary_file_name("bun") — the lookup expects bun.exe on Windows. These three tests were among the 38 Windows failures that became visible once #499 made cargo test loadable on Windows (see #498). New regression tests cover the drive-prefixed/rootful forms on Windows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH * fixup: tighten validate_runtime_name to a single Normal component Gemini review caught a pre-existing hole the has_root/Prefix check kept: "." passes validation, and remove_from_root(root, ".") resolves to the runtimes root itself — remove_dir_all would delete every installed runtime. Require exactly one Component::Normal instead, which rejects ".", "./x", separator-containing names, rootful and drive-prefixed paths in one predicate. New regression test covers "." and multi-component names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH * refactor: tighten runtime name validation to explicitly reject all path separators and directory aliases --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Xoshbin <xoshbin@gmail.com>
Xoshbin
pushed a commit
that referenced
this pull request
Jul 14, 2026
The commands::files tests built declared glob patterns from
std::fs::canonicalize()'d temp roots. On Windows that returns a verbatim
`\\?\C:\...` path whose `?` is a glob metacharacter, so:
- files:glob's literal-prefix parse rejected every pattern ("must begin
with an absolute literal prefix"), and
- files:read coverage never matched (the pattern also carried native
backslashes, which globset treats as escapes, not separators).
Fix in the test helpers only — declared patterns are forward-slash by
convention (the product's own error text says 'C:/Steam/appcache/**',
and #460 already settled that backslash patterns aren't supported):
- canonicalize temp roots with dunce (no `\\?\` prefix), matching what
the product does to the requested path, and
- build pattern strings via a `slashes()` helper that forward-slashes the
root. Requested paths and assertions stay native — they match the
product's native output.
Fixes 13 of the 38 Windows failures surfaced by #499 (see #498). No
product change; Unix behavior is byte-identical (dunce == canonicalize
and no backslashes to replace).
Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Xoshbin
pushed a commit
that referenced
this pull request
Jul 14, 2026
… Windows (#505) * test(file_index): make index/query/snapshot/watcher tests portable to Windows Two distinct Windows-only failures, both test-side: 1) Fixtures hardcoded unix paths ("/tmp/rootA", "/r/..."). The index materializes paths with MAIN_SEPARATOR and compares that against requested paths, so on Windows a "/tmp/rootA" root joined with '\' ("/tmp/rootA\docs") never equals the unix-spelled lookup key — breaking 6 index, 2 query, and 1 snapshot test. Route fixture paths and their expected strings through a local `np()` helper (unix '/' -> MAIN_SEPARATOR; no-op on Unix). Slash-token queries are unaffected: tokenize() splits the query on '/' and matches component names, not the stored separator. 2) The two real-filesystem watcher e2e tests used the OS temp dir as their watched root. On Windows that is C:\Users\<u>\AppData\Local\Temp, and "AppData/Local" is one of DEFAULT_IGNORE_PATTERNS — so build_exclusion_set(&[]) matched the tests' own root via **/AppData/Local/** and the coalescer correctly dropped every event ("got []"). It only worked on Linux because /tmp matches no default pattern. Give these tests an exclusion set scoped to just the pattern under test (node_modules, or none for the rename test) so the temp root isn't collaterally excluded; the default patterns are covered by the pure-coalescer unit tests. Also replaced the fixed post-arm sleep with an active arming probe + bounded poll — robust to notify's async ReadDirectoryChangesW arming and self-diagnosing if a backend ever genuinely fails to deliver. Not a product concern: production watches user-configured roots (never under AppData/Local), and both fixes are test-only. Fixes 11 of the 38 Windows failures surfaced by #499 (see #498). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH * fixup: build native path in np() without MAIN_SEPARATOR_STR Gemini review: MAIN_SEPARATOR_STR was stabilized in Rust 1.78; map chars to MAIN_SEPARATOR (stable since 1.0) instead so the helper doesn't assume a minimum toolchain. (The repo declares no MSRV and CI pins 1.97, so this is defensive, not a fix — but it's a wash and removes the question.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Xoshbin
pushed a commit
that referenced
this pull request
Jul 14, 2026
…#507) Two Windows-only test failures, both fixture-side: - ext_builder::secret_scan: the expected offending path was built with dir.join("src/config.ts"), whose forward-slash suffix stays literal on Windows (backslash root + "/config.ts"), while the scanner returns a native all-backslash path. Build the expected with sequential joins so it uses the native separator. - application::uninstall: validate_data_path_rejects_missing passed "/tmp/__asyar_nonexistent_data_path__", which is not absolute on Windows, so the absolute-path check rejected it before the missing-path check the test asserts (NotFound). Derive an absolute-but-missing path from the fake home's temp dir instead. Fixes the last of the 38 Windows failures surfaced by #499 (see #498), excluding the 6 agents::builtin_tools::shell_test cases deliberately left for the in-flight agents rework. No product change. Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Xoshbin
pushed a commit
that referenced
this pull request
Jul 14, 2026
ShellExecTool spawns an executable directly (Command::new(command).args), with no implicit shell. The tests drove it with POSIX commands — `echo`, `sh`, `ls` — which on Windows are cmd builtins with no .exe (`echo`, `ls`) or absent entirely (`sh`), so the spawn failed and six tests errored once #499 made the suite runnable on Windows. Route the shell-dependent cases through the host shell (`cmd /C` on Windows, `sh -c` elsewhere) via a SHELL constant, with per-platform spellings where the syntax differs (`1>&2` vs `>&2`, `dir /B` vs `ls`). The args-omitted case uses `hostname` — a real no-arg executable present on all three platforms — so it still exercises the "args absent → runs" path without a shell. No product change; ShellExecTool's direct-exec contract is unchanged. Clears the last of the 38 Windows failures surfaced by #499 (see #498). Independent of #500, which only touches the frontend agents UI, not this Rust tool. Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Xoshbin
pushed a commit
that referenced
this pull request
Jul 14, 2026
…ut settings (#506) * test(keyboard): stop the layout tests from mutating the user's OS input settings The windows_key_resolver tests loaded FR/DE/US-Intl keyboard layouts with LoadKeyboardLayoutW(KLF_ACTIVATE) + ActivateKeyboardLayout, then restored only the previously *active* layout — never unloading what they loaded. Every `cargo test` run therefore accumulated layouts in the user's input list and surfaced the taskbar language picker. This was invisible until #499 made the suite runnable on Windows. It also never worked: resolve_keypress reads the *foreground window's* layout, not the test thread's, so activating a layout on the test thread had no effect — the AZERTY/QWERTZ tests silently resolved against the tester's own active layout, and their expectations were never validated. Fix both by making resolution testable without touching global state: - Split resolve_with_hkl(key, shift, hkl) out of resolve_keypress; the public fn still derives the HKL from the foreground window. - Tests load a layout with KLF_NOTELLSHELL (no activation, no shell notification), drive resolve_with_hkl against the returned HKL, then UnloadKeyboardLayout it — but only if it wasn't already loaded, so a layout the user actually installed is never removed. Net OS effect: none. - Pin the US-layout tests to an explicitly loaded US HKL so they no longer depend on whatever layout the tester happens to have active. Running the tests for real against the target layouts also corrected two expectations that had never actually executed: on French AZERTY `:` is the US-`.` key *unshifted* (shifted is `/`), not Shift+`.` as on German QWERTZ; and the US-International `^` dead key does not commit a standalone glyph (the resolver returns None by contract) — so that test now asserts only the guarantee it exists to protect, that querying the dead key leaves no kernel composition state behind. Fixes the windows_key_resolver failures among the 38 surfaced by #499 (see #498), and removes the side effect that was polluting the language bar. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH * fixup: unload the test layout via an RAII guard so a panic can't leak it Gemini review: if an assertion inside the test closure panics, the stack unwinds past the UnloadKeyboardLayout call — leaking the loaded layout into the user's input list, exactly the OS-state mutation this PR removes. Move the unload into a Drop guard so it runs on both normal return and unwind, keeping the "only unload a layout we actually added" logic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH * fixup: rustfmt the with_layout_hkl guard The previous fixup wasn't run through rustfmt; CI's `cargo fmt --check` formats regardless of target cfg, so the Windows-only file tripped it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH * fixup: test against already-loaded layouts, never LoadKeyboardLayoutW Maintainer review: LoadKeyboardLayoutW is not guaranteed non-activating on Windows 8+ even with KLF_NOTELLSHELL (it can activate when the caller owns the focused window; the flag only suppresses shell notification), so the tests could still alter the developer's input list — and a "clean" run might merely be reusing layouts a prior buggy run already installed. It also needed a mutex, since concurrent tests loaded/unloaded shared OS state. Drop LoadKeyboardLayoutW/UnloadKeyboardLayout entirely. Add loaded_hkl_for_langid(), which reads GetKeyboardLayoutList and matches the LANGID (low word of the HKL); each layout test binds an already-loaded HKL or skips with a note. No load ⇒ mutation is structurally impossible; the enumeration and ToUnicodeEx queries are side-effect-free, so no lock is needed despite concurrent tests. Layouts absent locally are covered by the full matrix in a disposable Windows CI (follow-up — CI is ubuntu-only now). US-International shares LANGID 0x0409 with plain US and can't be singled out from an already-loaded HKL without activating, so the dead-key test runs against whichever US-English layout is loaded; its invariant holds on both, with the real dead-key path exercised when US-Intl is that layout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MGJAx66y29o33szDvVYTBH --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 14, 2026
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.
Closes #498.
What
Makes
cargo testrunnable on Windows. The test harness binary previously failed to load —STATUS_ENTRYPOINT_NOT_FOUND(0xc0000139) before a single test ran — so the Rust suite has never been runnable on Windows at all (CI runs it on ubuntu only).Why it broke
The default tauri-build app manifest — whose entire content is the Common-Controls v6 side-by-side dependency — is embedded as a compiled resource that embed-resource links into bins only. The
cargo testharness for the lib target is not a bin: it gets no manifest, binds legacy comctl32 v5, and the loader kills it on the comctl6-only exportTaskDialogIndirect(imported transitively via tao/muda/dialog code). Full diagnosis in #498.How
build.rs, Windows/MSVC only — switch the manifest from a compiled resource to the linker, so every linked product gets it:tauri_build::try_buildwithWindowsAttributes::new_without_app_manifest()(drops the resource-based manifest), pluscargo:rustc-link-arg=/MANIFEST:EMBEDandcargo:rustc-link-arg=/MANIFESTDEPENDENCY:<Common-Controls v6>— applied to the app binary and test harnesses alike. The dependency string reproducestauri-build's defaultwindows-app-manifest.xml1:1.Why not something smaller:
cargo:rustc-link-arg-testsonly applies totests/*.rstargets (LinkArgTarget::Test => target.is_test()in cargo) — it never reaches the lib unit-test harness, which is the binary that fails./MANIFEST:EMBEDglobally would double-manifest the app binary (duplicateRT_MANIFEST)..cargo/config.tomlcan't be used —src-tauri/.cargo/config.tomlis deliberately gitignored as a per-developer file.No-op on macOS/Linux (
CARGO_CFG_TARGET_OS/CARGO_CFG_TARGET_ENVgated; non-Windows keeps the exact previoustauri_build::build()behavior viaAttributes::default()).Verification
rustc 1.96.1:cargo testnow loads and runs — 2808 passed, 38 failed, 9 ignored (previously: instant loader death). The 38 are pre-existing Windows-environment test issues that were invisible while the harness couldn't load (POSIX-shell assumptions, path semantics, keyboard-layout-dependent tests) — tracked in cargo test on Windows: test harness fails to load — missing comctl32 v6 (TaskDialogIndirect) because the test binary carries no app manifest #498 as a follow-up, not addressed here.asyar.exewithmt -inputresourcestill declaresMicrosoft.Windows.Common-Controls6.0.0.0; dev launcher runs normally. One cosmetic delta: the linker-generated manifest also contains the default<requestedExecutionLevel level="asInvoker"/>trustInfo block — functionally identical, asasInvokeris what Windows assumes when the section is absent.cargo checkclean;cargo fmtclean. The new code path is cfg-gated off, preserving previous behavior — ubuntu CI unaffected.🤖 Generated with Claude Code