Re: [PATCH] utils.py: Use shutil.rmtree if the path we wish to remove is a directory.

[Martin Ertsaas <martiert@gmail.com>]

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

On 01/10/13 15:18, Chris Larson wrote:
>
> On Thu, Jan 10, 2013 at 7:11 AM, Chris Larson <clarson@kergoth.com
> <mailto:clarson@kergoth.com>> wrote:
>
>
>     On Thu, Jan 10, 2013 at 7:10 AM, Chris Larson <clarson@kergoth.com
>     <mailto:clarson@kergoth.com>> wrote:
>
>         On Thu, Jan 10, 2013 at 1:46 AM, Martin Ertsaas
>         <martiert@gmail.com <mailto:martiert@gmail.com>> wrote:
>
>             On mac, os.unlink does not remove directories, and we
>             therefor have
>             to explicitly use shutil.rmtree if the path is a directory.
>             ---
>              lib/bb/utils.py |    5 ++++-
>              1 file changed, 4 insertions(+), 1 deletion(-)
>
>             diff --git a/lib/bb/utils.py b/lib/bb/utils.py
>             index cef0fdd..8b6d3f5 100644
>             --- a/lib/bb/utils.py
>             +++ b/lib/bb/utils.py
>             @@ -561,7 +561,10 @@ def remove(path, recurse=False):
>                  import os, errno, shutil, glob
>                  for name in glob.glob(path):
>                      try:
>             -            os.unlink(name)
>             +            if os.path.isdir(name):
>             +                shutil.rmtree(name)
>             +            else:
>             +                os.unlink(name)
>                      except OSError as exc:
>                          if recurse and exc.errno == errno.EISDIR:
>                              shutil.rmtree(name)
>
>
>
>         Look 2 lines down, where it checks to see if the os.unlink
>         failed due to it being a directory and runs shutil.rmtree if
>         that's the case.
>
>
>     I'm guessing you're trying to use it without passing recurse=True.
>
>
> Having through about it further, I think it might be best to be alter
> the code to also handle the case where recurse==False and the path is
> a directory by calling os.rmdir(). Perhaps this would reduce confusion.
> -- 
> Christopher Larson

You are almost right. On normal folders, this will work with
recurse=True, but on /tmp folders you get: OSError: [Errno 1] Operation
not permitted: '/tmp/tmpl8qBDW' on the unlink call. This makes the if
test inside the except catching false, and nothing happens.

I'm not sure why this happens, as I am allowed to remove it using
shutil.rmtree. Making a folder in linux with the exact same permissions
does work however, so it seems to be some difference in how the os
handles the call.

Other than that, I feel that it is cleaner to have an explicit check
like this, and not use the exception framework for control flow. Which
means in the except just use raise, or just ignore it and let someone
else handle it like we actually do.
To get back the same functionality though, we should change the isdir
call to: 'if os.path.isdir(name) and recurse:'

Are you totally opposed to changing this? What I don't want to do is to
add exc.errno == errno.EPERM to the line, which will just bloath it.

- Martin

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