framework: Use os.mkdirs instead of os.mkdir for summary subfolders

Submitted by Jason Ekstrand on Aug. 30, 2014, 6:49 p.m.

Details

Message ID 1409424591-493-1-git-send-email-jason.ekstrand@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Jason Ekstrand Aug. 30, 2014, 6:49 p.m.
This way test names can contain "/" characters.  I to name tests after git
branches and I tend to have git branches named wip/whatever.  This prevents
piglit from crashing.
---
 framework/summary.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/framework/summary.py b/framework/summary.py
index 332aa40..1815dc7 100644
--- a/framework/summary.py
+++ b/framework/summary.py
@@ -454,7 +454,7 @@  class Summary:
 
         # Iterate across the tests creating the various test specific files
         for each in self.results:
-            os.mkdir(path.join(destination, each.name))
+            os.makedirs(path.join(destination, each.name))
 
             if each.time_elapsed is not None:
                 time = datetime.timedelta(0, each.time_elapsed)

Comments

[+dylan, who's quite familiar with all these little details]

Can you elaborate a little on the situation that's causing your
issues? Is http://cgit.freedesktop.org/piglit/commit/?id=3457f015314e57007b74918f79d8d02e4ada6ad7
going to make things even worse for you?

On Sat, Aug 30, 2014 at 2:49 PM, Jason Ekstrand <jason@jlekstrand.net> wrote:
> This way test names can contain "/" characters.  I to name tests after git
> branches and I tend to have git branches named wip/whatever.  This prevents
> piglit from crashing.
> ---
>  framework/summary.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/framework/summary.py b/framework/summary.py
> index 332aa40..1815dc7 100644
> --- a/framework/summary.py
> +++ b/framework/summary.py
> @@ -454,7 +454,7 @@ class Summary:
>
>          # Iterate across the tests creating the various test specific files
>          for each in self.results:
> -            os.mkdir(path.join(destination, each.name))
> +            os.makedirs(path.join(destination, each.name))
>
>              if each.time_elapsed is not None:
>                  time = datetime.timedelta(0, each.time_elapsed)
> --
> 2.1.0
>
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
Let's just not put / in test names instead?
Sorry, I should have been more clear by "test name".  I mean the name for
the entire run; i.e., "piglit run -n 'wip/whatever' quick.py
results_folder".  We should do something intelligent here rather than crash
when generating the summary.  If we don't like directories, we could mangle
names instead ('../foo' would also be a problem).
--Jason
On Aug 30, 2014 1:04 PM, "Matt Turner" <mattst88@gmail.com> wrote:

> Let's just not put / in test names instead?
>
I'll need to think about it and read the code to be sure, I think that
using '/' in test run names may cause some strange behavior in the html
pages because of assumptions. There's also the windows '\\' issue to
think about.

But, either way you need to put makedirs in a try/except block
try:
	os.makedirs(...)
except OSError:
	pass


On Saturday, August 30, 2014 11:49:51 AM Jason Ekstrand wrote:
> This way test names can contain "/" characters.  I to name tests after git
> branches and I tend to have git branches named wip/whatever.  This prevents
> piglit from crashing.
> ---
>  framework/summary.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/framework/summary.py b/framework/summary.py
> index 332aa40..1815dc7 100644
> --- a/framework/summary.py
> +++ b/framework/summary.py
> @@ -454,7 +454,7 @@ class Summary:
>  
>          # Iterate across the tests creating the various test specific files
>          for each in self.results:
> -            os.mkdir(path.join(destination, each.name))
> +            os.makedirs(path.join(destination, each.name))
>  
>              if each.time_elapsed is not None:
>                  time = datetime.timedelta(0, each.time_elapsed)
> -- 
> 2.1.0
> 
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
On Aug 30, 2014 3:23 PM, "Dylan Baker" <baker.dylan.c@gmail.com> wrote:
>
> I'll need to think about it and read the code to be sure, I think that
> using '/' in test run names may cause some strange behavior in the html
> pages because of assumptions. There's also the windows '\\' issue to
> think about.

Yeah, just letting paths be paths is probably not the solution.  Does
python provide a nice way to sanatize names for this sort of thing?  I'm
not a fan of adding silly restrictions to things that are otherwise
arbitrary strings.  Let me know what you think.

If I have to, I can munge things in my shell script before passing the name
to piglit but then they're munged in the table labels in the HTML.
--Jason

> But, either way you need to put makedirs in a try/except block
> try:
>         os.makedirs(...)
> except OSError:
>         pass

Yup

> On Saturday, August 30, 2014 11:49:51 AM Jason Ekstrand wrote:
> > This way test names can contain "/" characters.  I to name tests after
git
> > branches and I tend to have git branches named wip/whatever.  This
prevents
> > piglit from crashing.
> > ---
> >  framework/summary.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/framework/summary.py b/framework/summary.py
> > index 332aa40..1815dc7 100644
> > --- a/framework/summary.py
> > +++ b/framework/summary.py
> > @@ -454,7 +454,7 @@ class Summary:
> >
> >          # Iterate across the tests creating the various test specific
files
> >          for each in self.results:
> > -            os.mkdir(path.join(destination, each.name))
> > +            os.makedirs(path.join(destination, each.name))
> >
> >              if each.time_elapsed is not None:
> >                  time = datetime.timedelta(0, each.time_elapsed)
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Piglit mailing list
> > Piglit@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/piglit
On Saturday, August 30, 2014 03:38:23 PM Jason Ekstrand wrote:
> On Aug 30, 2014 3:23 PM, "Dylan Baker" <baker.dylan.c@gmail.com> wrote:
> >
> > I'll need to think about it and read the code to be sure, I think that
> > using '/' in test run names may cause some strange behavior in the html
> > pages because of assumptions. There's also the windows '\\' issue to
> > think about.
> 
> Yeah, just letting paths be paths is probably not the solution.  Does
> python provide a nice way to sanatize names for this sort of thing?  I'm
> not a fan of adding silly restrictions to things that are otherwise
> arbitrary strings.  Let me know what you think.
> 
> If I have to, I can munge things in my shell script before passing the name
> to piglit but then they're munged in the table labels in the HTML.
> --Jason

I think we'd be better off restricting test names not to include path
separators (on windows '\' and '\\' are equally bad). It I think rather
than denying people the option we should just silently convert them into
a sane character (like _) we do this in some other places, mostly
because of windows filename character limitations.

I don't think this should be too hard, I'll mock something up.

[snip]