Unexpected targets' cleanup order

[slaperche-scality]

I wanted a task that creates a directory hierarchy, so I was careful to sort my list of targets from the root to the leaves and I was expecting that the cleanup would be done in the reverse order (thus, leaves would be rmdired before the root but it seems the cleanup iterate over the targets from the start.

Given this dodo.py

#!/usr/bin/env python
# coding: utf-8

import os

def task_test():
    """test"""
    def mkdir(targets):
        for directory in targets:
            os.mkdir(directory)

    return {
        'actions': [mkdir],
        'targets': ['foo', 'foo/bar'],
        'clean': True,
    }

I got

% doit clean test
test - cannot remove (it is not empty) 'foo'
test - removing dir 'foo/bar'

Now, it's easy to workaround (my action can iterate over reversed(targets) or I can write a custom cleanup action) but it's not ideal.

Is it a bug or was I having unjustified expectations here?

[schettino72]

Is it a bug or was I having unjustified expectations here?

You are right. Makes sense clean-up order is reversed. It is already reverses tasks, not reversing the list of targets was an oversight. Should be a trivial fix.

[facundofc]

Hi there! I've been taking a look at the code that removes the targets (here) and as you can see it's not even removing the targets in order. It actually splits the list into files and directories and removes them in that order. I track down this to the very old commit 97855edf.

I guess the rationale was to first remove the files so as to maybe then leave the directories empty and be able to delete them too, so that if your task has 'targets': ['a_path/a_file', 'a_path'] then the cleaning works.

I can think of two solutions:

1. We reverse both lists, files and dirs, but keep deleting first files and then directories.
2. We reverse the targets' list and iterate over it, testing whether current element isdir or isfile and deleting as appropriate.

Both solutions have the potential to break existing tasks, but I think option 1. should be preferred since it seems less likely to do so (if some figured out this splitting mechanism, then fixing such a task would just require to reverse the targets' list; while with solution two it might be necessary to put some thought into the targets' reordering to make the cleaning work again).

What's your opinion?

[slaperche-scality]

Solution 2 has the advantage of being predictable (there is no "magic" that separate files and directories) and consistent (is this splitting also done for creation? Like, creating directories first and then files? If not, it may be confusing).

But 1. is indeed less risky (even though one could argue that if people were relying on this, then they were relying on a non-documented internal implementation details and were taking risks).

I have a slight preference for 2, but I'm not against 1: in the end both are fine to me so you can pick what's easier/more maintainable for you :slightly_smiling_face:

[schettino72]

My original thinking was: removal order does not matter, files belonging to a directory should be removed first so it succeeds in removing the directory. The problem was my naive and simplistic implementation.

So I would suggest a third solution: remove targets in reverse lexical order. This would guarantee files would be removed before the directories they belong to. And it would not break for people who were relying in current behaviour to remove all targets.

And of course it someone really wants to remove targets in a specific order (for whatever reason) they can do so by writing a custom clean() function. But I dont think even @slaperche-scality really cares about that as long as everything is removed.

What you guys think?

[slaperche-scality]

Sounds good to me.

I don't have constraints on the deletion order, as long as everything is cleaned up.

Actually, I was doing something similar in my code to copy a file tree, except that I was sorting by depth (number of path separators) instead of lexicographically (but should work as well), so that my mkdir where executed in from the root to the leaves (and I was expecting clean to reverse the list, hence this issue).