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

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

[Martin Ertsaas]

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)
-- 
1.7.10.2 (Apple Git-33)

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

[Chris Larson]

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

On Thu, Jan 10, 2013 at 1:46 AM, Martin Ertsaas <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.
-- 
Christopher Larson

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

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

[Chris Larson]

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

On Thu, Jan 10, 2013 at 7:10 AM, Chris Larson <clarson@kergoth.com> wrote:

> On Thu, Jan 10, 2013 at 1:46 AM, Martin Ertsaas <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.
-- 
Christopher Larson

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

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

[Chris Larson]

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

On Thu, Jan 10, 2013 at 7:11 AM, Chris Larson <clarson@kergoth.com> wrote:

>
> On Thu, Jan 10, 2013 at 7:10 AM, Chris Larson <clarson@kergoth.com> wrote:
>
>> On Thu, Jan 10, 2013 at 1:46 AM, Martin Ertsaas <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

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

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

[Martin Ertsaas]

[-- 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 --]

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

[Martin Ertsaas]

[-- Attachment #1: Type: text/plain, Size: 2641 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 think to recursively call remove on the children of the directory?
Wouldn't something like:

for name in glob.glob(path):
    if os.path.isdir(name) and recurse:
        shutil.rmtree(name)
    elif os.path.isdir(name):
        os.rmdir(name)
    else:
        os.unlink(name)

Then we will possibly get an exception from os.rmdir, which we can
catch. But this also avoids the need to use the exception as a control
flow structure as it is used today.

At least in my mind, that is a lot easier to grasp than the if statement
in the exception handling, which I totally overlooked as it is an exception.

- Martin

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