Skip to content

Commit 29028a6

Browse files
committed
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.
1 parent ca82400 commit 29028a6

2 files changed

Lines changed: 79 additions & 3 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,17 @@
77

88
### 修复
99

10+
- **Windows 上 `mcpp toolchain remove <msvc toolset>` 报 "Access is denied"。**
11+
12+
`remove_all` 直接上,不清只读位、不重试、报错也不说是哪个文件。
13+
payload 是从 .vsix/.msi 解出来的,归档条目带只读属性;POSIX 只要**目录**
14+
可写就能 unlink 子项,所以这个问题在 Linux/macOS 上根本不出现。
15+
另外 `/Zi` 构建之后 mspdbsrv.exe 还会在 payload 里活几秒。
16+
17+
两个原因表现完全一样(都是 "Access is denied"),所以两个都处理:
18+
先清可写位重试,再给活着的进程一个**有上限**的等待窗口(10 × 300ms)。
19+
报错现在会指出卡在哪个文件 —— 光一句 "Access is denied" 没法处理。
20+
1021
- **半装的 Windows SDK 被当成装好的,链接到最后才炸。**
1122

1223
`find_windows_sdk()` 认一个根的条件是 `Include\<v>\ucrt\corecrt.h`

‎src/toolchain/lifecycle.cppm‎

Lines changed: 68 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,66 @@ void msvc_print_detected(const mcpp::toolchain::msvc::MsvcInstallation& inst,
208208
// headers much later. `has_usable_msvc()` exists for exactly this reason;
209209
// this is the same judgement applied to the managed origin, where the SDK
210210
// arrives as a package dependency and can therefore fail on its own.
211+
// Delete a payload tree, coping with the two things that make a plain
212+
// `remove_all` fail on Windows and on nothing else:
213+
//
214+
// read-only files — payloads are unpacked from .vsix/.msi, and archive
215+
// entries carry the attribute through. POSIX only needs the DIRECTORY
216+
// writable to unlink a child, so this never shows up on Linux or macOS.
217+
// a lingering handle — a build with /Zi leaves mspdbsrv.exe running for a
218+
// few seconds after cl.exe exits, and it lives inside the payload.
219+
//
220+
// Both surface as the same "Access is denied", which is why this handles both
221+
// rather than picking one: clear the attribute, then give a live process a
222+
// bounded moment to go away. Retries are capped and short — `toolchain
223+
// remove` should not hang because something holds the directory forever.
224+
bool remove_payload_tree(const std::filesystem::path& root,
225+
std::error_code& ec) {
226+
std::filesystem::remove_all(root, ec);
227+
if (!ec) return true;
228+
229+
// Second pass: make everything writable, then try again.
230+
std::error_code ignore;
231+
for (auto it = std::filesystem::recursive_directory_iterator(
232+
root, std::filesystem::directory_options::skip_permission_denied,
233+
ignore);
234+
it != std::filesystem::recursive_directory_iterator{}; it.increment(ignore)) {
235+
std::filesystem::permissions(it->path(), std::filesystem::perms::owner_write,
236+
std::filesystem::perm_options::add, ignore);
237+
}
238+
for (int attempt = 0; attempt < 10; ++attempt) {
239+
ec.clear();
240+
std::filesystem::remove_all(root, ec);
241+
if (!ec) return true;
242+
std::this_thread::sleep_for(std::chrono::milliseconds{300});
243+
}
244+
return !std::filesystem::exists(root, ignore);
245+
}
246+
247+
// The first entry that is still there after a failed removal. The error code
248+
// alone says "Access is denied" and not by whom or to what, which is the
249+
// difference between a report someone can act on and one they cannot.
250+
//
251+
// It probes by trying to delete, and keeps whatever it manages to delete --
252+
// acceptable only because the caller has already failed a remove_all and the
253+
// tree is being torn down anyway. Do not call it on a tree meant to survive.
254+
std::optional<std::filesystem::path>
255+
first_undeletable(const std::filesystem::path& root) {
256+
std::error_code ec;
257+
if (!std::filesystem::exists(root, ec)) return std::nullopt;
258+
for (auto it = std::filesystem::recursive_directory_iterator(
259+
root, std::filesystem::directory_options::skip_permission_denied, ec);
260+
it != std::filesystem::recursive_directory_iterator{}; it.increment(ec)) {
261+
if (ec) break;
262+
if (it->is_regular_file(ec)) {
263+
std::error_code rm;
264+
std::filesystem::remove(it->path(), rm);
265+
if (rm) return it->path();
266+
}
267+
}
268+
return root;
269+
}
270+
211271
void msvc_warn_if_sdk_missing(const mcpp::toolchain::msvc::MsvcInstallation& inst) {
212272
auto roots = mcpp::toolchain::msvc::sibling_sdk_roots(inst.clPath);
213273
if (auto sdk = mcpp::toolchain::msvc::find_windows_sdk(roots)) {
@@ -841,9 +901,14 @@ export int toolchain_remove(const mcpp::config::GlobalConfig& cfg,
841901
mcpp::ui::error(std::format("{} is not installed", spec));
842902
return 1;
843903
}
844-
std::filesystem::remove_all(installDir, ec);
845-
if (ec) {
846-
mcpp::ui::error(std::format("remove failed: {}", ec.message()));
904+
if (!remove_payload_tree(installDir, ec)) {
905+
mcpp::ui::error(std::format(
906+
"remove failed: {}{}", ec.message(),
907+
first_undeletable(installDir)
908+
.transform([](const std::filesystem::path& p) {
909+
return std::format("\n stuck at: {}", p.string());
910+
})
911+
.value_or(std::string{})));
847912
return 1;
848913
}
849914
mcpp::ui::status("Removed", spec);

0 commit comments

Comments
 (0)