Skip to content
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

Merged
merged 3 commits into from
Feb 21, 2021

Conversation

rylev
Copy link
Member

@rylev rylev commented Feb 16, 2021

Windows does not allow deleting currently running executables.

This an addition to @jyn514's change in #80574.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 16, 2021
@rust-log-analyzer

This comment has been minimized.

@rylev rylev force-pushed the no-delete-bootstrap-windows branch from e5bc977 to 237661c Compare February 16, 2021 09:44
@the8472
Copy link
Member

the8472 commented Feb 16, 2021

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
@rylev rylev force-pushed the no-delete-bootstrap-windows branch from 237661c to f52caa7 Compare February 16, 2021 10:17
@rylev
Copy link
Member Author

rylev commented Feb 16, 2021

@the8472 I believe we could theoretically use _unlink on Windows 10, but this doesn't work on older versions of Windows which is why the std library doesn't use it. Do we want to limit bootstrapping to Windows 10?

@the8472
Copy link
Member

the8472 commented Feb 16, 2021

I mean we could avoid skipping cleanup if the system supports unlink semantics.

@Mark-Simulacrum
Copy link
Member

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.

@rylev
Copy link
Member Author

rylev commented Feb 16, 2021

@Mark-Simulacrum I've added more handling so the user is informed when something goes wrong on Windows.

@rust-log-analyzer

This comment has been minimized.

@rylev rylev force-pushed the no-delete-bootstrap-windows branch from 8fb030a to 5b0ed02 Compare February 16, 2021 20:20
@ChrisDenton
Copy link
Member

I thought windows 10 introduced a way to use posix unlink semantics, can't we use that?

Not for running binaries, IIRC. Even with posix unlink semantics (which is now the default for DeleteFileW), Windows honours exclusive file locks. And last I checked the exe loader still loads the binary without the FILE_SHARE_DELETE permission so nobody else can delete it.

There's no harm in trying though.

@jyn514
Copy link
Member

jyn514 commented Feb 17, 2021

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.

@ehuss
Copy link
Contributor

ehuss commented Feb 17, 2021

There's no harm in trying though.

I tried a quick test with _unlink, and it still fails if the executable is running. I'm pretty sure it is not possible.

@ChrisDenton
Copy link
Member

ChrisDenton commented Feb 17, 2021

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 FILE_SHARE_DELETE which makes the .exe impossible to delete.

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.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 20, 2021

📌 Commit 5b0ed02 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 21, 2021
…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.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 21, 2021
…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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2021
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
@bors bors merged commit 13a3c6e into rust-lang:master Feb 21, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants