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

Deletion of symbolic link directories is inconsistent across systems #13194

Closed
HertzDevil opened this issue Mar 16, 2023 · 5 comments · Fixed by #13224
Closed

Deletion of symbolic link directories is inconsistent across systems #13194

HertzDevil opened this issue Mar 16, 2023 · 5 comments · Fixed by #13224

Comments

@HertzDevil
Copy link
Contributor

The following will raise only on Windows, not on Unix-like systems:

Dir.mkdir("foo")
File.symlink("foo", "bar")
File.delete("bar") # Error deleting file: 'bar': Permission denied (File::AccessDeniedError)

Whereas the opposite holds for the following:

Dir.mkdir("foo")
File.symlink("foo", "bar")
Dir.delete("bar") # Unable to remove directory: 'bar': Not a directory (File::Error)

This might be due to the fact that "is a symbolic link" and "is a directory" are two distinct properties on Windows, rather than two values of a single property in POSIX. This is affecting the following spec since FileUtils.rm_r always calls File.delete on symlinks:

pending_win32 "doesn't follow symlinks" do
with_tempfile("rm_r-removed", "rm_r-linked") do |removed_path, linked_path|
link_path = File.join(removed_path, "link")
file_path = File.join(linked_path, "file")
test_with_string_and_path(removed_path) do |arg|
Dir.mkdir(removed_path)
Dir.mkdir(linked_path)
File.symlink(linked_path, link_path)
File.write(file_path, "")
Dir.exists?(removed_path).should be_true
Dir.exists?(linked_path).should be_true
File.exists?(file_path).should be_true
FileUtils.rm_r(arg)
Dir.exists?(removed_path).should be_false
Dir.exists?(linked_path).should be_true
File.exists?(file_path).should be_true
FileUtils.rm_rf(linked_path)
end
end
end

What is the desired behavior here?

@jhass
Copy link
Member

jhass commented Mar 17, 2023

I think these are basically trying to mirror rm/rmdir. How does CMD del/rmdir behave in these situations?

I'd err on the UNIX side of things.

@HertzDevil
Copy link
Contributor Author

This successfully deletes the symlink directory without touching the target:

> mkdir foo
> mklink /D bar foo
> rmdir bar

On Windows, if a directory is given to del, it automatically deletes the directory's children rather than the directory itself. In this case del bar traverses the symlink and deletes foo/*, leaving both directories intact. There isn't a command that fails when the argument names a directory.

Now CMD doesn't prevent you from creating a file symlink to a directory. It must then be deleted like a file and cannot be cd'ed into:

> mkdir foo
> mklink bar foo
> cd bar
The directory name is invalid.
> rmdir bar
The directory name is invalid.
> del bar

The opposite is also true, you can create a directory symlink to a file:

> echo 1 > foo
> mklink /D bar foo
> cd bar
The directory name is invalid.
> del bar
The directory name is invalid.
> rmdir bar

@straight-shoota
Copy link
Member

I think we should stick with the current semantics, which are obviously inspired by Unix tools, and implement the same behaviour on Windows.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Mar 17, 2023

So, am I getting this right?

Windows

Method Symlink? Type Action
File.delete No ✖ File call Crystal::System::File.delete
Dir.delete No ✖ File raise File::Error
File.delete No ✖ Directory raise File::Error
Dir.delete No ✖ Directory call Crystal::System::Dir.delete
File.delete Yes ✔ File call Crystal::System::File.delete
Dir.delete Yes ✔ File raise File::Error
File.delete Yes ✔ Directory raise File::Error call Crystal::System::Dir.delete
Dir.delete Yes ✔ Directory call Crystal::System::Dir.delete raise File::Error

POSIX

Method Symlink? Type Action
File.delete No ✖ File call Crystal::System::File.delete
Dir.delete No ✖ File raise File::Error
File.delete No ✖ Directory raise File::Error
Dir.delete No ✖ Directory call Crystal::System::Dir.delete
File.delete Yes ✔ N/A call Crystal::System::File.delete
Dir.delete Yes ✔ N/A raise File::Error

This should make FileUtils.rm_r work without any modifications.

@straight-shoota
Copy link
Member

Looks good. The behaviour of works vs. does not work is identical on Windows on POSIX when expanding the N/A type into File and Directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants