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

Fix Linux move_to_trash wrongly reporting files as not found #79284

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Jul 10, 2023

We can't rely on the error code from gio or kioclient5, in my rudimentary testing they return 1 for both missing files and other situations like not having a Trash can on the mounted volume.

I added path existence checks to all platforms as that seemed useful, even if it was wrongly implemented for Linux.
Edit: This would need more work, I left it out of this PR for now.

Fixes #79108.
Partially reverts #67158, but still fixes #67157.

CC @amoriqbal @romlok @starccy I only did basic tests on Linux, would appreciate you checking that it solves #79108.
You can test with CI artifacts once the build finishes.

@akien-mga akien-mga added this to the 4.2 milestone Jul 10, 2023
@akien-mga akien-mga requested review from a team as code owners July 10, 2023 13:27
@starccy
Copy link

starccy commented Jul 10, 2023

Thank you for your work, I'll test it later.

@starccy
Copy link

starccy commented Jul 10, 2023

I can confirm the error has disappeared on my system (Linux + Gnome).

@romlok
Copy link
Contributor

romlok commented Jul 10, 2023

The only Linux editor artifact I could see was the -mono version, which didn't work on my machine, but I was able to compile the branch locally and can confirm that this change fixes the issue for me too 👍

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving based on the comments above.

@starccy
Copy link

starccy commented Jul 11, 2023

Oops, I missed a special use case.
When I create a broken symlink file, I cannot delete the file itself. And the "No such file or directory" error be presented.

if I get it correctly, the DirAccess::file_exists only checks the file it points to, but not for the symlink itself.

bool success = (stat(p_file.utf8().get_data(), &flags) == 0);

I was wondering why to use stat here instead of lstat, is this an expected behavior?

@akien-mga
Copy link
Member Author

akien-mga commented Jul 11, 2023

Thanks for testing further, that's indeed a regression compared to 4.0.3, where move_to_trash handles deleting symlinks fine on Linux.

Changing stat to lstat in DirAccessUnix could be considered but it's a very core change with a lot of implications, that would need to be assessed in depth.

For this PR I'll remove the check added in all platforms to handle missing files, and just let Linux behave like Windows and macOS: no validation.


On my system the PR in its current state (a6e75f3) prints these errors when trying to remove a file which doesn't exist:

gio: file:///home/akien/Test/test4: Error trashing file /home/akien/Test/test4: No such file or directory
kf.kio.workers.trash: Directory "/media/windows/.Trash-1000" exists but didn't pass the security checks, can't use it
kioclient5: Access denied to trash:/http:⁄⁄test4⁄.
ERROR: Could not create child process: gvfs-trash
   at: execute (./drivers/unix/os_unix.cpp:541)
ERROR: Can't rename file "test4" to "/test4"
   at: move_to_trash (platform/linuxbsd/os_linuxbsd.cpp:1075)

It's bad, but it's the same as in 4.0.3 so this still fixes the regression and should be suitable for a 4.1.1 cherry-pick.

The whole move_to_trash functionality could use a good cleanup on all platforms to properly handle validating input, and make the Linux one a bit sturdier as the current code seems a bit brittle to me.

We can't rely on the error code from `gio` or `kioclient5`, in my
rudimentary testing they return `1` for both missing files and other
situations like not having a Trash can on the mounted volume.

Fixes godotengine#79108.
@akien-mga akien-mga changed the title Check that path exists in OS::move_to_trash, fix Linux regression Fix Linux move_to_trash wrongly reporting files as not found Jul 11, 2023
@starccy
Copy link

starccy commented Jul 11, 2023

Yes, the error messages from gio/kioclient5 are annoying, but thankfully the method still works as properly : )

Changing stat to lstat in DirAccessUnix could be considered but it's a very core change with a lot of implications, that would need to be assessed in depth.

In fact, some languages like Python, have separate APIs to let users decide to choose the one they think is appropriate: os.path.exist and os.path.lexists, which can be imitated in gdscript to avoid breaking changes as much as possible. AFAIK, the gdscript users have no chance to determine if a file is a symlink. They might never or hard to check existence for a broken symbolic link.

@akien-mga akien-mga merged commit 5dbbdaf into godotengine:master Jul 11, 2023
@akien-mga akien-mga deleted the fix-linux-os-move_to_trash branch July 11, 2023 09:32
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 11, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment