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

feat: Move files to .trash folder if they are in use #953

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

adament
Copy link
Contributor

@adament adament commented Nov 19, 2024

Description

Attempt to move files to .trash folder if they are in use, this should solve #578. The issue is that on Windows we cannot delete files that are in use. When using a global package cache and hardlinking files into environments this means that if a given DLL or exe is in use in any environment with the same package the deletion files. But we can rename the hardlink. Conda and mamba have similar fallback behaviour.

The solution in this PR is slightly different than the ones used in conda and mamba as far as I am aware. This PR:

  • Creates a folder in .trash in the root of the environment rather than having a file like mamba_trash.txt to keep track of all the files which could not be deleted. Then a separate empty_trash procedure should simply attempt to delete all files in the .trash folder and if successful, it should remove the .trash folder.
  • When moving a file to .trash it is renamed to uuid.rattler_trash. Mamba and conda kept the original filename and appended .mamba_trash, but then they had to worry about filename collisions and having some fallback procedure for coming up with new names on a collision. It thus seemed simpler to me to just give a unique name to each file in the trash folder. Though in this way we lose information about the origin of the file.

Please tell me if you think the approach or the implementation is wrong.

@adament adament changed the title RFC: Move files to .trash folder if they are in use feat: Move files to .trash folder if they are in use Nov 19, 2024
On Windows attempting to delete a file which is in use results in a
permission denied error. However it is possible to rename the file, so
move the files to a folder target_prefix\.trash instead for later
cleanup.

The files are renamed to UUID.rattler_trash to avoid having to worry
about handling filename collisions.
@wolfv
Copy link
Contributor

wolfv commented Nov 19, 2024

Awesome! I am wondering - did you check that "moving" to a different folder works when the file is in use? If yes, then this is a great solution.

I also thought that we could try to have filename.<uuid>.trash as a filename but it might get longer than the allowed length.

I think going for UUID is fine with me. We would probably also be on the safe side with, idk, filename + 4 random characters or so.

There are also ways to add metadata to files that are NTFS specific (similar to xattrs on Unix) but not sure if that would be worth it to investigate.

@adament
Copy link
Contributor Author

adament commented Nov 20, 2024

I have attempted to add a test to the test suite which should test that the trash procedure works when a program is running, and I just checked manually in explorer that I can move python.exe when it is in use. My current workaround for pixi not having trash support is to just move the entire .pixi folder somewhere else and then run pixi install. So I feel reasonably confident that it should work.

I am fine with any sort of filename template. However when I looked at it rand is not currently a dependency of rattler, so generating a uuid seemed the simplest way to get a sufficient amount of randomness to not have to worry about collisions. I like the filename.<uuid>.trash template but as you write I am also a bit worried about exceeding path length if the filename is very long. Do you want me to make the change to include the filename?

@baszalmstra
Copy link
Collaborator

Lets add the filename indeed to make it slightly easier to debug. If filename becomes to long we can worry about it then.

Furtermore change the test to be based on bat rather than lzmainfo, as
this seems more robust in my tests on my machine.
@adament
Copy link
Contributor Author

adament commented Nov 20, 2024

I have changed it to the filename.<uuid>.trash pattern and attempted to make the test more robust.

@baszalmstra baszalmstra linked an issue Nov 21, 2024 that may be closed by this pull request
@baszalmstra baszalmstra merged commit 08c395c into conda:main Nov 21, 2024
15 checks passed
@baszalmstra
Copy link
Collaborator

Thank you so much for this PR and your patience reviewing our comments! Highly appreciated! :)

@baszalmstra baszalmstra mentioned this pull request Nov 21, 2024
@adament
Copy link
Contributor Author

adament commented Nov 21, 2024

Thank you for taking the time to review and comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add retry/backoff to file removing on Windows
3 participants