-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not delete bootstrap.exe on Windows during clean #82177
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
e5bc977
to
237661c
Compare
I thought windows 10 introduced a way to use posix unlink semantics, can't we use that? |
Windows does not allow deleting currently running executables
237661c
to
f52caa7
Compare
I mean we could avoid skipping cleanup if the system supports unlink semantics. |
I don't think platform version specific logic is worth it, personally, but I would like for us to warn the user when we fail to fully clean - it might also make sense to still try to clean up most of the bootstrap directory, even if we can't quite get the bootstrap binary. |
@Mark-Simulacrum I've added more handling so the user is informed when something goes wrong on Windows. |
This comment has been minimized.
This comment has been minimized.
8fb030a
to
5b0ed02
Compare
Not for running binaries, IIRC. Even with posix unlink semantics (which is now the default for There's no harm in trying though. |
Rustup has had serious problems with this (cc @kinnison), so if you figure out a way please let them know. I'm inclined to think it's not worth messing with, though. |
I tried a quick test with |
Yes, that it's not currently possible was the message I was trying to convey with my explanation. Obviously I worded that badly, sorry. To rephrase: The Windows executable loader (which is outside Rust's control) does not currently set My point in that last sentence was only that there is not a problem with attempting to delete and handling the expected failure. The worst case it fails gracefully, which can be handled, and in the best case some future version of Windows allows deleting running applications. |
@bors r+ |
📌 Commit 5b0ed02 has been approved by |
…r=Mark-Simulacrum Do not delete bootstrap.exe on Windows during clean Windows does not allow deleting currently running executables. This an addition to `@jyn514's` change in rust-lang#80574.
…r=Mark-Simulacrum Do not delete bootstrap.exe on Windows during clean Windows does not allow deleting currently running executables. This an addition to ``@jyn514's`` change in rust-lang#80574.
Rollup of 11 pull requests Successful merges: - rust-lang#81300 (BTree: share panicky test code & test panic during clear, clone) - rust-lang#81706 (Document BinaryHeap unsafe functions) - rust-lang#81833 (parallelize x.py test tidy) - rust-lang#81966 (Add new `rustc` target for Arm64 machines that can target the iphonesimulator) - rust-lang#82154 (Update RELEASES.md 1.50 to include methods stabilized in rust-lang#79342) - rust-lang#82177 (Do not delete bootstrap.exe on Windows during clean) - rust-lang#82181 (Add check for ES5 in CI) - rust-lang#82229 (Add [A-diagnostics] bug report template) - rust-lang#82233 (try-back-block-type test: Use TryFromSliceError for From test) - rust-lang#82302 (Remove unsafe impl Send for CompletedTest & TestResult) - rust-lang#82349 (test: Print test name only once on timeout) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Windows does not allow deleting currently running executables.
This an addition to @jyn514's change in #80574.