Repository: reframe-hpc/reframe

Issue:
`--keep-stage-files --dont-restage` fails when used together with `sourcesdir`
Something like

```
class MPIWrapper(rfm.CompileOnlyRegressionTest):
    sourcesdir = 'https://github.com/eschnett/MPIwrapper'
    build_system = 'CMake'
```

fails., when used with `--keep-stage-files --dont-restage`

```

Valentin Churavy
  Thursday at 4:38 PM
FAILURE INFO for MPIWrapper ~local:default+julia_1.10 (run: 1/1)
  * Description: 
  * System partition: local
  * Environment: julia_1.10
  * Stage directory: /home/vchuravy/src/trixi-parallel-performance/stage/local/default/julia_1.10/MPIWrapper_e9d9ef7f
  * Node list: 
  * Job type: local (id=None)
  * Dependencies (conceptual): []
  * Dependencies (actual): []
  * Maintainers: []
  * Failing phase: compile
  * Reason: spawned process error: command 'git clone  https://github.com/eschnett/MPIwrapper /home/vchuravy/src/trixi-parallel-performance/stage/local/default/julia_1.10/MPIWrapper_e9d9ef7f' failed with exit code 128:
--- stdout ---
--- stdout ---
--- stderr ---
fatal: destination path '/home/vchuravy/src/trixi-parallel-performance/stage/local/default/julia_1.10/MPIWrapper_e9d9ef7f' already exists and is not an empty directory.
```

Hints from issue comments:
@vkarak: I reproduced this. We should skip the clone in case of `--dont-restage`.

Solution patch:
diff --git a/docs/manpage.rst b/docs/manpage.rst
index d464c1e1e..927bf10c2 100644
--- a/docs/manpage.rst
+++ b/docs/manpage.rst
@@ -477,6 +477,10 @@ Options controlling ReFrame output
 
    .. versionadded:: 3.1
 
+   .. warning::
+
+      Running a test with :option:`--dont-restage` on a stage directory that was created with a different ReFrame version is undefined behaviour.
+
 .. option:: --keep-stage-files
 
    Keep test stage directories even for tests that finish successfully.
diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py
index 3907a6137..a89c0baa4 100644
--- a/reframe/core/pipeline.py
+++ b/reframe/core/pipeline.py
@@ -20,6 +20,7 @@
 import numbers
 import os
 import shutil
+from pathlib import Path
 
 import reframe.core.fields as fields
 import reframe.core.hooks as hooks
@@ -1779,6 +1780,21 @@ def setup(self, partition, environ, **job_opts):
         self._setup_container_platform()
         self._resolve_fixtures()
 
+    def _requires_stagedir_contents(self):
+        '''Return true if the contents of the stagedir need to be generated'''
+
+        # Every time the stage directory is created a fresh mark is created.
+        # Normally, this is wiped out before running the test, unless
+        # `--dont-restage` is passed. In this case, we want to leave the
+        # existing stagedir untouched.
+
+        mark = Path(self.stagedir) / '.rfm_mark'
+        if mark.exists():
+            return False
+        else:
+            mark.touch()
+            return True
+
     def _copy_to_stagedir(self, path):
         self.logger.debug(f'Copying {path} to stage directory')
         self.logger.debug(f'Symlinking files: {self.readonly_files}')
@@ -1832,11 +1848,12 @@ def compile(self):
                     f'interpreted as relative to it'
                 )
 
-            if osext.is_url(self.sourcesdir):
-                self._clone_to_stagedir(self.sourcesdir)
-            else:
-                self._copy_to_stagedir(os.path.join(self._prefix,
-                                                    self.sourcesdir))
+            if self._requires_stagedir_contents():
+                if osext.is_url(self.sourcesdir):
+                    self._clone_to_stagedir(self.sourcesdir)
+                else:
+                    self._copy_to_stagedir(os.path.join(self._prefix,
+                                                        self.sourcesdir))
 
         # Set executable (only if hasn't been provided)
         if not hasattr(self, 'executable'):
@@ -2628,7 +2645,7 @@ def run(self):
         The resources of the test are copied to the stage directory and the
         rest of execution is delegated to the :func:`RegressionTest.run()`.
         '''
-        if self.sourcesdir:
+        if self.sourcesdir and self._requires_stagedir_contents():
             if osext.is_url(self.sourcesdir):
                 self._clone_to_stagedir(self.sourcesdir)
             else:


Test patch:
diff --git a/unittests/test_cli.py b/unittests/test_cli.py
index 4e3b1c44b..87eaed30d 100644
--- a/unittests/test_cli.py
+++ b/unittests/test_cli.py
@@ -339,23 +339,42 @@ def test_check_sanity_failure(run_reframe, tmp_path, run_action):
 
 
 def test_dont_restage(run_reframe, tmp_path):
-    run_reframe(
-        checkpath=['unittests/resources/checks/frontend_checks.py'],
-        more_options=['-n', 'SanityFailureCheck']
-    )
+    # Here we test four properties of `--dont-restage`
+    # 1. If the stage directory has not been populated before, it will
+    # 2. If the stage directory has been populated, it will stay untouched
+    # 3. The sourcesdir will not be copied again
+    # 4. When combined with `--max-retries`, the stage directory is reused
+
+    returncode = run_reframe(
+        checkpath=['unittests/resources/checks/'],
+        more_options=['-n', 'SanityFailureCheck', '-n', '^HelloTest$',
+                      '--dont-restage', '--keep-stage-files']
+    )[0]
+    # Assert property (1)
+    assert returncode != 0
 
-    # Place a random file in the test's stage directory and rerun with
-    # `--dont-restage` and `--max-retries`
+    stagedir_runonly = (tmp_path / 'stage' / 'generic' / 'default' /
+                        'builtin' / 'SanityFailureCheck')
     stagedir = (tmp_path / 'stage' / 'generic' / 'default' /
-                'builtin' / 'SanityFailureCheck')
+                'builtin' / 'HelloTest')
+
+    # Place a new file in the stagedir to test (2)
     (stagedir / 'foobar').touch()
+    (stagedir_runonly / 'foobar').touch()
+
+    # Remove a not-needed file to test (2) and (3)
+    (stagedir / 'Makefile').unlink()
+    (stagedir_runonly / 'Makefile').unlink()
+
+    # Use `--max-retries` to test (4)
     returncode, stdout, stderr = run_reframe(
         checkpath=['unittests/resources/checks/frontend_checks.py'],
         more_options=['-n', 'SanityFailureCheck',
                       '--dont-restage', '--max-retries=1']
     )
-    assert os.path.exists(stagedir / 'foobar')
-    assert not os.path.exists(f'{stagedir}_retry1')
+    assert os.path.exists(stagedir_runonly / 'foobar')
+    assert not os.path.exists(stagedir_runonly / 'Makefile')
+    assert not os.path.exists(f'{stagedir_runonly}_retry1')
 
     # And some standard assertions
     assert 'Traceback' not in stdout