-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
std.fs: Get some more rename tests passing on Windows #18632
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think this is kind of handy behavior by the kernel actually. Before going through with this fallback mechanism, I think it would be worth looking into something like this:
std.os.renameatW
. Don't try to provide a posix-style API for this function on Windows.std.os.rename
give a compile error if called on Windows for the same reason.std.fs
and other former callsites to renameatW to directly use the Windows API rather than going through this posix layer.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.
I'm having trouble imagining what the usage of the proposed API would be like. If I'm writing cross-platform code,
fs.rename
/Dir.rename
having quite different properties on different platforms seems like it'd make for a rather hard-to-use API, since any relevant properties can't actually be relied on to be consistent across platforms. For example, if you write your code such that it can take advantage of being able to rename a directory onto an existing file, then that property will only hold on Windows, so you'd need to do something else for non-Windows platforms at every callsite of rename. To me, it seems like this would just sort-of move the burden of creating a cross-platform abstraction to the users/callsites ofrename
(and force users to keep in their heads all the different semantics for each platform).Maybe it would make sense to split
rename
into a POSIX-like version and a "native" version? This would still give access to consistent cross-platformrename
semantics while also providing a public API for the most syscall-optimalrename
implementation for each platform.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.
Mainly, one would be aware of the differences in platforms and then take the following actions:
This is a reasonable set of requirements, still allows for useful abstraction, and eliminates the fallback code from being needed at all.
Most users will not need this fallback logic, and the ones that do should absolutely have this logic pushed up into the application domain.
This is the intention of
std.os.rename
(related: #5019). My suggestion here is to make this be one of the functions that does not attempt a posix compatibility layer on Windows, since it's not really a directly lowerable operation.I suppose we could consider trying hard to provide posix compatibilty layers for windows, but I think it would result in better software in practice if users were strongly encouraged instead towards structuring their projects to avoid not-strictly-necessary compatibility layers.
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.
Just to give a complete picture, these are all the known differences between Windows/POSIX rename when not using
FILE_RENAME_POSIX_SEMANTICS
(and ignoring symlinks, not tested the symlink behavior yet; I expect that to be another can of worms):ENOTDIR
on POSIX systemsACCESS_DENIED
on WindowsENOTEMPTY
, but fails on Windows withACCESS_DENIED
EISDIR
, but fails on Windows withACCESS_DENIED
I also did some very basic/preliminary benchmarking out of curiosity (not really relevant to the API design question, just potentially interesting on its own):
Benchmark code
The results on my computer are:
where:
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.
Nice work, as usual.
Having this flow chart in the doc comments, along with a list of behaviors that are fully cross platform, I think is an underrated deliverable. This is how we make "sweet spot" abstractions that will provide the building blocks for truly reusable code.
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.
Another wrinkle:
OBJECT_NAME_COLLISION
instead ofACCESS_DENIED
, but the rename semantics are the same as outlined above:rename
gains POSIX-like semantics and returnsOBJECT_NAME_COLLISION
for the error cases:So there may not technically be a knowable/defined behavior for
FileRenameInformation
on Windows, since it seems to ultimately be dictated by the underlying filesystem.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.
Would preferring failure if one of the error cases are true be possible to unify the behavior in std across os and drives/filesystems?
EDIT: Ignore the following proposal. Not atomic thus not very well thought out.