From cb55463d1e4c7a292ecf719c123c696cdb72e9cf Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Sat, 5 Mar 2016 09:02:26 -0500 Subject: [PATCH 1/7] TEST: TempFATFS allows testing Windows FS on POSIX --- .travis.yml | 1 + circle.yml | 2 +- nipype/testing/__init__.py | 2 +- nipype/testing/tests/test_utils.py | 12 ++++++ nipype/testing/utils.py | 69 ++++++++++++++++++++++++++++++ 5 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 nipype/testing/tests/test_utils.py diff --git a/.travis.yml b/.travis.yml index 7feeaf7e9d..7074e18fcc 100644 --- a/.travis.yml +++ b/.travis.yml @@ -34,6 +34,7 @@ before_install: echo 'include_dirs = /usr/include:/usr/include/X11' >> $HOME/.numpy-site.cfg; fi install: +- sudo apt-get install fusefat - conda update --yes conda - conda create -n testenv --yes pip python=$TRAVIS_PYTHON_VERSION - source activate testenv diff --git a/circle.yml b/circle.yml index c077ffbb60..8967919746 100644 --- a/circle.yml +++ b/circle.yml @@ -12,7 +12,7 @@ dependencies: - bash <(wget -q -O- http://neuro.debian.net/_files/neurodebian-travis.sh) override: # Install apt packages - - sudo apt-get install -y fsl-core fsl-atlases fsl-mni152-templates fsl-feeds afni swig python-vtk xvfb + - sudo apt-get install -y fsl-core fsl-atlases fsl-mni152-templates fsl-feeds afni swig python-vtk xvfb fusefat - echo 'source /etc/fsl/fsl.sh' >> $HOME/.profile - echo 'source /etc/afni/afni.sh' >> $HOME/.profile - mkdir -p ~/examples/ && ln -sf /usr/share/fsl-feeds/ ~/examples/feeds diff --git a/nipype/testing/__init__.py b/nipype/testing/__init__.py index 9a4848fb50..97fa976ee6 100644 --- a/nipype/testing/__init__.py +++ b/nipype/testing/__init__.py @@ -29,7 +29,7 @@ from numpy.testing import * from . import decorators as dec -from .utils import skip_if_no_package, package_check +from .utils import skip_if_no_package, package_check, TempFATFS skipif = dec.skipif diff --git a/nipype/testing/tests/test_utils.py b/nipype/testing/tests/test_utils.py new file mode 100644 index 0000000000..02587ef70f --- /dev/null +++ b/nipype/testing/tests/test_utils.py @@ -0,0 +1,12 @@ +# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*- +# vi: set ft=python sts=4 ts=4 sw=4 et: +"""Test testing utilities +""" + +from nipype.testing.utils import TempFATFS +from nose.tools import assert_true + + +def test_tempfatfs(): + with TempFATFS() as tmpdir: + yield assert_true, tmpdir is not None diff --git a/nipype/testing/utils.py b/nipype/testing/utils.py index 83847df5d9..97b8c09aa6 100644 --- a/nipype/testing/utils.py +++ b/nipype/testing/utils.py @@ -4,6 +4,12 @@ """ __docformat__ = 'restructuredtext' +import os +import time +import shutil +import signal +import subprocess +from tempfile import mkdtemp from ..utils.misc import package_check from nose import SkipTest @@ -19,3 +25,66 @@ def skip_if_no_package(*args, **kwargs): package_check(exc_failed_import=SkipTest, exc_failed_check=SkipTest, *args, **kwargs) + + +class TempFATFS(object): + def __init__(self, size_in_mbytes=8, delay=0.5): + """Temporary filesystem for testing non-POSIX filesystems on a POSIX + system. + + with TempFATFS() as fatdir: + target = os.path.join(fatdir, 'target') + copyfile(file1, target, copy=False) + assert_false(os.path.islink(target)) + + Arguments + --------- + size_in_mbytes : int + Size (in MiB) of filesystem to create + delay : float + Time (in seconds) to wait for fusefat to start, stop + """ + self.delay = delay + self.tmpdir = mkdtemp() + self.dev_null = open(os.devnull, 'wb') + + vfatfile = os.path.join(self.tmpdir, 'vfatblock') + self.vfatmount = os.path.join(self.tmpdir, 'vfatmount') + self.canary = os.path.join(self.vfatmount, '.canary') + + with open(vfatfile, 'wb') as fobj: + fobj.write(b'\x00' * (int(size_in_mbytes) << 20)) + os.mkdir(self.vfatmount) + + mkfs_args = ['mkfs.vfat', vfatfile] + mount_args = ['fusefat', '-o', 'rw+', '-f', vfatfile, self.vfatmount] + + subprocess.check_call(args=mkfs_args, stdout=self.dev_null, + stderr=self.dev_null) + self.fusefat = subprocess.Popen(args=mount_args, stdout=self.dev_null, + stderr=self.dev_null) + time.sleep(self.delay) + + if self.fusefat.poll() is not None: + raise IOError("fatfuse terminated too soon") + + open(self.canary, 'wb').close() + + def __enter__(self): + return self.vfatmount + + def __exit__(self, exc_type, exc_val, exc_tb): + if self.fusefat is not None: + self.fusefat.send_signal(signal.SIGINT) + + # Allow 1s to return without sending terminate + for count in range(10): + time.sleep(0.1) + if self.fusefat.poll() is not None: + break + else: + self.fusefat.terminate() + time.sleep(self.delay) + assert not os.path.exists(self.canary) + self.dev_null.close() + shutil.rmtree(self.tmpdir) From 1e71d6c928311016dabf65f1ddaf83a6a35d5183 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Sat, 5 Mar 2016 11:40:31 -0500 Subject: [PATCH 2/7] TEST: Test fallback behavior of copyfile Attempt to copy and link files from POSIX to FAT, verify we always copy without throwing errors --- nipype/utils/tests/test_filemanip.py | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/nipype/utils/tests/test_filemanip.py b/nipype/utils/tests/test_filemanip.py index b3a4f71330..4bfc68134b 100644 --- a/nipype/utils/tests/test_filemanip.py +++ b/nipype/utils/tests/test_filemanip.py @@ -5,7 +5,7 @@ import os from tempfile import mkstemp, mkdtemp -from nipype.testing import assert_equal, assert_true, assert_false +from nipype.testing import assert_equal, assert_true, assert_false, TempFATFS from nipype.utils.filemanip import (save_json, load_json, fname_presuffix, fnames_presuffix, hash_rename, check_forhash, @@ -167,6 +167,29 @@ def test_linkchain(): os.unlink(orig_hdr) +def test_copyfallback(): + if os.name is not 'posix': + return + orig_img, orig_hdr = _temp_analyze_files() + pth, imgname = os.path.split(orig_img) + pth, hdrname = os.path.split(orig_hdr) + with TempFATFS() as fatdir: + tgt_img = os.path.join(fatdir, imgname) + tgt_hdr = os.path.join(fatdir, hdrname) + for copy in (True, False): + for use_hardlink in (True, False): + copyfile(orig_img, tgt_img, copy=copy, + use_hardlink=use_hardlink) + yield assert_true, os.path.exists(tgt_img) + yield assert_true, os.path.exists(tgt_hdr) + yield assert_false, os.path.islink(tgt_img) + yield assert_false, os.path.islink(tgt_hdr) + yield assert_false, os.path.samefile(orig_img, tgt_img) + yield assert_false, os.path.samefile(orig_hdr, tgt_hdr) + os.unlink(tgt_img) + os.unlink(tgt_hdr) + + def test_filename_to_list(): x = filename_to_list('foo.nii') yield assert_equal, x, ['foo.nii'] From 050b257c5d164b94a0245d8ff103bdf54eb96721 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Thu, 3 Mar 2016 12:33:38 -0500 Subject: [PATCH 3/7] FIX: Fallback to copy=True on symlink failure --- nipype/utils/filemanip.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index 646686a441..f911d0778f 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -259,7 +259,11 @@ def copyfile(originalfile, newfile, copy=False, create_new=False, if newhash != orighash: os.unlink(newfile) if (newhash is None) or (newhash != orighash): - os.symlink(originalfile, newfile) + try: + os.symlink(originalfile, newfile) + except OSError: + return copyfile(originalfile, newfile, True, create_new, + hashmethod, use_hardlink) else: if newhash: if hashmethod == 'timestamp': From 8540590eb0486a4b74aa02bc56c1fc5131a4ceef Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Thu, 3 Mar 2016 14:45:50 -0500 Subject: [PATCH 4/7] RF: Simplify copyfiles, updating logic for links --- nipype/utils/filemanip.py | 154 +++++++++++++++++++------------------- 1 file changed, 79 insertions(+), 75 deletions(-) diff --git a/nipype/utils/filemanip.py b/nipype/utils/filemanip.py index f911d0778f..65e8f27c9c 100644 --- a/nipype/utils/filemanip.py +++ b/nipype/utils/filemanip.py @@ -15,11 +15,11 @@ import os import re import shutil +import posixpath import numpy as np from .misc import is_container -from .config import mkdir_p from ..external.six import string_types from ..interfaces.traits_extension import isdefined @@ -31,31 +31,6 @@ class FileNotFoundError(Exception): pass -def nipype_hardlink_wrapper(raw_src, raw_dst): - """Attempt to use hard link instead of file copy. - The intent is to avoid unnnecessary duplication - of large files when using a DataSink. - Hard links are not supported on all file systems - or os environments, and will not succeed if the - src and dst are not on the same physical hardware - partition. - If the hardlink fails, then fall back to using - a standard copy. - """ - # Use realpath to avoid hardlinking symlinks - src = os.path.realpath(raw_src) - # Use normpath, in case destination is a symlink - dst = os.path.normpath(raw_dst) - del raw_src - del raw_dst - if src != dst and os.path.exists(dst): - os.unlink(dst) # First remove destination - try: - os.link(src, dst) # Reference same inode to avoid duplication - except: - shutil.copyfile(src, dst) # Fall back to traditional copy - - def split_filename(fname): """Split a filename into parts: path, base filename and extension. @@ -201,7 +176,13 @@ def hash_timestamp(afile): def copyfile(originalfile, newfile, copy=False, create_new=False, hashmethod=None, use_hardlink=False): - """Copy or symlink ``originalfile`` to ``newfile``. + """Copy or link ``originalfile`` to ``newfile``. + + If ``use_hardlink`` is True, and the file can be hard-linked, then a + link is created, instead of copying the file. + + If a hard link is not created and ``copy`` is False, then a symbolic + link is created. Parameters ---------- @@ -212,6 +193,9 @@ def copyfile(originalfile, newfile, copy=False, create_new=False, copy : Bool specifies whether to copy or symlink files (default=False) but only for POSIX systems + use_hardlink : Bool + specifies whether to hard-link files, when able + (Default=False), taking precedence over copy Returns ------- @@ -237,67 +221,87 @@ def copyfile(originalfile, newfile, copy=False, create_new=False, if hashmethod is None: hashmethod = config.get('execution', 'hash_method').lower() - elif os.path.exists(newfile): - if hashmethod == 'timestamp': - newhash = hash_timestamp(newfile) - elif hashmethod == 'content': - newhash = hash_infile(newfile) - fmlogger.debug("File: %s already exists,%s, copy:%d" - % (newfile, newhash, copy)) - # the following seems unnecessary - # if os.name is 'posix' and copy: - # if os.path.lexists(newfile) and os.path.islink(newfile): - # os.unlink(newfile) - # newhash = None - if os.name is 'posix' and not copy: - if os.path.lexists(newfile): - if hashmethod == 'timestamp': - orighash = hash_timestamp(originalfile) - elif hashmethod == 'content': - orighash = hash_infile(originalfile) - fmlogger.debug('Original hash: %s, %s' % (originalfile, orighash)) - if newhash != orighash: - os.unlink(newfile) - if (newhash is None) or (newhash != orighash): - try: - os.symlink(originalfile, newfile) - except OSError: - return copyfile(originalfile, newfile, True, create_new, - hashmethod, use_hardlink) - else: - if newhash: + # Existing file + # ------------- + # Options: + # symlink + # to originalfile (keep if not (use_hardlink or copy)) + # to other file (unlink) + # regular file + # hard link to originalfile (keep) + # copy of file (same hash) (keep) + # different file (diff hash) (unlink) + keep = False + if os.path.lexists(newfile): + if os.path.islink(newfile): + if all(os.path.readlink(newfile) == originalfile, not use_hardlink, + not copy): + keep = True + elif posixpath.samefile(newfile, originalfile): + keep = True + else: if hashmethod == 'timestamp': - orighash = hash_timestamp(originalfile) + hashfn = hash_timestamp elif hashmethod == 'content': - orighash = hash_infile(originalfile) - if (newhash is None) or (newhash != orighash): - try: - fmlogger.debug("Copying File: %s->%s" % - (newfile, originalfile)) - if use_hardlink: - nipype_hardlink_wrapper(originalfile, newfile) - else: - shutil.copyfile(originalfile, newfile) - except shutil.Error as e: - fmlogger.warn(e.message) - else: + hashfn = hash_infile + newhash = hashfn(newfile) + fmlogger.debug("File: %s already exists,%s, copy:%d" % + (newfile, newhash, copy)) + orighash = hashfn(originalfile) + keep = newhash == orighash + if keep: fmlogger.debug("File: %s already exists, not overwriting, copy:%d" % (newfile, copy)) + else: + os.unlink(newfile) + + # New file + # -------- + # use_hardlink & can_hardlink => hardlink + # ~hardlink & ~copy & can_symlink => symlink + # ~hardlink & ~symlink => copy + if not keep and use_hardlink: + try: + fmlogger.debug("Linking File: %s->%s" % (newfile, originalfile)) + # Use realpath to avoid hardlinking symlinks + os.link(os.path.realpath(originalfile), newfile) + except OSError: + use_hardlink = False # Disable hardlink for associated files + else: + keep = True + + if not keep and not copy and os.name == 'posix': + try: + fmlogger.debug("Symlinking File: %s->%s" % (newfile, originalfile)) + os.symlink(originalfile, newfile) + except OSError: + copy = True # Disable symlink for associated files + else: + keep = True + + if not keep: + try: + fmlogger.debug("Copying File: %s->%s" % (newfile, originalfile)) + shutil.copyfile(originalfile, newfile) + except shutil.Error as e: + fmlogger.warn(e.message) + + # Associated files if originalfile.endswith(".img"): hdrofile = originalfile[:-4] + ".hdr" hdrnfile = newfile[:-4] + ".hdr" matofile = originalfile[:-4] + ".mat" if os.path.exists(matofile): matnfile = newfile[:-4] + ".mat" - copyfile(matofile, matnfile, copy, create_new, hashmethod, - use_hardlink) - copyfile(hdrofile, hdrnfile, copy, create_new, hashmethod, - use_hardlink) + copyfile(matofile, matnfile, copy, hashmethod=hashmethod, + use_hardlink=use_hardlink) + copyfile(hdrofile, hdrnfile, copy, hashmethod=hashmethod, + use_hardlink=use_hardlink) elif originalfile.endswith(".BRIK"): hdrofile = originalfile[:-5] + ".HEAD" hdrnfile = newfile[:-5] + ".HEAD" - copyfile(hdrofile, hdrnfile, copy, create_new, hashmethod, - use_hardlink) + copyfile(hdrofile, hdrnfile, copy, hashmethod=hashmethod, + use_hardlink=use_hardlink) return newfile From 5a382ac0de97d127326f9e5761191769314fb7b6 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Mon, 7 Mar 2016 11:39:41 -0500 Subject: [PATCH 5/7] DOC: Add changes for gh-1391 --- CHANGES | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES b/CHANGES index 589474b6d6..abe825748a 100644 --- a/CHANGES +++ b/CHANGES @@ -24,6 +24,7 @@ Next release * ENH: New interfaces for interacting with AWS S3: S3DataSink and S3DataGrabber (https://github.com/nipy/nipype/pull/1201) * ENH: Interfaces for MINC tools (https://github.com/nipy/nipype/pull/1304) * FIX: Use realpath to determine hard link source (https://github.com/nipy/nipype/pull/1388) +* FIX: Correct linking/copying fallback behavior (https://github.com/nipy/nipype/pull/1391) Release 0.11.0 (September 15, 2015) ============ From 0d9ff3de04b4c4c2c73e9b0cf2f743d4741a37e2 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Sun, 20 Mar 2016 21:00:43 -0400 Subject: [PATCH 6/7] TEST: Copy fallback tests optional Should pass on at least one CI server, but a hard FUSE requirement is probably too much to demand just to test nipype. --- nipype/utils/tests/test_filemanip.py | 39 +++++++++++++++++----------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/nipype/utils/tests/test_filemanip.py b/nipype/utils/tests/test_filemanip.py index 4bfc68134b..7389ab4f16 100644 --- a/nipype/utils/tests/test_filemanip.py +++ b/nipype/utils/tests/test_filemanip.py @@ -4,6 +4,7 @@ import os from tempfile import mkstemp, mkdtemp +import warnings from nipype.testing import assert_equal, assert_true, assert_false, TempFATFS from nipype.utils.filemanip import (save_json, load_json, @@ -173,21 +174,29 @@ def test_copyfallback(): orig_img, orig_hdr = _temp_analyze_files() pth, imgname = os.path.split(orig_img) pth, hdrname = os.path.split(orig_hdr) - with TempFATFS() as fatdir: - tgt_img = os.path.join(fatdir, imgname) - tgt_hdr = os.path.join(fatdir, hdrname) - for copy in (True, False): - for use_hardlink in (True, False): - copyfile(orig_img, tgt_img, copy=copy, - use_hardlink=use_hardlink) - yield assert_true, os.path.exists(tgt_img) - yield assert_true, os.path.exists(tgt_hdr) - yield assert_false, os.path.islink(tgt_img) - yield assert_false, os.path.islink(tgt_hdr) - yield assert_false, os.path.samefile(orig_img, tgt_img) - yield assert_false, os.path.samefile(orig_hdr, tgt_hdr) - os.unlink(tgt_img) - os.unlink(tgt_hdr) + try: + fatfs = TempFATFS() + except IOError: + warnings.warn('Fuse mount failed. copyfile fallback tests skipped.') + else: + with fatfs as fatdir: + tgt_img = os.path.join(fatdir, imgname) + tgt_hdr = os.path.join(fatdir, hdrname) + for copy in (True, False): + for use_hardlink in (True, False): + copyfile(orig_img, tgt_img, copy=copy, + use_hardlink=use_hardlink) + yield assert_true, os.path.exists(tgt_img) + yield assert_true, os.path.exists(tgt_hdr) + yield assert_false, os.path.islink(tgt_img) + yield assert_false, os.path.islink(tgt_hdr) + yield assert_false, os.path.samefile(orig_img, tgt_img) + yield assert_false, os.path.samefile(orig_hdr, tgt_hdr) + os.unlink(tgt_img) + os.unlink(tgt_hdr) + finally: + os.unlink(orig_img) + os.unlink(orig_hdr) def test_filename_to_list(): From c557058a7a7206167108535129bc0b160e4fe62b Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Mon, 21 Mar 2016 08:45:10 -0400 Subject: [PATCH 7/7] TEST: Add warning for TempFATFS test --- nipype/testing/tests/test_utils.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/nipype/testing/tests/test_utils.py b/nipype/testing/tests/test_utils.py index 02587ef70f..64be810008 100644 --- a/nipype/testing/tests/test_utils.py +++ b/nipype/testing/tests/test_utils.py @@ -3,10 +3,17 @@ """Test testing utilities """ +import os +import warnings from nipype.testing.utils import TempFATFS from nose.tools import assert_true def test_tempfatfs(): - with TempFATFS() as tmpdir: - yield assert_true, tmpdir is not None + try: + fatfs = TempFATFS() + except IOError: + warnings.warn("Cannot mount FAT filesystems with FUSE") + else: + with fatfs as tmpdir: + yield assert_true, os.path.exists(tmpdir)