From 636d850c848a8ce90c037901621a1c391283d8ae Mon Sep 17 00:00:00 2001 From: Aman Sharma Date: Fri, 15 Nov 2019 01:54:40 +0530 Subject: [PATCH 1/2] Ensure `remove` accepts str and Path-like objects --- dvc/external_repo.py | 2 +- dvc/remote/local.py | 2 +- dvc/repo/destroy.py | 2 +- dvc/repo/get.py | 2 +- dvc/repo/init.py | 2 +- dvc/state.py | 2 +- dvc/utils/__init__.py | 14 -------------- dvc/utils/fs.py | 17 +++++++++++++++++ tests/basic_env.py | 2 +- tests/unit/utils/test_fs.py | 12 ++++++++++++ 10 files changed, 36 insertions(+), 21 deletions(-) diff --git a/dvc/external_repo.py b/dvc/external_repo.py index 874240bc7f..46657fc4d7 100644 --- a/dvc/external_repo.py +++ b/dvc/external_repo.py @@ -9,7 +9,7 @@ from dvc.config import NoRemoteError from dvc.exceptions import RemoteNotSpecifiedInExternalRepoError -from dvc.utils import remove +from dvc.utils.fs import remove REPO_CACHE = {} diff --git a/dvc/remote/local.py b/dvc/remote/local.py index 2145fc1701..bd20e91b55 100644 --- a/dvc/remote/local.py +++ b/dvc/remote/local.py @@ -26,13 +26,13 @@ from dvc.utils import file_md5 from dvc.utils import makedirs from dvc.utils import relpath -from dvc.utils import remove from dvc.utils import tmp_fname from dvc.utils import walk_files from dvc.utils.compat import fspath_py35 from dvc.utils.compat import open from dvc.utils.compat import str from dvc.utils.fs import move +from dvc.utils.fs import remove logger = logging.getLogger(__name__) diff --git a/dvc/repo/destroy.py b/dvc/repo/destroy.py index e5938e195a..0f92728541 100644 --- a/dvc/repo/destroy.py +++ b/dvc/repo/destroy.py @@ -1,4 +1,4 @@ -from dvc.utils import remove +from dvc.utils.fs import remove def destroy(self): diff --git a/dvc/repo/get.py b/dvc/repo/get.py index 0117006840..21d01ed7e7 100644 --- a/dvc/repo/get.py +++ b/dvc/repo/get.py @@ -11,8 +11,8 @@ from dvc.path_info import PathInfo from dvc.stage import Stage from dvc.state import StateNoop -from dvc.utils import remove from dvc.utils import resolve_output +from dvc.utils.fs import remove logger = logging.getLogger(__name__) diff --git a/dvc/repo/init.py b/dvc/repo/init.py index fce49b7d7f..e06307e0a1 100644 --- a/dvc/repo/init.py +++ b/dvc/repo/init.py @@ -11,7 +11,7 @@ from dvc.scm import SCM from dvc.utils import boxify from dvc.utils import relpath -from dvc.utils import remove +from dvc.utils.fs import remove logger = logging.getLogger(__name__) diff --git a/dvc/state.py b/dvc/state.py index a7d6f733df..6ef52a114a 100644 --- a/dvc/state.py +++ b/dvc/state.py @@ -10,7 +10,6 @@ from dvc.exceptions import DvcException from dvc.utils import current_timestamp from dvc.utils import relpath -from dvc.utils import remove from dvc.utils import to_chunks from dvc.utils.compat import fspath_py35 from dvc.utils.compat import is_py2 @@ -18,6 +17,7 @@ from dvc.utils.compat import urlunparse from dvc.utils.fs import get_inode from dvc.utils.fs import get_mtime_and_size +from dvc.utils.fs import remove SQLITE_MAX_VARIABLES_NUMBER = 999 diff --git a/dvc/utils/__init__.py b/dvc/utils/__init__.py index 864c9e5f56..a318842115 100644 --- a/dvc/utils/__init__.py +++ b/dvc/utils/__init__.py @@ -8,7 +8,6 @@ import math import os import re -import shutil import stat import sys import time @@ -173,19 +172,6 @@ def _chmod(func, p, excinfo): func(p) -def remove(path): - logger.debug("Removing '{}'".format(relpath(path))) - - try: - if os.path.isdir(path): - shutil.rmtree(path, onerror=_chmod) - else: - _chmod(os.unlink, path, None) - except OSError as exc: - if exc.errno != errno.ENOENT: - raise - - def _split(list_to_split, chunk_size): return [ list_to_split[i : i + chunk_size] diff --git a/dvc/utils/fs.py b/dvc/utils/fs.py index 76be6f9a0a..eff297693c 100644 --- a/dvc/utils/fs.py +++ b/dvc/utils/fs.py @@ -10,9 +10,11 @@ from dvc.exceptions import DvcException from dvc.system import System +from dvc.utils import _chmod from dvc.utils import dict_md5 from dvc.utils import fspath from dvc.utils import fspath_py35 +from dvc.utils import relpath from dvc.utils import walk_files from dvc.utils.compat import str @@ -103,3 +105,18 @@ def move(src, dst, mode=None): os.chmod(tmp, mode) shutil.move(tmp, dst) + + +def remove(path): + path = fspath_py35(path) + + logger.debug("Removing '{}'".format(relpath(path))) + + try: + if os.path.isdir(path): + shutil.rmtree(path, onerror=_chmod) + else: + _chmod(os.unlink, path, None) + except OSError as exc: + if exc.errno != errno.ENOENT: + raise diff --git a/tests/basic_env.py b/tests/basic_env.py index ae80d60e5b..1fba67a550 100644 --- a/tests/basic_env.py +++ b/tests/basic_env.py @@ -13,9 +13,9 @@ from git.exc import GitCommandNotFound from dvc.repo import Repo as DvcRepo -from dvc.utils import remove from dvc.utils.compat import open from dvc.utils.compat import str +from dvc.utils.fs import remove logger = logging.getLogger("dvc") diff --git a/tests/unit/utils/test_fs.py b/tests/unit/utils/test_fs.py index 32fa749a5b..a8faabb0c1 100644 --- a/tests/unit/utils/test_fs.py +++ b/tests/unit/utils/test_fs.py @@ -16,6 +16,7 @@ from dvc.utils.fs import get_inode from dvc.utils.fs import get_mtime_and_size from dvc.utils.fs import move +from dvc.utils.fs import remove from tests.basic_env import TestDir from tests.utils import spy @@ -152,3 +153,14 @@ def test_move(repo_dir): move(src_info, dest_info) assert not os.path.isfile(src_info.fspath) assert len(os.listdir(dest_info.fspath)) == 1 + + +def test_remove(repo_dir): + path = repo_dir.FOO + path_info = PathInfo(repo_dir.BAR) + + remove(path) + assert not os.path.isfile(path) + + remove(path_info) + assert not os.path.isfile(path_info.fspath) From d9cd9ece8b7dd296617a3488be006e46248bd9b6 Mon Sep 17 00:00:00 2001 From: Aman Sharma Date: Sat, 16 Nov 2019 00:44:20 +0530 Subject: [PATCH 2/2] Move `_chmod` to fs.py --- dvc/utils/__init__.py | 16 ---------------- dvc/utils/fs.py | 16 +++++++++++++++- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/dvc/utils/__init__.py b/dvc/utils/__init__.py index a318842115..61b25eee10 100644 --- a/dvc/utils/__init__.py +++ b/dvc/utils/__init__.py @@ -1,14 +1,12 @@ """Helpers for other modules.""" from __future__ import unicode_literals -import errno import hashlib import json import logging import math import os import re -import stat import sys import time @@ -158,20 +156,6 @@ def makedirs(path, exist_ok=False, mode=None): os.umask(umask) -def _chmod(func, p, excinfo): - perm = os.lstat(p).st_mode - perm |= stat.S_IWRITE - - try: - os.chmod(p, perm) - except OSError as exc: - # broken symlink or file is not owned by us - if exc.errno not in [errno.ENOENT, errno.EPERM]: - raise - - func(p) - - def _split(list_to_split, chunk_size): return [ list_to_split[i : i + chunk_size] diff --git a/dvc/utils/fs.py b/dvc/utils/fs.py index eff297693c..c73a23563e 100644 --- a/dvc/utils/fs.py +++ b/dvc/utils/fs.py @@ -4,13 +4,13 @@ import logging import os import shutil +import stat import nanotime from shortuuid import uuid from dvc.exceptions import DvcException from dvc.system import System -from dvc.utils import _chmod from dvc.utils import dict_md5 from dvc.utils import fspath from dvc.utils import fspath_py35 @@ -107,6 +107,20 @@ def move(src, dst, mode=None): shutil.move(tmp, dst) +def _chmod(func, p, excinfo): + perm = os.lstat(p).st_mode + perm |= stat.S_IWRITE + + try: + os.chmod(p, perm) + except OSError as exc: + # broken symlink or file is not owned by us + if exc.errno not in [errno.ENOENT, errno.EPERM]: + raise + + func(p) + + def remove(path): path = fspath_py35(path)