From a813ebaeb771104efb59ce6df18fdb7f6241bc4f Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Thu, 26 Oct 2017 15:28:46 +0300 Subject: [PATCH] dvc: use hardlinks instead of symlinks Fixes #225 Fixes #273 Signed-off-by: Ruslan Kuprieiev --- dvc/command/import_file.py | 3 +-- dvc/command/init.py | 24 ++++----------------- dvc/command/repro.py | 7 ++----- dvc/command/run.py | 17 +++++++-------- dvc/command/traverse.py | 2 +- dvc/data_cloud.py | 16 ++++++++++---- dvc/git_wrapper.py | 11 ++++++++++ dvc/path/data_item.py | 17 ++++++--------- dvc/path/factory.py | 15 ++++++++----- dvc/system.py | 43 +++----------------------------------- 10 files changed, 57 insertions(+), 98 deletions(-) diff --git a/dvc/command/import_file.py b/dvc/command/import_file.py index 2ff486398..9bcb3d304 100644 --- a/dvc/command/import_file.py +++ b/dvc/command/import_file.py @@ -132,8 +132,7 @@ class CmdImportFile(CmdBase): Logger.debug('Skipping creating state file for failed import {}'.format(data_item.state.relative)) continue - Logger.debug('Creating symlink {} --> {}'.format(data_item.symlink_file, data_item.data.relative)) - System.symlink(data_item.symlink_file, data_item.data.relative) + Logger.debug('Creating state file for {}'.format(data_item.data.relative)) state_file = StateFile(StateFile.COMMAND_IMPORT_FILE, data_item, diff --git a/dvc/command/init.py b/dvc/command/init.py index 2e3568cdc..d0ddeb2cf 100644 --- a/dvc/command/init.py +++ b/dvc/command/init.py @@ -116,16 +116,11 @@ ProjectName = conf_file.write(self.CONFIG_TEMPLATE.format(data_dir_path.name)) conf_file.close() - message = 'DVC init. data dir {}, cache dir {}, state dir {}, '.format( - data_dir_path.name, - cache_dir_path.name, - state_dir_path.name - ) - if self.commit_if_needed(message) == 1: - return 1 + self.git.modify_gitignore([os.path.join(config_dir_path.name, cache_dir_path.name), + os.path.join(config_dir_path.name, os.path.basename(self.git.lock_file))]) - self.modify_gitignore(config_dir_path.name, cache_dir_path.name) - return self.commit_if_needed('DVC init. Commit .gitignore file') + message = 'DVC init. cache dir {}, state dir {}, '.format(cache_dir_path.name, state_dir_path.name) + return self.commit_if_needed(message) def create_empty_file(self): empty_data_path = os.path.join(self.parsed_args.data_dir, self.EMPTY_FILE_NAME) @@ -141,14 +136,3 @@ ProjectName = output_files=[], lock=False).save(is_update_target_metrics=False) pass - - def modify_gitignore(self, config_dir_name, cache_dir_name): - gitignore_file = os.path.join(self.git.git_dir, '.gitignore') - if not os.path.exists(gitignore_file): - open(gitignore_file, 'a').close() - Logger.info('File .gitignore was created') - with open(gitignore_file, 'a') as fd: - fd.write('\n{}'.format(os.path.join(config_dir_name, cache_dir_name))) - fd.write('\n{}'.format(os.path.join(config_dir_name, os.path.basename(self.git.lock_file)))) - - Logger.info('Directory {} was added to .gitignore file'.format(cache_dir_name)) diff --git a/dvc/command/repro.py b/dvc/command/repro.py index f618cd341..ba1cbdb1d 100644 --- a/dvc/command/repro.py +++ b/dvc/command/repro.py @@ -119,9 +119,6 @@ class ReproChange(object): self._recursive = recursive self._force = force - if not System.islink(data_item.data.relative): - raise ReproError('data item {} is not symlink'.format(data_item.data.relative)) - try: self._state = StateFile.load(data_item, self.git) except Exception as ex: @@ -246,8 +243,8 @@ class ReproChange(object): result = True Logger.debug(u'Repro data item {}: dependency {} was changed'.format( data_item_dvc, data_item.data.dvc)) - elif data_item.data.dvc in self._globally_changed_files: - msg = u'Repro data item {}: dependency {} was not changed but the data item global checksum was changed' + elif data_item.state.dvc in self._globally_changed_files: + msg = u'Repro data item {}: dependency {} was not changed but the state file was changed' Logger.debug(msg.format(data_item_dvc, data_item.data.dvc)) result = True else: diff --git a/dvc/command/run.py b/dvc/command/run.py index 9c8a94de7..51759977b 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -116,7 +116,7 @@ class CmdRun(CmdBase): result = [] for data_item in repo_change.changed_data_items: - Logger.debug('Move output file "{}" to cache dir "{}" and create a symlink'.format( + Logger.debug('Move output file "{}" to cache dir "{}" and create a hardlink'.format( data_item.data.relative, data_item.cache_dir)) data_item.move_data_to_cache() @@ -168,16 +168,13 @@ class CmdRun(CmdBase): not_data_items = [] for arg in argv: - try: - if os.path.isfile(arg): - data_item = self.settings.path_factory.data_item(arg) - data_items.append(data_item) - except NotInGitDirError as ex: - msg = 'File {} from argv is outside of git directory and cannot be traced: {}' - Logger.warn(msg.format(arg, ex)) - except NotInDataDirError: + if self.settings.path_factory.is_data_item(arg): + data_items.append(self.settings.path_factory.data_item(arg)) + elif os.path.isfile(arg): not_data_items.append(arg) - pass + else: + msg = 'File {} from argv is outside of git directory and cannot be traced' + Logger.warn(msg.format(arg)) return data_items, not_data_items diff --git a/dvc/command/traverse.py b/dvc/command/traverse.py index 63e030521..6958d8241 100644 --- a/dvc/command/traverse.py +++ b/dvc/command/traverse.py @@ -72,7 +72,7 @@ class Traverse(CmdBase): try: data_item = self.settings.path_factory.existing_data_item(target) except DataItemError: - Logger.warn(u'[TraverseFileTree] Data file {} is not valid symbolic link'.format(target)) + Logger.warn(u'[TraverseFileTree] Data file {} does not have existing state file'.format(target)) data_item = self.settings.path_factory.data_item(target) return data_item diff --git a/dvc/data_cloud.py b/dvc/data_cloud.py index 5a84692d0..25ae03a26 100644 --- a/dvc/data_cloud.py +++ b/dvc/data_cloud.py @@ -219,11 +219,13 @@ class DataCloudLOCAL(DataCloudBase): tmp_file = self.tmp_file(item.data.dvc) try: copyfile(i, tmp_file) - item.import_cache(tmp_file) except Exception as exc: Logger.error('Failed to copy "{}": {}'.format(i, exc)) return None + os.rename(tmp_file, item.data.dvc) + item.move_data_to_cache() + return item def pull(self, item): @@ -363,7 +365,8 @@ class DataCloudHTTP(DataCloudBase): if not self._verify_md5(req, tmp_file): return None - item.import_cache(tmp_file) + os.rename(tmp_file, item.data.dvc) + item.move_data_to_cache() return item @@ -476,11 +479,13 @@ class DataCloudAWS(DataCloudBase): num_retries=10) try: key.get_contents_to_filename(tmp_file, cb=create_cb(name), res_download_handler=res_h) - data_item.import_cache(tmp_file) except Exception as exc: Logger.error('Failed to download "{}": {}'.format(key_name, exc)) return None + os.rename(tmp_file, fname) + data_item.move_data_to_cache() + progress.finish_target(name) Logger.debug('Downloading completed') @@ -726,6 +731,9 @@ class DataCloudGCP(DataCloudBase): Logger.error('Failed to download "{}": {}'.format(key, exc)) return None + os.rename(tmp_file, fname) + data_item.move_data_to_cache() + progress.finish_target(name) Logger.debug('Downloading completed') @@ -877,7 +885,7 @@ class DataCloud(object): """ Collect target as a file or directory. """ - if System.islink(target): + if self._settings.path_factory.is_data_item(target): item = self._settings.path_factory.data_item(target) return [item] elif os.path.isdir(target): diff --git a/dvc/git_wrapper.py b/dvc/git_wrapper.py index 1b309ac8b..ff3a41341 100644 --- a/dvc/git_wrapper.py +++ b/dvc/git_wrapper.py @@ -96,6 +96,17 @@ class GitWrapperI(object): def commit_all_changes_and_log_status(self, message): pass + @staticmethod + def modify_gitignore(flist): + for fname in flist: + # We're modifying .gitignore in the same dir as the file itself, so + # that if dir consists only of data files it will be still tracked by + # git and we will not have any problems with --porcelain output for + # specific changed files. + gitignore = os.path.join(os.path.dirname(fname), '.gitignore') + with open(gitignore, 'a') as fd: + fd.write('\n' + os.path.basename(fname)) + class GitWrapper(GitWrapperI): FLOATS_FROM_STRING = re.compile(r'[-+]?(?:(?:\d*\.\d+)|(?:\d+\.?))(?:[Ee][+-]?\d+)?') diff --git a/dvc/path/data_item.py b/dvc/path/data_item.py index 6719a75a5..e5478f7a5 100644 --- a/dvc/path/data_item.py +++ b/dvc/path/data_item.py @@ -125,15 +125,10 @@ class DataItem(object): def state_dir_abs(self): return os.path.join(self._git.git_dir_abs, self._config.state_dir) - @property - def symlink_file(self): - data_file_dir = os.path.dirname(self.data.relative) - return os.path.relpath(self.cache.relative, data_file_dir) - def move_data_to_cache(self): - self.import_cache(self.data.relative) - System.symlink(self.symlink_file, self.data.relative) - - def import_cache(self, fname): - self._cache_file = os.path.join(self.cache_dir, file_md5(fname)[0]) - os.rename(fname, self._cache_file) + md5 = file_md5(self.data.relative)[0] + self._cache_file = os.path.join(self.cache_dir, md5) + self._git.modify_gitignore([self.data.relative]) + if os.path.isfile(self.cache.relative): + return + System.hardlink(self.data.relative, self.cache.relative) diff --git a/dvc/path/factory.py b/dvc/path/factory.py index 23f20e0d2..68078f74a 100644 --- a/dvc/path/factory.py +++ b/dvc/path/factory.py @@ -25,11 +25,16 @@ class PathFactory(object): path = Path.from_dvc_path(dvc_path, self._git) return self.existing_data_item(path.relative) - def existing_data_item(self, file): - if not System.islink(file): - raise DataItemError(u'Data file "%s" must be a symbolic link' % file) - resolved_symlink = System.realpath(file) - return DataItem(file, self._git, self._config, resolved_symlink) + def is_data_item(self, fname): + data = os.path.relpath(os.path.realpath(fname), self._config.data_dir) + state = os.path.join(self._config.state_dir, data + DataItem.STATE_FILE_SUFFIX) + return os.path.isfile(state) + + def existing_data_item(self, fname): + if not self.is_data_item(fname): + raise DataItemError(u'No state file found for data file {}'.format(fname)) + + return DataItem(fname, self._git, self._config) def to_data_items(self, files): result = [] diff --git a/dvc/system.py b/dvc/system.py index 43c52053a..bb4d41acc 100644 --- a/dvc/system.py +++ b/dvc/system.py @@ -10,7 +10,6 @@ from dvc.executor import Executor class System(object): - SYMLINK_OUTPUT = '' LONG_PATH_BUFFER_SIZE = 1024 @staticmethod @@ -18,47 +17,11 @@ class System(object): return os.name != 'nt' @staticmethod - def symlink(source, link_name): + def hardlink(source, link_name): if System.is_unix(): - return os.symlink(source, link_name) + return os.link(source, link_name) - flags = 0 - if source is not None and os.path.isdir(source): - flags = 1 - - func = ctypes.windll.kernel32.CreateSymbolicLinkW - func.argtypes = (ctypes.c_wchar_p, ctypes.c_wchar_p, ctypes.c_uint32) - func.restype = ctypes.c_ubyte - - if func(link_name, source, flags) == 0: - raise ctypes.WinError() - - @staticmethod - def islink(path): - # It is definitely not the best way to check a symlink. - - if System.is_unix(): - return os.path.islink(path) - - if not os.path.exists(path): - return False - - if not os.path.isfile(path): - return False - - return System._get_symlink_string(path) is not None - - @staticmethod - def _get_symlink_string(path): - code, output, _ = Executor.exec_cmd(["dir", path], shell=True) - if code != 0: - return None - - lines = output.split('\n') - for line in lines: - if System.SYMLINK_OUTPUT in line: - return line - return None + raise Exception('Not Implemented Yet') @staticmethod def realpath(path):