All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] utils: Use rm -rf in remove()
@ 2013-02-07 23:55 Richard Purdie
  2013-02-08  1:08 ` Chris Larson
  2013-02-08 13:47 ` Peter Kjellerstedt
  0 siblings, 2 replies; 7+ messages in thread
From: Richard Purdie @ 2013-02-07 23:55 UTC (permalink / raw)
  To: bitbake-devel

Whilst shutils.rmtree() is pythonic, its also slow. Its faster to
use rm -rf which makes optimal use of the right syscalls.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
diff --git a/bitbake/lib/bb/utils.py b/bitbake/lib/bb/utils.py
index 484fb2d..94ef447 100644
--- a/bitbake/lib/bb/utils.py
+++ b/bitbake/lib/bb/utils.py
@@ -533,13 +533,15 @@ def remove(path, recurse=False):
     """Equivalent to rm -f or rm -rf"""
     if not path:
         return
-    import os, errno, shutil, glob
+    import os, errno, glob, subprocess
     for name in glob.glob(path):
         try:
             os.unlink(name)
         except OSError as exc:
             if recurse and exc.errno == errno.EISDIR:
-                shutil.rmtree(name)
+                # shutil.rmtree(name) would be ideal but its too slow
+                subprocess.call('rm -rf %s' % path, shell=True)
+                return
             elif exc.errno != errno.ENOENT:
                 raise
 





^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] utils: Use rm -rf in remove()
  2013-02-07 23:55 [PATCH] utils: Use rm -rf in remove() Richard Purdie
@ 2013-02-08  1:08 ` Chris Larson
  2013-02-15 16:00   ` Richard Purdie
  2013-02-08 13:47 ` Peter Kjellerstedt
  1 sibling, 1 reply; 7+ messages in thread
From: Chris Larson @ 2013-02-08  1:08 UTC (permalink / raw)
  To: Richard Purdie; +Cc: bitbake-devel

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]

On Thu, Feb 7, 2013 at 4:55 PM, Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> -                shutil.rmtree(name)
> +                # shutil.rmtree(name) would be ideal but its too slow
> +                subprocess.call('rm -rf %s' % path, shell=True)
>

This is a good idea, but I'm curious about forking off a shell process for
it. I'd think this would work as well: subprocess.call(['rm', '-rf', path])
-- 
Christopher Larson

[-- Attachment #2: Type: text/html, Size: 876 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] utils: Use rm -rf in remove()
  2013-02-07 23:55 [PATCH] utils: Use rm -rf in remove() Richard Purdie
  2013-02-08  1:08 ` Chris Larson
@ 2013-02-08 13:47 ` Peter Kjellerstedt
  1 sibling, 0 replies; 7+ messages in thread
From: Peter Kjellerstedt @ 2013-02-08 13:47 UTC (permalink / raw)
  To: Richard Purdie, bitbake-devel

> -----Original Message-----
> From: bitbake-devel-bounces@lists.openembedded.org [mailto:bitbake-
> devel-bounces@lists.openembedded.org] On Behalf Of Richard Purdie
> Sent: den 8 februari 2013 00:55
> To: bitbake-devel
> Subject: [bitbake-devel] [PATCH] utils: Use rm -rf in remove()
> 
> Whilst shutils.rmtree() is pythonic, its also slow. Its faster to
> use rm -rf which makes optimal use of the right syscalls.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
> diff --git a/bitbake/lib/bb/utils.py b/bitbake/lib/bb/utils.py
> index 484fb2d..94ef447 100644
> --- a/bitbake/lib/bb/utils.py
> +++ b/bitbake/lib/bb/utils.py
> @@ -533,13 +533,15 @@ def remove(path, recurse=False):
>      """Equivalent to rm -f or rm -rf"""
>      if not path:
>          return
> -    import os, errno, shutil, glob
> +    import os, errno, glob, subprocess
>      for name in glob.glob(path):
>          try:
>              os.unlink(name)
>          except OSError as exc:
>              if recurse and exc.errno == errno.EISDIR:
> -                shutil.rmtree(name)
> +                # shutil.rmtree(name) would be ideal but its too slow

its -> it is

> +                subprocess.call('rm -rf %s' % path, shell=True)
> +                return

Shouldn't the last two lines above be:

                   subprocess.call('rm -rf %s' % name, shell=True)

to maintain behavior.

>              elif exc.errno != errno.ENOENT:
>                  raise

Alternatively, wouldn't it be better to check for recurse 
first:

    if recurse:
        import subprocess
        subprocess.call('rm -rf %s' % path, shell=True)
    else:
        import os, errno, glob
        for name in glob.glob(path):
            try:
                os.unlink(name)
            except OSError as exc:
                if exc.errno != errno.ENOENT:
                    raise

//Peter





^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] utils: Use rm -rf in remove()
  2013-02-08  1:08 ` Chris Larson
