-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Windows: Fix error in fs::rename
on Windows 1607
#137528
Conversation
@bors try |
This comment was marked as outdated.
This comment was marked as outdated.
Windows: Fix error in `fs::rename` on Windows 1607 Fixes rust-lang#137499 There's a bug in our Windows implementation of `fs::rename` that only manifests on a specific version of Windows. Both newer and older versions of Windows work. I took the safest route to fixing this by using the old `MoveFileExW` function to implement this and only falling back to the new behaviour if that fails. This is similar to what is done in `unlink` (just above this function). try-job: dist-x86_64-mingw try-job: dist-x86_64-msvc `@bors` r? ghost
This comment was marked as outdated.
This comment was marked as outdated.
r? libs |
9f75c8e
to
7e5b1c2
Compare
@bors try |
Windows: Fix error in `fs::rename` on Windows 1607 Fixes rust-lang#137499 There's a bug in our Windows implementation of `fs::rename` that only manifests on a specific version of Windows. Both newer and older versions of Windows work. I took the safest route to fixing this by using the old `MoveFileExW` function to implement this and only falling back to the new behaviour if that fails. This is similar to what is done in `unlink` (just above this function). try-job: dist-x86_64-mingw try-job: dist-x86_64-msvc
☀️ Try build successful - checks-actions |
This has now been tested and does fix the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me, but I think the fs::rename
documentation will need an update. Also, it might be nice to return an error if the memory allocation fails.
unsafe { | ||
file_rename_info = alloc(layout).cast::<c::FILE_RENAME_INFO>(); | ||
if file_rename_info.is_null() { | ||
handle_alloc_error(layout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also return an OutOfMemory
error here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Great, this looks fine. |
Windows: Fix error in `fs::rename` on Windows 1607 Fixes rust-lang#137499 There's a bug in our Windows implementation of `fs::rename` that only manifests on a specific version of Windows. Both newer and older versions of Windows work. I took the safest route to fixing this by using the old `MoveFileExW` function to implement this and only falling back to the new behaviour if that fails. This is similar to what is done in `unlink` (just above this function). try-job: dist-x86_64-mingw try-job: dist-x86_64-msvc
…kingjubilee Rollup of 12 pull requests Successful merges: - rust-lang#136667 (Revert vita's c_char back to i8) - rust-lang#136780 (std: move stdio to `sys`) - rust-lang#137107 (Override default `Write` methods for cursor-like types) - rust-lang#137363 (compiler: factor Windows x86-32 ABI impl into its own file) - rust-lang#137528 (Windows: Fix error in `fs::rename` on Windows 1607) - rust-lang#137537 (Prevent `rmake.rs` from using unstable features, and fix 3 run-make tests that currently do) - rust-lang#137777 (Specialize `OsString::push` and `OsString as From` for UTF-8) - rust-lang#137832 (Fix crash in BufReader::peek()) - rust-lang#137904 (Improve the generic MIR in the default `PartialOrd::le` and friends) - rust-lang#138115 (Suggest typo fix for static lifetime) - rust-lang#138125 (Simplify `printf` and shell format suggestions) - rust-lang#138129 (Stabilize const_char_classify, const_sockaddr_setters) r? `@ghost` `@rustbot` modify labels: rollup
Windows: Fix error in `fs::rename` on Windows 1607 Fixes rust-lang#137499 There's a bug in our Windows implementation of `fs::rename` that only manifests on a specific version of Windows. Both newer and older versions of Windows work. I took the safest route to fixing this by using the old `MoveFileExW` function to implement this and only falling back to the new behaviour if that fails. This is similar to what is done in `unlink` (just above this function). try-job: dist-x86_64-mingw try-job: dist-x86_64-msvc
☔ The latest upstream changes (presumably #138155) made this pull request unmergeable. Please resolve the merge conflicts. |
Rebased. Code is otherwise unchanged. @bors r=joboet |
Rollup of 5 pull requests Successful merges: - rust-lang#136642 (Put the alloc unit tests in a separate alloctests package) - rust-lang#137528 (Windows: Fix error in `fs::rename` on Windows 1607) - rust-lang#137685 (self-contained linker: conservatively default to `-znostart-stop-gc` on x64 linux) - rust-lang#137757 (On long spans, trim the middle of them to make them fit in the terminal width) - rust-lang#138189 (Mention `env` and `option_env` macros in `std::env::var` docs) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#137528 - ChrisDenton:rename-win, r=joboet Windows: Fix error in `fs::rename` on Windows 1607 Fixes rust-lang#137499 There's a bug in our Windows implementation of `fs::rename` that only manifests on a specific version of Windows. Both newer and older versions of Windows work. I took the safest route to fixing this by using the old `MoveFileExW` function to implement this and only falling back to the new behaviour if that fails. This is similar to what is done in `unlink` (just above this function). try-job: dist-x86_64-mingw try-job: dist-x86_64-msvc
Fixes #137499
There's a bug in our Windows implementation of
fs::rename
that only manifests on a specific version of Windows. Both newer and older versions of Windows work.I took the safest route to fixing this by using the old
MoveFileExW
function to implement this and only falling back to the new behaviour if that fails. This is similar to what is done inunlink
(just above this function).try-job: dist-x86_64-mingw
try-job: dist-x86_64-msvc