add 'change_into_dir' named argument to 'extract_file' + print deprecation warning if it's not specified

Author: boegel

easybuild/framework/easyblock.py

@@ -1911,7 +1911,9 @@ def extract_step(self):
         """
         for src in self.src:
             self.log.info("Unpacking source %s" % src['name'])
-            srcdir = extract_file(src['path'], self.builddir, cmd=src['cmd'], extra_options=self.cfg['unpack_options'])
+            srcdir = extract_file(src['path'], self.builddir, cmd=src['cmd'],
+                                  extra_options=self.cfg['unpack_options'], change_into_dir=False)
+            change_dir(srcdir)
             if srcdir:
                 self.src[self.src.index(src)]['finalpath'] = srcdir
             else:

easybuild/framework/extensioneasyblock.py

@@ -103,7 +103,9 @@ def run(self, unpack_src=False):
         # unpack file if desired
         if unpack_src:
             targetdir = os.path.join(self.master.builddir, remove_unwanted_chars(self.name))
-            self.ext_dir = extract_file("%s" % self.src, targetdir, extra_options=self.unpack_options)
+            self.ext_dir = extract_file(self.src, targetdir, extra_options=self.unpack_options,
+                                        change_into_dir=False)
+            change_dir(self.ext_dir)
 
             if self.start_dir and os.path.isdir(self.start_dir):
                 self.log.debug("Using start_dir: %s", self.start_dir)

easybuild/tools/filetools.py

@@ -372,7 +372,7 @@ def change_dir(path):
     return cwd
 
 
-def extract_file(fn, dest, cmd=None, extra_options=None, overwrite=False, forced=False):
+def extract_file(fn, dest, cmd=None, extra_options=None, overwrite=False, forced=False, change_into_dir=None):
     """
     Extract file at given path to specified directory
     :param fn: path to file to extract
