From 8102ede5cc9b414d3bc93040c1b1481ffc6d063e Mon Sep 17 00:00:00 2001 From: Batuhan Taskaya Date: Fri, 19 Feb 2021 16:22:59 +0300 Subject: [PATCH] add, import-url: always resolve the output in the local path for --to-remote (#5473) * add, import-url: always resolve the output in the local path for --to-remote * windows * use the proper fixture --- dvc/repo/add.py | 14 ++++++++++++-- dvc/repo/imp_url.py | 4 +++- dvc/utils/__init__.py | 5 ++++- tests/func/test_add.py | 29 ++++++++++++++++++++++++++--- tests/func/test_import_url.py | 15 +++++++++++++++ 5 files changed, 60 insertions(+), 7 deletions(-) diff --git a/dvc/repo/add.py b/dvc/repo/add.py index d86b46360..5e63f2bf6 100644 --- a/dvc/repo/add.py +++ b/dvc/repo/add.py @@ -52,6 +52,8 @@ def add( # noqa: C901 invalid_opt = "--no-commit option" elif recursive: invalid_opt = "--recursive option" + elif kwargs.get("external"): + invalid_opt = "--external option" else: message = "{option} can't be used without --to-remote" if kwargs.get("remote"): @@ -88,7 +90,12 @@ def add( # noqa: C901 ) stages = _create_stages( - repo, sub_targets, fname, pbar=pbar, **kwargs, + repo, + sub_targets, + fname, + pbar=pbar, + transfer=to_remote or to_cache, + **kwargs, ) try: @@ -230,6 +237,7 @@ def _create_stages( external=False, glob=False, desc=None, + transfer=False, **kwargs, ): from dvc.dvcfile import Dvcfile @@ -246,7 +254,9 @@ def _create_stages( ): if kwargs.get("out"): out = resolve_output(out, kwargs["out"]) - path, wdir, out = resolve_paths(repo, out) + path, wdir, out = resolve_paths( + repo, out, always_local=transfer and not kwargs.get("out") + ) stage = create_stage( Stage, repo, diff --git a/dvc/repo/imp_url.py b/dvc/repo/imp_url.py index 3fc0ca488..5b3f0b354 100644 --- a/dvc/repo/imp_url.py +++ b/dvc/repo/imp_url.py @@ -27,7 +27,9 @@ def imp_url( from dvc.stage import Stage, create_stage, restore_meta out = resolve_output(url, out) - path, wdir, out = resolve_paths(self, out) + path, wdir, out = resolve_paths( + self, out, always_local=to_remote and not out + ) if to_remote and no_exec: raise InvalidArgumentError( diff --git a/dvc/utils/__init__.py b/dvc/utils/__init__.py index 0785b6911..d8d9db9b9 100644 --- a/dvc/utils/__init__.py +++ b/dvc/utils/__init__.py @@ -357,7 +357,7 @@ def resolve_output(inp, out): return ret -def resolve_paths(repo, out): +def resolve_paths(repo, out, always_local=False): from urllib.parse import urlparse from ..dvcfile import DVC_FILE_SUFFIX @@ -393,6 +393,9 @@ def resolve_paths(repo, out): wdir = dirname out = base + if always_local: + out = base + path = os.path.join(wdir, base + DVC_FILE_SUFFIX) return (path, wdir, out) diff --git a/tests/func/test_add.py b/tests/func/test_add.py index d94b18a9f..0ac536dfc 100644 --- a/tests/func/test_add.py +++ b/tests/func/test_add.py @@ -27,7 +27,10 @@ from dvc.objects.db import ODBManager from dvc.output.base import OutputAlreadyTrackedError, OutputIsStageFileError from dvc.repo import Repo as DvcRepo from dvc.stage import Stage -from dvc.stage.exceptions import StagePathNotFoundError +from dvc.stage.exceptions import ( + StageExternalOutputsError, + StagePathNotFoundError, +) from dvc.system import System from dvc.utils import LARGE_DIR_SIZE, file_md5, relpath from dvc.utils.fs import path_isin @@ -324,8 +327,6 @@ def test_add_filtered_files_in_dir( indirect=["workspace"], ) def test_add_external_file(tmp_dir, dvc, workspace, hash_name, hash_value): - from dvc.stage.exceptions import StageExternalOutputsError - workspace.gen("file", "file") with pytest.raises(StageExternalOutputsError): @@ -1013,12 +1014,34 @@ def test_add_to_remote(tmp_dir, dvc, local_cloud, local_remote): assert local_remote.hash_to_path_info(hash_info.value).read_text() == "foo" +def test_add_to_remote_absolute(tmp_dir, make_tmp_dir, dvc, local_remote): + tmp_abs_dir = make_tmp_dir("abs") + tmp_foo = tmp_abs_dir / "foo" + tmp_foo.write_text("foo") + + dvc.add(str(tmp_foo), to_remote=True) + tmp_foo.unlink() + + foo = tmp_dir / "foo" + assert foo.with_suffix(".dvc").exists() + assert not os.path.exists(tmp_foo) + + dvc.pull("foo") + assert not os.path.exists(tmp_foo) + assert foo.read_text() == "foo" + + with pytest.raises(StageExternalOutputsError): + tmp_bar = tmp_abs_dir / "bar" + dvc.add(str(tmp_foo), out=str(tmp_bar), to_remote=True) + + @pytest.mark.parametrize( "invalid_opt, kwargs", [ ("multiple targets", {"targets": ["foo", "bar", "baz"]}), ("--no-commit", {"targets": ["foo"], "no_commit": True}), ("--recursive", {"targets": ["foo"], "recursive": True},), + ("--external", {"targets": ["foo"], "external": True}), ], ) def test_add_to_remote_invalid_combinations(dvc, invalid_opt, kwargs): diff --git a/tests/func/test_import_url.py b/tests/func/test_import_url.py index 896b8a815..c515cd505 100644 --- a/tests/func/test_import_url.py +++ b/tests/func/test_import_url.py @@ -340,6 +340,21 @@ def test_import_url_to_remote_directory(tmp_dir, dvc, workspace, local_remote): ) +def test_import_url_to_remote_absolute( + tmp_dir, make_tmp_dir, dvc, local_remote +): + tmp_abs_dir = make_tmp_dir("abs") + tmp_foo = tmp_abs_dir / "foo" + tmp_foo.write_text("foo") + + stage = dvc.imp_url(str(tmp_foo), to_remote=True) + + foo = tmp_dir / "foo" + assert stage.deps[0].fspath == str(tmp_foo) + assert stage.outs[0].fspath == os.fspath(foo) + assert foo.with_suffix(".dvc").exists() + + def test_import_url_to_remote_invalid_combinations(dvc): with pytest.raises(InvalidArgumentError, match="--no-exec"): dvc.imp_url("s3://bucket/foo", no_exec=True, to_remote=True)