From f306ea8d1451e95fd7d445845211fa4f7ea0e57b Mon Sep 17 00:00:00 2001 From: FaQ Date: Sun, 5 May 2019 19:27:09 +0200 Subject: [PATCH 1/3] Fix #285 by removing targets in reverse lexical order (idea: @schettino72) --- CHANGES | 1 + doit/task.py | 32 +++++++++++++------------------- tests/test_task.py | 23 ++++++++++++++++++++--- 3 files changed, 34 insertions(+), 22 deletions(-) diff --git a/CHANGES b/CHANGES index de1562fe..2ac3bf14 100644 --- a/CHANGES +++ b/CHANGES @@ -21,6 +21,7 @@ Changes - Add configuration `DOIT_CONFIG` `action_string_formatting` to control command action formatter. - Fix `result_dep`, use result **after** its execution - Fix #286: Cannot use functools.partial as title + - Fix #285: Unexpected targets' cleanup order 0.31.1 (*2018-03-18*) diff --git a/doit/task.py b/doit/task.py index 4859e33b..f2fbe029 100644 --- a/doit/task.py +++ b/doit/task.py @@ -573,26 +573,20 @@ def dict_to_task(task_dict): def clean_targets(task, dryrun): """remove all targets from a task""" - files = [path for path in task.targets if os.path.isfile(path)] - dirs = [path for path in task.targets if os.path.isdir(path)] - - # remove all files - for file_ in files: - print("%s - removing file '%s'" % (task.name, file_)) - if not dryrun: - os.remove(file_) - - # remove all directories (if empty) - for dir_ in dirs: - if os.listdir(dir_): - msg = "%s - cannot remove (it is not empty) '%s'" - print(msg % (task.name, dir_)) - else: - msg = "%s - removing dir '%s'" - print(msg % (task.name, dir_)) + for target in sorted(task.targets, reverse=True): + if os.path.isfile(target): + print("%s - removing file '%s'" % (task.name, target)) if not dryrun: - os.rmdir(dir_) - + os.remove(target) + elif os.path.isdir(target): + if os.listdir(target): + msg = "%s - cannot remove (it is not empty) '%s'" + print(msg % (task.name, target)) + else: + msg = "%s - removing dir '%s'" + print(msg % (task.name, target)) + if not dryrun: + os.rmdir(target) # uptodate diff --git a/tests/test_task.py b/tests/test_task.py index 8d63679d..b59d1e2e 100644 --- a/tests/test_task.py +++ b/tests/test_task.py @@ -374,8 +374,11 @@ class TestTaskClean(object): def tmpdir(self, request): tmpdir = {} tmpdir['dir'] = tempfile.mkdtemp(prefix='doit-') + tmpdir['subdir'] = tempfile.mkdtemp(dir=tmpdir['dir']) files = [os.path.join(tmpdir['dir'], fname) - for fname in ['a.txt', 'b.txt']] + for fname in ['a.txt', + 'b.txt', + os.path.join(tmpdir['subdir'], 'c.txt')]] tmpdir['files'] = files # create empty files for filename in tmpdir['files']: @@ -412,14 +415,15 @@ def test_clean_non_existent_targets(self): def test_clean_empty_dirs(self, tmpdir): # Remove empty directories listed in targets - targets = tmpdir['files'] + [tmpdir['dir']] + targets = tmpdir['files'] + [tmpdir['subdir']] t = task.Task("xxx", None, targets=targets, clean=True) assert True == t._remove_targets assert 0 == len(t.clean_actions) t.clean(StringIO(), False) for filename in tmpdir['files']: assert not os.path.exists(filename) - assert not os.path.exists(tmpdir['dir']) + assert not os.path.exists(tmpdir['subdir']) + assert os.path.exists(tmpdir['dir']) def test_keep_non_empty_dirs(self, tmpdir): # Keep non empty directories listed in targets @@ -433,6 +437,19 @@ def test_keep_non_empty_dirs(self, tmpdir): assert expected == os.path.exists(filename) assert os.path.exists(tmpdir['dir']) + def test_clean_in_reverse_lexical_order(self, tmpdir): + # Remove targets in reverse lexical order so that subdirectories' order + # in the targets array is irrelevant + targets = tmpdir['files'] + [tmpdir['dir'], tmpdir['subdir']] + t = task.Task("xxx", None, targets=targets, clean=True) + assert True == t._remove_targets + assert 0 == len(t.clean_actions) + t.clean(StringIO(), False) + for filename in tmpdir['files']: + assert not os.path.exists(filename) + assert not os.path.exists(tmpdir['dir']) + assert not os.path.exists(tmpdir['subdir']) + def test_clean_actions(self, tmpdir): # a clean action can be anything, it can even not clean anything! c_path = tmpdir['files'][0] From 30b11bad6a15182eca8269fbb828cee5d72e1159 Mon Sep 17 00:00:00 2001 From: FaQ Date: Sun, 5 May 2019 19:57:30 +0200 Subject: [PATCH 2/3] Document removal order --- doc/cmd_other.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/doc/cmd_other.rst b/doc/cmd_other.rst index 14ef13cf..f06a3442 100644 --- a/doc/cmd_other.rst +++ b/doc/cmd_other.rst @@ -121,6 +121,12 @@ include a *clean* attribute. This attribute can be ``True`` to remove all of its target files. If there is a folder as a target it will be removed if the folder is empty, otherwise it will display a warning message. +.. note:: + + The targets' removal order will be the reverse of their lexical ordering. + This ensures that the directory structure formed by the targets is correctly + removed irrespective of their order in the ``targets`` array. + The *clean* attribute can be a list of actions. An action could be a string with a shell command or a tuple with a python callable. From 2b8d41df6de9b8486c5ce7756fdff3de2360988e Mon Sep 17 00:00:00 2001 From: FaQ Date: Mon, 6 May 2019 23:24:34 +0200 Subject: [PATCH 3/3] Describe what changed on CHANGES file, not issue title --- CHANGES | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGES b/CHANGES index 2ac3bf14..76d5a78c 100644 --- a/CHANGES +++ b/CHANGES @@ -20,8 +20,8 @@ Changes - Make it possible to use a custom encoder when using config_changed with a dict. - Add configuration `DOIT_CONFIG` `action_string_formatting` to control command action formatter. - Fix `result_dep`, use result **after** its execution - - Fix #286: Cannot use functools.partial as title - - Fix #285: Unexpected targets' cleanup order + - Fix #286: Support `functools.partial` on tasks' dict metadata `task.title` + - Fix #285: `clean` command, remove targets in reverse DFS order 0.31.1 (*2018-03-18*)