From 455a51146f0940f7adc6386002de63e189d7e995 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Sun, 16 Aug 2026 22:24:14 +0800 Subject: [PATCH] fix(toolchain): narrow the sweep, and stop spawning cl.exe to ask a yes/no Both found by reviewing the 2026.8.16.3 diff after tagging it. **The sweep could delete another version mid-install.** It walked the whole family directory and removed any version directory containing no regular files. But an install populates a version directory over time -- which is why package_fetcher tracks completeness with a marker file rather than by existence -- so a DIFFERENT version being extracted right now is briefly indistinguishable from a leftover skeleton. Two mcpp processes against one MCPP_HOME is ordinary on a shared or self-hosted runner, and this feature exists for long-lived installs on exactly those machines. `.trash-*` is still swept family-wide: that name is only ever written by this code, so deleting one is safe whoever else is running. The file-less-skeleton rule now applies to the single version the command names. **`msvc_available_here()` ran a compiler to answer a yes/no.** It wanted to know whether a usable toolset exists and called the full `installation_at()`, which spawns cl.exe for its banner to identify the version. That question is asked on every build at the MSVC-ABI gate, so a machine with several installed toolsets paid several subprocess spawns per build -- and those are precisely the machines this predicate was added for. `installation_at(..., identifyVersion = false)` skips the banner. The layout still has exactly ONE implementation; this is a parameter, not a second copy of the path arithmetic. The new test was run against the family-wide sweep and fails there. --- CHANGELOG.md | 24 ++++++++++++++ src/toolchain/lifecycle.cppm | 42 ++++++++++++++++--------- src/toolchain/msvc.cppm | 30 ++++++++++++------ tests/unit/test_toolchain_lifecycle.cpp | 31 ++++++++++++++++-- 4 files changed, 102 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36538a85..192514a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,30 @@ > 本文件追踪 `mcpp-community/mcpp` 公开仓的版本演进。 > 格式参考 [Keep a Changelog](https://keepachangelog.com/zh-CN/1.1.0/)。 +## [Unreleased] + +### 修复 + +- **卸载后的清扫会波及**别的版本**,而那可能正在被另一个进程解压。** + + `sweep_parked_payloads` 原来把整个 family 目录扫一遍,把**任何**没有文件的 + 版本目录删掉。但安装是**逐步**往版本目录里写文件的(所以 package_fetcher + 用标记文件而不是"目录在"来判断装完没有),于是**另一个正在解压的版本**在那 + 短暂窗口里和"残骨架"长得一模一样 —— 共用一个 `MCPP_HOME` 的机器上 + (自托管 runner、共享开发机)两个 mcpp 进程同时跑是常态。 + + `.trash-*` 照旧全扫(那个名字只有这段代码会写);"没有文件的骨架"这一条 + 收窄成**只扫这条命令点名的那一个版本**。 + +- **`msvc_available_here()` 每次构建都要为每个已装 payload 起一次 cl.exe。** + + 它只想知道"这儿有没有能用的 toolset",却走了完整的 `installation_at()`, + 那里面会跑一次 cl 拿 banner 来定版本。而这个判据在**每次构建**的 MSVC ABI + 门上都会被问到 —— 正好是装了多个 toolset 的机器最慢。 + + `installation_at(..., identifyVersion=false)` 跳过 banner。**布局仍然只有 + 一份实现** —— 不是再抄一遍路径拼接。 + ## [2026.8.16.3] — 2026-08-16 ### 修复 diff --git a/src/toolchain/lifecycle.cppm b/src/toolchain/lifecycle.cppm index 1445418f..a982503d 100644 --- a/src/toolchain/lifecycle.cppm +++ b/src/toolchain/lifecycle.cppm @@ -330,20 +330,33 @@ export bool remove_payload_tree(const std::filesystem::path& root, // 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) { +export void sweep_parked_payloads(const std::filesystem::path& pkgRoot, + const std::filesystem::path& skeleton) { std::error_code ec; if (!std::filesystem::is_directory(pkgRoot, ec)) return; + + // `.trash-*` is a name only this code writes, so deleting any of them is + // safe regardless of who else is running. 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); - } 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. + if (e.is_directory(ec) + && e.path().filename().string().starts_with(".trash-")) std::filesystem::remove_all(e.path(), ec); - } } + + // The file-less skeleton, on the other hand, is swept for ONE named + // version — the one this command is about — and never for whatever else + // happens to be sitting in the family directory. + // + // "No files in it" is not a safe thing to conclude about someone else's + // directory: an install populates a version directory over time (it is + // why package_fetcher tracks completeness with a marker file rather than + // by existence), so a concurrent `toolchain install` of a DIFFERENT + // version is briefly indistinguishable from a skeleton, and sweeping the + // whole family would delete it mid-extraction. Two mcpp processes against + // one MCPP_HOME is ordinary on a shared or self-hosted runner. + if (!skeleton.empty() && std::filesystem::is_directory(skeleton, ec) + && !any_regular_file(skeleton)) + std::filesystem::remove_all(skeleton, ec); } // The first entry that is still there after a failed removal. The error code @@ -763,10 +776,11 @@ export int toolchain_install(const mcpp::config::GlobalConfig& cfg, // 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 vdir = mcpp::xlings::paths::xim_tool( + mcpp::config::make_xlings_env(cfg), pkg.ximName, pkg.ximVersion); + sweep_parked_payloads(vdir.parent_path(), vdir); + } 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)); @@ -1015,7 +1029,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()); + sweep_parked_payloads(installDir.parent_path(), installDir); 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/src/toolchain/msvc.cppm b/src/toolchain/msvc.cppm index f2506cba..60369e1f 100644 --- a/src/toolchain/msvc.cppm +++ b/src/toolchain/msvc.cppm @@ -93,8 +93,13 @@ std::optional detect_installation(); // anywhere, which is what makes the managed path testable off Windows. The // cl banner simply stays unparsed there and `display_version()` falls back // to the declared version. +// `identifyVersion = false` skips running cl.exe for its banner. Use it when +// the question is only "is there a usable toolset here" -- `msvc_available_here` +// asks that on every build, and spawning a compiler per installed payload to +// answer it is a latency regression on exactly the machines that have several. std::optional installation_at(const std::filesystem::path& vsRoot, - std::string_view toolsVersion); + std::string_view toolsVersion, + bool identifyVersion = true); // Parse a cl.exe banner into (version, arch). Token-based so localized // banners work: first "d.d.d[.d]" run is the version, arch is the arm64/x64/ @@ -514,7 +519,8 @@ namespace { // on a Linux CI runner. std::optional installation_from_tools_dir(const std::filesystem::path& vsRoot, - const std::filesystem::path& tools); + const std::filesystem::path& tools, + bool identifyVersion = true); // Capture cl.exe's banner. cl prints it (plus a usage complaint) when run // bare; the exit status is irrelevant — parse whatever came out. @@ -526,7 +532,8 @@ std::string capture_cl_banner(const std::filesystem::path& cl) { std::optional installation_from_tools_dir(const std::filesystem::path& vsRoot, - const std::filesystem::path& tools) { + const std::filesystem::path& tools, + bool identifyVersion) { MsvcInstallation inst; inst.vsRoot = vsRoot; inst.vsProduct = product_from_vs_root(vsRoot); @@ -558,9 +565,11 @@ installation_from_tools_dir(const std::filesystem::path& vsRoot, // Version identification: banner is authoritative; tolerate failure // (clVersion stays empty and display_version() falls back to the // tools-dir version). Off Windows that failure is the normal case. - if (auto parsed = parse_cl_banner(capture_cl_banner(inst.clPath))) { - inst.clVersion = parsed->first; - if (!parsed->second.empty()) inst.arch = parsed->second; + if (identifyVersion) { + if (auto parsed = parse_cl_banner(capture_cl_banner(inst.clPath))) { + inst.clVersion = parsed->first; + if (!parsed->second.empty()) inst.arch = parsed->second; + } } return inst; } @@ -568,12 +577,13 @@ installation_from_tools_dir(const std::filesystem::path& vsRoot, } // namespace std::optional installation_at(const std::filesystem::path& vsRoot, - std::string_view toolsVersion) { + std::string_view toolsVersion, + bool identifyVersion) { if (toolsVersion.empty()) return std::nullopt; auto tools = vsRoot / "VC" / "Tools" / "MSVC" / std::string(toolsVersion); std::error_code ec; if (!std::filesystem::is_directory(tools, ec)) return std::nullopt; - return installation_from_tools_dir(vsRoot, tools); + return installation_from_tools_dir(vsRoot, tools, identifyVersion); } std::optional detect_installation() { @@ -708,7 +718,9 @@ bool msvc_available_here([[maybe_unused]] const std::filesystem::path& pkgsDir) 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; + // identifyVersion=false: this asks IF a toolset is here, never which. + if (installation_at(v.path(), v.path().filename().string(), + /*identifyVersion=*/false)) return true; } return false; #else diff --git a/tests/unit/test_toolchain_lifecycle.cpp b/tests/unit/test_toolchain_lifecycle.cpp index c93cb5b1..fa762720 100644 --- a/tests/unit/test_toolchain_lifecycle.cpp +++ b/tests/unit/test_toolchain_lifecycle.cpp @@ -47,6 +47,33 @@ TEST(ToolchainRemove, AnOrdinaryPayloadIsJustDeleted) { std::filesystem::remove_all(dir, ec); } +TEST(ToolchainRemove, TheSweepDoesNotTouchAnotherVersionThatHasNoFilesYet) { + // An install populates a version directory over time, so a DIFFERENT + // version being extracted right now is briefly indistinguishable from a + // leftover skeleton. Two mcpp processes against one MCPP_HOME is ordinary + // on a shared or self-hosted runner, and deleting someone else's + // half-extracted toolchain is not a cleanup. + // + // So the file-less sweep applies to the ONE version this command names. + auto pkgRoot = std::filesystem::temp_directory_path() + / std::format("mcpp-sweep3-{}", std::chrono::steady_clock::now() + .time_since_epoch().count()); + auto mine = pkgRoot / "14.44.35207"; // the one being removed + auto theirs = pkgRoot / "14.52.36629"; // someone else, mid-extract + std::filesystem::create_directories(mine / "bin"); + std::filesystem::create_directories(theirs / "VC" / "Tools"); + + sweep_parked_payloads(pkgRoot, mine); + + EXPECT_FALSE(std::filesystem::exists(mine)) << "the named skeleton survived"; + EXPECT_TRUE(std::filesystem::exists(theirs)) + << "swept a version this command was not about — that is someone " + "else's install being extracted"; + + std::error_code ec; + std::filesystem::remove_all(pkgRoot, 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. @@ -58,7 +85,7 @@ TEST(ToolchainRemove, TheSweepDeletesParkedPayloadsAndNothingElse) { std::filesystem::create_directories(pkgRoot / "14.44.35207" / "bin"); std::ofstream{pkgRoot / "14.44.35207" / "bin" / "cl.exe"} << "keep me"; - sweep_parked_payloads(pkgRoot); + sweep_parked_payloads(pkgRoot, {}); EXPECT_FALSE(std::filesystem::exists(pkgRoot / ".trash-14.44.35207-1")) << "parked payload was not swept"; @@ -100,7 +127,7 @@ TEST(ToolchainRemove, TheSweepAlsoClearsAFileLessSkeleton) { 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); + sweep_parked_payloads(pkgRoot, pkgRoot / "14.44.35207"); EXPECT_FALSE(std::filesystem::exists(pkgRoot / "14.44.35207")) << "the empty skeleton survived the sweep";