fix(link): #6023 LNK1158 'cannot run mt.exe' on Windows MSVC UI builds - #6051
Conversation
MSVC link.exe implements /MANIFEST:EMBED by shelling out to the Windows SDK's mt.exe. Perry launches a vswhere-located link.exe from a plain shell (not a vcvars64.bat developer prompt), so the SDK bin dir is not on PATH and every UI build since the comctl32-v6 manifest embed landed (v0.5.1129 / #4683) died with LNK1158. - When the linker is MSVC link.exe, probe for mt.exe: if it isn't on PATH already, locate the Windows SDK bin\<ver>\<arch> dir that holds it (registry KitsRoot10, ProgramFiles roots, legacy path — same probe order as find_msvc_lib_paths) and prepend it to the child's PATH. - If mt.exe can't be found anywhere, skip the manifest embed with a loud warning instead of failing the link — an unthemed app that builds beats a fatal LNK1158 (mirrors the existing manifest-write- failure fallback). lld-link embeds manifests in-process and is untouched. - Pass /IGNORE:4006: /FORCE:MULTIPLE makes the duplicate-definition merge deliberate, and the hundreds of LNK4006 lines it produced buried the real error in the report. Closes #6023 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds Windows SDK ChangesWindows link.exe manifest and LNK4006 fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Cmd as Compile Command
participant EmbedManifest as embed_app_manifest
participant LinkerCheck as linker_is_msvc_link_exe
participant MtCheck as ensure_mt_exe_reachable
participant SdkProbe as find_windows_sdk_mt_dir
Cmd->>EmbedManifest: needs_ui = true
EmbedManifest->>LinkerCheck: is this link.exe?
LinkerCheck-->>EmbedManifest: true/false
alt is link.exe
EmbedManifest->>MtCheck: ensure mt.exe reachable
MtCheck->>MtCheck: where mt.exe on PATH
alt not found
MtCheck->>SdkProbe: find_windows_sdk_mt_dir()
SdkProbe-->>MtCheck: bin dir or None
MtCheck->>MtCheck: prepend dir to PATH
end
MtCheck-->>EmbedManifest: reachable true/false
alt unreachable
EmbedManifest-->>Cmd: warn and skip embed
else reachable
EmbedManifest-->>Cmd: embed manifest args
end
else not link.exe
EmbedManifest-->>Cmd: embed manifest args
end
Related issues: Suggested labels: windows, bug, linker Suggested reviewers: perry-maintainers 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry/src/commands/compile/windows_link_tests.rs (1)
185-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest comment overstates coverage of arch preference.
The comment says the probe "prefers x64 over x86," but the assertion only exercises version-ordering (10.0.22621.0 wins over 10.0.19041.0 because the latter lacks a newer mt.exe-bearing dir at a higher version); no case has two arches present under the same winning version to actually verify x64-over-x86 selection. Consider adding a fixture where the newest version directory has both
x64andx86withmt.exe, assertingx64wins.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/windows_link_tests.rs` around lines 185 - 208, The test mt_dir_probe_picks_newest_versioned_sdk currently verifies version ordering but not the stated x64-over-x86 preference. Update the fixture in newest_mt_dir_under coverage so the winning SDK version contains both x64 and x86 mt.exe dirs, then assert that x64 is selected; keep the existing newer-version skip case to preserve the probe behavior being tested.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry/src/commands/compile/windows_link_tests.rs`:
- Around line 185-208: The test mt_dir_probe_picks_newest_versioned_sdk
currently verifies version ordering but not the stated x64-over-x86 preference.
Update the fixture in newest_mt_dir_under coverage so the winning SDK version
contains both x64 and x86 mt.exe dirs, then assert that x64 is selected; keep
the existing newer-version skip case to preserve the probe behavior being
tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9268398-e405-4d3c-881c-bf8c056e2352
📒 Files selected for processing (5)
crates/perry/src/commands/compile/library_search.rscrates/perry/src/commands/compile/link/mod.rscrates/perry/src/commands/compile/link/platform_cmd.rscrates/perry/src/commands/compile/link/windows_link.rscrates/perry/src/commands/compile/windows_link_tests.rs
…xe probe cargo fmt wrapped the long fixture array (lint gate), and per CodeRabbit review the newest-version fixture now carries both x64 and x86 mt.exe so the asserted x64 pick actually exercises the arch preference instead of just version ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
Since v0.5.1129, every
perry/uibuild on Windows with the MSVC toolchain fails:preceded by hundreds of
LNK4006duplicate-definition warnings (#6023, follow-up to #2169 which is unverifiable while the build is broken).Root cause
v0.5.1129 (#4683) added the comctl32 v6 application-manifest embed (
/MANIFEST:EMBED+/MANIFESTINPUT:) for themed controls.lld-linkembeds manifests in-process, but MSVClink.exeshells out to the Windows SDK'smt.exefor this. Perry locateslink.exevia vswhere and spawns it from a plain shell — not avcvars64.batdeveloper prompt — so the SDKbindir is not onPATH,link.execan't findmt.exe, and the link dies. The old doc comment inwindows_link.rsexplicitly (and incorrectly) claimed no externalmt.exewas needed, which is why this shipped.Fix
mt.exereachability for MSVC link.exe (windows_link.rs): before embedding, checkmt.exeis resolvable. If not onPATH, probe the Windows SDKbin\<ver>\<arch>dirs (registryKitsRoot10, ProgramFiles roots, legacy hardcoded path — same probe orderfind_msvc_lib_pathsalready uses forLib) and prepend the dir holdingmt.exeto the child'sPATH.mt.exegenuinely doesn't exist, skip the embed with a loud warning (controls render classic-style) instead of failing the link — mirrors the existing manifest-temp-file-write-failure fallback from feat(compile): embed comctl32 v6 manifest so Windows UI apps get themed controls #4683.lld-linkpath is untouched./IGNORE:4006(platform_cmd.rs): the duplicate-definition merge is deliberate (/FORCE:MULTIPLE), so the per-symbol LNK4006 flood is pure noise that buried the real error in the report. lld-link silently ignores unknown/IGNOREcodes.Tests
6 new unit tests in
windows_link_tests.rs(all host-independent, run on any CI OS):/MANIFEST:EMBEDarg setmt.exe, pre-10.0.15063 unversioned layout, missing/empty rootscargo test -p perry --bin perry windows_link: 18 passed. Thecfg(target_os = "windows")bodies were additionally type-checked standalone on the dev host (they use only cross-platform std APIs).Notes
onChangehalf of Regression: TextField onChange broken and LNK1158 build error in v0.5.1220 (follow-up to #2169) #6023 (originally demo text input event not work #2169) should be re-verified on Windows once this lands and unblocks compilation; the manifest embed itself is unrelated to event delivery.Closes #6023
🤖 Generated with Claude Code
Summary by CodeRabbit
mt.exe(using Windows SDK search) and skipping manifest embedding with a warning if it can’t be reached.LNK4006output when merging duplicate symbols.mt.exepath/version selection across common layouts.