@@ -381,8 +381,16 @@ def extract_file(fn, dest, cmd=None, extra_options=None, overwrite=False, forced
     :param extra_options: extra options to pass to extract command
     :param overwrite: overwrite existing unpacked file
     :param forced: force extraction in (extended) dry run mode
+    :param change_into_dir: change into resulting directory;
+                          None (current default) implies True, but this is deprecated,
+                          this named argument should be set to False or True explicitely
+                          (in a future major release, default will be changed to False)
     :return: path to directory (in case of success)
     """
+    if change_into_dir is None:
+        _log.deprecated("extract_file function was called without specifying value for change_into_dir", '5.0')
+        change_into_dir = True
+
     if not os.path.isfile(fn) and not build_option('extended_dry_run'):
         raise EasyBuildError("Can't extract file %s: no such file", fn)
 
@@ -392,8 +400,8 @@ def extract_file(fn, dest, cmd=None, extra_options=None, overwrite=False, forced
     abs_dest = os.path.abspath(dest)
 
     # change working directory
-    _log.debug("Unpacking %s in directory %s.", fn, abs_dest)
-    change_dir(abs_dest)
+    _log.debug("Unpacking %s in directory %s", fn, abs_dest)
+    cwd = change_dir(abs_dest)
 
     if not cmd:
         cmd = extract_cmd(fn, overwrite=overwrite)
@@ -408,7 +416,18 @@ def extract_file(fn, dest, cmd=None, extra_options=None, overwrite=False, forced
 
     run.run_cmd(cmd, simple=True, force_in_dry_run=forced)
 
-    return find_base_dir()
+    # note: find_base_dir also changes into the base dir!
+    base_dir = find_base_dir()
+
+    # if changing into obtained directory is not desired,
+    # change back to where we came from (unless that was a non-existing directory)
+    if not change_into_dir:
+        if cwd is None:
+            _log.warning("Can't change back to non-existing directory after extracting %s in %s", fn, dest)
+        else:
+            change_dir(cwd)
+
+    return base_dir
 
 
 def which(cmd, retain_all=False, check_perms=True, log_ok=True, log_error=True):
@@ -1186,7 +1205,8 @@ def apply_patch(patch_file, dest, fn=None, copy=False, level=None, use_git_am=Fa
             workdir = tempfile.mkdtemp(prefix='eb-patch-')
             _log.debug("Extracting the patch to: %s", workdir)
             # extracting the patch
-            apatch_dir = extract_file(apatch, workdir)
+            apatch_dir = extract_file(apatch, workdir, change_into_dir=False)
+            change_dir(apatch_dir)
             apatch = os.path.join(apatch_dir, apatch_name)
 
     if level is None and build_option('extended_dry_run'):

easybuild/tools/github.py

@@ -50,7 +50,7 @@
 from easybuild.framework.easyconfig.parser import EasyConfigParser
 from easybuild.tools.build_log import EasyBuildError, print_msg, print_warning
 from easybuild.tools.config import build_option
-from easybuild.tools.filetools import apply_patch, copy_dir, copy_easyblocks, copy_framework_files
+from easybuild.tools.filetools import apply_patch, change_dir, copy_dir, copy_easyblocks, copy_framework_files
 from easybuild.tools.filetools import det_patched_files, download_file, extract_file
 from easybuild.tools.filetools import get_easyblock_class_name, mkdir, read_file, symlink, which, write_file
 from easybuild.tools.py2vs3 import HTTPError, URLError, ascii_letters, urlopen
@@ -360,7 +360,9 @@ def download_repo(repo=GITHUB_EASYCONFIGS_REPO, branch='master', account=GITHUB_
     download_file(base_name, url, target_path, forced=True)
     _log.debug("%s downloaded to %s, extracting now" % (base_name, path))
 
-    extracted_path = os.path.join(extract_file(target_path, path, forced=True), extracted_dir_name)
+    base_dir = extract_file(target_path, path, forced=True, change_into_dir=False)
+    change_dir(base_dir)
+    extracted_path = os.path.join(base_dir, extracted_dir_name)
 
     # check if extracted_path exists
     if not os.path.isdir(extracted_path):

test/framework/filetools.py

@@ -1327,7 +1327,8 @@ def test_apply_patch(self):
         """ Test apply_patch """
         testdir = os.path.dirname(os.path.abspath(__file__))
         tmpdir = self.test_prefix
-        path = ft.extract_file(os.path.join(testdir, 'sandbox', 'sources', 'toy', 'toy-0.0.tar.gz'), tmpdir)
+        toy_tar_gz = os.path.join(testdir, 'sandbox', 'sources', 'toy', 'toy-0.0.tar.gz')
+        path = ft.extract_file(toy_tar_gz, tmpdir, change_into_dir=False)
         toy_patch_fn = 'toy-0.0_fix-silly-typo-in-printf-statement.patch'
         toy_patch = os.path.join(testdir, 'sandbox', 'sources', 'toy', toy_patch_fn)
 
@@ -1597,19 +1598,24 @@ def test_change_dir(self):
 
     def test_extract_file(self):
         """Test extract_file"""
+        cwd = os.getcwd()
+
         testdir = os.path.dirname(os.path.abspath(__file__))
         toy_tarball = os.path.join(testdir, 'sandbox', 'sources', 'toy', 'toy-0.0.tar.gz')
 
         self.assertFalse(os.path.exists(os.path.join(self.test_prefix, 'toy-0.0', 'toy.source')))
-        path = ft.extract_file(toy_tarball, self.test_prefix)
+        path = ft.extract_file(toy_tarball, self.test_prefix, change_into_dir=False)
         self.assertTrue(os.path.exists(os.path.join(self.test_prefix, 'toy-0.0', 'toy.source')))
         self.assertTrue(os.path.samefile(path, self.test_prefix))
+        # still in same directory as before if change_into_dir is set to False
+        self.assertTrue(os.path.samefile(os.getcwd(), cwd))
         shutil.rmtree(os.path.join(path, 'toy-0.0'))
 
         toy_tarball_renamed = os.path.join(self.test_prefix, 'toy_tarball')
         shutil.copyfile(toy_tarball, toy_tarball_renamed)
 
-        path = ft.extract_file(toy_tarball_renamed, self.test_prefix, cmd="tar xfvz %s")
+        path = ft.extract_file(toy_tarball_renamed, self.test_prefix, cmd="tar xfvz %s", change_into_dir=False)
+        self.assertTrue(os.path.samefile(os.getcwd(), cwd))
         self.assertTrue(os.path.exists(os.path.join(self.test_prefix, 'toy-0.0', 'toy.source')))
         self.assertTrue(os.path.samefile(path, self.test_prefix))
         shutil.rmtree(os.path.join(path, 'toy-0.0'))
@@ -1622,17 +1628,56 @@ def test_extract_file(self):
         init_config(build_options=build_options)
 
         self.mock_stdout(True)
-        path = ft.extract_file(toy_tarball, self.test_prefix)
+        path = ft.extract_file(toy_tarball, self.test_prefix, change_into_dir=False)
         txt = self.get_stdout()
         self.mock_stdout(False)
+        self.assertTrue(os.path.samefile(os.getcwd(), cwd))
 
         self.assertTrue(os.path.samefile(path, self.test_prefix))
         self.assertFalse(os.path.exists(os.path.join(self.test_prefix, 'toy-0.0')))
         self.assertTrue(re.search('running command "tar xzf .*/toy-0.0.tar.gz"', txt))
 
-        path = ft.extract_file(toy_tarball, self.test_prefix, forced=True)
+        path = ft.extract_file(toy_tarball, self.test_prefix, forced=True, change_into_dir=False)
         self.assertTrue(os.path.exists(os.path.join(self.test_prefix, 'toy-0.0', 'toy.source')))
         self.assertTrue(os.path.samefile(path, self.test_prefix))
+        self.assertTrue(os.path.samefile(os.getcwd(), cwd))
+
+        build_options['extended_dry_run'] = False
+        init_config(build_options=build_options)
+
+        ft.remove_dir(os.path.join(self.test_prefix, 'toy-0.0'))
+
+        # a deprecation warning is printed (which is an error in this context)
+        # if the 'change_into_dir' named argument was left unspecified
+        error_pattern = "extract_file function was called without specifying value for change_into_dir"
+        self.assertErrorRegex(EasyBuildError, error_pattern, ft.extract_file, toy_tarball, self.test_prefix)
+        self.allow_deprecated_behaviour()
+
+        # make sure we're not in self.test_prefix now (checks below assumes so)
+        self.assertFalse(os.path.samefile(os.getcwd(), self.test_prefix))
+
+        # by default, extract_file changes to directory in which source file was unpacked
+        self.mock_stderr(True)
+        path = ft.extract_file(toy_tarball, self.test_prefix)
+        stderr = self.get_stderr().strip()
+        self.mock_stderr(False)
+        self.assertTrue(os.path.samefile(path, self.test_prefix))
+        self.assertTrue(os.path.samefile(os.getcwd(), self.test_prefix))
+        regex = re.compile("^WARNING: .*extract_file function was called without specifying value for change_into_dir")
+        self.assertTrue(regex.search(stderr), "Pattern '%s' found in: %s" % (regex.pattern, stderr))
+
+        ft.change_dir(cwd)
+        self.assertFalse(os.path.samefile(os.getcwd(), self.test_prefix))
+
+        # no deprecation warning when change_into_dir is set to True
+        self.mock_stderr(True)
+        path = ft.extract_file(toy_tarball, self.test_prefix, change_into_dir=True)
+        stderr = self.get_stderr().strip()
+        self.mock_stderr(False)
+
+        self.assertTrue(os.path.samefile(path, self.test_prefix))
+        self.assertTrue(os.path.samefile(os.getcwd(), self.test_prefix))
+        self.assertFalse(stderr)
 
     def test_remove(self):
         """Test remove_file, remove_dir and join remove functions."""