@ 2013-02-15 16:00   ` Richard Purdie
  2013-02-16 13:51     ` Martin Ertsås
  0 siblings, 1 reply; 7+ messages in thread
From: Richard Purdie @ 2013-02-15 16:00 UTC (permalink / raw)
  To: Chris Larson, Martin Ertsaas; +Cc: bitbake-devel

On Thu, 2013-02-07 at 18:08 -0700, Chris Larson wrote:
> 
> On Thu, Feb 7, 2013 at 4:55 PM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
>         -                shutil.rmtree(name)
>         +                # shutil.rmtree(name) would be ideal but its
>         too slow
>         +                subprocess.call('rm -rf %s' % path,
>         shell=True)
> 
> This is a good idea, but I'm curious about forking off a shell process
> for it. I'd think this would work as well: subprocess.call(['rm',
> '-rf', path])

path can have wildcards in it. The code wasn't entirely obvious so I've
tweaked it after your/Peter's comments. I'm hoping it will help the
problems Martin was seeing too.

Cheers,

Richard





^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] utils: Use rm -rf in remove()
  2013-02-15 16:00   ` Richard Purdie
@ 2013-02-16 13:51     ` Martin Ertsås
  2013-02-16 14:54       ` Richard Purdie
  0 siblings, 1 reply; 7+ messages in thread
From: Martin Ertsås @ 2013-02-16 13:51 UTC (permalink / raw)
  To: Richard Purdie; +Cc: Chris Larson, bitbake-devel

On 02/15/13 17:00, Richard Purdie wrote:
> On Thu, 2013-02-07 at 18:08 -0700, Chris Larson wrote:
>> On Thu, Feb 7, 2013 at 4:55 PM, Richard Purdie
>> <richard.purdie@linuxfoundation.org> wrote:
>>          -                shutil.rmtree(name)
>>          +                # shutil.rmtree(name) would be ideal but its
>>          too slow
>>          +                subprocess.call('rm -rf %s' % path,
>>          shell=True)
>>
>> This is a good idea, but I'm curious about forking off a shell process
>> for it. I'd think this would work as well: subprocess.call(['rm',
>> '-rf', path])
> path can have wildcards in it. The code wasn't entirely obvious so I've
> tweaked it after your/Peter's comments. I'm hoping it will help the
> problems Martin was seeing too.
>
> Cheers,
>
> Richard
>
>
I think this is a good idea as well. One thing I would question though, 
is to have this in the except clause. Why not:

     for name in glob.glob(path):
         try:
	    if recurse and os.path.isdir(path):
	       subprocess.call('rm -rf %s' % path, shell=True)
	       return
	    os.unlink(name)
         except OSError as exc:
             if exc.errno != errno.ENOENT:
                 raise


Personally I feel it is cleaner to have all the remove possibilities in 
the try, and let the exceptions be in except. Kind of feel it is wrong 
to let the recursive case be handled in the exception, as I don't see it 
being an exceptional case to delete a folder.

Also, unfortunately your patch would not fix osx, as EISDIR is not the 
error that is returned there. What I get is either a EPERM or an EACCES 
(not on a mac now, so can't check it until monday). So osx would still 
fall through to the raise.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] utils: Use rm -rf in remove()
  2013-02-16 13:51     ` Martin Ertsås
@ 2013-02-16 14:54       ` Richard Purdie
  2013-02-16 15:27         ` Martin Ertsås
  0 siblings, 1 reply; 7+ messages in thread
From: Richard Purdie @ 2013-02-16 14:54 UTC (permalink / raw)
  To: Martin Ertsås; +Cc: Chris Larson, bitbake-devel

