Support for removing of symlinks

[Radim Novotny]

_uninstall method of the Buildout class does not support removing of symbolic links pointing to directory.
I'm writing recipe which creates symbolic links to other locations and this recipe fails on reinstall. _uninstall method should be changed to:
(first check if f is file or link and then check if f is directory. Symlink pointing to directory is treated as directory)

    def _uninstall(self, installed):
        for f in installed.split('\n'):
            if not f:
                continue
            f = self._buildout_path(f)
            if os.path.isfile(f) or os.path.islink(f):
                os.remove(f)
            elif os.path.isdir(f):
                shutil.rmtree(f)

[Petri Savolainen (petri-savolainen)]

To rephrase Radim, symbolic links pointing to directories are not properly removed by buildout. This is because in _uninstall, removed filesystem items are first checked for whether they are directories. An os.path.isdir(link) check for a link pointing to a directory returns True, similarly to check on a link pointing to a file returns True for an isfile(link) check.

Buildout checks first for whether the filesystem item to be removed returns True for isdir(). This is wrong, because buildout then tries to rmtree(link), which causes havoc.

Instead, in _uninstall, the check for file & link should be first.The issue can thus simply be fixed by reversing the checks.

[Petri Savolainen (petri-savolainen)]

This bug is misnamed; it should be "fix removal of symlinks pointing to a directory" or something like that.

[Petri Savolainen (petri-savolainen)]

patch attached

[Kazuhiko (kazuhiko)]

+1 for this change.

BTW, if 'chmod(path, 0600)' trick in buildout.rmtree ALREADY succeeded on the symlink to a directory, the destination directory had 0600 permission and its contents can never be deleted. i.e. deletion of the content raises an exception and buildout.rmtree is called, but chmod(path, 0600) still fails because its parent directory has no 'x' permission.

So I propose that chmod 2nd argument in biuldout.rmtree should be 0700, at least for directories.

[Michael Hudson-Doyle (mwhudson)]

I've just been bitten by this :/

[Petri Savolainen (petri-savolainen)]

FYI, there's been a new zc.buildout release. Sadly, it contains no fix to this bug, but I have forked zc.buildout 1.6.0 (and applied the bug fix to it) at github; see https://github.com/koodaamo/zc.buildout

Kazuhiko: are you saying the mode change to 0700 in buildout.rmtree should be done also? In other words, even if the code already has the fix for bug #144228 ?

[Petri Savolainen (petri-savolainen)]

BTW, note that the version of the forked package at github is now "1.6.0-fix-lp144228", not 1.6.0...

[Kazuhiko (kazuhiko)]

Petri:
I just looked at your code and it should be good to prevent this issue.
My proposal,"mode change to 0700 in buildout.rmtree", is required to save the case where the directory was *already* polluted by this issue, i.e. the the mode of the destination directory of a symlink was alreay changed to 0600 that raises exception when buildout tries to delete its contents.

So if we only care further buildout results that are created the buildout with this fix, it is not mandatory. But if we care existing buildout results that are created by buildout without this fix, it will make some people happy.

[Domen Kožar (ielectric+)]

If you provide a patch with a test, I'll pull the changes :-)

[Petri Savolainen (petri-savolainen)]

Thanks Kazuhiko. Domen, what should we patch & provide a test for now? While there's a 1.7 beta release, I can only see a 1.6 branch at https://github.com/buildout/buildout ...?

[Petri Savolainen (petri-savolainen)]

BTW, should we copy this issue to the github issue tracker as well? I would really like to see this bug fixed, but I am rather a novice what it comes to writing buildout tests.

[Jim Fulton (jim-zope)]

The 1.6.x branch is miss-named. It should have been named
"1". I'll rename it.

In the mean time, I suggest creating a pull request against master.

Someone can then cherry-pick this to apply it to 1 if desired.

[Petri Savolainen (petri-savolainen)]

I have forked master and applied the fix.

I've not applied Kazuhiko's further suggestion (see above): afaik, it runs only when rmtree is called with a link pointing to a directory. As that will no longer be case with the fix applied, the modified code will not run (for the linked directories rmtree has previously left at mode 0600) any more. Please do correct me if I'm mistaken.

It then occurred to me to checked whether rmtree is called from elsewhere in buildout.py. I found another case that will potentially cause trouble exactly the same way. See code starting at line 1271, in the Options._call method:

https://github.com/buildout/buildout/blob/master/src/zc/buildout/buildout.py#L1272

Unless anyone objects, I will fix that as well and then submit a pull request. Although I don't know in what scenario one would pass a link (pointing to a directory) to Options._call, I'd rather fix it now than risk running into this issue again later.

[Kazuhiko (kazuhiko)]

I mean that once a directory was polluted by chmod(0600), the contents in the directory will no longer be deleted unless we chmod it with '+x'.

I don't care if my suggestion is applied or not so much if this pollution will no longer happen, but even though this pollution should no longer happen, still

    def retry_writeable (func, path, exc):
        os.chmod (path, 0600)
        func (path)

this code is anyway wrong for directories and can make more undeletable.

[Petri Savolainen (petri-savolainen)]

Good point. I wasn't thinking that far. I'll add it to the pull request. Thanks for persisting!

[Wouter Vanden Hove (wouter-wvhconsulting)]

any uodate on this?
bug still exists in buildout 2.2.1