From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ea0-f172.google.com ([209.85.215.172]) by linuxtogo.org with esmtp (Exim 4.72) (envelope-from ) id 1TtYY3-0003X1-7I for bitbake-devel@lists.openembedded.org; Fri, 11 Jan 2013 07:57:04 +0100 Received: by mail-ea0-f172.google.com with SMTP id f13so572259eaa.3 for ; Thu, 10 Jan 2013 22:41:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:message-id:date:from:user-agent:mime-version:to:cc :subject:references:in-reply-to:content-type; bh=A68YEB6lN9YqcB+RiiN8qX4R+mbgfNUJnJqMYr04Pgw=; b=Ov33RbFdNsJb50g3YxqAvLlTBkG1yvQ9o1YT0zkDaC8f7rTAW+BxwiBV7px0nlfBYP UBgRK63upy0irpl08GMcHmtgG6ZmY8P3EsRaadFDZLYC1LC26J/L6ijr4trxxkHwdtO2 w9AnoZoCrV+BGS1NTvyEguwWQs6PXR5vxzLuf8Ij5HxgoCvmnxNjsbzNzUI3knGzZYQV JnwFKGWcb1/6jXR9UnqgpIoxsplWkjtH+d1IzaWIXNYsoXJr7vyijuT5sNBiucoJl40m HvJ3WWoo6uaRcJ/qqvzWjNHrWEsz5srP7hxBH+RLdLW940L3jV6SXzqHatowsTb/69Mw d3kQ== X-Received: by 10.14.219.72 with SMTP id l48mr199464920eep.37.1357886507689; Thu, 10 Jan 2013 22:41:47 -0800 (PST) Received: from [10.54.86.72] (64-103-25-233.cisco.com. [64.103.25.233]) by mx.google.com with ESMTPS id w44sm7277899eep.6.2013.01.10.22.41.45 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 10 Jan 2013 22:41:46 -0800 (PST) Message-ID: <50EFB428.8090700@gmail.com> Date: Fri, 11 Jan 2013 07:41:44 +0100 From: Martin Ertsaas User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.11) Gecko/20121128 Thunderbird/10.0.11 MIME-Version: 1.0 To: Chris Larson References: <1357807596-31587-1-git-send-email-martiert@gmail.com> In-Reply-To: Cc: "bitbake-devel@lists.openembedded.org" Subject: Re: [PATCH] utils.py: Use shutil.rmtree if the path we wish to remove is a directory. X-BeenThere: bitbake-devel@lists.openembedded.org X-Mailman-Version: 2.1.11 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Jan 2013 06:57:04 -0000 Content-Type: multipart/alternative; boundary="------------040302000506010103000104" --------------040302000506010103000104 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 01/10/13 15:18, Chris Larson wrote: > > On Thu, Jan 10, 2013 at 7:11 AM, Chris Larson > wrote: > > > On Thu, Jan 10, 2013 at 7:10 AM, Chris Larson > wrote: > > On Thu, Jan 10, 2013 at 1:46 AM, Martin Ertsaas > > 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 --------------040302000506010103000104 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit On 01/10/13 15:18, Chris Larson wrote:

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

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
--------------040302000506010103000104--