From 803ffaec5700971a7742b2b1483b1ade54ed994e Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Fri, 11 Oct 2019 00:32:49 +0300 Subject: [PATCH] remote: local: don't auto-create directory in `__init__` Imagine the scenario of shared cache. To set it up, user would do: ``` dvc config cache.dir /path dvc config cache.shared group ``` Now, when he runs any dvc operation that involves the Repo instance, RemoteLOCAL will also be initialized and it will currently auto-create the directory with a regular `os.mkdir`, which is not using group permissions. To solve that, we can't simply use `self.makdirs` instead, as it will cause same issues if you run any dvc command before `dvc config cache.shared group`. So what we should do is create directories on demand, instead of creating it in advance, same as we do in other remotes. The change itself is pretty trivial and involved removing that `mkdir` line, adjusting tests to not rely on cache dir existence and also adjusting `dvc version` to not rely on cache directory existance. Fixes #2563 --- dvc/command/version.py | 18 +++++++++++++++--- dvc/remote/local.py | 3 --- tests/func/test_add.py | 2 +- tests/func/test_cache.py | 5 ++--- tests/func/test_repro.py | 2 +- tests/func/test_run.py | 2 +- tests/func/test_version.py | 6 +++++- 7 files changed, 25 insertions(+), 13 deletions(-) diff --git a/dvc/command/version.py b/dvc/command/version.py index 616a9b144..50758b2aa 100644 --- a/dvc/command/version.py +++ b/dvc/command/version.py @@ -48,9 +48,21 @@ class CmdVersion(CmdBaseNoRepo): repo = Repo() root_directory = repo.root_dir - info += "Cache: {cache}\n".format( - cache=self.get_linktype_support_info(repo) - ) + # cache_dir might not exist yet (e.g. after `dvc init`), and we + # can't auto-create it, as it might cause issues if the user + # later decides to enable shared cache mode with + # `dvc config cache.shared group`. + if os.path.exists(repo.cache.local.cache_dir): + info += "Cache: {cache}\n".format( + cache=self.get_linktype_support_info(repo) + ) + else: + logger.warning( + "Unable to detect supported link types, as cache " + "directory '{}' doesn't exist. It is usually auto-created " + "by commands such as `dvc add/fetch/pull/run/import`, but " + "you could create it manually to enable this check." + ) if psutil: info += ( diff --git a/dvc/remote/local.py b/dvc/remote/local.py index 7592ecb27..5c73a4729 100644 --- a/dvc/remote/local.py +++ b/dvc/remote/local.py @@ -69,9 +69,6 @@ class RemoteLOCAL(RemoteBASE): cwd = config[Config.PRIVATE_CWD] cache_dir = os.path.abspath(os.path.join(cwd, cache_dir)) - if cache_dir is not None and not os.path.exists(cache_dir): - os.mkdir(cache_dir) - self.path_info = PathInfo(cache_dir) if cache_dir else None self._dir_info = {} diff --git a/tests/func/test_add.py b/tests/func/test_add.py index f1e293641..3c5fa59a1 100644 --- a/tests/func/test_add.py +++ b/tests/func/test_add.py @@ -325,7 +325,7 @@ class TestAddCommit(TestDvc): ret = main(["add", self.FOO, "--no-commit"]) self.assertEqual(ret, 0) self.assertTrue(os.path.isfile(self.FOO)) - self.assertEqual(len(os.listdir(self.dvc.cache.local.cache_dir)), 0) + self.assertFalse(os.path.exists(self.dvc.cache.local.cache_dir)) ret = main(["commit", self.FOO + ".dvc"]) self.assertEqual(ret, 0) diff --git a/tests/func/test_cache.py b/tests/func/test_cache.py index be4f01cbe..9c9673573 100644 --- a/tests/func/test_cache.py +++ b/tests/func/test_cache.py @@ -1,7 +1,6 @@ import os import stat import pytest -import shutil import configobj from dvc.cache import Cache @@ -66,7 +65,7 @@ class TestExternalCacheDir(TestDvc): ret = main(["config", "cache.dir", cache_dir]) self.assertEqual(ret, 0) - shutil.rmtree(".dvc/cache") + self.assertFalse(os.path.exists(self.dvc.cache.local.cache_dir)) ret = main(["add", self.FOO]) self.assertEqual(ret, 0) @@ -101,7 +100,7 @@ class TestSharedCacheDir(TestDir): ret = main(["config", "cache.dir", cache_dir]) self.assertEqual(ret, 0) - shutil.rmtree(os.path.join(".dvc", "cache")) + self.assertFalse(os.path.exists(os.path.join(".dvc", "cache"))) with open("common", "w+") as fd: fd.write("common") diff --git a/tests/func/test_repro.py b/tests/func/test_repro.py index adfbc0d1a..5e1abc3fa 100644 --- a/tests/func/test_repro.py +++ b/tests/func/test_repro.py @@ -1280,7 +1280,7 @@ class TestReproNoCommit(TestRepro): shutil.rmtree(self.dvc.cache.local.cache_dir) ret = main(["repro", self.file1_stage, "--no-commit"]) self.assertEqual(ret, 0) - self.assertEqual(len(os.listdir(self.dvc.cache.local.cache_dir)), 0) + self.assertFalse(os.path.exists(self.dvc.cache.local.cache_dir)) class TestReproAlreadyCached(TestRepro): diff --git a/tests/func/test_run.py b/tests/func/test_run.py index 148bb8853..dabaa419e 100644 --- a/tests/func/test_run.py +++ b/tests/func/test_run.py @@ -706,7 +706,7 @@ class TestRunCommit(TestDvc): ) self.assertEqual(ret, 0) self.assertTrue(os.path.isfile(fname)) - self.assertEqual(len(os.listdir(self.dvc.cache.local.cache_dir)), 0) + self.assertFalse(os.path.exists(self.dvc.cache.local.cache_dir)) ret = main(["commit", fname + ".dvc"]) self.assertEqual(ret, 0) diff --git a/tests/func/test_version.py b/tests/func/test_version.py index d0fe6f2eb..7bb80bb2b 100644 --- a/tests/func/test_version.py +++ b/tests/func/test_version.py @@ -5,7 +5,11 @@ from dvc.main import main from dvc.command.version import psutil -def test_info_in_repo(dvc_repo, caplog): +def test_info_in_repo(repo_dir, dvc_repo, caplog): + # adding a file so that dvc creates `.dvc/cache`, that is needed for proper + # supported link types check. + assert main(["add", repo_dir.FOO]) == 0 + assert main(["version"]) == 0 assert re.search(re.compile(r"DVC version: \d+\.\d+\.\d+"), caplog.text)