From 0bfb3c0ab2ee229762d551bb622258b99e889ceb Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 15 Oct 2019 22:42:52 +0300 Subject: [PATCH] get/import/url: handle existing output directory (#2610) * get/import/url: handle existing output directory Currently if you run ``` dvc get https://github.com/iterative/example-get-started model.pkl --out . ``` it will nuke your cwd. To fix that we need to properly handle existing directories, same as usual unix cli utilities do that. * travis: specify python version when installing from chocolatey Fixes #2609 --- dvc/repo/get.py | 5 ++--- dvc/repo/get_url.py | 4 ++-- dvc/repo/imp_url.py | 4 ++-- dvc/utils/__init__.py | 11 +++++++++++ scripts/ci/before_install.sh | 2 +- tests/func/test_get.py | 17 +++++++++++++++++ tests/func/test_get_url.py | 19 +++++++++++++++++++ tests/func/test_import.py | 18 ++++++++++++++++++ tests/func/test_import_url.py | 28 ++++++++++++++++++++++++---- 9 files changed, 96 insertions(+), 12 deletions(-) diff --git a/dvc/repo/get.py b/dvc/repo/get.py index ca6176ccc..9096216c9 100644 --- a/dvc/repo/get.py +++ b/dvc/repo/get.py @@ -13,15 +13,14 @@ from dvc.exceptions import ( from dvc.path_info import PathInfo from dvc.external_repo import external_repo from dvc.state import StateNoop -from dvc.utils import remove -from dvc.utils.compat import urlparse +from dvc.utils import remove, resolve_output logger = logging.getLogger(__name__) @staticmethod def get(url, path, out=None, rev=None): - out = out or os.path.basename(urlparse(path).path) + out = resolve_output(path, out) if Stage.is_valid_filename(out): raise GetDVCFileError() diff --git a/dvc/repo/get_url.py b/dvc/repo/get_url.py index ca9663746..ff191e149 100644 --- a/dvc/repo/get_url.py +++ b/dvc/repo/get_url.py @@ -3,12 +3,12 @@ import os import dvc.output as output import dvc.dependency as dependency -from dvc.utils.compat import urlparse +from dvc.utils import resolve_output @staticmethod def get_url(url, out=None): - out = out or os.path.basename(urlparse(url).path) + out = resolve_output(url, out) if os.path.exists(url): url = os.path.abspath(url) diff --git a/dvc/repo/imp_url.py b/dvc/repo/imp_url.py index 4ce67104c..5888b7da9 100644 --- a/dvc/repo/imp_url.py +++ b/dvc/repo/imp_url.py @@ -1,4 +1,4 @@ -from dvc.utils.compat import pathlib +from dvc.utils import resolve_output from dvc.repo.scm_context import scm_context from . import locked as locked_repo @@ -9,7 +9,7 @@ from . import locked as locked_repo def imp_url(self, url, out=None, fname=None, erepo=None, locked=True): from dvc.stage import Stage - out = out or pathlib.PurePath(url).name + out = resolve_output(url, out) stage = Stage.create( self, cmd=None, deps=[url], outs=[out], fname=fname, erepo=erepo diff --git a/dvc/utils/__init__.py b/dvc/utils/__init__.py index cc5e3bd78..594d6cce6 100644 --- a/dvc/utils/__init__.py +++ b/dvc/utils/__init__.py @@ -460,3 +460,14 @@ def relpath(path, start=os.curdir): ): return path return os.path.relpath(path, start) + + +def resolve_output(inp, out): + from dvc.utils.compat import urlparse + + name = os.path.basename(urlparse(inp).path) + if not out: + return name + if os.path.isdir(out): + return os.path.join(out, name) + return out diff --git a/scripts/ci/before_install.sh b/scripts/ci/before_install.sh index 4f48cc6f6..9bf8a7eac 100644 --- a/scripts/ci/before_install.sh +++ b/scripts/ci/before_install.sh @@ -48,7 +48,7 @@ if [[ "$TRAVIS_OS_NAME" == "windows" ]]; then choco install python2 echo 'PATH="/c/Python27:/c/Python27/Scripts:$PATH"' >> env.sh else - choco install python + choco install python --version 3.7.4 echo 'PATH="/c/Python37:/c/Python37/Scripts:$PATH"' >> env.sh fi elif [[ "$TRAVIS_OS_NAME" == "osx" ]]; then diff --git a/tests/func/test_get.py b/tests/func/test_get.py index ea435f5b7..a52b0b582 100644 --- a/tests/func/test_get.py +++ b/tests/func/test_get.py @@ -1,3 +1,5 @@ +from __future__ import unicode_literals + import os import filecmp @@ -5,6 +7,7 @@ import pytest from dvc.exceptions import UrlNotDvcRepoError, GetDVCFileError from dvc.repo import Repo +from dvc.utils import makedirs from tests.utils import trees_equal @@ -51,3 +54,17 @@ def test_get_from_non_dvc_repo(git_erepo): def test_get_a_dvc_file(repo_dir, erepo): with pytest.raises(GetDVCFileError): Repo.get(erepo.root_dir, "some_file.dvc") + + +@pytest.mark.parametrize("dname", [".", "dir", "dir/subdir"]) +def test_get_to_dir(dname, repo_dir, erepo): + src = erepo.FOO + + makedirs(dname, exist_ok=True) + + Repo.get(erepo.root_dir, src, dname) + + dst = os.path.join(dname, os.path.basename(src)) + + assert os.path.isdir(dname) + assert filecmp.cmp(repo_dir.FOO, dst, shallow=False) diff --git a/tests/func/test_get_url.py b/tests/func/test_get_url.py index ca1a5ae4f..eae2b556e 100644 --- a/tests/func/test_get_url.py +++ b/tests/func/test_get_url.py @@ -1,7 +1,12 @@ +from __future__ import unicode_literals + import os import filecmp +import pytest + from dvc.repo import Repo +from dvc.utils import makedirs def test_get_file(repo_dir): @@ -13,3 +18,17 @@ def test_get_file(repo_dir): assert os.path.exists(dst) assert os.path.isfile(dst) assert filecmp.cmp(repo_dir.FOO, dst, shallow=False) + + +@pytest.mark.parametrize("dname", [".", "dir", "dir/subdir"]) +def test_get_url_to_dir(dname, repo_dir): + src = repo_dir.DATA + + makedirs(dname, exist_ok=True) + + Repo.get_url(src, dname) + + dst = os.path.join(dname, os.path.basename(src)) + + assert os.path.isdir(dname) + assert filecmp.cmp(repo_dir.DATA, dst, shallow=False) diff --git a/tests/func/test_import.py b/tests/func/test_import.py index 6711b9cf2..0d1f17e99 100644 --- a/tests/func/test_import.py +++ b/tests/func/test_import.py @@ -1,3 +1,5 @@ +from __future__ import unicode_literals + import os import filecmp @@ -6,6 +8,7 @@ from mock import patch from tests.utils import trees_equal from dvc.stage import Stage +from dvc.utils import makedirs from dvc.exceptions import DownloadError @@ -80,3 +83,18 @@ def test_download_error_pulling_imported_stage(dvc_repo, erepo): "dvc.remote.RemoteLOCAL._download", side_effect=Exception ), pytest.raises(DownloadError): dvc_repo.pull(["foo_imported.dvc"]) + + +@pytest.mark.parametrize("dname", [".", "dir", "dir/subdir"]) +def test_import_to_dir(dname, repo_dir, dvc_repo, erepo): + src = erepo.FOO + + makedirs(dname, exist_ok=True) + + stage = dvc_repo.imp(erepo.root_dir, src, dname) + + dst = os.path.join(dname, os.path.basename(src)) + + assert stage.outs[0].fspath == os.path.abspath(dst) + assert os.path.isdir(dname) + assert filecmp.cmp(repo_dir.FOO, dst, shallow=False) diff --git a/tests/func/test_import_url.py b/tests/func/test_import_url.py index 4938d8e49..e94390f64 100644 --- a/tests/func/test_import_url.py +++ b/tests/func/test_import_url.py @@ -1,12 +1,17 @@ -import dvc - -from dvc.utils.compat import str +from __future__ import unicode_literals import os from uuid import uuid4 -from dvc.main import main +import pytest +import filecmp from mock import patch + +import dvc +from dvc.main import main +from dvc.utils import makedirs +from dvc.utils.compat import str + from tests.basic_env import TestDvc from tests.utils import spy @@ -86,3 +91,18 @@ class TestImportFilename(TestDvc): ret = main(["import-url", "--file", path, self.external_source]) self.assertEqual(0, ret) self.assertTrue(os.path.exists(path)) + + +@pytest.mark.parametrize("dname", [".", "dir", "dir/subdir"]) +def test_import_url_to_dir(dname, repo_dir, dvc_repo): + src = repo_dir.DATA + + makedirs(dname, exist_ok=True) + + stage = dvc_repo.imp_url(src, dname) + + dst = os.path.join(dname, os.path.basename(src)) + + assert stage.outs[0].fspath == os.path.abspath(dst) + assert os.path.isdir(dname) + assert filecmp.cmp(repo_dir.DATA, dst, shallow=False)