search

wbond / package_control

The Sublime Text package manager
https://packagecontrol.io
4.8k stars 818 forks source link

Avoid data loss by not clearing directory that is pointed to by a symlink #1473

Closed rchl closed 4 years ago

rchl commented 4 years ago

Don't remove the package directory that is pointed to by a symlink. Instead, remove the symlink itself.

Handle symlink case from delete_directory() and in various places where clear_directory() is used so that directory pointed to by a symlink is never cleared.

Here is a list of all the places that deal with removal:

clear_directory()

delete_directory()

Resolves #1460

rchl commented 4 years ago

Linux tested OK.

BTW, the official release of PC not only removes all files within the symlinked directory but also fails to remove the symlink itself:

Exception in thread Thread-5:
Traceback (most recent call last):
  File "./python3.3/threading.py", line 901, in _bootstrap_inner
  File "/home/me/.config/sublime-text-3/Packages/Package Control/package_control/commands/remove_package_command.py", line 84, in run
    self.result = self.manager.remove_package(self.package)
  File "/home/me/.config/sublime-text-3/Packages/Package Control/package_control/package_manager.py", line 1960, in remove_package
    os.rmdir(package_dir)
NotADirectoryError: [Errno 20] Not a directory: '/home/me/.config/sublime-text-3/Packages/TrailingSpaces'

This PR obviously fixes that issue.

rchl commented 4 years ago

So I could work on that but I'd like some assurance first that it's gonna be handled and integrated in not too long.

It's not that I'm impatient but I don't want to waste my time on something that will linger for months and potentially will need to be picked up again later anyway.

I know you are around @wbond and the project is well alive but I would prefer to handle this when you are available to handle feedback and integration. So let me know when is a good time please.

(I'm basing this comment mostly on the fact that no PRs were integrated in over 6 months.)

wbond commented 4 years ago

I will definitely look into this when I'm nest working on the PC codebase. That should probably be relatively soon (maybe a few weeks to a month?) since I need to work out how dependencies are going to work with Python 3.8 and all.

rchl commented 4 years ago

Pushed what I believe is a better fix and updated my initial comment with a list of all affected places.

rchl commented 4 years ago

Windows and macOs tested OK too.

michaelblyons commented 4 years ago

Fixes #1476?

wbond commented 4 years ago

I think that delete_directory() should be renamed to something like unlink_or_delete_directory() to make it clear in usage that it will unlink a symlink if possible.

deathaxe commented 4 years ago

Note, that stdlib functions of python 3.8 are said to handle symlinks/junctions correctly, even on Windows OS. So once the transition has been made, only parts of this PR might be relevant then.

rchl commented 4 years ago

Note, that stdlib functions of python 3.8 are said to handle symlinks/junctions correctly, even on Windows OS. So once the transition has been made, only parts of this PR might be relevant then.

Then it would probably help this case:

            if sys.platform == 'win32':
                os.rmdir(path)
            else:
                os.unlink(path)

but probably (speculating) none of the other changes as it seems to me more like a logic error than deficiency of the the python API.

What will shutil.rmtree do in py38 for a directory symlink? Will it still remove the target of the symlink or the symlink itself?

deathaxe commented 4 years ago

shutil.rmtree throws OSError: Cannot call rmtree on a symbolic link when path is a symlink or junction. I guess it is a relic of older python not handling symlinks well. Maybe should open a bug report if not yet exists.

For details see: https://bugs.python.org/issue1669

os.rmdir() removes the junction/symlink but leaves the source directory intact.

wbond commented 4 years ago

Thanks for your work on this!

sparr commented 4 years ago

Thanks everyone for this. Losing hours of work on a package was really demotivating, and I'm glad to see this particular failure mode won't happen again.

wbond commented 4 years ago

@sparr I'm sure you've figured this out already, but if you are working on a package, make sure it is not being managed by Package Control. Look for package-metadata.json. If that exists, Package Control will treat it as managed, and will perform standard operations like removing orphaned packages, whether they are symlinks or not.

rchl commented 4 years ago

I think that triggering remove package has deleted it whether it was managed or not. That's how I lost some of my work as I wanted to test something but forgot that I was using local package.