[1/2] framework/programs/run.py: Delete tests directory if it exists

Submitted by Dylan Baker on Aug. 3, 2015, 11:46 p.m.

Details

Message ID 1438645583-31021-1-git-send-email-baker.dylan.c@gmail.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Dylan Baker Aug. 3, 2015, 11:46 p.m.
This prevents piglit from aggregating tests from a previous run into the
new run by deleting the old tests directory before starting.

Signed-off-by: Dylan Baker <dylanx.c.baker@intel.com>
---
 framework/programs/run.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/framework/programs/run.py b/framework/programs/run.py
index 2981ffa..6465750 100644
--- a/framework/programs/run.py
+++ b/framework/programs/run.py
@@ -28,6 +28,7 @@  import os.path as path
 import time
 import ConfigParser
 import ctypes
+import shutil
 
 from framework import core, backends, exceptions
 import framework.results
@@ -265,7 +266,14 @@  def run(input_):
     # Change working directory to the root of the piglit directory
     piglit_dir = path.dirname(path.realpath(sys.argv[0]))
     os.chdir(piglit_dir)
-    core.checkDir(args.results_path, False)
+    if os.path.exists(args.results_path):
+        try:
+            shutil.rmtree(os.path.join(args.results_path, 'tests'))
+        except OSError:
+            pass
+    else:
+        os.makedirs(args.results_path)
+
 
     results = framework.results.TestrunResult()
     backends.set_meta(args.backend, results)

Comments

On Mon, Aug 03, 2015 at 04:46:22PM -0700, Dylan Baker wrote:
> This prevents piglit from aggregating tests from a previous run into the
> new run by deleting the old tests directory before starting.
> 
> Signed-off-by: Dylan Baker <dylanx.c.baker@intel.com>
> ---
>  framework/programs/run.py | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/framework/programs/run.py b/framework/programs/run.py
> index 2981ffa..6465750 100644
> --- a/framework/programs/run.py
> +++ b/framework/programs/run.py
> @@ -28,6 +28,7 @@ import os.path as path
>  import time
>  import ConfigParser
>  import ctypes
> +import shutil
>  
>  from framework import core, backends, exceptions
>  import framework.results
> @@ -265,7 +266,14 @@ def run(input_):
>      # Change working directory to the root of the piglit directory
>      piglit_dir = path.dirname(path.realpath(sys.argv[0]))
>      os.chdir(piglit_dir)
> -    core.checkDir(args.results_path, False)
> +    if os.path.exists(args.results_path):
> +        try:
> +            shutil.rmtree(os.path.join(args.results_path, 'tests'))
> +        except OSError:
> +            pass

Doesn't this have the effect of accidentally removing old results you
might want to keep if you do a type (with e.g. shell completion)? Old
piglit refused to run if the results dir was there iirc, so maybe we need
a --force-overwrite option or similar?
-Daniel

> +    else:
> +        os.makedirs(args.results_path)
> +
>  
>      results = framework.results.TestrunResult()
>      backends.set_meta(args.backend, results)
> -- 
> 2.5.0
> 
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
On Tue, Aug 4, 2015 at 2:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> Doesn't this have the effect of accidentally removing old results you
> might want to keep if you do a type (with e.g. shell completion)? Old
> piglit refused to run if the results dir was there iirc, so maybe we need
> a --force-overwrite option or similar?
> -Daniel
>

For summary it does fail if the results already exist, but for run I don't
think it ever has required --overwrite option. This is a bug of the
transition from having a single open file to using one file per test to
atomicity in piglit.

Since we moved to JSON, the results file would be opened, each test would
be written into it serially, and at the end it would be closed. This leads
to some odd bugs if you do something like this:
`piglit run quick deqp-gles3 foo -c`

let it run, so 35,000 tests, and then stop it with ctrl+\, and then do:
`piglit run quick foo -c`

Since the quick profile only as ~25000 tests IIRC, you'll end up with up to
10,000 tests from the original run scooped up into the new run, leading to
very odd bugs. So we remove then at the start. In fact, the only way these
files should be used is during a `piglit resume`

This doesn't change the fact that results.json will be overwritten if you
do this. If we want to have a --overwrite switch we can do that, but that's
a separate change.

Does that make sense? Do I just need to change the commit message to be
clearer?

[snip]
On Tue, Aug 04, 2015 at 02:54:56PM -0700, Dylan Baker wrote:
> > On Tue, Aug 4, 2015 at 2:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > Doesn't this have the effect of accidentally removing old results you
> > might want to keep if you do a type (with e.g. shell completion)? Old
> > piglit refused to run if the results dir was there iirc, so maybe we need
> > a --force-overwrite option or similar?
> > -Daniel
> 
> Wow, I should not write emails right after I get up, sigh.
> Here's a better version of that email:
>  
> For summary it does fail if the results already exist, but for run I don't
> think it ever has required --overwrite option.

Indeed I quickly checked an old version of piglit and piglit-run happily
overwrites files there too. No concern from my side, but the extended
commit message would indeed be nice ;-)
-Daniel

> 
> The bug this patch attempts to fix is one introduced by the transition
> from having a single open file to using one file per test to atomicity
> in piglit.  Since we moved to JSON, the results file would be opened,
> each test would be written into it serially, and at the end it would be
> closed.
> 
> After the transition to atomicity we write a single file per test and
> then combine them into a single JSON file at the end. 
> 
> This leads to some odd bugs if you do something like this:
> `piglit run quick deqp-gles3 foo -c`
> 
> let it run, so 35,000 tests, and then stop it with ctrl+\, and then do:
> `piglit run quick foo -c`
> 
> Since the quick profile only as ~25000 tests IIRC, you'll end up with up to
> 10,000 tests from the original run scooped up into the new run, leading to
> very odd bugs. So we remove then at the start. In fact, the only way
> these files should be used is during a `piglit resume` since they are
> deleted after a succesful completion of `piglit run`
> 
> This doesn't change the fact that results.json will be overwritten if you
> do this. If we want to have a --overwrite switch we can do that, but that's
> a separate change.
> 
> Does that make sense? Do I just need to change the commit message to be
> clearer?
> 
> Sorry for the awful email.
> 
> -Dylan