From bd9e2e96629091b5e68dee9649f4a165122824bf Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 17:00:32 +0800 Subject: [PATCH 01/11] fix(toolchain): the msvc payload's location is known, not inferred error: msvc payload installed at 'C:\Users\...\xpkgs\xim-x-msvc\14.44.35207\VC', but no cl.exe under VC/Tools/MSVC/14.44.35207 Note the `\VC` on the end. `XpkgPayload::root` treats the version directory as the root only when it directly contains bin/ include/ lib/, and otherwise descends into a lone subdirectory (package_fetcher.cppm:1027). An installed msvc payload has exactly one entry -- `VC/` -- so the root came back one level too deep and a perfectly good toolset read as missing. That heuristic is right for the payloads it was written for; it is simply not an answer to "where is this package". The answer is (store, name, version), and all three are known at both call sites: `xim_tool(env, name, version)` gives the version directory outright. `resolve_xpkg_path` still does the installing -- it just stops being asked where. Found on the first e2e run where the install actually succeeded. Every earlier attempt died in the recipe, so this was standing behind three other defects the whole time. The test pins both directions: the version directory resolves, and the `VC` subdirectory does NOT -- an implementation that searched upward from whatever it was handed would pass the first assertion and fail the second. --- src/build/prepare.cppm | 11 +++++++++-- src/toolchain/lifecycle.cppm | 16 ++++++++++++++-- tests/unit/test_toolchain_msvc.cpp | 22 ++++++++++++++++++++++ 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/build/prepare.cppm b/src/build/prepare.cppm index 4715df91..0066d1b8 100644 --- a/src/build/prepare.cppm +++ b/src/build/prepare.cppm @@ -1320,12 +1320,19 @@ prepare_build(bool print_fingerprint, // (cl.exe is four levels deeper) and the ELF post-install fixup // (there is nothing to patchelf on a PE toolchain). if (spec->family == mcpp::toolchain::Family::Msvc) { + // Not `payload->root`: that field is the fetcher's guess at where + // the useful tree starts, and it descends into a lone + // subdirectory when the version dir has no bin/ include/ lib/. + // An msvc payload's only entry is `VC/`, so the guess lands one + // level too deep. (store, name, version) is known — use it. + auto verDir = mcpp::xlings::paths::xim_tool( + mcpp::config::make_xlings_env(**cfg), pkg.ximName, pkg.ximVersion); auto inst = mcpp::toolchain::msvc::installation_at( - payload->root, pkg.ximVersion); + verDir, pkg.ximVersion); if (!inst) { return std::unexpected(std::format( "msvc payload at '{}' has no cl.exe under VC/Tools/MSVC/{}", - payload->root.string(), pkg.ximVersion)); + verDir.string(), pkg.ximVersion)); } explicit_compiler = inst->clPath; mcpp::ui::info("Resolved", std::format( diff --git a/src/toolchain/lifecycle.cppm b/src/toolchain/lifecycle.cppm index b3814add..e0b82e70 100644 --- a/src/toolchain/lifecycle.cppm +++ b/src/toolchain/lifecycle.cppm @@ -618,14 +618,26 @@ export int toolchain_install(const mcpp::config::GlobalConfig& cfg, // VC/Tools/MSVC//bin/Hostx64/x64, and there is nothing to // patchelf on a PE toolchain. if (spec->family == mcpp::toolchain::Family::Msvc) { + // NOT `payload->root` — that is a GUESS, and it guesses wrong + // here. resolve_xpkg_path calls the version directory the root + // only when it directly contains bin/ include/ lib/; otherwise it + // descends into a lone subdirectory. An installed msvc payload + // has exactly one entry, `VC/`, so the "root" comes back as + // …/14.44.35207/VC and the toolset then looks like it is missing. + // + // The location is not something to infer: it is (store, name, + // version), and all three are known here. resolve_xpkg_path above + // is what INSTALLS; this is what says where. + auto verDir = mcpp::xlings::paths::xim_tool( + mcpp::config::make_xlings_env(cfg), pkg.ximName, pkg.ximVersion); auto inst = mcpp::toolchain::msvc::installation_at( - payload->root, pkg.ximVersion); + verDir, pkg.ximVersion); if (!inst) { mcpp::ui::error(std::format( "msvc payload installed at '{}', but no cl.exe under " "VC/Tools/MSVC/{} — the payload is not what this version " "claims to be", - payload->root.string(), pkg.ximVersion)); + verDir.string(), pkg.ximVersion)); return 1; } msvc_print_detected(*inst, "Installed"); diff --git a/tests/unit/test_toolchain_msvc.cpp b/tests/unit/test_toolchain_msvc.cpp index 4a69dd0c..12224056 100644 --- a/tests/unit/test_toolchain_msvc.cpp +++ b/tests/unit/test_toolchain_msvc.cpp @@ -234,6 +234,28 @@ TEST(MsvcManaged, ResolvesTheDeclaredToolsetAndNotItsNeighbour) { EXPECT_EQ(newer->toolsVersion, "14.52.36629"); } +TEST(MsvcManaged, TheVersionDirIsTheRootNotItsLoneSubdirectory) { + // A real installed payload has exactly ONE entry: `VC/`. The fetcher's + // `XpkgPayload::root` treats the version directory as the root only when + // it directly contains bin/ include/ lib/, and otherwise descends into a + // lone subdirectory -- so for msvc it hands back `/VC`, and the + // toolset then looks missing on a payload that installed perfectly. + // + // Pinning both sides here: the version directory resolves, and the `VC` + // subdirectory does NOT. The second half is what makes this a test rather + // than a restatement -- an implementation that searched upward from + // whatever it was given would pass the first and fail this. + FakeToolset t{"verdir"}; + t.add_toolset("14.44.35207"); + + auto ok = msvc::installation_at(t.root, "14.44.35207"); + ASSERT_TRUE(ok.has_value()) << "the version directory must be the root"; + + EXPECT_FALSE(msvc::installation_at(t.root / "VC", "14.44.35207").has_value()) + << "a caller handing in the VC subdir is passing the wrong thing, and " + "must be told so rather than quietly rescued"; +} + TEST(MsvcManaged, AbsentToolsetIsNulloptNotASubstitute) { FakeToolset t{"absent"}; t.add_toolset("14.44.35207"); From ca82400974d169bc0111670d21d5ddeb158d2836 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 18:51:34 +0800 Subject: [PATCH 02/11] fix(toolchain): an SDK is headers AND libraries, not headers find_windows_sdk() accepted a root on `Include\\ucrt\corecrt.h` alone. A managed windows-sdk payload missing the MSI that carries kernel32.lib satisfied that, ranked ABOVE the machine's own complete SDK because its version was higher, compiled every translation unit, and then: LINK : fatal error LNK1104: cannot open file 'kernel32.lib' with not one line in the build log mentioning the SDK. This is e2e 239's failure on the managed toolset, and the same shape as the defect has_usable_msvc() was written to prevent -- selecting on a weaker signal than the build actually needs -- so the fix is the one that predicate already models: require both halves. A root now needs `Include\\ucrt\corecrt.h` AND `Lib\\um\\kernel32.lib`. Partial roots are skipped, so the search falls through to the next one instead of poisoning the build. Three tests, each run against the old gate: headers-only is rejected, a complete root is still accepted (so the check cannot pass by rejecting everything), and a partial root loses to a complete one even when its version is higher -- which is exactly how the payload outranked the system SDK. The first and third fail without the fix. FakeToolset::add_sdk() now builds a complete SDK; add_sdk_headers_only() builds the half-installed one. --- CHANGELOG.md | 22 ++++++++++ docs/03-toolchains.md | 6 +++ src/toolchain/msvc.cppm | 36 +++++++++++++++- tests/e2e/239_msvc_managed_toolset.sh | 2 +- tests/unit/test_toolchain_msvc.cpp | 60 +++++++++++++++++++++++++++ 5 files changed, 123 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92b7bb41..0405e5fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,28 @@ > 本文件追踪 `mcpp-community/mcpp` 公开仓的版本演进。 > 格式参考 [Keep a Changelog](https://keepachangelog.com/zh-CN/1.1.0/)。 +## [Unreleased] + +### 修复 + +- **半装的 Windows SDK 被当成装好的,链接到最后才炸。** + + `find_windows_sdk()` 认一个根的条件是 `Include\\ucrt\corecrt.h` + 存在 —— 只看头文件。而 SDK 是头文件**和**导入库两半。 + + 托管的 `xim:windows-sdk` payload 少了带 `kernel32.lib` 的那个 MSI 时, + 这个根照样"找到了",而且因为版本号更高,**排在机器自己那套完整 SDK 前面**。 + 于是每个 TU 都编过了,一直到最后一步: + + LINK : fatal error LNK1104: cannot open file 'kernel32.lib' + + 日志里没有任何一行提到 SDK。 + + 现在两半都要:`Include\\ucrt\corecrt.h` 和 + `Lib\\um\\kernel32.lib`。半装的根被跳过,搜索落到下一个, + 本来就能用的构建就能用了 —— 和 `has_usable_msvc()` 坚持"两半都要"是同一条 + 理由,只是这次轮到 SDK 自己。 + ## [2026.8.16.2] — 2026-08-16 ### 修复 diff --git a/docs/03-toolchains.md b/docs/03-toolchains.md index f89469a2..c6be373c 100644 --- a/docs/03-toolchains.md +++ b/docs/03-toolchains.md @@ -310,6 +310,12 @@ pinned toolset in mcpp's store, then `C:\Program Files (x86)\Windows Kits\10`. A missing SDK fails the build with guidance (`mcpp self doctor` reports SDK status). +A root only counts as an SDK when it has **both** halves — `Include\\ucrt\ +corecrt.h` *and* `Lib\\um\\kernel32.lib`. A root with headers and no +import libraries is skipped rather than selected, so a partially unpacked +payload cannot outrank the machine's complete SDK and turn into +`LNK1104: cannot open file 'kernel32.lib'` at the very end of a build. + **CRT model.** `/MD` (host-coupled) by default; `/MT` when either ```toml diff --git a/src/toolchain/msvc.cppm b/src/toolchain/msvc.cppm index a0417e7d..4ed89bf5 100644 --- a/src/toolchain/msvc.cppm +++ b/src/toolchain/msvc.cppm @@ -575,6 +575,17 @@ sibling_sdk_roots(const std::filesystem::path& clPath) { return out; } +// The `Lib\\um\` subdirectory whose absence makes a root unusable +// FOR THIS HOST. Spelled from the host architecture rather than the build's +// target: a cross-compiling link is the caller's business (`build_env_for_cl` +// takes an arch), but a root with no host-arch libs at all is not an SDK this +// machine can link against. The names are the SDK's own. +#if defined(_M_ARM64) || defined(__aarch64__) +constexpr std::string_view sdk_lib_arch = "arm64"; +#else +constexpr std::string_view sdk_lib_arch = "x64"; +#endif + std::optional find_windows_sdk( std::span extraRoots) { // Highest version dir under `root/Include` that actually carries the UCRT @@ -582,17 +593,38 @@ std::optional find_windows_sdk( // (Registry Installed Roots would be marginally more correct — the path // scan covers every real installer layout seen so far and needs no Win32 // API surface.) + // BOTH halves, for the same reason `has_usable_msvc()` asks for both: an + // SDK is headers AND import libraries, and a root carrying only the first + // is a half-installed state that this function used to call "found". + // + // That is not hypothetical. A managed `xim:windows-sdk` payload whose + // ucrt MSI had unpacked but whose um-libs MSI had not left + // `Include//ucrt/corecrt.h` on disk with no `kernel32.lib` anywhere; + // the header check passed, the root was selected over the machine's own + // complete SDK, every translation unit compiled, and the build died at + // LINK : fatal error LNK1104: cannot open file 'kernel32.lib' + // with nothing in the log naming the SDK. Rejecting the partial root + // makes the search fall through to the next one, which is the behaviour + // a user would expect from a probe that reports "not found". + // + // kernel32.lib is the right sentinel: every link needs it, and unlike the + // ucrt libs it is not spread across the SDK's optional pieces. auto pick = [](const std::filesystem::path& root, std::string_view want) -> std::optional { std::error_code ec; auto inc = root / "Include"; if (!std::filesystem::is_directory(inc, ec)) return std::nullopt; + auto usable = [&](const std::filesystem::path& verDir, + const std::string& v) { + return std::filesystem::exists(verDir / "ucrt" / "corecrt.h", ec) + && std::filesystem::exists( + root / "Lib" / v / "um" / sdk_lib_arch / "kernel32.lib", ec); + }; std::string best; for (auto& e : std::filesystem::directory_iterator(inc, ec)) { if (!e.is_directory(ec)) continue; auto v = e.path().filename().string(); - if (!std::filesystem::exists(e.path() / "ucrt" / "corecrt.h", ec)) - continue; + if (!usable(e.path(), v)) continue; if (!want.empty() && v == want) return WindowsSdk{root, v}; if (v > best) best = v; } diff --git a/tests/e2e/239_msvc_managed_toolset.sh b/tests/e2e/239_msvc_managed_toolset.sh index c465086c..af90b6cb 100755 --- a/tests/e2e/239_msvc_managed_toolset.sh +++ b/tests/e2e/239_msvc_managed_toolset.sh @@ -13,7 +13,7 @@ # - `msvc@system` on the same machine must still resolve to the SYSTEM cl, # i.e. the two origins do not contaminate each other # -# Network: installs xim:msvc (~85 MB) + xim:windows-sdk (~135 MB). Skips +# Network: installs xim:msvc (~85 MB) + xim:windows-sdk (~291 MB). Skips # cleanly when the index cannot be reached, because an offline runner has # nothing to say about this. set -e diff --git a/tests/unit/test_toolchain_msvc.cpp b/tests/unit/test_toolchain_msvc.cpp index 12224056..461081e9 100644 --- a/tests/unit/test_toolchain_msvc.cpp +++ b/tests/unit/test_toolchain_msvc.cpp @@ -169,7 +169,20 @@ struct FakeToolset { std::ofstream{tools / "modules" / "std.ixx"} << "export module std;"; } } + // A COMPLETE SDK: headers and import libraries. Both, because + // find_windows_sdk() requires both — a root with only headers is the + // half-installed state `add_sdk_headers_only()` below exists to build. + // Libs for both architectures so the fixture does not care which host it + // is running on. void add_sdk(std::string_view version) { + add_sdk_headers_only(version); + for (auto arch : {"x64", "arm64"}) { + auto lib = root / "Lib" / std::string(version) / "um" / arch; + std::filesystem::create_directories(lib); + std::ofstream{lib / "kernel32.lib"} << "not a library"; + } + } + void add_sdk_headers_only(std::string_view version) { auto inc = root / "Include" / std::string(version) / "ucrt"; std::filesystem::create_directories(inc); std::ofstream{inc / "corecrt.h"} << "#pragma once"; @@ -362,6 +375,53 @@ TEST(MsvcSdk, IncompleteSdkRootIsNotAnAnswer) { if (sdk) EXPECT_NE(sdk->root, t.root) << "an SDK-less root was accepted"; } +TEST(MsvcSdk, HeadersWithoutImportLibsIsNotAnAnswer) { + // The half that used to pass. `Include//ucrt/corecrt.h` is there and + // `Lib/` is not, which is exactly what a managed windows-sdk payload + // looked like when its um-libs MSI had not unpacked: every TU compiled + // and the link died with + // LINK : fatal error LNK1104: cannot open file 'kernel32.lib' + // An SDK is headers AND libraries; reporting "found" for half of one puts + // this root ahead of the machine's complete SDK and breaks a build that + // would otherwise have worked. + NoSdkEnv clean; + FakeToolset t{"sdkheadersonly"}; + t.add_sdk_headers_only("10.0.26100.0"); + std::array roots{t.root}; + auto sdk = msvc::find_windows_sdk(roots); + if (sdk) EXPECT_NE(sdk->root, t.root) + << "a headers-only SDK was accepted; the link would fail on kernel32.lib"; +} + +TEST(MsvcSdk, ACompleteRootIsStillAccepted) { + // The other direction, so the check above cannot be satisfied by + // rejecting everything. + NoSdkEnv clean; + FakeToolset t{"sdkcomplete"}; + t.add_sdk("10.0.26100.0"); + std::array roots{t.root}; + auto sdk = msvc::find_windows_sdk(roots); + ASSERT_TRUE(sdk) << "a complete SDK root was rejected"; + EXPECT_EQ(sdk->root, t.root); + EXPECT_EQ(sdk->version, "10.0.26100.0"); +} + +TEST(MsvcSdk, APartialRootYieldsToACompleteOne) { + // Ordering, not just acceptance: given both, the usable one must win even + // though the partial one carries the HIGHER version — which is how the + // managed payload outranked the system SDK in the first place. + NoSdkEnv clean; + FakeToolset partial{"sdkpartialhigh"}; + partial.add_sdk_headers_only("10.0.99999.0"); + FakeToolset complete{"sdkcompletelow"}; + complete.add_sdk("10.0.26100.0"); + std::array roots{partial.root, complete.root}; + auto sdk = msvc::find_windows_sdk(roots); + ASSERT_TRUE(sdk); + EXPECT_EQ(sdk->root, complete.root) + << "the partial root won on version; it cannot link"; +} + TEST(MsvcSdk, DeclaredRootOutranksTheManagedOne) { // Both available. WindowsSdkDir is someone saying which one to use, and // it has to win — the same precedence VSINSTALLDIR has over vswhere. From 29028a614f5801a15144508d8879749674c869a4 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 19:13:42 +0800 Subject: [PATCH 03/11] fix(toolchain): removing a payload could not survive Windows `toolchain remove` called remove_all and reported whatever it got. On Windows that is "Access is denied", from either of two causes that look identical: - payload files come out of .vsix/.msi carrying the read-only attribute. POSIX only needs the DIRECTORY writable to unlink a child, so this never appears on Linux or macOS -- and every unit test runs there. - a /Zi build leaves mspdbsrv.exe alive for a few seconds INSIDE the payload it is being asked to delete. Handles both rather than betting on one: clear the write bit across the tree and retry, then allow a bounded window (10 x 300ms) for a live process to exit. Bounded because `toolchain remove` must not hang on a directory something holds forever. The error now names the file it is stuck on. "Access is denied" without a path is not something a user can act on. No unit test: on the only platform CI would run one, removing a read-only file succeeds with or without this change, so the test could not fail. e2e 239 on the Windows runner is the gate -- it is where this surfaced, after the SDK fix let the build get far enough to reach the remove step. --- CHANGELOG.md | 11 ++++++ src/toolchain/lifecycle.cppm | 71 ++++++++++++++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0405e5fa..216d2444 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ ### 修复 +- **Windows 上 `mcpp toolchain remove ` 报 "Access is denied"。** + + `remove_all` 直接上,不清只读位、不重试、报错也不说是哪个文件。 + payload 是从 .vsix/.msi 解出来的,归档条目带只读属性;POSIX 只要**目录** + 可写就能 unlink 子项,所以这个问题在 Linux/macOS 上根本不出现。 + 另外 `/Zi` 构建之后 mspdbsrv.exe 还会在 payload 里活几秒。 + + 两个原因表现完全一样(都是 "Access is denied"),所以两个都处理: + 先清可写位重试,再给活着的进程一个**有上限**的等待窗口(10 × 300ms)。 + 报错现在会指出卡在哪个文件 —— 光一句 "Access is denied" 没法处理。 + - **半装的 Windows SDK 被当成装好的,链接到最后才炸。** `find_windows_sdk()` 认一个根的条件是 `Include\\ucrt\corecrt.h` diff --git a/src/toolchain/lifecycle.cppm b/src/toolchain/lifecycle.cppm index e0b82e70..0ce4cde0 100644 --- a/src/toolchain/lifecycle.cppm +++ b/src/toolchain/lifecycle.cppm @@ -208,6 +208,66 @@ void msvc_print_detected(const mcpp::toolchain::msvc::MsvcInstallation& inst, // headers much later. `has_usable_msvc()` exists for exactly this reason; // this is the same judgement applied to the managed origin, where the SDK // arrives as a package dependency and can therefore fail on its own. +// Delete a payload tree, coping with the two things that make a plain +// `remove_all` fail on Windows and on nothing else: +// +// read-only files — payloads are unpacked from .vsix/.msi, and archive +// entries carry the attribute through. POSIX only needs the DIRECTORY +// writable to unlink a child, so this never shows up on Linux or macOS. +// a lingering handle — a build with /Zi leaves mspdbsrv.exe running for a +// few seconds after cl.exe exits, and it lives inside the payload. +// +// Both surface as the same "Access is denied", which is why this handles both +// rather than picking one: clear the attribute, then give a live process a +// bounded moment to go away. Retries are capped and short — `toolchain +// remove` should not hang because something holds the directory forever. +bool remove_payload_tree(const std::filesystem::path& root, + std::error_code& ec) { + std::filesystem::remove_all(root, ec); + if (!ec) return true; + + // Second pass: make everything writable, then try again. + std::error_code ignore; + for (auto it = std::filesystem::recursive_directory_iterator( + root, std::filesystem::directory_options::skip_permission_denied, + ignore); + it != std::filesystem::recursive_directory_iterator{}; it.increment(ignore)) { + std::filesystem::permissions(it->path(), std::filesystem::perms::owner_write, + std::filesystem::perm_options::add, ignore); + } + for (int attempt = 0; attempt < 10; ++attempt) { + ec.clear(); + std::filesystem::remove_all(root, ec); + if (!ec) return true; + std::this_thread::sleep_for(std::chrono::milliseconds{300}); + } + return !std::filesystem::exists(root, ignore); +} + +// The first entry that is still there after a failed removal. The error code +// alone says "Access is denied" and not by whom or to what, which is the +// difference between a report someone can act on and one they cannot. +// +// It probes by trying to delete, and keeps whatever it manages to delete -- +// acceptable only because the caller has already failed a remove_all and the +// tree is being torn down anyway. Do not call it on a tree meant to survive. +std::optional +first_undeletable(const std::filesystem::path& root) { + std::error_code ec; + if (!std::filesystem::exists(root, ec)) return std::nullopt; + for (auto it = std::filesystem::recursive_directory_iterator( + root, std::filesystem::directory_options::skip_permission_denied, ec); + it != std::filesystem::recursive_directory_iterator{}; it.increment(ec)) { + if (ec) break; + if (it->is_regular_file(ec)) { + std::error_code rm; + std::filesystem::remove(it->path(), rm); + if (rm) return it->path(); + } + } + return root; +} + void msvc_warn_if_sdk_missing(const mcpp::toolchain::msvc::MsvcInstallation& inst) { auto roots = mcpp::toolchain::msvc::sibling_sdk_roots(inst.clPath); if (auto sdk = mcpp::toolchain::msvc::find_windows_sdk(roots)) { @@ -841,9 +901,14 @@ export int toolchain_remove(const mcpp::config::GlobalConfig& cfg, mcpp::ui::error(std::format("{} is not installed", spec)); return 1; } - std::filesystem::remove_all(installDir, ec); - if (ec) { - mcpp::ui::error(std::format("remove failed: {}", ec.message())); + if (!remove_payload_tree(installDir, ec)) { + mcpp::ui::error(std::format( + "remove failed: {}{}", ec.message(), + first_undeletable(installDir) + .transform([](const std::filesystem::path& p) { + return std::format("\n stuck at: {}", p.string()); + }) + .value_or(std::string{}))); return 1; } mcpp::ui::status("Removed", spec); From 1578edae6e1f43e57e40d532ebbd43b9d80a207c Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 19:30:55 +0800 Subject: [PATCH 04/11] fix(toolchain): a failed remove is not a no-op, and must not deepen the damage Two corrections to the removal path, both found by using it. The diagnostic worked -- it named `bin\Hostx64\x64\Microsoft.VisualStudio.Telemetry.dll`, which is neither a read-only file nor mspdbsrv but `vctip.exe`, the background telemetry uploader cl.exe spawns, holding a DLL inside the payload it is being asked to delete. Nothing on this side can delete a file another process holds open, so the real fix is openxlings/xim-pkgindex#637 (stop installing vctip.exe at all). What stays here is the generic fallback and an error someone can act on. 1. The probe no longer deletes. It was finding the stuck file by trying to remove each entry, which makes a diagnostic into a second act of damage. `remove_all` has already deleted everything it could, so the first SURVIVING file is the one that blocked it -- no destruction required. 2. The error now says the payload is INCOMPLETE. `remove_all` deletes what it can before stopping, so a failed remove leaves a toolchain with holes. "remove failed" alone reads as "nothing happened", and the next thing that someone meets is a build error. --- CHANGELOG.md | 10 ++++++++++ src/toolchain/lifecycle.cppm | 28 +++++++++++++++++++--------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 216d2444..8e6a957a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,16 @@ 先清可写位重试,再给活着的进程一个**有上限**的等待窗口(10 × 300ms)。 报错现在会指出卡在哪个文件 —— 光一句 "Access is denied" 没法处理。 + 这个诊断当场就派上用场了:它点名的是 + `bin\Hostx64\x64\Microsoft.VisualStudio.Telemetry.dll` —— 既不是只读位 + 也不是 mspdbsrv,而是 cl.exe 拉起的后台遥测进程 `vctip.exe` 占着它。 + **占用者不是 mcpp 能删掉的东西**,真正的修复在 payload 那边 + (openxlings/xim-pkgindex#637 不再安装 vctip.exe);这边留下的是通用兜底 + 和那句能读懂的报错。 + + 报错同时会说清楚:**失败的 remove 不是空操作** —— `remove_all` 会先删掉 + 能删的,所以剩下的是一个有洞的工具链,得重来一次而不是接着用。 + - **半装的 Windows SDK 被当成装好的,链接到最后才炸。** `find_windows_sdk()` 认一个根的条件是 `Include\\ucrt\corecrt.h` diff --git a/src/toolchain/lifecycle.cppm b/src/toolchain/lifecycle.cppm index 0ce4cde0..edc7e386 100644 --- a/src/toolchain/lifecycle.cppm +++ b/src/toolchain/lifecycle.cppm @@ -248,9 +248,11 @@ bool remove_payload_tree(const std::filesystem::path& root, // alone says "Access is denied" and not by whom or to what, which is the // difference between a report someone can act on and one they cannot. // -// It probes by trying to delete, and keeps whatever it manages to delete -- -// acceptable only because the caller has already failed a remove_all and the -// tree is being torn down anyway. Do not call it on a tree meant to survive. +// It does not probe by deleting. `remove_all` has already removed everything +// it could, so whatever SURVIVED is exactly what blocked it -- reporting the +// first survivor needs no further destruction. (An earlier version did probe +// by deleting, which turns a diagnostic into a second act of damage on a +// payload the caller may well want to keep and retry.) std::optional first_undeletable(const std::filesystem::path& root) { std::error_code ec; @@ -259,11 +261,7 @@ first_undeletable(const std::filesystem::path& root) { root, std::filesystem::directory_options::skip_permission_denied, ec); it != std::filesystem::recursive_directory_iterator{}; it.increment(ec)) { if (ec) break; - if (it->is_regular_file(ec)) { - std::error_code rm; - std::filesystem::remove(it->path(), rm); - if (rm) return it->path(); - } + if (it->is_regular_file(ec)) return it->path(); } return root; } @@ -902,8 +900,20 @@ export int toolchain_remove(const mcpp::config::GlobalConfig& cfg, return 1; } if (!remove_payload_tree(installDir, ec)) { + // Say that the payload is now BROKEN, not merely that removal + // failed. `remove_all` deletes what it can before it stops, so a + // failed remove is not a no-op: what is left is a toolchain with + // holes in it, and someone who reads "remove failed" and moves on + // will meet those holes as a build error instead. mcpp::ui::error(std::format( - "remove failed: {}{}", ec.message(), + "remove failed: {}{}\n" + " The payload is now INCOMPLETE — files were deleted " + "before this one\n" + " blocked the rest. Close whatever holds it and run " + "the same command\n" + " again; do not build with this toolchain until it " + "removes cleanly.", + ec.message(), first_undeletable(installDir) .transform([](const std::filesystem::path& p) { return std::format("\n stuck at: {}", p.string()); From 418e9c90022c01056b39fc34688b41e389387bc6 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 15:30:07 +0800 Subject: [PATCH 05/11] test(e2e): 239 could pass or skip, never fail The skip was decided AFTER the install by pattern-matching the failure text, and one of the patterns was `*"index"*`. Nearly every mcpp command prints "package index" somewhere, so EVERY genuine install failure took the skip branch. It hid a real one, on the first Windows run where the package was actually installable: tar: Cannot connect to C: resolve failed tar -xf "C:\Users\...\.payloads\Microsoft.VC...vsix" -C "..." [error] msvc installed but registered none of the programs it declares GNU tar reads `C:` as a hostname. The install ran for 135 seconds, failed, and this script printed PASS. Skip is now decided BEFORE the work, by a positive check for what would make the test impossible (no msvc row in `toolchain list`). Everything after that is a failure, and the output is printed rather than folded into a one-line message. The recipe-side fix is openxlings/xim-pkgindex#632. A skip decided by the shape of a failure is not a skip; it is a way of not looking. --- tests/e2e/239_msvc_managed_toolset.sh | 30 ++++++++++++++++++++------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/tests/e2e/239_msvc_managed_toolset.sh b/tests/e2e/239_msvc_managed_toolset.sh index af90b6cb..e68550b4 100755 --- a/tests/e2e/239_msvc_managed_toolset.sh +++ b/tests/e2e/239_msvc_managed_toolset.sh @@ -36,16 +36,30 @@ restore() { trap restore EXIT cd "$TMP" -# 1) install it. This runs BEFORE the discoverability check below, so that a -# runner which cannot reach the index skips instead of failing an assertion -# about a list the index would have filled in. +# 0) Decide SKIP here, from a positive check, and never again. +# +# ⚠️ This used to be decided AFTER the install by pattern-matching the +# failure text, and one of the patterns was `*"index"*`. Nearly every mcpp +# command prints "package index" somewhere, so every genuine install +# failure took the skip branch: the test could pass or skip, never fail. +# +# It hid a real one. `tar -xf "C:\...vsix"` fails under GNU tar, which +# reads `C:` as a hostname ("Cannot connect to C: resolve failed"); the +# install ran for 135 seconds, failed, and this script reported PASS. +# +# A skip has to be decided by what is ABSENT before the work starts, not by +# what the failure looked like afterwards. +if ! "$MCPP" toolchain list 2>&1 | grep -qi "msvc"; then + echo "SKIP: this index offers no msvc toolset (offline runner?)" + exit 0 +fi + +# 1) install it. Any failure from here on is a FAILURE. rc=0; out=$("$MCPP" toolchain install msvc "$TOOLSET" 2>&1) || rc=$? if [[ $rc -ne 0 ]]; then - case "$out" in - *"index"*|*"network"*|*"resolve"*|*"offline"*|*"connect"*|*"not found"*) - echo "SKIP: xim:msvc@$TOOLSET unreachable: $out"; exit 0 ;; - *) echo "FAIL: install msvc $TOOLSET: $out"; exit 1 ;; - esac + echo "FAIL: install msvc $TOOLSET (rc=$rc):" + echo "$out" + exit 1 fi [[ "$out" == *"$TOOLSET"* ]] \ || { echo "FAIL: install did not report the toolset: $out"; exit 1; } From 130ec32f9ddda963ba69711996364131f9e3c9c8 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 19:47:39 +0800 Subject: [PATCH 06/11] test(e2e): 239 asks for the toolset it needs, and checks the SDK arrived Two follow-ups to making 239 unable to skip its way to green. The skip grepped `toolchain list` for "msvc" -- the FAMILY. During an index publish window the family is listed while this toolset is not yet, so a hard failure would report a timing artifact as a defect. It now greps for $TOOLSET, which is the question the skip actually needs answered. And it now asserts the install reported a Windows SDK. `install` prints `windows sdk: ()`, but this script discarded the output on success -- so a half-installed SDK dependency said nothing here and turned up 100 lines later as LINK : fatal error LNK1104: cannot open file 'kernel32.lib' with nothing in the log naming the SDK. That is exactly how this defect presented today. Asserting it at the step that knows turns "the link failed for an unstated reason" into "the SDK dependency did not install". --- tests/e2e/239_msvc_managed_toolset.sh | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/e2e/239_msvc_managed_toolset.sh b/tests/e2e/239_msvc_managed_toolset.sh index e68550b4..93bffec6 100755 --- a/tests/e2e/239_msvc_managed_toolset.sh +++ b/tests/e2e/239_msvc_managed_toolset.sh @@ -49,8 +49,13 @@ cd "$TMP" # # A skip has to be decided by what is ABSENT before the work starts, not by # what the failure looked like afterwards. -if ! "$MCPP" toolchain list 2>&1 | grep -qi "msvc"; then - echo "SKIP: this index offers no msvc toolset (offline runner?)" +# Ask for THIS toolset, not the family. Grepping for "msvc" is true during +# an index publish window -- the family is listed, this version is not yet +# -- and a hard failure then reports a timing artifact as a defect. The +# question the skip needs answered is "can this index give me $TOOLSET", +# so that is the question to ask. +if ! "$MCPP" toolchain list 2>&1 | grep -q "$TOOLSET"; then + echo "SKIP: this index does not offer msvc $TOOLSET (publish window? offline?)" exit 0 fi @@ -64,6 +69,17 @@ fi [[ "$out" == *"$TOOLSET"* ]] \ || { echo "FAIL: install did not report the toolset: $out"; exit 1; } +# 1a) the SDK must have arrived with it. `msvc_warn_if_sdk_missing()` prints +# `windows sdk: ()` on success, and this script used to +# swallow the install output whenever the install exited 0 -- so a payload +# whose SDK dependency was half-installed said nothing here and failed +# ~100 lines later as +# LINK : fatal error LNK1104: cannot open file 'kernel32.lib' +# with no line in the log naming the SDK. Assert at the step that knows. +[[ "$out" == *"windows sdk:"* ]] \ + || { echo "FAIL: toolset installed but no Windows SDK was reported:"; \ + echo "$out"; exit 1; } + # 1b) it must now be LISTED. A toolset that installs but never appears is # indistinguishable from one that did not install, and `toolchain list` is # where a user looks. From 7d281c613c3d740fcf4863964652952efde33aed Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 20:16:28 +0800 Subject: [PATCH 07/11] fix(toolchain): a payload nothing can delete can still be moved With vctip.exe gone the holder became mspdbcore.dll -- that is mspdbsrv.exe, the PDB server a /Zi build spawns, which outlives cl.exe by tens of seconds and lives INSIDE the payload being removed. Removing a toolset right after building with it is the normal case, not a corner one. Waiting it out is not a fix: any timeout is a guess, and a CLI that hangs on one is worse than the failure. So when the retries are exhausted the payload is RENAMED aside instead. Windows refuses to delete a directory containing an open file and permits renaming one -- the handle keeps working and follows. What `toolchain remove` promises is that the toolchain stops being installed, and after the rename it has. The bytes are swept by the next lifecycle command, by which time nothing holds them; `sweep_parked_payloads` runs before both install and remove. Three tests, all run against the missing fallback. A tree with an unenumerable subdirectory is the portable way to make remove_all fail -- the CAUSE differs per platform (open handle on Windows, permissions here), the contract does not. The third asserts an ordinary payload is still just deleted: a fallback that fires always is not a fallback, and it is the only one of the three that passes without this change. --- CHANGELOG.md | 6 ++ src/toolchain/lifecycle.cppm | 56 +++++++++++- tests/unit/test_toolchain_lifecycle.cpp | 114 ++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 2 deletions(-) create mode 100644 tests/unit/test_toolchain_lifecycle.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e6a957a..268f2034 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,12 @@ 报错同时会说清楚:**失败的 remove 不是空操作** —— `remove_all` 会先删掉 能删的,所以剩下的是一个有洞的工具链,得重来一次而不是接着用。 + 修完 vctip 之后占用者换成了 `mspdbcore.dll` —— `/Zi` 构建拉起的 + **mspdbsrv.exe**,它构建完还活几十秒,而且就住在正要被删的 payload 里。 + 等它不现实(等多久都是猜),所以改成**挪走**:Windows 拒绝删除含打开文件的 + 目录,但允许**重命名**。`remove` 的承诺是"这个工具链不再装着",改名之后 + 它确实不再装着;剩下的字节在下一条生命周期命令里清扫(那时占用者早退了)。 + - **半装的 Windows SDK 被当成装好的,链接到最后才炸。** `find_windows_sdk()` 认一个根的条件是 `Include\\ucrt\corecrt.h` diff --git a/src/toolchain/lifecycle.cppm b/src/toolchain/lifecycle.cppm index edc7e386..f0b15e5d 100644 --- a/src/toolchain/lifecycle.cppm +++ b/src/toolchain/lifecycle.cppm @@ -221,7 +221,7 @@ void msvc_print_detected(const mcpp::toolchain::msvc::MsvcInstallation& inst, // rather than picking one: clear the attribute, then give a live process a // bounded moment to go away. Retries are capped and short — `toolchain // remove` should not hang because something holds the directory forever. -bool remove_payload_tree(const std::filesystem::path& root, +export bool remove_payload_tree(const std::filesystem::path& root, std::error_code& ec) { std::filesystem::remove_all(root, ec); if (!ec) return true; @@ -241,7 +241,51 @@ bool remove_payload_tree(const std::filesystem::path& root, if (!ec) return true; std::this_thread::sleep_for(std::chrono::milliseconds{300}); } - return !std::filesystem::exists(root, ignore); + if (!std::filesystem::exists(root, ignore)) return true; + + // Still held. Move it out of the way instead of waiting on a process we + // do not own. + // + // Windows refuses to DELETE a directory containing an open file, but it + // will RENAME one — the open handle keeps working and follows the new + // name. So the toolchain can leave the place it occupied even while + // mspdbsrv.exe is still holding mspdbcore.dll inside it, which is the + // normal state of affairs for the first ~seconds after a /Zi build with + // the very toolset being removed. + // + // This is the difference between a `remove` that works and one that asks + // the user to guess how long to wait: what they asked for is that the + // toolchain stop being installed, and after the rename it is. The bytes + // are swept on the next lifecycle command, by which time nothing holds + // them. + auto parked = root.parent_path() / + std::format(".trash-{}-{}", root.filename().string(), + std::chrono::steady_clock::now() + .time_since_epoch().count()); + std::error_code ren; + std::filesystem::rename(root, parked, ren); + if (ren) return false; // could not even move it — real failure + + std::filesystem::remove_all(parked, ignore); // usually works; fine if not + ec.clear(); + return true; +} + +// Delete `.trash-*` left behind by a removal that had to park a held payload. +// Takes the directory the payload lived IN (`/xim-x-`), which is +// where park() puts them -- one place, one level, nothing to walk. +// +// Best-effort and run before a lifecycle operation rather than after one: by +// the next command the process that held the bytes is normally gone, so this +// is where they actually get freed. +export void sweep_parked_payloads(const std::filesystem::path& pkgRoot) { + std::error_code ec; + if (!std::filesystem::is_directory(pkgRoot, ec)) return; + for (auto& e : std::filesystem::directory_iterator(pkgRoot, ec)) { + if (!e.is_directory(ec)) continue; + if (e.path().filename().string().starts_with(".trash-")) + std::filesystem::remove_all(e.path(), ec); + } } // The first entry that is still there after a failed removal. The error code @@ -658,6 +702,13 @@ export int toolchain_install(const mcpp::config::GlobalConfig& cfg, } mcpp::log::verbose("toolchain", std::format("installing main: {}", pkg.target())); + // A previous `remove` may have parked a held payload beside this one; + // by now whatever held it has exited, so free the bytes before adding + // another few hundred MB. + sweep_parked_payloads( + mcpp::xlings::paths::xim_tool(mcpp::config::make_xlings_env(cfg), + pkg.ximName, pkg.ximVersion) + .parent_path()); auto payload = fetcher.resolve_xpkg_path(pkg.target(), /*autoInstall=*/true, &progress); mcpp::log::verbose("toolchain", std::format("main install result: {}", payload ? ("ok → " + payload->root.string()) : payload.error().message)); @@ -899,6 +950,7 @@ export int toolchain_remove(const mcpp::config::GlobalConfig& cfg, mcpp::ui::error(std::format("{} is not installed", spec)); return 1; } + sweep_parked_payloads(installDir.parent_path()); if (!remove_payload_tree(installDir, ec)) { // Say that the payload is now BROKEN, not merely that removal // failed. `remove_all` deletes what it can before it stops, so a diff --git a/tests/unit/test_toolchain_lifecycle.cpp b/tests/unit/test_toolchain_lifecycle.cpp new file mode 100644 index 00000000..4bf29468 --- /dev/null +++ b/tests/unit/test_toolchain_lifecycle.cpp @@ -0,0 +1,114 @@ +#include + +import std; +import mcpp.toolchain.lifecycle; + +using namespace mcpp::toolchain; + +namespace { + +// A payload tree with one subdirectory that cannot be enumerated, which is +// the portable way to make `remove_all` fail. On Windows the same failure +// arrives via an open handle (mspdbsrv.exe holding mspdbcore.dll); the CAUSE +// differs per platform, the contract this fixture pins does not. +struct UnremovableTree { + std::filesystem::path parent; + std::filesystem::path root; + std::filesystem::path blocked; + + UnremovableTree() { + parent = std::filesystem::temp_directory_path() + / std::format("mcpp-rm-{}", + std::chrono::steady_clock::now() + .time_since_epoch().count()); + root = parent / "14.44.35207"; + blocked = root / "bin" / "locked"; + std::filesystem::create_directories(blocked); + std::ofstream{blocked / "mspdbcore.dll"} << "held"; + std::filesystem::permissions(blocked, std::filesystem::perms::none); + } + ~UnremovableTree() { + std::error_code ec; + std::filesystem::permissions(blocked, std::filesystem::perms::all, ec); + for (auto& e : std::filesystem::directory_iterator(parent, ec)) { + std::filesystem::permissions(e.path() / "bin" / "locked", + std::filesystem::perms::all, ec); + } + std::filesystem::remove_all(parent, ec); + } + UnremovableTree(const UnremovableTree&) = delete; + UnremovableTree& operator=(const UnremovableTree&) = delete; + + std::vector parked() const { + std::vector out; + std::error_code ec; + for (auto& e : std::filesystem::directory_iterator(parent, ec)) + if (e.path().filename().string().starts_with(".trash-")) + out.push_back(e.path()); + return out; + } +}; + +} // namespace + +TEST(ToolchainRemove, AHeldPayloadStillLeavesItsLocation) { + // What `toolchain remove` promises is that the toolchain stops being + // installed — not that every byte is already gone. A payload whose files + // are still open cannot be deleted, but it CAN be moved: Windows refuses + // to delete a directory containing an open file and allows renaming one. + // + // Before this, remove reported "Access is denied" and left the toolchain + // exactly where it was, seconds after a build with that same toolset — + // which is the normal case, not a corner one, because /Zi leaves + // mspdbsrv.exe running inside the payload being removed. + UnremovableTree t; + std::error_code ec; + + ASSERT_FALSE(std::filesystem::remove_all(t.root, ec) && !ec) + << "fixture is not actually unremovable; the test would prove nothing"; + + ec.clear(); + EXPECT_TRUE(remove_payload_tree(t.root, ec)) + << "a held payload was reported as un-removable"; + EXPECT_FALSE(std::filesystem::exists(t.root)) + << "the toolchain is still at the path it was removed from"; +} + +TEST(ToolchainRemove, ParkedBytesAreSweptOnceNothingHoldsThem) { + // The other half of the promise: parking is a deferral, not a leak. The + // next lifecycle command sweeps, and by then the process that held the + // files has exited — modelled here by dropping the permission block. + UnremovableTree t; + std::error_code ec; + remove_payload_tree(t.root, ec); + + auto parked = t.parked(); + ASSERT_EQ(parked.size(), 1u) << "expected exactly one parked payload"; + + std::filesystem::permissions(parked[0] / "bin" / "locked", + std::filesystem::perms::all, ec); + sweep_parked_payloads(t.parent); + EXPECT_TRUE(t.parked().empty()) << "parked payload was never swept"; +} + +TEST(ToolchainRemove, AnOrdinaryPayloadIsJustDeleted) { + // The common path must not grow a `.trash-` directory: parking is the + // fallback, and a fallback that fires always is not a fallback. + auto dir = std::filesystem::temp_directory_path() + / std::format("mcpp-rm-ok-{}", + std::chrono::steady_clock::now() + .time_since_epoch().count()); + std::filesystem::create_directories(dir / "14.44.35207" / "bin"); + std::ofstream{dir / "14.44.35207" / "bin" / "cl.exe"} << "not a compiler"; + + std::error_code ec; + EXPECT_TRUE(remove_payload_tree(dir / "14.44.35207", ec)); + EXPECT_FALSE(std::filesystem::exists(dir / "14.44.35207")); + + bool anyParked = false; + for (auto& e : std::filesystem::directory_iterator(dir, ec)) + anyParked |= e.path().filename().string().starts_with(".trash-"); + EXPECT_FALSE(anyParked) << "a deletable payload was parked instead of deleted"; + + std::filesystem::remove_all(dir, ec); +} From e6f117ce96330ed1198cb9e935dc78bd4044d6c2 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 20:19:07 +0800 Subject: [PATCH 08/11] ci: the xlings-managed msvc toolset gets its own workflow `msvc@system` and `msvc@` are different subjects and were sharing a job. Everything MSVC in the main suite (95, 99, 177, 180, 182) tests mcpp against the machine's own Visual Studio -- mcpp's code and nothing else's. 239 tests mcpp against the xlings ECOSYSTEM: index, mirror, payload set, unpack recipe, most of it in another repository moving on its own schedule. Today that difference cost several cycles of reading "your change broke Windows" when what had actually happened was that a package index needed a fix. Three consequences, all of them reasons to split: - a red tick means different things, and mixed together it means neither; - ~380 MB of downloads next to 100+ tests that take seconds each; - the index publish window makes this job flaky in a way the rest is not. Split by CAPABILITY, not by a file list: tests declare `# requires: xlings-msvc`, granted only by MCPP_E2E_XLINGS_MSVC=1, which only ci-windows-msvc-xlings.yml sets. The main suite therefore skips them by construction, and a new test joins the new job by declaring the capability -- there is no second list to drift. Adds E2E_ONLY to run_all.sh so a single-subject workflow can name what it runs. A filter that stops matching would otherwise produce a green tick for running nothing, which looks identical to passing, so the job asserts the glob still selects something. --- .github/workflows/ci-windows-msvc-xlings.yml | 98 ++++++++++++++++++++ tests/e2e/239_msvc_managed_toolset.sh | 4 +- tests/e2e/run_all.sh | 24 ++++- 3 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 .github/workflows/ci-windows-msvc-xlings.yml diff --git a/.github/workflows/ci-windows-msvc-xlings.yml b/.github/workflows/ci-windows-msvc-xlings.yml new file mode 100644 index 00000000..a363850e --- /dev/null +++ b/.github/workflows/ci-windows-msvc-xlings.yml @@ -0,0 +1,98 @@ +name: ci-windows-msvc-xlings + +# mcpp driving the xlings-MANAGED MSVC toolset (`msvc@`) — the whole +# chain, end to end: index → payload → unpack → resolve → build → run → +# remove. +# +# WHY THIS IS NOT IN ci-windows-e2e.yml, which also runs MSVC tests: +# +# Different subject. Everything MSVC in the main suite (95_msvc_system, +# 99_msvc_native_build, 177, 180, 182) tests mcpp against the machine's own +# Visual Studio. That is mcpp's code and nothing else's. This job tests +# mcpp against the xlings ECOSYSTEM — a package index, a mirror, a payload +# set, an unpack recipe — most of which lives in another repository and +# moves on its own schedule. +# +# Different failure meaning. When this job goes red it usually means the +# index moved, not that the pull request broke something. Mixed into the +# main suite that reads as "your change broke Windows", and the honest +# signal (100+ fast tests, all about mcpp) gets buried under one slow test +# about somebody else's package. Keeping them apart keeps both readable. +# +# Different cost. ~380 MB of downloads (xim:msvc + xim:windows-sdk) and a +# real toolchain install, against a suite whose other tests are seconds +# each. +# +# The split is enforced by a capability, not by a file list: the tests carry +# `# requires: xlings-msvc`, granted only by MCPP_E2E_XLINGS_MSVC=1 below. So +# the main suite skips them by construction, and a new test joins this job by +# declaring the capability — there is no second list to keep in sync. +# +# Paired workflows: ci-windows.yml, ci-windows-e2e.yml. + +on: + push: + branches: [ main ] + pull_request: + branches: [ main ] + workflow_dispatch: + +concurrency: + group: ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + xlings-msvc: + name: xlings-managed msvc toolset (windows x64, self-host) + runs-on: windows-latest + timeout-minutes: 45 + env: + MCPP_HOME: C:\Users\runneradmin\.mcpp + steps: + - uses: actions/checkout@v4 + - uses: ./.github/actions/bootstrap-mcpp + + - name: Build mcpp from source (self-host) + shell: bash + run: | + export MCPP_VENDORED_XLINGS="$XLINGS_BIN" + "$MCPP" build + # Newest mcpp.exe, not an arbitrary one — `target/` is restored from + # cache and keeps a directory per build fingerprint. Same reasoning + # as ci-windows-e2e.yml, where picking wrong ran the previous + # release's binary. + MCPP_SELF=$(find target -name "mcpp.exe" -path "*/bin/*" -printf "%T@ %p\n" \ + | sort -rn | head -1 | cut -d" " -f2-) + test -n "$MCPP_SELF" || { echo "FAIL: no mcpp.exe"; exit 1; } + MCPP_SELF=$(cd "$(dirname "$MCPP_SELF")" && pwd)/$(basename "$MCPP_SELF") + "$MCPP_SELF" --version + echo "MCPP_SELF=$MCPP_SELF" >> "$GITHUB_ENV" + + - name: xlings-managed msvc e2e + shell: bash + timeout-minutes: 30 + env: + # Grants the `xlings-msvc` capability. Without it these tests skip + # everywhere, which is exactly what the main suite wants. + MCPP_E2E_XLINGS_MSVC: '1' + # Name what this job runs, so the job title and its contents cannot + # drift apart. Widen the glob when a second test joins. + E2E_ONLY: '239_*.sh' + run: | + export MCPP="$MCPP_SELF" + export MCPP_VENDORED_XLINGS="$XLINGS_BIN" + export MCPP_E2E_TOOLCHAIN_MIRROR=GLOBAL + "$MCPP_SELF" self config --mirror GLOBAL + bash tests/e2e/run_all.sh + + # A run that matched nothing is a green tick for having done nothing, + # and it looks exactly like a run that passed. E2E_ONLY is a glob typed + # by hand; if it stops matching, say so here rather than in a report + # nobody reads. + - name: Fail if the filter selected no tests + if: always() + shell: bash + run: | + n=$(ls tests/e2e/239_*.sh 2>/dev/null | wc -l) + test "$n" -gt 0 || { echo "FAIL: E2E_ONLY matched no tests"; exit 1; } + echo "selected $n test(s)" diff --git a/tests/e2e/239_msvc_managed_toolset.sh b/tests/e2e/239_msvc_managed_toolset.sh index 93bffec6..d66b7402 100755 --- a/tests/e2e/239_msvc_managed_toolset.sh +++ b/tests/e2e/239_msvc_managed_toolset.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# requires: msvc -# 97_msvc_managed_toolset.sh — `msvc@`: the toolset the manifest +# requires: msvc xlings-msvc +# 239_msvc_managed_toolset.sh — `msvc@`: the toolset the manifest # names is the one that compiles, regardless of what this machine has. # # WHY THIS TEST CANNOT BE SATISFIED BY THE MACHINE'S COMPILER, which is the diff --git a/tests/e2e/run_all.sh b/tests/e2e/run_all.sh index 0ac88e08..8a7dad47 100755 --- a/tests/e2e/run_all.sh +++ b/tests/e2e/run_all.sh @@ -132,6 +132,21 @@ if ls "${MCPP_HOME}/registry/data/xpkgs/xim-x-llvm"/*/share/libc++/v1/std.cppm 2 CAPS+=(import-std-libcxx) fi +# xlings-msvc: mcpp driving the xlings-MANAGED toolset (`msvc@`) -- +# a different subject from `msvc`, which means "this machine has a Visual +# Studio". Opt-in rather than detected, for two reasons: +# +# - it downloads ~380 MB (xim:msvc + xim:windows-sdk) and installs a +# toolchain, so it does not belong in a suite whose other 100+ tests are +# seconds each; +# - its subject is the xlings ECOSYSTEM (index -> payload -> unpack -> +# build -> remove), so it fails for reasons that have nothing to do with +# the change under test, and mixing it in makes every unrelated PR look +# broken when the index moves. +# +# It runs in ci-windows-msvc-xlings.yml, which sets this and nothing else. +[[ "${MCPP_E2E_XLINGS_MSVC:-}" == "1" ]] && CAPS+=(xlings-msvc) + echo "Detected capabilities: ${CAPS[*]:-}" # --------------------------------------------------------------------------- @@ -149,7 +164,7 @@ echo "Detected capabilities: ${CAPS[*]:-}" # sync when adding a capability. KNOWN_CAPS=(elf fresh-sandbox gcc import-std-libcxx macos mingw-cross msvc musl nasm no-msvc pack patchelf python3 scan-deps symlink unix-shell - windows wine) + windows wine xlings-msvc) bad_tokens=0 for tf in "$HERE"/[0-9]*.sh; do @@ -268,7 +283,14 @@ if [[ -n "${E2E_SHARD:-}" ]]; then fi SHARD_POS=0 +# Optional name filter: E2E_ONLY="" runs just the matching tests. +# Used by the workflows that own ONE subject (see ci-windows-msvc-xlings.yml) +# so the job name and the thing it runs cannot drift apart. for test in "$HERE"/[0-9]*.sh; do + if [[ -n "${E2E_ONLY:-}" ]]; then + # shellcheck disable=SC2053 — glob match is the point + [[ "$(basename "$test")" == $E2E_ONLY ]] || continue + fi if (( SHARD_TOTAL > 1 )); then _mine=$(( (SHARD_POS % SHARD_TOTAL) + 1 )) SHARD_POS=$(( SHARD_POS + 1 )) From 93743f1acf1c86917d361112561e67f0d4feac4f Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 20:27:08 +0800 Subject: [PATCH 09/11] docs: architecture review of toolchain origins, selection and distribution Written from the round that actually got `msvc@` working: nine layers of defect, five index publish windows, three tests that could not fail. Every finding is anchored to a file:line, not a preference. The two-axis model (acquisition vs resolution) is right and nothing here proposes replacing it. The findings are all one sentence: that axis exists only for MSVC, and it is only carried half way. Ten findings, including three that are live defects rather than design debt: - doctor.cppm:398 still uses the pre-#436 `toolchain_frontend(root/"bin")` shape, so an installed msvc toolset is visible to `toolchain list` and invisible to `doctor`. One line. - a /MD build with a managed toolset links vcruntime140.dll, which is not an OS component and appears nowhere in src/ -- on a clean Windows box `mcpp build` succeeds and `mcpp run` cannot start. CI hides it by having Visual Studio installed. - `has_usable_msvc()` probes the machine but gates three decisions that also apply to managed toolsets, so a box with a pinned toolset and no VS answers "no MSVC here". And the largest structural gap: `mcpp pack` is ELF-only, and distribution.cppm's contract never reaches pack.cppm at all, so `cxx_runtime` has no enforcer at packaging time on either platform. --- ...026-08-16-toolchain-architecture-review.md | 421 ++++++++++++++++++ 1 file changed, 421 insertions(+) create mode 100644 .agents/docs/2026-08-16-toolchain-architecture-review.md diff --git a/.agents/docs/2026-08-16-toolchain-architecture-review.md b/.agents/docs/2026-08-16-toolchain-architecture-review.md new file mode 100644 index 00000000..a84cb6a0 --- /dev/null +++ b/.agents/docs/2026-08-16-toolchain-architecture-review.md @@ -0,0 +1,421 @@ +# 工具链架构 review:两种来源、选择与切换、构建与分发(2026-08-16) + +> 配套 `2026-08-16-msvc-as-a-managed-toolchain.md`(设计)与 +> `2026-08-16-msvc-ecosystem-cross-repo-plan.md`(跨仓库落地)。 +> 那两份讲**已经做了什么**;这份讲**现在的形状哪里还不对,以及怎么更优雅**。 +> +> 依据是把 `msvc@` 真正跑通的这一轮:九层缺陷、五次索引发布窗口、 +> 三处"不可能失败的测试"。下面每一条都指得出具体的 `file:line`, +> 不是风格偏好。 + +--- + +## 0. 结论先行 + +已经做对的那件事,是**把"获取"和"解析"拆成两条正交的轴**: + +| | 获取(哪来的) | 解析(怎么找到 cl.exe) | +|---|---|---| +| `msvc@system` | 探测这台机器 | `installation_from_tools_dir()` | +| `msvc@` | xim 装的 payload | **同一个** `installation_from_tools_dir()` | + +受管 toolset 因此不是第二条代码路径,长不出自己的 bug。**这个模型是对的, +下面所有建议都是把它贯彻得更彻底,没有一条要推翻它。** + +问题集中在一句话:**这条轴只对 MSVC 存在,而且只贯彻了一半。** + +--- + +## 1. `@system` 是一个通用概念,却被钉死在 MSVC 上 ⭐ 最高优先级 + +### 现状 + +`registry.cppm:428`: + +```cpp +bool is_system_toolchain(const ToolchainSpec& spec) { + return spec.family == Family::Msvc + && (spec.version.empty() || spec.version == "system"); +} +``` + +**族被写进了判据。** 于是 `gcc@system` / `llvm@system` 根本无法表达 —— +而"用这台机器自己的编译器"在 Linux 上是最常见的情形(发行版 gcc)。 +今天 mcpp 要求 gcc 必须走 xim 装一份。 + +代价不是"四处特判"这么轻。把两个文件扫一遍:**lifecycle.cppm 里 13 处、 +prepare.cppm 里 13 处** msvc 专有分支,而 gcc / llvm / mingw 三个族在 +lifecycle.cppm 里**一处都没有**(mingw 更是零分支 —— 它整个被表达成一个 +*target*,`x86_64-windows-gnu`,那才是这套设计想要的形状)。 + +其中直接由 `is_system_toolchain` 触发的四处: + +| 位置 | 内容 | +|---|---| +| `lifecycle.cppm:637` | `toolchain install` 拒绝安装 system | +| `lifecycle.cppm:848` | `toolchain default` 持久化 `"msvc@system"` **并清掉 `default_target`**(别的族没有这一步) | +| `lifecycle.cppm:931` | `toolchain remove` 拒绝 | +| `prepare.cppm:1260` | 构建期短路整条 xim 路径 | + +另外三处是同一条规则的**重复拼写**,值得单独记: + +- `xim_tool()` + `installation_at()` + 那段错误信息,在 + `prepare.cppm:1322-1341` 和 `lifecycle.cppm:730-755` **几乎逐字重复**。 + 第三份出现的时候就是下一个 #436。 +- sysroot 依赖规则有两份:`prepare.cppm:1471-1480`(只看 musl) + vs `lifecycle.cppm:694-702`(musl + PE + 宿主),**两者并不等价**, + 而 `lifecycle.cppm:692` 的注释声称它们互相对应。 +- `prepare.cppm:1002-1005` 那段注释说解析是 4 步,实际链路有 **9 个输入** + (manifest → `--toolchain` → 全局 default → Windows 无 VS 的 target 种子 → + `[target.X].toolchain` → 词表 pin → 四个互斥的获取分支 → MSVC ABI 修复门 → + `build.mcpp` 的宿主解析)。注释比代码老,而这段代码是**决定用哪个编译器**的。 + +`prepare.cppm:1257` 那个变量还叫 `tcSpecIsMsvc`,但它其实是 +`is_system_toolchain` 的结果 —— 名字说的是"族",值说的是"来源"。 +**名字和值不是一回事,这正是这一轮所有缺陷的形状。** + +### 建议 + +把 `@system` 提到 spec 层,对所有族成立: + +```cpp +// 来源由 VERSION 轴决定,与族无关。 +bool is_system_toolchain(const ToolchainSpec& spec) { + if (spec.version == "system") return true; + // 兼容:裸 `msvc` 历来就是"这台机器的 VS",保留。 + // 裸 `gcc` 历来是"装好的最新一个",不是 system —— 不能一起改。 + return spec.family == Family::Msvc && spec.version.empty(); +} +``` + +每个族再提供一个 `detect_system_installation()`。msvc 已经有了 +(`msvc::detect_installation()`);gcc/llvm 的探测能力在 +`toolchain/detect.cppm` 里基本齐了,是接线而不是新写。 + +**收益**:四处特判塌缩成一次分发;`gcc@system` 成为可表达的东西; +用户学一个概念而不是两个;msvc 不再是"那个特殊的族"。 + +**风险与边界**:裸 `gcc` 的语义**不能**跟着变(那会改变现有 manifest 的行为)。 +只有显式写 `@system` 才是新语义 —— 这条必须有测试钉住。 + +--- + +## 2. SDK 是"依赖",却被当成"搜索路径" ⭐ 高优先级 + +### 现状 + +编译器拿到了版本轴,**SDK 没有**。 + +`msvc.cppm:589` 的 `find_windows_sdk()` 按顺序扫: +`WindowsSdkDir` → 兄弟 xpkgs → 写死的 `C:\Program Files (x86)\Windows Kits\10`。 + +但对 `msvc@` 来说,SDK 是 recipe 里**声明过的依赖** +(`xim:windows-sdk@10.0.26100`),位置是确定的。确定的东西被拿去搜, +就会出现今天这两个后果: + +1. **半装的 payload 因为版本号更高,排在机器自己那套完整 SDK 前面**, + 每个 TU 都编过,最后炸在 `LNK1104: cannot open file 'kernel32.lib'`, + 而日志里没有一行提到 SDK。(已修:`pick()` 现在两半都要, + 但那是"别选错",不是"直接知道选哪个"。) +2. **manifest 无法钉 SDK 版本**。可以写 `msvc@14.44.35207`, + 不能写"配 10.0.26100 那套 SDK"。两台机器仍可能用不同 SDK 编同一份源码 —— + 正是版本轴当初要解决的那个问题,只是换到了下一层。 + +### 建议 + +对受管 toolset,**绑定**而不是搜索: + +``` +msvc@ → SDK 从同一条 xim 解析拿(和编译器同一个机制) +msvc@system → 维持现在的搜索链(机器上的东西只能靠找) +``` + +再给 manifest 一个可选的显式轴: + +```toml +[toolchain] +windows = "msvc@14.44.35207" +windows_sdk = "10.0.26100" # 可选;省略则用 toolset 声明的那个依赖 +``` + +**收益**:两条来源在**两半**上都对称(编译器 + SDK),而不是只在编译器这一半; +"声明什么就用什么"这句承诺覆盖整个工具链而不是它的一部分。 + +--- + +## 3. `cxx_runtime = "toolchain-coupled"` 对 MSVC 被拒,但工具链**确实带着**运行时 ⭐ 高优先级 + +### 现状 + +`distribution.cppm:432`: + +> `cxx_runtime = "toolchain-coupled"` has no meaning for the MSVC runtime +> (it ships with the OS/redistributable, **not with the toolchain**) + +这句话对 `msvc@system` 是对的,对 `msvc@` 是**错的**。 +受管 payload 里就有: + +``` +VC/Redist/MSVC/14.44.35112/x64/Microsoft.VC143.CRT/ + vcruntime140.dll msvcp140.dll msvcp140_2.dll vcruntime140_threads.dll +``` + +recipe 的 payload 集里明确包含 `Microsoft.VC..CRT.Redist.X64.base.vsix` +(`pkgs/m/msvc.lua:134`)—— **我们下载了它,然后一个字节都没用。** +而 VS 自身安装也带同样的目录,所以两条来源都能支持。 + +于是分发矩阵上出现一个洞: + +| | 自包含 | 宿主耦合 | 工具链耦合 | +|---|---|---|---| +| gcc / libstdc++ | 静态链接 | 用系统 libstdc++ | **拷 libstdc++ 到产物旁** ✅ | +| MSVC | `/MT` ✅ | `/MD` ✅ | **拒绝** ❌ | + +### 建议 + +PE 上实现 toolchain-coupled:把 `Microsoft.VC*.CRT/*.dll` 拷到产物旁边 —— +和 gcc 拷 libstdc++ 是同一件事、同一个语义。 + +**收益**:`/MD` 构建**可分发**,不再要求目标机装 VC++ 运行时; +三行分发契约在两个 ABI 上一致;已经付过的下载有了用处。 + +### 而且这不只是"少个功能",是 `mcpp run` 的一个潜在失败 + +默认是 `/MD`,产物链的是 `vcruntime140.dll` / `msvcp140.dll`。 +这两个**不是** Windows 自带的(自带的是 `ucrtbase.dll`,Win10 起进系统), +它们来自 VC++ 可再发行包。 + +而全仓库搜不到一处 `Redist` / `vcruntime140` / `msvcp140`: + +``` +$ grep -rn "Redist\|vcruntime140\|msvcp140" src/ +（无匹配） +``` + +也就是说:一台**只装了受管 toolset、没装过 VS 也没装过 redist** 的干净 Windows, +`mcpp build` 会成功,`mcpp run` 会以「找不到 vcruntime140.dll」失败。 +今天没暴露,是因为 CI runner 上装着 Visual Studio —— +**又一次「验收环境比目标环境富裕」**,和 §5 是同一个形状。 + +`runtimeLibraryDirs`(Windows 上就是 PATH,`env.cppm:163`)这条通道是现成的, +缺的只是把受管 payload 的 `VC/Redist/.../Microsoft.VC143.CRT` 放进去。 +所以这一条同时修两件事:**运行**(把 DLL 放上 PATH)和 +**分发**(把 DLL 拷到产物旁)。 + +**注意**:debug 的 `vcruntime140d.dll` 在 `debug_nonredist/` 下, +**不可再分发** —— 实现时必须只取 `x64/Microsoft.VC143.CRT/`,不能取 +`debug_nonredist/`。这一条要写进测试。 + +--- + +## 3b. `mcpp doctor` 里还留着 #436 修掉的那个 bug —— 现成的第四份拷贝 🔴 立刻可修 + +`doctor.cppm:398`: + +```cpp +auto bin = mcpp::toolchain::toolchain_frontend( + vEntry.path() / "bin", mcpp::toolchain::to_xim_package(s)); +if (bin.empty()) continue; // ← 装好的 msvc toolset 在这里被丢掉 +``` + +这就是 #436 修的那一条:cl.exe 在 `VC/Tools/MSVC//bin/Host//`, +深四层,`bin/` 下什么都没有 → `continue`。#436 把 +`toolchain list` 换成了 `payload_frontend(root, pkg, family)`, +**但 doctor 没跟着换。** + +后果:同一台机器上 `mcpp toolchain list` 看得见的 msvc toolset, +`mcpp doctor` 看不见 —— 两个都"成功",而且不一致。 + +**修法**:`doctor.cppm:398` 改用 `payload_frontend`。一行。 +这也是 §1 那句"布局规则被抄了第 N 份"的活证据:#436 消掉了第三份, +第四份一直在。 + +--- + +## 3c. `has_usable_msvc()` 把"这台机器有 VS"当成了"这里能用 MSVC" ⭐ 中高优先级 + +`msvc.cppm:660` 只探测机器: + +```cpp +return find_std_module_source().has_value() && find_windows_sdk().has_value(); +``` + +但它被用在三个对**受管 toolset 同样适用**的决策上: + +| 位置 | 决策 | +|---|---| +| `prepare.cppm:1097` | 首次运行时要不要把 Windows 用户导向 mingw | +| `prepare.cppm:1389` | 离线错误文案 | +| `prepare.cppm:1558` | MSVC ABI 修复门 | + +于是:**一台钉了 `msvc@14.44.35207`、但没装 Visual Studio 的机器, +这个判据回答 `false`** —— 明明工具链就在 payload 里躺着。 + +这和 §1 是同一个病:**判据问的是"族/机器",而答案取决于"来源"。** + +**建议**:拆成两个问题 —— `system_msvc_is_usable()`(现在这个)和 +`msvc_is_available(spec)`(system 就探测,受管就看 payload 在不在)。 +调用点各自选一个,而不是共用一个含糊的。 + +--- + +## 3d. Windows 上的"打包分发"是空的 ⭐ 高优先级(范围最大) + +`pack.cppm:637`: + +```cpp +#if defined(_WIN32) + // `mcpp pack` is not yet supported on Windows. +``` + +整个 `mcpp pack` 是 ELF 专用的:`LD_TRACE_LOADED_OBJECTS` / `ldd` / +`patchelf` / `tar`。Windows 上直接返回错误,让人去用 CI 的 zip。 + +而且**两套系统并不通话**: + +- `distribution.cppm` 决定的是**契约**(自包含 / 宿主耦合 / 工具链耦合), + 只影响编译链接**旗标**; +- `pack.cppm` 决定的是**真的拷哪些文件**,它**从不读 Contract**, + 也不认识工具链。 + +所以今天 `cxx_runtime` 这个契约在打包这一步是**没有执行者**的 —— +ELF 上靠 `ldd` 闭包歪打正着,PE 上则完全没有这一步。 + +**建议**(按性价比排序): + +1. **先让 §3 的 toolchain-coupled 落地**(把 VC CRT DLL 拷到产物旁)。 + 这一步不需要 `pack` —— 它属于构建产物布局,而且顺手修掉 §3 那个 + `mcpp run` 在干净机器上的失败。 +2. 再做 PE 版 `pack`:DLL 闭包用 `dumpbin /dependents` 或读 PE 导入表 + (后者无外部依赖,更符合这套代码库的口味),`is_system_lib()` 换成 + Windows 的系统 DLL 白名单(kernel32/user32/ucrtbase/…),打 zip 而不是 tar。 + 已有设计稿:`.agents/docs/2026-05-19-pack-windows-design.md`。 +3. 让 `pack` **读 Contract**:`--mode` 与 `cxx_runtime` 现在是两套词汇, + 讲的是同一件事。至少要在两者矛盾时报出来 + (`pack.cppm:192-229` 已经有这类拒绝的先例,可以照着长)。 + +--- + +## 4. `installed()` 的语义:必须是"这份 recipe 产出的状态" ⭐ 跨仓库规则 + +这一轮九层缺陷里,真正致命的几层全是这一条。 + +### 两条子规则 + +**(a) 它必须是覆盖,不是抽样。** 每条断言对应一个"只有它才提供"的 payload。 +`windows-sdk` 现在是这样做的:`gdi32.lib` 只在 Desktop Libs +(它的 365 个库和 Store Apps Libs 的 116 个**完全不相交**),所以哪个 payload +没到,报错就点名哪一个。 + +**(b) 它必须能表达"不该在什么"。** 版本号不会因为 recipe 改了就变, +所以 `installed()` 是**唯一**能把老机器拉回来的东西。#637 让 recipe 不再安装 +`vctip.exe`,但已经装了的机器一点没变 —— 直到 #639 让 `installed()` +检查它**不在**。 + +> 新增文件靠断言"它在"就能发现,**删掉的文件必须显式说"它不该在"**。 + +### 建议 + +1. 写进 `.agents/skills/xpkg-creater/SKILL.md`(规则 + 这两个反例)。 +2. 加一条 CI lint:`install()` 落 N 个 payload 的 recipe, + `required_files()` 至少要提到 N 个互不相同的路径。 + 便宜,而且正好挡住"覆盖退化成抽样"。 + +--- + +## 5. 不运行被测物的验收步骤,验的是解压 ⭐ 中优先级 + +索引侧 windows-test 做的是「装 → 检查 → 卸」,**全程不编译**。 +而 `vctip.exe` 是 `cl.exe` **运行时**才被拉起的 —— 不跑编译器就没有进程, +卸载自然成功。于是 msvc.lua 连续几个 PR 的 windows-test 全绿, +包括「post-uninstall checks」,而卸载对任何真正构建过东西的人都是坏的。 + +### 建议 + +工具链类包的 windows-test 增加一步:**编译并链接一个真程序**,再卸载。 +至少把包自己 `programs` 里声明的东西各跑一次。 +代价是几十秒,挡住的是"装得上但用不了"这整类缺陷 —— 这一轮里它出现了四次。 + +--- + +## 6. 卸载的健壮性只做在 mcpp 一侧 ⭐ 中优先级 + +`remove_payload_tree()`(`lifecycle.cppm:224`)现在会:清只读位 → 有上限重试 +→ **改名挪走** → 下次生命周期命令清扫。 + +但 `xlings remove msvc` 有**一模一样**的问题:Windows 不让删含打开文件的目录, +而 `/Zi` 构建留下的 `mspdbsrv.exe` 就住在 payload 里。 + +### 建议 + +把 park-and-sweep 下沉到 xlings 的卸载路径。谁装的谁卸,这个能力不该只有 +mcpp 有 —— 否则每个消费者都要自己重写一遍,而写错的方式很多 +(我第一版就是"靠试删来找占用者",诊断变成了第二次破坏)。 + +--- + +## 7. 索引发布窗口没有可观测的"就绪"信号 ⭐ 中优先级 + +今天撞了**五次**。「合入 → 索引 → 装得到」之间有个肉眼不可见的窗口, +而窗口期内的失败信息是 `not found` —— 和"这个包根本不存在"一模一样。 + +最难受的是第 3 次和第 5 次:它们让**已经修好**的缺陷看起来像没修好, +只有对着时间戳才分得清。 + +### 建议 + +索引发布一个**修订号**,消费者能等到 `>= rev`。 +退一步:至少让"刚发布、还没到"与"没有这个包"在报错里能区分开。 +`xlings` 已经有 `run \`xlings update\` if the package was just published` +这句提示 —— 方向是对的,只是 CI 里没有人能照做。 + +--- + +## 8. 已经做对、不要动的部分 + +写下来是为了避免后续 review 把它们"顺手改掉": + +- **两条正交的轴**(§0)。这是整个设计的承重墙。 +- **`msvc_wants_static_crt()` 单一推导**(`dialect.cppm:134`)。 + 三处调用(`flags.cppm:613`、`flags.cppm:851`、`prepare.cppm:5023`) + 传的都是**manifest 原始标量**,所以编译器收到的 `/MT` 和分发表说的 + 自包含不可能不一致。**不要**改成读解析后的 contract —— 那会让每个 + Windows 构建都翻成 `/MT`。 +- **payload 多地址 + 单一 sha256**。镜像不是第二个信任根,是同一份字节的 + 第二个地址。 +- **能力驱动的 e2e 拆分**(`ci-windows-msvc-xlings.yml`)。用能力而不是文件清单 + 分流,新测试靠声明 `# requires: xlings-msvc` 加入,没有第二份清单会走样。 + +--- + +## 9. 建议的落地顺序 + +| # | 项 | 规模 | 影响面 | 依赖 | +|---|---|---|---|---| +| 0 | **§3b doctor 用 `payload_frontend`** | **一行** | mcpp | 无。现成的 bug,先修 | +| 1 | §4 `installed()` 规则 + lint | 小 | xim-pkgindex | 无。最便宜,挡住的最多 | +| 2 | §5 索引 CI 真编译一次 | 小 | xim-pkgindex | 无 | +| 3 | §3 MSVC toolchain-coupled(含 `mcpp run` 的 PATH) | 中 | mcpp | 无。DLL 已在 payload 里 | +| 4 | §3c 拆开 `has_usable_msvc` | 小 | mcpp | 无 | +| 5 | §2 SDK 绑定 + 版本轴 | 中 | mcpp | 无 | +| 6 | §6 park-and-sweep 下沉 | 小 | xlings | 无 | +| 7 | §1 `@system` 通用化 | 大 | mcpp | 建议在 §2/§3c 之后 —— 都动 spec 层 | +| 8 | §3d PE 版 `pack` | 大 | mcpp | 排在 §3 之后:§3 先解决"能跑" | +| 9 | §7 索引修订号 | 大 | xlings + 索引 | 收益最分散 | + +第 0–4 项互不依赖,可以并行,合计不大。§1 和 §3d 是两块真正的工作量, +建议各自单独一轮 —— 尤其 §1 动的是 spec 层,改完之后其余几项的 diff 会更小。 + +**如果只做三件**:§3b(一行)、§4(规则,挡住的最多)、§3(让干净 Windows +上 `mcpp run` 真的能跑)。 + +--- + +## 10. 一句话 + +这一轮九层缺陷,没有一层是"写错了"。全都是**验收判据比"能用"弱**: +`installed()` 只查 cl.exe、只查目录;`find_windows_sdk()` 只查头文件; +索引 CI 从不编译;e2e 只能 pass 或 skip。 + +把判据改成"能用"之后,它们一层层自己冒出来了。 +上面七条建议,本质上是同一句话在七个位置的应用。 From 62bddef5bdad6c439f678219f503e52a3bd7cda9 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 20:41:00 +0800 Subject: [PATCH 10/11] fix(toolchain): move the held FILES aside, not the directory that holds them The park fallback was built on a wrong premise and CI said so. Windows lets you rename an open FILE -- that is how an updater replaces a running .exe -- but it does NOT let you rename a DIRECTORY containing one. Renaming the payload root failed with the same "Access is denied", so remove still reported failure and `mspdbcore.dll` was still named as the blocker. What works is the other way round: move every surviving file to a sibling `.trash-*` directory (allowed while open, same volume, the handle follows), then delete the payload tree, which by then holds only directories. If any file cannot be moved, the scratch directory is removed and the whole thing reports failure -- a half-moved payload is worse than one still fully there. Tests: the two that depended on a POSIX "unremovable" fixture are gone, and the reason is worth recording. The only POSIX way to make a file undeletable is to drop write on its parent, and this function's second pass ADDS write back across the tree on purpose -- that is the read-only-payload case it exists to fix. The fixture becomes removable the moment the code under test touches it. That is the function working, not a hole, and a test that cannot fail is the thing this whole round has been removing. e2e 239 on the Windows runner is the gate. What stays is what holds everywhere: the ordinary deletion takes the ordinary route, and the sweep eats `.trash-*` and not the installed toolset beside it. Also corrects the architecture review: its first draft proposed generalising `@system` to gcc/llvm, which is backwards. xlings' principle is to depend on the host as little as possible, and `msvc@system` is a Windows-specific exception rather than an instance of a general capability. The finding stands (one exception, 26 branches); the remedy is to CONTAIN it behind a single resolved Origin -- and to reject `gcc@system` explicitly rather than leave it merely unimplemented. --- ...026-08-16-toolchain-architecture-review.md | 68 ++++++--- src/toolchain/lifecycle.cppm | 74 ++++++--- tests/unit/test_toolchain_lifecycle.cpp | 142 ++++++------------ 3 files changed, 147 insertions(+), 137 deletions(-) diff --git a/.agents/docs/2026-08-16-toolchain-architecture-review.md b/.agents/docs/2026-08-16-toolchain-architecture-review.md index a84cb6a0..1a66353e 100644 --- a/.agents/docs/2026-08-16-toolchain-architecture-review.md +++ b/.agents/docs/2026-08-16-toolchain-architecture-review.md @@ -26,7 +26,7 @@ --- -## 1. `@system` 是一个通用概念,却被钉死在 MSVC 上 ⭐ 最高优先级 +## 1. `@system` 只对 MSVC 存在 —— 这是**对的**,但它的代价被摊在了 26 处 ⭐ ### 现状 @@ -39,9 +39,21 @@ bool is_system_toolchain(const ToolchainSpec& spec) { } ``` -**族被写进了判据。** 于是 `gcc@system` / `llvm@system` 根本无法表达 —— -而"用这台机器自己的编译器"在 Linux 上是最常见的情形(发行版 gcc)。 -今天 mcpp 要求 gcc 必须走 xim 装一份。 +**族被写进了判据。** + +> **先说清楚:这个限制本身是对的,不要改。** +> xlings 的原则是**能不依赖 host 就不依赖 host** —— 工具链应当由 xim 提供, +> 这样"声明什么就用什么"在每台机器上成立。`gcc@system` / `llvm@system` +> **不该**支持:那是把不确定性请回来。 +> +> `msvc@system` 是 **Windows 平台的特例**,不是通用能力的一个实例: +> Visual Studio 常常已经装在机器上、又不总是能被重新分发,所以必须能用它。 +> +> 我这份 review 的初稿建议"把 `@system` 通用化到所有族",**那是错的**, +> 方向正好反了。下面改成正确的问题。 + +真正的问题不是"为什么只有 MSVC 有",而是:**一个平台特例,凭什么要在 26 个 +地方各写一遍。** 代价不是"四处特判"这么轻。把两个文件扫一遍:**lifecycle.cppm 里 13 处、 prepare.cppm 里 13 处** msvc 专有分支,而 gcc / llvm / mingw 三个族在 @@ -74,29 +86,40 @@ lifecycle.cppm 里**一处都没有**(mingw 更是零分支 —— 它整个被 `is_system_toolchain` 的结果 —— 名字说的是"族",值说的是"来源"。 **名字和值不是一回事,这正是这一轮所有缺陷的形状。** -### 建议 +### 建议:**收拢这个特例,而不是推广它** + +保持 `@system` 只有 MSVC 能用(甚至值得在 `parse_toolchain_spec` 里 +**显式拒绝** `gcc@system`,并给一句"xlings 不依赖 host 工具链,用 +`gcc@`"—— 让原则**可执行**,而不是靠没人去写)。 -把 `@system` 提到 spec 层,对所有族成立: +要减的是**摊开的代价**,做法是把"来源"变成一个显式的、解析一次的东西: ```cpp -// 来源由 VERSION 轴决定,与族无关。 -bool is_system_toolchain(const ToolchainSpec& spec) { - if (spec.version == "system") return true; - // 兼容:裸 `msvc` 历来就是"这台机器的 VS",保留。 - // 裸 `gcc` 历来是"装好的最新一个",不是 system —— 不能一起改。 - return spec.family == Family::Msvc && spec.version.empty(); -} +enum class Origin { Managed, SystemMsvc }; // 只有两种,而且只会有两种 + +struct ResolvedToolchain { + Origin origin; + std::filesystem::path compiler; // payload 里的 cl/gcc,或机器上的 cl + // …… +}; ``` -每个族再提供一个 `detect_system_installation()`。msvc 已经有了 -(`msvc::detect_installation()`);gcc/llvm 的探测能力在 -`toolchain/detect.cppm` 里基本齐了,是接线而不是新写。 +`prepare` / `lifecycle` 各自解析一次 `Origin`,后面按它分发,而不是每个子命令 +自己再问一遍 `is_system_toolchain`。 -**收益**:四处特判塌缩成一次分发;`gcc@system` 成为可表达的东西; -用户学一个概念而不是两个;msvc 不再是"那个特殊的族"。 +配套把三处**重复拼写**消掉(这几处和特例本身无关,纯粹是复制): -**风险与边界**:裸 `gcc` 的语义**不能**跟着变(那会改变现有 manifest 的行为)。 -只有显式写 `@system` 才是新语义 —— 这条必须有测试钉住。 +1. `xim_tool()` + `installation_at()` + 错误信息 —— `prepare.cppm:1322-1341` + 与 `lifecycle.cppm:730-755` 几乎逐字重复 → 提一个 + `resolve_managed_msvc(env, pkg)`。 +2. sysroot 依赖规则两份且**不等价**(`prepare.cppm:1471-1480` vs + `lifecycle.cppm:694-702`)→ 合成一个谓词。 +3. `prepare.cppm:1002-1005` 的注释说 4 步、实际 9 个输入 → 要么补齐, + 要么把顺序变成一张数据表,让注释无处可撒谎。 + +**收益**:特例仍然是特例(符合原则),但只在**一处**被认出来; +26 处分支里绝大多数变成"按 Origin 分发"的普通代码。 +`gcc@system` 从"没实现"变成"**明确拒绝并说明理由**"。 --- @@ -399,13 +422,16 @@ mcpp 有 —— 否则每个消费者都要自己重写一遍,而写错的方式 | 4 | §3c 拆开 `has_usable_msvc` | 小 | mcpp | 无 | | 5 | §2 SDK 绑定 + 版本轴 | 中 | mcpp | 无 | | 6 | §6 park-and-sweep 下沉 | 小 | xlings | 无 | -| 7 | §1 `@system` 通用化 | 大 | mcpp | 建议在 §2/§3c 之后 —— 都动 spec 层 | +| 7 | §1 收拢 Origin + 消三处重复(**不**通用化 `@system`) | 大 | mcpp | 建议在 §2/§3c 之后 —— 都动 spec 层 | | 8 | §3d PE 版 `pack` | 大 | mcpp | 排在 §3 之后:§3 先解决"能跑" | | 9 | §7 索引修订号 | 大 | xlings + 索引 | 收益最分散 | 第 0–4 项互不依赖,可以并行,合计不大。§1 和 §3d 是两块真正的工作量, 建议各自单独一轮 —— 尤其 §1 动的是 spec 层,改完之后其余几项的 diff 会更小。 +⚠️ §1 的方向已修正:**收拢特例,不是推广它**。初稿建议的 `gcc@system` +与 xlings"能不依赖 host 就不依赖 host"的原则相悖,已作废。 + **如果只做三件**:§3b(一行)、§4(规则,挡住的最多)、§3(让干净 Windows 上 `mcpp run` 真的能跑)。 diff --git a/src/toolchain/lifecycle.cppm b/src/toolchain/lifecycle.cppm index f0b15e5d..57299e94 100644 --- a/src/toolchain/lifecycle.cppm +++ b/src/toolchain/lifecycle.cppm @@ -243,32 +243,60 @@ export bool remove_payload_tree(const std::filesystem::path& root, } if (!std::filesystem::exists(root, ignore)) return true; - // Still held. Move it out of the way instead of waiting on a process we + // Still held. Move the held FILES out instead of waiting on a process we // do not own. // - // Windows refuses to DELETE a directory containing an open file, but it - // will RENAME one — the open handle keeps working and follows the new - // name. So the toolchain can leave the place it occupied even while - // mspdbsrv.exe is still holding mspdbcore.dll inside it, which is the - // normal state of affairs for the first ~seconds after a /Zi build with - // the very toolset being removed. + // ⚠️ NOT by renaming the payload directory. Windows lets you rename an + // open FILE — that is how an updater replaces a running .exe — but it + // does NOT let you rename a DIRECTORY that contains one. An earlier + // version of this function renamed `root` and was wrong about exactly + // that: the rename failed with the same "Access is denied", and remove + // still reported failure. What follows is the shape that actually works: // - // This is the difference between a `remove` that works and one that asks - // the user to guess how long to wait: what they asked for is that the - // toolchain stop being installed, and after the rename it is. The bytes - // are swept on the next lifecycle command, by which time nothing holds - // them. - auto parked = root.parent_path() / - std::format(".trash-{}-{}", root.filename().string(), - std::chrono::steady_clock::now() - .time_since_epoch().count()); - std::error_code ren; - std::filesystem::rename(root, parked, ren); - if (ren) return false; // could not even move it — real failure - - std::filesystem::remove_all(parked, ignore); // usually works; fine if not + // move every surviving file to a sibling `.trash-*` directory (allowed + // even while open, same volume, the handle follows the file) + // then delete the payload tree, which now holds only directories + // + // This is what makes `remove` mean something after a /Zi build with the + // toolset being removed: mspdbsrv.exe lives INSIDE the payload and + // outlives cl.exe by tens of seconds, so "wait for it" is a guess and a + // CLI that hangs on a guess is worse than the failure. The bytes are + // swept by the next lifecycle command, when nothing holds them. + auto trash = root.parent_path() / + std::format(".trash-{}-{}", root.filename().string(), + std::chrono::steady_clock::now() + .time_since_epoch().count()); + std::vector survivors; + for (auto it = std::filesystem::recursive_directory_iterator( + root, std::filesystem::directory_options::skip_permission_denied, + ignore); + it != std::filesystem::recursive_directory_iterator{}; it.increment(ignore)) { + if (it->is_regular_file(ignore)) survivors.push_back(it->path()); + } + if (survivors.empty()) return false; // nothing to move; genuinely stuck + + std::filesystem::create_directories(trash, ignore); + // Flat, index-prefixed: two payload subdirectories can hold the same file + // name, and this directory exists to be deleted, not to be read. + std::size_t moved = 0; + for (std::size_t i = 0; i < survivors.size(); ++i) { + std::error_code ren; + std::filesystem::rename( + survivors[i], + trash / std::format("{}-{}", i, survivors[i].filename().string()), ren); + if (!ren) ++moved; + } + if (moved != survivors.size()) { + // Some file could not even be moved. Leave the rest where they are — + // a half-moved payload is worse than one that is still all there. + std::filesystem::remove_all(trash, ignore); + return false; + } + ec.clear(); - return true; + std::filesystem::remove_all(root, ec); // directories only now + std::filesystem::remove_all(trash, ignore); // usually works; fine if not + return !ec; } // Delete `.trash-*` left behind by a removal that had to park a held payload. @@ -297,7 +325,7 @@ export void sweep_parked_payloads(const std::filesystem::path& pkgRoot) { // first survivor needs no further destruction. (An earlier version did probe // by deleting, which turns a diagnostic into a second act of damage on a // payload the caller may well want to keep and retry.) -std::optional +export std::optional first_undeletable(const std::filesystem::path& root) { std::error_code ec; if (!std::filesystem::exists(root, ec)) return std::nullopt; diff --git a/tests/unit/test_toolchain_lifecycle.cpp b/tests/unit/test_toolchain_lifecycle.cpp index 4bf29468..1df9d74c 100644 --- a/tests/unit/test_toolchain_lifecycle.cpp +++ b/tests/unit/test_toolchain_lifecycle.cpp @@ -5,99 +5,33 @@ import mcpp.toolchain.lifecycle; using namespace mcpp::toolchain; -namespace { - -// A payload tree with one subdirectory that cannot be enumerated, which is -// the portable way to make `remove_all` fail. On Windows the same failure -// arrives via an open handle (mspdbsrv.exe holding mspdbcore.dll); the CAUSE -// differs per platform, the contract this fixture pins does not. -struct UnremovableTree { - std::filesystem::path parent; - std::filesystem::path root; - std::filesystem::path blocked; - - UnremovableTree() { - parent = std::filesystem::temp_directory_path() - / std::format("mcpp-rm-{}", - std::chrono::steady_clock::now() - .time_since_epoch().count()); - root = parent / "14.44.35207"; - blocked = root / "bin" / "locked"; - std::filesystem::create_directories(blocked); - std::ofstream{blocked / "mspdbcore.dll"} << "held"; - std::filesystem::permissions(blocked, std::filesystem::perms::none); - } - ~UnremovableTree() { - std::error_code ec; - std::filesystem::permissions(blocked, std::filesystem::perms::all, ec); - for (auto& e : std::filesystem::directory_iterator(parent, ec)) { - std::filesystem::permissions(e.path() / "bin" / "locked", - std::filesystem::perms::all, ec); - } - std::filesystem::remove_all(parent, ec); - } - UnremovableTree(const UnremovableTree&) = delete; - UnremovableTree& operator=(const UnremovableTree&) = delete; - - std::vector parked() const { - std::vector out; - std::error_code ec; - for (auto& e : std::filesystem::directory_iterator(parent, ec)) - if (e.path().filename().string().starts_with(".trash-")) - out.push_back(e.path()); - return out; - } -}; - -} // namespace - -TEST(ToolchainRemove, AHeldPayloadStillLeavesItsLocation) { - // What `toolchain remove` promises is that the toolchain stops being - // installed — not that every byte is already gone. A payload whose files - // are still open cannot be deleted, but it CAN be moved: Windows refuses - // to delete a directory containing an open file and allows renaming one. - // - // Before this, remove reported "Access is denied" and left the toolchain - // exactly where it was, seconds after a build with that same toolset — - // which is the normal case, not a corner one, because /Zi leaves - // mspdbsrv.exe running inside the payload being removed. - UnremovableTree t; - std::error_code ec; - - ASSERT_FALSE(std::filesystem::remove_all(t.root, ec) && !ec) - << "fixture is not actually unremovable; the test would prove nothing"; - - ec.clear(); - EXPECT_TRUE(remove_payload_tree(t.root, ec)) - << "a held payload was reported as un-removable"; - EXPECT_FALSE(std::filesystem::exists(t.root)) - << "the toolchain is still at the path it was removed from"; -} - -TEST(ToolchainRemove, ParkedBytesAreSweptOnceNothingHoldsThem) { - // The other half of the promise: parking is a deferral, not a leak. The - // next lifecycle command sweeps, and by then the process that held the - // files has exited — modelled here by dropping the permission block. - UnremovableTree t; - std::error_code ec; - remove_payload_tree(t.root, ec); - - auto parked = t.parked(); - ASSERT_EQ(parked.size(), 1u) << "expected exactly one parked payload"; - - std::filesystem::permissions(parked[0] / "bin" / "locked", - std::filesystem::perms::all, ec); - sweep_parked_payloads(t.parent); - EXPECT_TRUE(t.parked().empty()) << "parked payload was never swept"; -} +// WHAT IS AND IS NOT TESTED HERE, because the difference is not a gap. +// +// `remove_payload_tree()` falls back to moving held FILES aside so the tree +// can be deleted around them. That path is reachable only where a file can be +// renamed while something holds it open — which is Windows. On POSIX the two +// permissions are the same one: a file you cannot unlink is a file you cannot +// rename either, because both need write on the parent directory. There is no +// POSIX state that models "held open by another process". +// +// A POSIX fixture cannot stand in for it either, and the reason is worth +// keeping: the only POSIX way to make a file undeletable is to drop write on +// its parent directory — and this function's SECOND pass adds write back +// across the tree, on purpose, because that is exactly the read-only-payload +// case it must fix. The fixture therefore becomes removable the moment the +// code under test touches it. That is the function working, not a hole. +// +// So the held-file path is gated by e2e 239 on the Windows runner, and what +// is pinned here is what must hold on every platform: the ordinary deletion +// takes the ordinary route, and the sweep is precise about what it eats. +// Nothing here asserts something this platform cannot express. TEST(ToolchainRemove, AnOrdinaryPayloadIsJustDeleted) { - // The common path must not grow a `.trash-` directory: parking is the - // fallback, and a fallback that fires always is not a fallback. + // The common path must not grow a `.trash-` directory: moving files aside + // is the fallback, and a fallback that fires always is not a fallback. auto dir = std::filesystem::temp_directory_path() - / std::format("mcpp-rm-ok-{}", - std::chrono::steady_clock::now() - .time_since_epoch().count()); + / std::format("mcpp-rm-ok-{}", std::chrono::steady_clock::now() + .time_since_epoch().count()); std::filesystem::create_directories(dir / "14.44.35207" / "bin"); std::ofstream{dir / "14.44.35207" / "bin" / "cl.exe"} << "not a compiler"; @@ -105,10 +39,32 @@ TEST(ToolchainRemove, AnOrdinaryPayloadIsJustDeleted) { EXPECT_TRUE(remove_payload_tree(dir / "14.44.35207", ec)); EXPECT_FALSE(std::filesystem::exists(dir / "14.44.35207")); - bool anyParked = false; + bool anyTrash = false; for (auto& e : std::filesystem::directory_iterator(dir, ec)) - anyParked |= e.path().filename().string().starts_with(".trash-"); - EXPECT_FALSE(anyParked) << "a deletable payload was parked instead of deleted"; + anyTrash |= e.path().filename().string().starts_with(".trash-"); + EXPECT_FALSE(anyTrash) << "a deletable payload was parked instead of deleted"; std::filesystem::remove_all(dir, ec); } + +TEST(ToolchainRemove, TheSweepDeletesParkedPayloadsAndNothingElse) { + // The sweep runs before every install and remove, so it must be precise: + // `.trash-*` goes, an installed version directory beside it stays. + auto pkgRoot = std::filesystem::temp_directory_path() + / std::format("mcpp-sweep-{}", std::chrono::steady_clock::now() + .time_since_epoch().count()); + std::filesystem::create_directories(pkgRoot / ".trash-14.44.35207-1" / "sub"); + std::ofstream{pkgRoot / ".trash-14.44.35207-1" / "sub" / "held.dll"} << "x"; + std::filesystem::create_directories(pkgRoot / "14.44.35207" / "bin"); + std::ofstream{pkgRoot / "14.44.35207" / "bin" / "cl.exe"} << "keep me"; + + sweep_parked_payloads(pkgRoot); + + EXPECT_FALSE(std::filesystem::exists(pkgRoot / ".trash-14.44.35207-1")) + << "parked payload was not swept"; + EXPECT_TRUE(std::filesystem::exists(pkgRoot / "14.44.35207" / "bin" / "cl.exe")) + << "the sweep deleted an installed toolset"; + + std::error_code ec; + std::filesystem::remove_all(pkgRoot, ec); +} From 93761dbd35eb986c28604ef8c3d0407fd78d52a2 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 20:52:15 +0800 Subject: [PATCH 11/11] fix(toolchain): removed means no files left, not an empty tree deleted With the files moved aside, removal still failed -- this time on the DIRECTORY skeleton, with a sharing violation rather than access denied. That is Windows refusing to delete a directory some process has as its current directory, and mspdbsrv.exe is launched inside the payload, so it is the ordinary tail of a /Zi build with the toolset being removed. Every file was already gone at that point. A toolchain with no files in it is not installed, which is exactly what `remove` promises, so reporting failure was reporting the opposite of what happened. The criterion is now "no regular file remains"; the skeleton is swept by the next lifecycle command. That exposed the matching lie on the other side: `toolchain default` accepted any directory that EXISTS, so a skeleton would have been called an installed toolchain and handed to a build. It now asks `payload_frontend` for a resolvable compiler -- the same rule this whole round has been applying to `installed()`, `find_windows_sdk()` and the index CI. Present is not usable. Also here, from the architecture review: - doctor.cppm used the pre-#436 `toolchain_frontend(root/"bin")` shape, so an installed msvc toolset was visible to `toolchain list` and invisible to `doctor`. Same layout rule, fourth copy. - `msvc_available_here()` joins `has_usable_msvc()`: the latter probes the MACHINE, which is the wrong question on a box with a pinned toolset and no Visual Studio. The three prepare.cppm decisions that gate first-run diversion, offline guidance and the ABI repair now ask the origin-aware one and stop diverting a perfectly good toolchain to mingw. - `vc_redist_dir()` finds the toolset's own vcruntime140.dll/msvcp140.dll and puts it in `linkRuntimeDirs`, which is how `mcpp run` reaches it on PATH. Those DLLs are NOT OS components, so the DEFAULT (/MD) build could link on a machine with only a managed toolset and then fail to start. Five tests: found from the compiler path alone, the redist version is not the tools version (14.44.35112 vs 14.44.35207 -- deriving it finds nothing), newest wins, debug_nonredist is never returned because it may not be redistributed, and a toolset without one is not an error. --- src/build/prepare.cppm | 19 ++++- src/doctor.cppm | 11 ++- src/toolchain/lifecycle.cppm | 43 +++++++++- src/toolchain/msvc.cppm | 100 ++++++++++++++++++++++++ tests/unit/test_toolchain_lifecycle.cpp | 42 ++++++++++ tests/unit/test_toolchain_msvc.cpp | 89 +++++++++++++++++++++ 6 files changed, 295 insertions(+), 9 deletions(-) diff --git a/src/build/prepare.cppm b/src/build/prepare.cppm index 0066d1b8..2284b0c5 100644 --- a/src/build/prepare.cppm +++ b/src/build/prepare.cppm @@ -1090,11 +1090,22 @@ prepare_build(bool print_fingerprint, // rest: the vocabulary table already maps x86_64-windows-gnu to its pin // (winlibs GCC) and to static linkage, so the toolchain answer stays a // single derivation instead of being spelled out a second time here. + // "Is MSVC usable here" — either origin. Asking `has_usable_msvc()` (which + // probes the machine) would answer "no" on a box that has a pinned + // msvc@ payload and no Visual Studio, and every decision below + // would then divert a perfectly good toolchain to mingw. + auto msvc_usable_either_origin = [&]() -> bool { + auto c = get_cfg(); + if (!c) return mcpp::toolchain::msvc::has_usable_msvc(); + return mcpp::toolchain::msvc::msvc_available_here( + (*c)->xlingsHome() / "data" / "xpkgs"); + }; + bool windowsGnuFirstRun = false; if constexpr (mcpp::platform::is_windows) { if (!tcSpec.has_value() && overrides.target_triple.empty() && m->buildConfig.target.empty() - && !mcpp::toolchain::msvc::has_usable_msvc()) { + && !msvc_usable_either_origin()) { auto cfgW = get_cfg(); if (!cfgW || (*cfgW)->defaultTarget.empty()) { overrides.target_triple = @@ -1386,7 +1397,7 @@ prepare_build(bool print_fingerprint, // exactly what this machine cannot build. Name the toolchain that // will actually work there instead. if (mcpp::platform::is_windows - && !mcpp::toolchain::msvc::has_usable_msvc()) { + && !msvc_usable_either_origin()) { return std::unexpected(std::format( "no toolchain configured (and no Visual Studio found).\n" " run one of:\n" @@ -1441,7 +1452,7 @@ prepare_build(bool print_fingerprint, if constexpr (mcpp::platform::is_macos) { defaultSpec = std::string(pins::kFirstRunMac); } else if constexpr (mcpp::platform::is_windows) { - // Reaching here means has_usable_msvc() was true — the seed above + // Reaching here means msvc_usable_either_origin() was true — the seed above // diverts the no-Visual-Studio case onto the windows-gnu target // before the target block runs, so it never gets this far. defaultSpec = std::string(pins::kFirstRunWinMsvc); @@ -1555,7 +1566,7 @@ prepare_build(bool print_fingerprint, const bool targetsMsvcAbi = tc->compiler == mcpp::toolchain::CompilerId::MSVC || mcpp::toolchain::is_msvc_target(*tc); - if (targetsMsvcAbi && !mcpp::toolchain::msvc::has_usable_msvc()) { + if (targetsMsvcAbi && !msvc_usable_either_origin()) { // Native cl.exe is ALWAYS a deliberate choice: mcpp never selects // msvc@system on its own — it cannot install one — so the only way it // reaches config.toml is a user typing `mcpp toolchain default msvc`. diff --git a/src/doctor.cppm b/src/doctor.cppm index 7d99841d..ece85bb4 100644 --- a/src/doctor.cppm +++ b/src/doctor.cppm @@ -395,8 +395,15 @@ export int doctor_report() { s.family = id->family; s.version = vEntry.path().filename().string(); s.target = id->target; - auto bin = mcpp::toolchain::toolchain_frontend( - vEntry.path() / "bin", mcpp::toolchain::to_xim_package(s)); + // payload_frontend, not toolchain_frontend(root/"bin"): + // cl.exe is four levels down at + // VC/Tools/MSVC//bin/Host//, so the `bin/` + // lookup finds nothing and `continue` drops every + // installed msvc toolset. That is the defect #436 fixed + // in `toolchain list`; doctor kept its own copy, so the + // two commands disagreed about the same machine. + auto bin = mcpp::toolchain::payload_frontend( + vEntry.path(), mcpp::toolchain::to_xim_package(s), s.family); if (bin.empty()) continue; sawAny = true; diff --git a/src/toolchain/lifecycle.cppm b/src/toolchain/lifecycle.cppm index 57299e94..1445418f 100644 --- a/src/toolchain/lifecycle.cppm +++ b/src/toolchain/lifecycle.cppm @@ -221,6 +221,18 @@ void msvc_print_detected(const mcpp::toolchain::msvc::MsvcInstallation& inst, // rather than picking one: clear the attribute, then give a live process a // bounded moment to go away. Retries are capped and short — `toolchain // remove` should not hang because something holds the directory forever. +// Whether any FILE is left under `root`. Directories alone are not a +// toolchain: nothing resolves a compiler out of an empty tree. +bool any_regular_file(const std::filesystem::path& root) { + std::error_code ec; + for (auto it = std::filesystem::recursive_directory_iterator( + root, std::filesystem::directory_options::skip_permission_denied, ec); + it != std::filesystem::recursive_directory_iterator{}; it.increment(ec)) { + if (it->is_regular_file(ec)) return true; + } + return false; +} + export bool remove_payload_tree(const std::filesystem::path& root, std::error_code& ec) { std::filesystem::remove_all(root, ec); @@ -296,7 +308,19 @@ export bool remove_payload_tree(const std::filesystem::path& root, ec.clear(); std::filesystem::remove_all(root, ec); // directories only now std::filesystem::remove_all(trash, ignore); // usually works; fine if not - return !ec; + if (!ec) return true; + + // The files are gone and an empty directory skeleton would not die. That + // is a sharing violation on a DIRECTORY, which on Windows means some + // process has one of them as its current directory -- mspdbsrv.exe is + // launched inside the payload, so this is the normal tail of a /Zi build. + // + // A toolchain with no files in it is not installed, which is what + // `remove` promises. Reporting failure here would be reporting the + // opposite of what happened, and the skeleton is swept by the next + // lifecycle command once that process exits. + ec.clear(); + return !any_regular_file(root); } // Delete `.trash-*` left behind by a removal that had to park a held payload. @@ -311,8 +335,14 @@ export void sweep_parked_payloads(const std::filesystem::path& pkgRoot) { if (!std::filesystem::is_directory(pkgRoot, ec)) return; for (auto& e : std::filesystem::directory_iterator(pkgRoot, ec)) { if (!e.is_directory(ec)) continue; - if (e.path().filename().string().starts_with(".trash-")) + if (e.path().filename().string().starts_with(".trash-")) { std::filesystem::remove_all(e.path(), ec); + } else if (!any_regular_file(e.path())) { + // A version directory with no files in it is the skeleton a + // removal could not delete because something held a directory + // open. A real install always has files, so this cannot eat one. + std::filesystem::remove_all(e.path(), ec); + } } } @@ -912,7 +942,14 @@ export int toolchain_set_default(const mcpp::config::GlobalConfig& cfg, } auto installDir = mcpp::xlings::paths::xim_tool(xlEnv, pkg.ximName, pkg.ximVersion); - if (!std::filesystem::exists(installDir)) { + // A RESOLVABLE COMPILER, not a directory that exists. A removal that + // could not delete the last empty directories leaves a skeleton + // behind (see remove_payload_tree), and `exists()` would call that + // skeleton an installed toolchain and then hand it to a build. + // + // Same rule as everywhere else in this round: installed means usable, + // not present. + if (mcpp::toolchain::payload_frontend(installDir, pkg, spec->family).empty()) { // Before "not installed", check whether this is the retired // `msvc@` spelling — otherwise the advice is to // install a toolset that does not exist and never will. diff --git a/src/toolchain/msvc.cppm b/src/toolchain/msvc.cppm index 4ed89bf5..f2506cba 100644 --- a/src/toolchain/msvc.cppm +++ b/src/toolchain/msvc.cppm @@ -173,6 +173,35 @@ sibling_sdk_roots(const std::filesystem::path& clPath); // Always false off Windows: the whole discovery chain is Win32-only. bool has_usable_msvc(); +// Whether MSVC is usable HERE — either origin. `has_usable_msvc()` asks only +// about the machine, which is the wrong question wherever a managed toolset +// would serve just as well: a box with a pinned `msvc@14.44.35207` payload +// and no Visual Studio answers `false` to that one while being perfectly able +// to compile. +// +// `pkgsDir` is mcpp's payload store; a non-empty `xim-x-msvc/` with a +// resolvable cl.exe counts. Off Windows both are false — the whole chain is +// Win32-only. +bool msvc_available_here(const std::filesystem::path& pkgsDir); + +// The redistributable VC runtime that BELONGS TO THIS TOOLSET: +// \Redist\MSVC\\\Microsoft.VC.CRT\ +// vcruntime140.dll msvcp140.dll ... +// +// It matters because the default CRT model is /MD, and those DLLs are NOT +// Windows components — ucrtbase.dll ships with the OS, vcruntime140.dll does +// not. On a machine that has only a managed toolset (no Visual Studio, no +// redistributable installed) a /MD build links fine and then cannot start. +// +// The toolset carries its own copy, so this is the toolchain-coupled runtime +// in exactly the sense libstdc++ is for gcc, and it goes in the same field. +// +// `redistVer` is NOT the tools version (14.44.35112 vs 14.44.35207), so the +// newest directory is chosen rather than derived. `debug_nonredist\` is never +// returned: those DLLs may not be redistributed. +std::filesystem::path vc_redist_dir(const std::filesystem::path& clPath, + std::string_view arch = "x64"); + // Synthesize the environment cl.exe/link.exe need — what vcvars would set, // derived directly from the located VC tools + SDK (no vcvarsall.bat run): // INCLUDE = \include; \Include\\{ucrt,um,shared,winrt} @@ -668,6 +697,25 @@ bool has_usable_msvc() { #endif } +bool msvc_available_here([[maybe_unused]] const std::filesystem::path& pkgsDir) { +#if defined(_WIN32) + if (has_usable_msvc()) return true; + // A managed toolset is just as usable, and asking the machine about it + // gets the wrong answer. Any installed version whose cl.exe resolves + // counts; `installation_at` is the same resolution install and build use. + std::error_code ec; + auto root = pkgsDir / "xim-x-msvc"; + if (!std::filesystem::is_directory(root, ec)) return false; + for (auto& v : std::filesystem::directory_iterator(root, ec)) { + if (!v.is_directory(ec)) continue; + if (installation_at(v.path(), v.path().filename().string())) return true; + } + return false; +#else + return false; +#endif +} + std::vector build_env_for_cl(const std::filesystem::path& clPath, std::string_view arch, const WindowsSdk& sdk) { @@ -794,6 +842,40 @@ std::vector std_compat_build_commands( ref, crtFlag) }; } +std::filesystem::path vc_redist_dir(const std::filesystem::path& clPath, + std::string_view arch) { + // /Tools/MSVC//bin/Host//cl.exe → up 6 from the arch dir + auto vc = clPath.parent_path(); + for (int i = 0; i < 6 && !vc.empty(); ++i) vc = vc.parent_path(); + std::error_code ec; + auto redist = vc / "Redist" / "MSVC"; + if (!std::filesystem::is_directory(redist, ec)) return {}; + + std::filesystem::path best; + std::string bestVer; + for (auto& v : std::filesystem::directory_iterator(redist, ec)) { + if (!v.is_directory(ec)) continue; + auto archDir = v.path() / std::string(arch); + if (!std::filesystem::is_directory(archDir, ec)) continue; + for (auto& c : std::filesystem::directory_iterator(archDir, ec)) { + if (!c.is_directory(ec)) continue; + auto name = c.path().filename().string(); + // Microsoft.VC143.CRT — the CRT, not CXXAMP/OpenMP, and never + // anything under debug_nonredist (which is not redistributable + // and is a sibling of , not a child, but be explicit). + if (!name.starts_with("Microsoft.VC") || !name.ends_with(".CRT")) + continue; + if (c.path().string().find("debug_nonredist") != std::string::npos) + continue; + if (auto ver = v.path().filename().string(); ver > bestVer) { + bestVer = ver; + best = c.path(); + } + } + } + return best; +} + std::expected enrich_toolchain_from_cl(Toolchain& tc) { auto banner = capture_cl_banner(tc.binaryPath); auto parsed = parse_cl_banner(banner); @@ -841,6 +923,24 @@ std::expected enrich_toolchain_from_cl(Toolchain& tc) { if (auto sdk = find_windows_sdk(extraSdkRoots)) { tc.envOverrides = build_env_for_cl(tc.binaryPath, parsed->second, *sdk); } + + // The toolset's own redistributable CRT, in the same field gcc uses for + // libstdc++ — so `mcpp run` puts it on PATH exactly the way it puts a + // private libstdc++ on LD_LIBRARY_PATH. + // + // Without it, the DEFAULT build (/MD) links vcruntime140.dll and + // msvcp140.dll, which are not OS components, and a machine with only a + // managed toolset cannot start what it just built. That machine is not + // hypothetical — it is any box that installed `msvc@` and never + // had Visual Studio. CI does not see it because the runners have VS. + // + // Safe for the link line: hostflags returns early for MSVC before it + // emits -L, and flags.cppm's runtime-dir block is `supports_rpath`-gated, + // so nothing here reaches cl or link as a flag. + if (auto redist = vc_redist_dir(tc.binaryPath, parsed->second); + !redist.empty()) { + tc.linkRuntimeDirs.push_back(redist); + } return {}; } diff --git a/tests/unit/test_toolchain_lifecycle.cpp b/tests/unit/test_toolchain_lifecycle.cpp index 1df9d74c..c93cb5b1 100644 --- a/tests/unit/test_toolchain_lifecycle.cpp +++ b/tests/unit/test_toolchain_lifecycle.cpp @@ -68,3 +68,45 @@ TEST(ToolchainRemove, TheSweepDeletesParkedPayloadsAndNothingElse) { std::error_code ec; std::filesystem::remove_all(pkgRoot, ec); } + +TEST(ToolchainRemove, ADirectorySkeletonWithNoFilesCountsAsRemoved) { + // Windows can refuse to delete an empty DIRECTORY when a process has it + // as its current directory — mspdbsrv.exe is launched inside the payload, + // so this is the ordinary tail of a /Zi build with the toolset being + // removed. Every file is already gone at that point. + // + // A toolchain with no files in it is not installed, which is exactly what + // `remove` promises. Calling that a failure would report the opposite of + // what happened. + auto dir = std::filesystem::temp_directory_path() + / std::format("mcpp-skel-{}", std::chrono::steady_clock::now() + .time_since_epoch().count()); + std::filesystem::create_directories(dir / "14.44.35207" / "bin" / "Hostx64" / "x64"); + + std::error_code ec; + EXPECT_TRUE(remove_payload_tree(dir / "14.44.35207", ec)) + << "a payload with no files left was reported as still installed"; + + std::filesystem::remove_all(dir, ec); +} + +TEST(ToolchainRemove, TheSweepAlsoClearsAFileLessSkeleton) { + // The deferral half: whatever the removal could not delete goes on the + // next lifecycle command, once the process holding it has exited. + auto pkgRoot = std::filesystem::temp_directory_path() + / std::format("mcpp-skel2-{}", std::chrono::steady_clock::now() + .time_since_epoch().count()); + std::filesystem::create_directories(pkgRoot / "14.44.35207" / "bin" / "x64"); + std::filesystem::create_directories(pkgRoot / "14.52.36629" / "bin"); + std::ofstream{pkgRoot / "14.52.36629" / "bin" / "cl.exe"} << "a real one"; + + sweep_parked_payloads(pkgRoot); + + EXPECT_FALSE(std::filesystem::exists(pkgRoot / "14.44.35207")) + << "the empty skeleton survived the sweep"; + EXPECT_TRUE(std::filesystem::exists(pkgRoot / "14.52.36629" / "bin" / "cl.exe")) + << "the sweep ate an installed toolset"; + + std::error_code ec; + std::filesystem::remove_all(pkgRoot, ec); +} diff --git a/tests/unit/test_toolchain_msvc.cpp b/tests/unit/test_toolchain_msvc.cpp index 461081e9..034a9c0e 100644 --- a/tests/unit/test_toolchain_msvc.cpp +++ b/tests/unit/test_toolchain_msvc.cpp @@ -375,6 +375,95 @@ TEST(MsvcSdk, IncompleteSdkRootIsNotAnAnswer) { if (sdk) EXPECT_NE(sdk->root, t.root) << "an SDK-less root was accepted"; } +// ─── the toolset's own redistributable CRT ─────────────────────────────── + +namespace { + +// A toolset laid out the way MSVC actually lays one out, including the part +// that trips a derivation: the Redist version is NOT the tools version. +struct FakeRedist { + std::filesystem::path root, clPath; + + explicit FakeRedist(std::string_view toolsVer, std::string_view redistVer) { + root = std::filesystem::temp_directory_path() + / std::format("mcpp-redist-{}", std::chrono::steady_clock::now() + .time_since_epoch().count()); + auto tools = root / "VC" / "Tools" / "MSVC" / std::string(toolsVer); + auto bin = tools / "bin" / "Hostx64" / "x64"; + std::filesystem::create_directories(bin); + clPath = bin / "cl.exe"; + std::ofstream{clPath} << "not a compiler"; + add(redistVer, "x64", "Microsoft.VC143.CRT", "vcruntime140.dll"); + } + void add(std::string_view ver, std::string_view arch, + std::string_view comp, std::string_view file) { + auto d = root / "VC" / "Redist" / "MSVC" / std::string(ver) + / std::string(arch) / std::string(comp); + std::filesystem::create_directories(d); + std::ofstream{d / std::string(file)} << "dll"; + } + void add_debug(std::string_view ver) { + auto d = root / "VC" / "Redist" / "MSVC" / std::string(ver) + / "debug_nonredist" / "x64" / "Microsoft.VC143.DebugCRT"; + std::filesystem::create_directories(d); + std::ofstream{d / "vcruntime140d.dll"} << "dll"; + } + ~FakeRedist() { std::error_code ec; std::filesystem::remove_all(root, ec); } + FakeRedist(const FakeRedist&) = delete; + FakeRedist& operator=(const FakeRedist&) = delete; +}; + +} // namespace + +TEST(MsvcRedist, FoundFromTheCompilerPathAlone) { + // Nothing configured, no version derived: the compiler's own location + // says which toolset this is, and the toolset carries its runtime. + FakeRedist t{"14.44.35207", "14.44.35112"}; + auto d = msvc::vc_redist_dir(t.clPath, "x64"); + ASSERT_FALSE(d.empty()) << "the toolset's redistributable CRT was not found"; + EXPECT_TRUE(std::filesystem::exists(d / "vcruntime140.dll")); +} + +TEST(MsvcRedist, TheRedistVersionIsNotTheToolsVersion) { + // 14.44.35207 (tools) vs 14.44.35112 (redist) is what MSVC actually + // ships. Deriving one from the other finds nothing — this is why the + // directory is searched rather than composed. + FakeRedist t{"14.44.35207", "14.44.35112"}; + auto d = msvc::vc_redist_dir(t.clPath, "x64"); + ASSERT_FALSE(d.empty()); + EXPECT_NE(d.string().find("14.44.35112"), std::string::npos) + << "did not land in the redist version directory: " << d.string(); +} + +TEST(MsvcRedist, NewestRedistWins) { + FakeRedist t{"14.44.35207", "14.40.00000"}; + t.add("14.44.35112", "x64", "Microsoft.VC143.CRT", "vcruntime140.dll"); + auto d = msvc::vc_redist_dir(t.clPath, "x64"); + ASSERT_FALSE(d.empty()); + EXPECT_NE(d.string().find("14.44.35112"), std::string::npos) + << "older redist won: " << d.string(); +} + +TEST(MsvcRedist, TheDebugCrtIsNeverReturned) { + // debug_nonredist may NOT be redistributed. Returning it would put + // vcruntime140d.dll on PATH and, later, into a shipped artifact. + FakeRedist t{"14.44.35207", "14.44.35112"}; + t.add_debug("14.99.99999"); // newer, and must still lose + auto d = msvc::vc_redist_dir(t.clPath, "x64"); + ASSERT_FALSE(d.empty()); + EXPECT_EQ(d.string().find("debug_nonredist"), std::string::npos) + << "returned the non-redistributable debug CRT: " << d.string(); +} + +TEST(MsvcRedist, AToolsetWithoutARedistIsNotAnError) { + // msvc@system on a machine whose VS install omits the redist component, + // for instance. Empty means "nothing to add to PATH", not a failure. + FakeRedist t{"14.44.35207", "14.44.35112"}; + std::error_code ec; + std::filesystem::remove_all(t.root / "VC" / "Redist", ec); + EXPECT_TRUE(msvc::vc_redist_dir(t.clPath, "x64").empty()); +} + TEST(MsvcSdk, HeadersWithoutImportLibsIsNotAnAnswer) { // The half that used to pass. `Include//ucrt/corecrt.h` is there and // `Lib/` is not, which is exactly what a managed windows-sdk payload