From d4533be17c6d797318f009b041f4371051a50b50 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 7 Jul 2020 18:43:06 +0200 Subject: [PATCH 1/4] WIP: Dont restage --- reframe/core/runtime.py | 11 +++++++++-- reframe/frontend/cli.py | 5 +++++ reframe/schemas/config.json | 2 ++ unittests/test_cli.py | 25 +++++++++++++++++++++++++ 4 files changed, 41 insertions(+), 2 deletions(-) diff --git a/reframe/core/runtime.py b/reframe/core/runtime.py index 32052a5de6..3cad8b006f 100644 --- a/reframe/core/runtime.py +++ b/reframe/core/runtime.py @@ -42,6 +42,11 @@ def _makedir(self, *dirs, wipeout=False): return ret def _format_dirs(self, *dirs): + if not self.get_option('general/0/clean_stagedir'): + # If stagedir is to be reused, no new stage directories will be + # used for retries + return dirs + try: last = dirs[-1] except IndexError: @@ -134,11 +139,13 @@ def stage_prefix(self): return os.path.abspath(ret) - def make_stagedir(self, *dirs, wipeout=True): + def make_stagedir(self, *dirs): + wipeout = self.get_option('general/0/clean_stagedir') return self._makedir(self.stage_prefix, *self._format_dirs(*dirs), wipeout=wipeout) - def make_outputdir(self, *dirs, wipeout=True): + def make_outputdir(self, *dirs): + wipeout = self.get_option('general/0/clean_stagedir') return self._makedir(self.output_prefix, *self._format_dirs(*dirs), wipeout=wipeout) diff --git a/reframe/frontend/cli.py b/reframe/frontend/cli.py index 9fc324dd1f..f650fd70e7 100644 --- a/reframe/frontend/cli.py +++ b/reframe/frontend/cli.py @@ -126,6 +126,11 @@ def main(): help='Keep stage directories even for successful checks', envvar='RFM_KEEP_STAGE_FILES', configvar='general/keep_stage_files' ) + output_options.add_argument( + '--dont-restage', action='store_false', dest='clean_stagedir', + help='Reuse the test stage directory', + envvar='RFM_CLEAN_STAGEDIR', configvar='general/clean_stagedir' + ) output_options.add_argument( '--save-log-files', action='store_true', default=False, help='Save ReFrame log files to the output directory', diff --git a/reframe/schemas/config.json b/reframe/schemas/config.json index 433fff6ce5..4f11202346 100644 --- a/reframe/schemas/config.json +++ b/reframe/schemas/config.json @@ -350,6 +350,7 @@ "items": {"type": "string"} }, "check_search_recursive": {"type": "boolean"}, + "clean_stagedir": {"type": "boolean"}, "colorize": {"type": "boolean"}, "ignore_check_conflicts": {"type": "boolean"}, "keep_stage_files": {"type": "boolean"}, @@ -394,6 +395,7 @@ "environments/target_systems": ["*"], "general/check_search_path": ["${RFM_INSTALL_PREFIX}/checks/"], "general/check_search_recursive": false, + "general/clean_stagedir": true, "general/colorize": true, "general/ignore_check_conflicts": false, "general/keep_stage_files": false, diff --git a/unittests/test_cli.py b/unittests/test_cli.py index f7515ed803..a5cf59b1ea 100644 --- a/unittests/test_cli.py +++ b/unittests/test_cli.py @@ -221,6 +221,31 @@ def test_check_sanity_failure(run_reframe, tmp_path): ) +def test_dont_restage(run_reframe, tmp_path): + run_reframe( + checkpath=['unittests/resources/checks/frontend_checks.py'], + more_options=['-t', 'SanityFailureCheck'] + ) + + # Place a random file in the test's stage directory and rerun with + # `--dont-restage` and `--max-retries` + stagedir = (tmp_path / 'stage' / 'generic' / 'default' / + 'builtin-gcc' / 'SanityFailureCheck') + (stagedir / 'foobar').touch() + returncode, stdout, stderr = run_reframe( + checkpath=['unittests/resources/checks/frontend_checks.py'], + more_options=['-t', 'SanityFailureCheck', + '--dont-restage', '--max-retries=1'] + ) + assert os.path.exists(stagedir / 'foobar') + assert not os.path.exists(f'{stagedir}_retry1') + + # And some standard assertions + assert 'Traceback' not in stdout + assert 'Traceback' not in stderr + assert returncode != 0 + + def test_checkpath_symlink(run_reframe, tmp_path): # FIXME: This should move to test_loader.py checks_symlink = tmp_path / 'checks_symlink' From 7d4521785c53b105d02410d4df965a4e58fe4dc6 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Wed, 8 Jul 2020 00:38:18 +0200 Subject: [PATCH 2/4] Add option to disable cleaning of the test stage directory To better support this, the `os_ext.copytree()` was rewritten to comply with the `dirs_exist_ok` argument added in Python 3.8 --- reframe/core/pipeline.py | 6 ++-- reframe/core/runtime.py | 3 +- reframe/utility/os_ext.py | 66 +++++++++++++++++++++++++++------------ unittests/test_utility.py | 38 ++++++++++++---------- 4 files changed, 72 insertions(+), 41 deletions(-) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index de88b87e73..4afd1de7c4 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -1069,9 +1069,11 @@ def _copy_to_stagedir(self, path): (path, self._stagedir)) self.logger.debug('symlinking files: %s' % self.readonly_files) try: - os_ext.copytree_virtual(path, self._stagedir, self.readonly_files) + os_ext.copytree_virtual( + path, self._stagedir, self.readonly_files, dirs_exist_ok=True + ) except (OSError, ValueError, TypeError) as e: - raise PipelineError('virtual copying of files failed') from e + raise PipelineError('copying of files failed') from e def _clone_to_stagedir(self, url): self.logger.debug('cloning URL %s to stage directory (%s)' % diff --git a/reframe/core/runtime.py b/reframe/core/runtime.py index 3cad8b006f..4c74dcd95f 100644 --- a/reframe/core/runtime.py +++ b/reframe/core/runtime.py @@ -145,9 +145,8 @@ def make_stagedir(self, *dirs): *self._format_dirs(*dirs), wipeout=wipeout) def make_outputdir(self, *dirs): - wipeout = self.get_option('general/0/clean_stagedir') return self._makedir(self.output_prefix, - *self._format_dirs(*dirs), wipeout=wipeout) + *self._format_dirs(*dirs), wipeout=True) @property def modules_system(self): diff --git a/reframe/utility/os_ext.py b/reframe/utility/os_ext.py index 7a25d3910f..fe967e27ef 100644 --- a/reframe/utility/os_ext.py +++ b/reframe/utility/os_ext.py @@ -95,25 +95,44 @@ def osgroup(): def copytree(src, dst, symlinks=False, ignore=None, copy_function=shutil.copy2, - ignore_dangling_symlinks=False): - '''Same as shutil.copytree() but valid also if 'dst' exists. - - In this case it will first remove it and then call the standard - shutil.copytree().''' + ignore_dangling_symlinks=False, dirs_exist_ok=False): + '''Compatibility version of :py:func:`shutil.copytree()` for Python <= 3.8 + ''' if src == os.path.commonpath([src, dst]): raise ValueError("cannot copy recursively the parent directory " "`%s' into one of its descendants `%s'" % (src, dst)) - if os.path.exists(dst): - shutil.rmtree(dst) + if sys.version_info[2] >= 8: + return shutil.copytree(src, dst, symlinks, ignore, copy_function, + ignore_dangling_symlinks, dirs_exist_ok) + + if not dirs_exist_ok: + return shutil.copytree(src, dst, symlinks, ignore, copy_function, + ignore_dangling_symlinks) + + # dirs_exist_ok=True and Python < 3.8 + if not os.path.exists(dst): + return shutil.copytree(src, dst, symlinks, ignore, copy_function, + ignore_dangling_symlinks) + + # dst exists; manually descend into the subdirectories + _, subdirs, files = list(os.walk(src))[0] + ignore_paths = ignore(src, os.listdir(src)) if ignore else {} + for f in files: + if f not in ignore_paths: + copy_function(os.path.join(src, f), os.path.join(dst, f)) - shutil.copytree(src, dst, symlinks, ignore, copy_function, - ignore_dangling_symlinks) + for d in subdirs: + if d not in ignore_paths: + copytree(os.path.join(src, d), os.path.join(dst, d), symlinks, + ignore, copy_function, ignore_dangling_symlinks) + + return dst def copytree_virtual(src, dst, file_links=[], symlinks=False, copy_function=shutil.copy2, - ignore_dangling_symlinks=False): + ignore_dangling_symlinks=False, dirs_exist_ok=False): '''Copy `dst` to `src`, but create symlinks for the files in `file_links`. If `file_links` is empty, this is equivalent to `copytree()`. The rest of @@ -134,35 +153,42 @@ def copytree_virtual(src, dst, file_links=[], link_targets = set() for f in file_links: if os.path.isabs(f): - raise ValueError("copytree_virtual() failed: `%s': " - "absolute paths not allowed in file_links" % f) + raise ValueError(f'copytree_virtual() failed: {f!r}: ' + f'absolute paths not allowed in file_links') target = os.path.join(src, f) if not os.path.exists(target): - raise ValueError("copytree_virtual() failed: `%s' " - "does not exist" % target) + raise ValueError(f'copytree_virtual() failed: {target!r} ' + f'does not exist') if os.path.commonpath([src, target]) != src: - raise ValueError("copytree_virtual() failed: " - "`%s' not under `%s'" % (target, src)) + raise ValueError(f'copytree_virtual() failed: ' + f'{target!r} not under {src!r}') link_targets.add(os.path.abspath(target)) + if '.' in file_links or '..' in file_links: + raise ValueError(f"'.' or '..' are not allowed in file_links") + if not file_links: ignore = None else: def ignore(dir, contents): - return [c for c in contents - if os.path.join(dir, c) in link_targets] + return {c for c in contents + if os.path.join(dir, c) in link_targets} # Copy to dst ignoring the file_links copytree(src, dst, symlinks, ignore, - copy_function, ignore_dangling_symlinks) + copy_function, ignore_dangling_symlinks, dirs_exist_ok) # Now create the symlinks for f in link_targets: link_name = f.replace(src, dst) - os.symlink(f, link_name) + try: + os.symlink(f, link_name) + except FileExistsError: + if not dirs_exist_ok: + raise def rmtree(*args, max_retries=3, **kwargs): diff --git a/unittests/test_utility.py b/unittests/test_utility.py index ddef3d10ce..5e76f82908 100644 --- a/unittests/test_utility.py +++ b/unittests/test_utility.py @@ -61,15 +61,7 @@ def test_command_async(self): def test_copytree(self): dir_src = tempfile.mkdtemp() dir_dst = tempfile.mkdtemp() - - with pytest.raises(OSError): - shutil.copytree(dir_src, dir_dst) - - try: - os_ext.copytree(dir_src, dir_dst) - except Exception as e: - pytest.fail('custom copytree failed: %s' % e) - + os_ext.copytree(dir_src, dir_dst, dirs_exist_ok=True) shutil.rmtree(dir_src) shutil.rmtree(dir_dst) @@ -301,38 +293,50 @@ def verify_target_directory(self, file_links=[]): assert target_name == os.readlink(link_name) def test_virtual_copy_nolinks(self): - os_ext.copytree_virtual(self.prefix, self.target) + os_ext.copytree_virtual(self.prefix, self.target, dirs_exist_ok=True) self.verify_target_directory() def test_virtual_copy_valid_links(self): file_links = ['bar/', 'foo/bar.txt', 'foo.txt'] - os_ext.copytree_virtual(self.prefix, self.target, file_links) + os_ext.copytree_virtual(self.prefix, self.target, + file_links, dirs_exist_ok=True) self.verify_target_directory(file_links) def test_virtual_copy_inexistent_links(self): file_links = ['foobar/', 'foo/bar.txt', 'foo.txt'] with pytest.raises(ValueError): - os_ext.copytree_virtual(self.prefix, self.target, file_links) + os_ext.copytree_virtual(self.prefix, self.target, + file_links, dirs_exist_ok=True) def test_virtual_copy_absolute_paths(self): file_links = [os.path.join(self.prefix, 'bar'), 'foo/bar.txt', 'foo.txt'] with pytest.raises(ValueError): - os_ext.copytree_virtual(self.prefix, self.target, file_links) + os_ext.copytree_virtual(self.prefix, self.target, + file_links, dirs_exist_ok=True) def test_virtual_copy_irrelevenant_paths(self): file_links = ['/bin', 'foo/bar.txt', 'foo.txt'] with pytest.raises(ValueError): - os_ext.copytree_virtual(self.prefix, self.target, file_links) + os_ext.copytree_virtual(self.prefix, self.target, + file_links, dirs_exist_ok=True) file_links = [os.path.dirname(self.prefix), 'foo/bar.txt', 'foo.txt'] with pytest.raises(ValueError): - os_ext.copytree_virtual(self.prefix, self.target, file_links) + os_ext.copytree_virtual(self.prefix, self.target, + file_links, dirs_exist_ok=True) def test_virtual_copy_linkself(self): file_links = ['.'] - with pytest.raises(OSError): - os_ext.copytree_virtual(self.prefix, self.target, file_links) + with pytest.raises(ValueError): + os_ext.copytree_virtual(self.prefix, self.target, + file_links, dirs_exist_ok=True) + + def test_virtual_copy_linkparent(self): + file_links = ['..'] + with pytest.raises(ValueError): + os_ext.copytree_virtual(self.prefix, self.target, + file_links, dirs_exist_ok=True) def tearDown(self): shutil.rmtree(self.prefix) From 5b4cc6bedc4327d35faff0bb40bce6a302cdda51 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Wed, 8 Jul 2020 21:19:53 +0200 Subject: [PATCH 3/4] Document the clean_stagedir config and cmd option Plus a minor fix in the implementation along with unit test adaptation. --- docs/config_reference.rst | 10 ++++++++++ docs/manpage.rst | 23 +++++++++++++++++++++++ reframe/utility/os_ext.py | 5 +++-- unittests/test_utility.py | 7 +++++++ 4 files changed, 43 insertions(+), 2 deletions(-) diff --git a/docs/config_reference.rst b/docs/config_reference.rst index eaa0f5ff47..f725323a6c 100644 --- a/docs/config_reference.rst +++ b/docs/config_reference.rst @@ -1037,6 +1037,16 @@ General Configuration +.. js:attribute:: .general[].clean_stagedir + + :required: No + :default: ``true`` + + Clean stage directory of tests before populating it. + + .. versionadded:: 3.1 + + .. js:attribute:: .general[].colorize :required: No diff --git a/docs/manpage.rst b/docs/manpage.rst index d993f3b28f..c83151f895 100644 --- a/docs/manpage.rst +++ b/docs/manpage.rst @@ -219,6 +219,14 @@ Options controlling ReFrame output This option can also be set using the :envvar:`RFM_KEEP_STAGE_FILES` environment variable or the :js:attr:`keep_stage_files` general configuration parameter. +.. option:: --dont-restage + + Do not restage a test if its stage directory exists. + Normally, if the stage directory of a test exists, ReFrame will remove it and recreate it. + This option disables this behavior. + + .. versionadded:: 3.1 + .. option:: --save-log-files Save ReFrame log files in the output directory before exiting. @@ -585,6 +593,21 @@ Here is an alphabetical list of the environment variables recognized by ReFrame: ================================== ================== +.. envvar:: RFM_CLEAN_STAGEDIR + + Clean stage directory of tests before populating it. + + .. versionadded:: 3.1 + + .. table:: + :align: left + + ================================== ================== + Associated command line option :option:`--dont-restage` + Associated configuration parameter :js:attr:`clean_stagedir` general configuration parameter + ================================== ================== + + .. envvar:: RFM_COLORIZE Enable output coloring. diff --git a/reframe/utility/os_ext.py b/reframe/utility/os_ext.py index fe967e27ef..771cf1db1f 100644 --- a/reframe/utility/os_ext.py +++ b/reframe/utility/os_ext.py @@ -124,8 +124,9 @@ def copytree(src, dst, symlinks=False, ignore=None, copy_function=shutil.copy2, for d in subdirs: if d not in ignore_paths: - copytree(os.path.join(src, d), os.path.join(dst, d), symlinks, - ignore, copy_function, ignore_dangling_symlinks) + copytree(os.path.join(src, d), os.path.join(dst, d), + symlinks, ignore, copy_function, + ignore_dangling_symlinks, dirs_exist_ok) return dst diff --git a/unittests/test_utility.py b/unittests/test_utility.py index 5e76f82908..8c73ec8bf0 100644 --- a/unittests/test_utility.py +++ b/unittests/test_utility.py @@ -276,6 +276,9 @@ def setUp(self): open(os.path.join(self.prefix, 'bar.txt'), 'w').close() open(os.path.join(self.prefix, 'foo.txt'), 'w').close() + # Create also a subdirectory in target, so as to check the recursion + os.makedirs(os.path.join(self.target, 'foo'), exist_ok=True) + def verify_target_directory(self, file_links=[]): '''Verify the directory structure''' assert os.path.exists(os.path.join(self.target, 'bar', 'bar.txt')) @@ -296,6 +299,10 @@ def test_virtual_copy_nolinks(self): os_ext.copytree_virtual(self.prefix, self.target, dirs_exist_ok=True) self.verify_target_directory() + def test_virtual_copy_nolinks_dirs_exist(self): + with pytest.raises(FileExistsError): + os_ext.copytree_virtual(self.prefix, self.target) + def test_virtual_copy_valid_links(self): file_links = ['bar/', 'foo/bar.txt', 'foo.txt'] os_ext.copytree_virtual(self.prefix, self.target, From 79923646c6dd35741cdd6a64d1cd21e1f2ced18a Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Thu, 9 Jul 2020 00:29:38 +0200 Subject: [PATCH 4/4] Fix Python version check in copytree() --- reframe/utility/os_ext.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reframe/utility/os_ext.py b/reframe/utility/os_ext.py index 771cf1db1f..b32d58efde 100644 --- a/reframe/utility/os_ext.py +++ b/reframe/utility/os_ext.py @@ -102,7 +102,7 @@ def copytree(src, dst, symlinks=False, ignore=None, copy_function=shutil.copy2, raise ValueError("cannot copy recursively the parent directory " "`%s' into one of its descendants `%s'" % (src, dst)) - if sys.version_info[2] >= 8: + if sys.version_info[1] >= 8: return shutil.copytree(src, dst, symlinks, ignore, copy_function, ignore_dangling_symlinks, dirs_exist_ok)