On Sat, 2013-02-16 at 14:51 +0100, Martin Ertsås wrote:
> On 02/15/13 17:00, Richard Purdie wrote:
> > On Thu, 2013-02-07 at 18:08 -0700, Chris Larson wrote:
> >> On Thu, Feb 7, 2013 at 4:55 PM, Richard Purdie
> >> <richard.purdie@linuxfoundation.org> wrote:
> >>          -                shutil.rmtree(name)
> >>          +                # shutil.rmtree(name) would be ideal but its
> >>          too slow
> >>          +                subprocess.call('rm -rf %s' % path,
> >>          shell=True)
> >>
> >> This is a good idea, but I'm curious about forking off a shell process
> >> for it. I'd think this would work as well: subprocess.call(['rm',
> >> '-rf', path])
> > path can have wildcards in it. The code wasn't entirely obvious so I've
> > tweaked it after your/Peter's comments. I'm hoping it will help the
> > problems Martin was seeing too.
> >
> > Cheers,
> >
> > Richard
> >
> >
> I think this is a good idea as well. One thing I would question though, 
> is to have this in the except clause. Why not:
> 
>      for name in glob.glob(path):
>          try:
> 	    if recurse and os.path.isdir(path):
> 	       subprocess.call('rm -rf %s' % path, shell=True)
> 	       return
> 	    os.unlink(name)
>          except OSError as exc:
>              if exc.errno != errno.ENOENT:
>                  raise
> 
> 
> Personally I feel it is cleaner to have all the remove possibilities in 
> the try, and let the exceptions be in except. Kind of feel it is wrong 
> to let the recursive case be handled in the exception, as I don't see it 
> being an exceptional case to delete a folder.
> 
> Also, unfortunately your patch would not fix osx, as EISDIR is not the 
> error that is returned there. What I get is either a EPERM or an EACCES 
> (not on a mac now, so can't check it until monday). So osx would still 
> fall through to the raise.

Can you take a look at the patch I ended up committing? Its basically as
you describe above expect I moved the subprocess outside the for loop as
well.

(http://git.openembedded.org/bitbake/commit/?id=96088ebdec08e49ba9e8dbcac437bfcdc21f5983)

Cheers,

Richard






^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] utils: Use rm -rf in remove()
  2013-02-16 14:54       ` Richard Purdie
@ 2013-02-16 15:27         ` Martin Ertsås
  0 siblings, 0 replies; 7+ messages in thread
From: Martin Ertsås @ 2013-02-16 15:27 UTC (permalink / raw)
  To: Richard Purdie; +Cc: Chris Larson, bitbake-devel

On 02/16/13 15:54, Richard Purdie wrote:
> On Sat, 2013-02-16 at 14:51 +0100, Martin Ertsås wrote:
>> On 02/15/13 17:00, Richard Purdie wrote:
>>> On Thu, 2013-02-07 at 18:08 -0700, Chris Larson wrote:
>>>> On Thu, Feb 7, 2013 at 4:55 PM, Richard Purdie
>>>> <richard.purdie@linuxfoundation.org> wrote:
>>>>           -                shutil.rmtree(name)
>>>>           +                # shutil.rmtree(name) would be ideal but its
>>>>           too slow
>>>>           +                subprocess.call('rm -rf %s' % path,
>>>>           shell=True)
>>>>
>>>> This is a good idea, but I'm curious about forking off a shell process
>>>> for it. I'd think this would work as well: subprocess.call(['rm',
>>>> '-rf', path])
>>> path can have wildcards in it. The code wasn't entirely obvious so I've
>>> tweaked it after your/Peter's comments. I'm hoping it will help the
>>> problems Martin was seeing too.
>>>
>>> Cheers,
>>>
>>> Richard
>>>
>>>
>> I think this is a good idea as well. One thing I would question though,
>> is to have this in the except clause. Why not:
>>
>>       for name in glob.glob(path):
>>           try:
>> 	    if recurse and os.path.isdir(path):
>> 	       subprocess.call('rm -rf %s' % path, shell=True)
>> 	       return
>> 	    os.unlink(name)
>>           except OSError as exc:
>>               if exc.errno != errno.ENOENT:
>>                   raise
>>
>>
>> Personally I feel it is cleaner to have all the remove possibilities in
>> the try, and let the exceptions be in except. Kind of feel it is wrong
>> to let the recursive case be handled in the exception, as I don't see it
>> being an exceptional case to delete a folder.
>>
>> Also, unfortunately your patch would not fix osx, as EISDIR is not the
>> error that is returned there. What I get is either a EPERM or an EACCES
>> (not on a mac now, so can't check it until monday). So osx would still
>> fall through to the raise.
> Can you take a look at the patch I ended up committing? Its basically as
> you describe above expect I moved the subprocess outside the for loop as
> well.
>
> (http://git.openembedded.org/bitbake/commit/?id=96088ebdec08e49ba9e8dbcac437bfcdc21f5983)
>
> Cheers,
>
> Richard
>
>
>
That is great! Will fix up the mac problems without any problems, at 
least it looks like. I'll try it out on monday, and come back 
complaining if it doesn't fix it ;)

- Martin



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2013-02-16 15:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-07 23:55 [PATCH] utils: Use rm -rf in remove() Richard Purdie
2013-02-08  1:08 ` Chris Larson
2013-02-15 16:00   ` Richard Purdie
2013-02-16 13:51     ` Martin Ertsås
2013-02-16 14:54       ` Richard Purdie
2013-02-16 15:27         ` Martin Ertsås
2013-02-08 13:47 ` Peter Kjellerstedt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.