Merge d7b1881 into 13b036f

CHANGELOG.rst

@@ -8,6 +8,15 @@ The format is based on `Keep a Changelog
 <http://keepachangelog.com/en/1.0.0/>`_ and this project adheres to `Semantic
 Versioning <http://semver.org/spec/v2.0.0.html>`_.
 
+[UNRELEASED]
+============
+
+Fixed
+-----
+
+- Astrality now cleans up directories created by `symlink` and `copy` actions
+  when modules are cleaned up with the `--cleanup` flag.
+
 [1.1.0] - 2018-06-24
 ====================
 

astrality/actions.py

@@ -396,8 +396,7 @@ def execute(self, dry_run: bool = False) -> Dict[Path, Path]:
                 continue
 
             logger.info(log_msg)
-            symlink.parent.mkdir(parents=True, exist_ok=True)
-
+            self.creation_store.mkdir(symlink.parent)
             self.creation_store.backup(path=symlink)
             symlink.symlink_to(content)
             self.creation_store.insert_creation(
@@ -466,8 +465,7 @@ def execute(self, dry_run: bool = False) -> Dict[Path, Path]:
                 continue
 
             logger.info(log_msg)
-            copy.parent.mkdir(parents=True, exist_ok=True)
-
+            self.creation_store.mkdir(copy.parent)
             self.creation_store.backup(path=copy)
             utils.copy(
                 source=content,

astrality/persistence.py

@@ -17,6 +17,7 @@ class CreationMethod(Enum):
     """Ways modules can create files."""
     COMPILE = 'compiled'
     COPY = 'copied'
+    MKDIR = 'mkdir'
     SYMLINK = 'symlinked'
 
 
@@ -85,15 +86,16 @@ def insert(
         self,
         module: str,
         creation_method: CreationMethod,
-        contents: Iterable[Path],
+        contents: Iterable[Optional[Path]],
         targets: Iterable[Path],
     ) -> None:
         """
         Insert files created by a module.
 
         :param module: Name of module which has created the files.
         :param creation_method: Type of action which has created the file.
-        :param contents: The source files used in creating the files.
+        :param contents: The source files used in creating the files. None if
+          a created directory.
         :param targets: The files that have be created.
         """
         # We do not want to insert empty sections, to reduce reduntant clutter
@@ -122,7 +124,9 @@ def insert(
                     creation['hash'] = hashlib.md5(
                         target.read_bytes(),
                     ).hexdigest()
-                except PermissionError:
+                except (PermissionError, IsADirectoryError):
+                    # Currently, we do not hash directories or content with
+                    # lacking permissions to read.
                     creation['hash'] = None
 
         if modified:
@@ -151,7 +155,15 @@ def cleanup(self, module: str, dry_run: bool = False) -> None:
         """
         logger = logging.getLogger(__name__)
         module_creations = self.creations.get(module, {})
-        for creation, info in module_creations.items():
+
+        # This dictionary will be populated with all directories which we can't
+        # delete. Those should still be tracked after cleaning up the module.
+        dangling_directories: Dict[str, CreationInfo] = {}
+
+        for creation, info in sorted(
+            module_creations.items(),
+            key=lambda item: -len(Path(item[0]).parts),  # depth-first order
+        ):
             creation_method = info['method']
             content = info['content']
             backup = info['backup']
@@ -165,7 +177,18 @@ def cleanup(self, module: str, dry_run: bool = False) -> None:
                 continue
 
             creation_path = Path(creation)
-            if creation_path.exists():
+            if creation_path.is_dir():
+                try:
+                    logger.info(log_msg)
+                    creation_path.rmdir()
+                except OSError:
+                    logger.warning(
+                        f'Failed to remove created directory "{creation}", '
+                        'as it contains new non-module files since creation! '
+                        'Try to delete files manually and then cleanup again.',
+                    )
+                    dangling_directories[creation] = info
+            elif creation_path.exists():
                 logger.info(log_msg)
                 creation_path.unlink()
             else:
@@ -180,6 +203,8 @@ def cleanup(self, module: str, dry_run: bool = False) -> None:
 
         if not dry_run:
             self.creations.pop(module, None)
+            if dangling_directories:
+                self.creations[module] = dangling_directories
             utils.dump_yaml(data=self.creations, path=self.path)
 
     def backup(self, module: str, path: Path) -> Optional[Path]:
@@ -210,6 +235,33 @@ def backup(self, module: str, path: Path) -> Optional[Path]:
         utils.dump_yaml(data=self.creations, path=self.path)
         return backup
 
+    def mkdir(self, module: str, path: Path) -> None:
+        """
+        Create directory with parents and persist the creation.
+
+        A no-op if the directory already exists. All parent directories that
+        also needs to be created will also be persisted.
+
+        :param module: Module that requests the directory to be created.
+        :param path: Directory path to be made.
+        """
+        # First find out exactly which directories, including parents, will
+        # be created.
+        created_directories = []
+        for new_directory in (path, *path.parents):
+            if not new_directory.exists():
+                created_directories.append(new_directory)
+            else:
+                break
+
+        path.mkdir(parents=True, exist_ok=True)
+        self.insert(
+            module=module,
+            creation_method=CreationMethod.MKDIR,
+            contents=len(created_directories) * [None],
+            targets=created_directories,
+        )
+
     def __contains__(self, path) -> bool:
         """Return True if path has been created by Astrality."""
         return any(
@@ -245,6 +297,16 @@ def backup(self, path: Path) -> Optional[Path]:
         """
         return self.creation_store.backup(module=self.module, path=path)
 
+    def mkdir(self, path: Path) -> None:
+        """
+        Create directory with parents and persist the creation.
+
+        A no-op if the directory already exists.
+
+        :param path: Directory path to be made.
+        """
+        self.creation_store.mkdir(module=self.module, path=path)
+
     def insert_creation(
         self,
         content: Path,

astrality/tests/actions/test_copy_action.py

@@ -217,3 +217,36 @@ def test_backup_of_copy_target(create_temp_files):
     # And when cleaning up the module, the backup should be restored
     CreatedFiles().cleanup(module='test')
     assert target.read_text() == 'original'
+
+
+def test_cleanup_of_created_directory(create_temp_files, tmpdir):
+    """Created directories should be cleaned up."""
+    tmpdir = Path(tmpdir)
+    [content] = create_temp_files(1)
+
+    # The target requires a new directory to be created
+    directory = tmpdir / 'dir'
+    target = directory / 'target.tmp'
+
+    # Execute copy action
+    copy_options = {
+        'content': str(content.name),
+        'target': str(target),
+    }
+    created_files = CreatedFiles().wrapper_for(module='test')
+    copy_action = CopyAction(
+        options=copy_options,
+        directory=content.parent,
+        replacer=lambda x: x,
+        context_store={},
+        creation_store=created_files,
+    )
+    copy_action.execute()
+
+    # The directory should now be created and persisted
+    assert directory.is_dir()
+    assert directory in created_files.creation_store
+
+    # And it should be deleted on cleanup
+    created_files.creation_store.cleanup(module='test')
+    assert not directory.is_dir()

astrality/tests/actions/test_symlink_action.py

@@ -226,3 +226,36 @@ def test_backup_of_symlink_target(create_temp_files):
     # And when cleaning up the module, the backup should be restored
     CreatedFiles().cleanup(module='test')
     assert target.read_text() == 'original'
+
+
+def test_cleanup_of_created_directory(create_temp_files, tmpdir):
+    """Created directories should be cleaned up."""
+    tmpdir = Path(tmpdir)
+    [content] = create_temp_files(1)
+
+    # The target requires a new directory to be created
+    directory = tmpdir / 'dir'
+    target = directory / 'target.tmp'
+
+    # Execute the symlink action
+    symlink_options = {
+        'content': str(content.name),
+        'target': str(target),
+    }
+    created_files = CreatedFiles().wrapper_for(module='test')
+    symlink_action = SymlinkAction(
+        options=symlink_options,
+        directory=content.parent,
+        replacer=lambda x: x,
+        context_store={},
+        creation_store=created_files,
+    )
+    symlink_action.execute()
+
+    # The directory should now exist and be persisted
+    assert directory.is_dir()
+    assert directory in created_files.creation_store
+
+    # But it should be deleted on cleanup
+    CreatedFiles().cleanup(module='test')
+    assert not directory.is_dir()

astrality/tests/persistence/test_created_files.py

@@ -3,6 +3,8 @@
 from pathlib import Path
 import shutil
 
+import pytest
+
 from astrality.persistence import (
     CreatedFiles,
     CreationMethod,
@@ -329,3 +331,135 @@ def test_taking_backup_of_symlinks(create_temp_files):
     created_files.cleanup(module='name')
     assert original_symlink.resolve() == original_target
     assert original_symlink.read_text() == 'original content'
+
+
+def test_creation_and_cleanup_of_directory(create_temp_files):
+    """Created directories should be tracked."""
+    # my_module is going to copy one file
+    created_files = CreatedFiles().wrapper_for(module='my_module')
+
+    # Specifically content -> target
+    content, target = create_temp_files(2)
+
+    # The target directory does not exist yet, so it creates it first
+    created_files.insert_creation(
+        content=None,
+        target=target.parent,
+        method=CreationMethod.MKDIR,
+    )
+
+    # Then copies the file over
+    created_files.insert_creation(
+        content=content,
+        target=target,
+        method=CreationMethod.COPY,
+    )
+
+    # These two creations should now be tracked
+    global_created_files = CreatedFiles()
+    creations = global_created_files.by('my_module')
+    assert len(creations) == 2
+    assert target.parent in global_created_files
+    assert target in global_created_files
+
+    # And a small sanity check, the directory actually exists
+    assert target.parent.is_dir()
+
+    # Now we introduce a small complication; an file **not** created by
+    # astrality is placed within this created directory.
+    external_file = target.parent / 'external.tmp'
+    external_file.touch()
+
+    # If we now clean up the module, the copied file can be deleted, but not
+    # the created directory, as that would delete an external file!
+    global_created_files.cleanup(module='my_module')
+    assert not target.exists()
+    assert target.parent.is_dir()
+
+    # And the directory is still tracked, even after the cleanup
+    assert target.parent in CreatedFiles()
+
+    # Now we delet this external file,
+    # such that the directory can be cleaned up.
+    external_file.unlink()
+    global_created_files.cleanup(module='my_module')
+    assert not target.parent.is_dir()
+
+
+def test_cleanup_of_recursive_directories(tmpdir):
+    """Recursively created directories should be cleaned up."""
+    tmpdir = Path(tmpdir)
+
+    # Three recursive directories are created
+    a = tmpdir / 'a'
+    b = a / 'b'
+    c = b / 'c'
+    c.mkdir(parents=True)
+
+    # And these creations are persisted
+    created_files = CreatedFiles().wrapper_for(module='my_module')
+    for directory in (c, a, b):
+        created_files.insert_creation(
+            content=None,
+            target=directory,
+            method=CreationMethod.MKDIR,
+        )
+
+    # And a file is copied over into the deepest of these directories
+    content = tmpdir / 'content.tmp'
+    target = c / 'target.tmp'
+    content.touch()
+    target.touch()
+    created_files.insert_creation(
+        content=content,
+        target=target,
+        method=CreationMethod.COPY,
+    )
+
+    # All these directories should be cleaned up
+    CreatedFiles().cleanup(module='my_module')
+    for directory in (a, b, c):
+        assert not directory.exists()
+
+    # Also the copied file
+    assert not target.exists()
+
+
+@pytest.mark.parametrize('with_wrapper', (False, True))
+def test_mkdir_method_of_created_files(tmpdir, with_wrapper):
+    """CreatedFiles should be able to create and persist directories."""
+    tmpdir = Path(tmpdir)
+
+    # Directory "a" already exists as a directory
+    a = tmpdir / 'a'
+    a.mkdir(parents=True)
+
+    # But directory b and c are supposed to be created
+    b = a / 'b'
+    c = b / 'c'
+
+    # We now want to create directory c and all parents
+    created_files = CreatedFiles()
+
+    if with_wrapper:
+        created_files.wrapper_for(module='my_module').mkdir(path=c)
+    else:
+        created_files.mkdir(module='my_module', path=c)
+
+    # The directories have been created
+    assert b.is_dir()
+    assert c.is_dir()
+
+    # And persisted
+    assert len(created_files.by('my_module')) == 2
+    assert b in created_files
+    assert c in created_files
+
+    # But a should not be counted as a created directory
+    assert a not in created_files
+
+    # Cleanup should only clean b and c, but not a
+    created_files.cleanup(module='my_module')
+    assert not b.is_dir()
+    assert not c.is_dir()
+    assert a.is_dir()