[PATCH] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[PATCH] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[Enrico Scholz]

Doing a '-c clean' operation on a staged package with very much files
(e.g. glibc) took several minutes because

* every removed file was reported
* an 'rm' instance was spawned for every file

This patch uses the native 'os.unlink()' method for removing files and
reports only the removed root directory instead of the single files.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 classes/packaged-staging.bbclass |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/classes/packaged-staging.bbclass b/classes/packaged-staging.bbclass
index 1ede25c..5ab9eb5 100644
--- a/classes/packaged-staging.bbclass
+++ b/classes/packaged-staging.bbclass
@@ -26,6 +26,14 @@ PSTAGE_NATIVEDEPENDS = "\
 
 BB_STAMP_WHITELIST = "${PSTAGE_NATIVEDEPENDS}"
 
+def _package_unlink (f):
+    import os
+    try:
+	os.unlink(f)
+	return True
+    except:
+	return False
+
 python () {
     pstage_allowed = True
 
@@ -87,10 +95,10 @@ def pstage_manualclean(srcname, destvarname, d):
 	dest = bb.data.getVar(destvarname, d, True)
 
 	for walkroot, dirs, files in os.walk(src):
+		bb.note("rm %s" % walkroot)
 		for file in files:
 			filepath = os.path.join(walkroot, file).replace(src, dest)
-			bb.note("rm %s" % filepath)
-			os.system("rm %s" % filepath)
+			_package_unlink(filepath)
 
 def pstage_set_pkgmanager(d):
     path = bb.data.getVar("PATH", d, 1)
@@ -162,6 +170,7 @@ PSTAGE_TASKS_COVERED = "fetch unpack munge patch configure qa_configure rig_loca
 SCENEFUNCS += "packagestage_scenefunc"
 
 python packagestage_scenefunc () {
+    import glob
     if bb.data.getVar("PSTAGING_ACTIVE", d, 1) == "0":
         return
 
@@ -217,7 +226,9 @@ python packagestage_scenefunc () {
 
         # Remove the stamps and files we added above
         # FIXME - we should really only remove the stamps we added
-        os.system('rm -f ' + stamp + '.*')
+	for fname in glob.glob(stamp + '.*'):
+	    _package_unlink(fname)
+
         os.system(bb.data.expand("rm -rf ${WORKDIR}/tstage", d))
 
         if stageok:
@@ -251,8 +262,8 @@ python packagedstage_stampfixing_eventhandler() {
                     # so we need to remove the autogenerated stamps.
                     for task in taskscovered:
                         dir = "%s.do_%s" % (e.stampPrefix[fn], task)
-                        os.system('rm -f ' + dir)
-                    os.system('rm -f ' + stamp)
+			_package_unlink(dir)
+		    _package_unlink(stamp)
 
     return NotHandled
 }
-- 
1.6.6.1

Re: [PATCH] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[Tom Rini]

On Tue, 2010-03-16 at 13:23 +0100, Enrico Scholz wrote:
> Doing a '-c clean' operation on a staged package with very much files
> (e.g. glibc) took several minutes because
> 
> * every removed file was reported
> * an 'rm' instance was spawned for every file
> 
> This patch uses the native 'os.unlink()' method for removing files and
> reports only the removed root directory instead of the single files.

We call os.unlink by itself in other places.  If we don't wrap it, could
we get an exception, really (that wouldn't otherwise be a bug to fix) ?

-- 
Tom Rini <tom_rini@mentor.com>
Mentor Graphics Corporation

Re: [PATCH] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[Enrico Scholz]

Tom Rini <tom_rini@mentor.com> writes:

> On Tue, 2010-03-16 at 13:23 +0100, Enrico Scholz wrote:
>> Doing a '-c clean' operation on a staged package with very much files
>> (e.g. glibc) took several minutes because
>> 
>> * every removed file was reported
>> * an 'rm' instance was spawned for every file
>> 
>> This patch uses the native 'os.unlink()' method for removing files and
>> reports only the removed root directory instead of the single files.
>
> We call os.unlink by itself in other places.  If we don't wrap it, could
> we get an exception, really (that wouldn't otherwise be a bug to fix) ?

os.unlink() throws an exception:

$ python -c 'import os; os.unlink("xxx")'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
OSError: [Errno 2] No such file or directory: 'xxx'


When I worked on the patch I got such an exception (at least in one of
the hunks which replace 'rm -f').  I decided to be tolerant on errors
because there is no way to recover when '-c clean' fails.



Enrico

Re: [PATCH] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[Tom Rini]

On Tue, 2010-03-16 at 13:23 +0100, Enrico Scholz wrote:
> Doing a '-c clean' operation on a staged package with very much files
> (e.g. glibc) took several minutes because
> 
> * every removed file was reported
> * an 'rm' instance was spawned for every file
> 
> This patch uses the native 'os.unlink()' method for removing files and
> reports only the removed root directory instead of the single files.
> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>

Acked-by: Tom Rini <tom_rini@mentor.com>

> ---
>  classes/packaged-staging.bbclass |   21 ++++++++++++++++-----
>  1 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/classes/packaged-staging.bbclass b/classes/packaged-staging.bbclass
> index 1ede25c..5ab9eb5 100644
> --- a/classes/packaged-staging.bbclass
> +++ b/classes/packaged-staging.bbclass
> @@ -26,6 +26,14 @@ PSTAGE_NATIVEDEPENDS = "\
>  
>  BB_STAMP_WHITELIST = "${PSTAGE_NATIVEDEPENDS}"
>  
> +def _package_unlink (f):
> +    import os
> +    try:
> +	os.unlink(f)
> +	return True
> +    except:
> +	return False
> +
>  python () {
>      pstage_allowed = True
>  
> @@ -87,10 +95,10 @@ def pstage_manualclean(srcname, destvarname, d):
>  	dest = bb.data.getVar(destvarname, d, True)
>  
>  	for walkroot, dirs, files in os.walk(src):
> +		bb.note("rm %s" % walkroot)
>  		for file in files:
>  			filepath = os.path.join(walkroot, file).replace(src, dest)
> -			bb.note("rm %s" % filepath)
> -			os.system("rm %s" % filepath)
> +			_package_unlink(filepath)
>  
>  def pstage_set_pkgmanager(d):
>      path = bb.data.getVar("PATH", d, 1)
> @@ -162,6 +170,7 @@ PSTAGE_TASKS_COVERED = "fetch unpack munge patch configure qa_configure rig_loca
>  SCENEFUNCS += "packagestage_scenefunc"
>  
>  python packagestage_scenefunc () {
> +    import glob
>      if bb.data.getVar("PSTAGING_ACTIVE", d, 1) == "0":
>          return
>  
> @@ -217,7 +226,9 @@ python packagestage_scenefunc () {
>  
>          # Remove the stamps and files we added above
>          # FIXME - we should really only remove the stamps we added
> -        os.system('rm -f ' + stamp + '.*')
> +	for fname in glob.glob(stamp + '.*'):
> +	    _package_unlink(fname)
> +
>          os.system(bb.data.expand("rm -rf ${WORKDIR}/tstage", d))
>  
>          if stageok:
> @@ -251,8 +262,8 @@ python packagedstage_stampfixing_eventhandler() {
>                      # so we need to remove the autogenerated stamps.
>                      for task in taskscovered:
>                          dir = "%s.do_%s" % (e.stampPrefix[fn], task)
> -                        os.system('rm -f ' + dir)
> -                    os.system('rm -f ' + stamp)
> +			_package_unlink(dir)
> +		    _package_unlink(stamp)
>  
>      return NotHandled
>  }


-- 
Tom Rini <tom_rini@mentor.com>
Mentor Graphics Corporation

Re: [PATCH] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[Michael 'Mickey' Lauer]

> > +def _package_unlink (f):
> > +    import os
> > +    try:
> > +	os.unlink(f)
> > +	return True
> > +    except:
> > +	return False
> > +

While it won't hurt in this particular case, a catch-all 'except'
usually is frowned upon. Better catch specific errors and let other
errors be handled by the upper layers. I'd recommend catching IOError
only here.

:M:

Re: [PATCH] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[Tom Rini]

On Wed, 2010-03-17 at 18:39 +0100, Michael 'Mickey' Lauer wrote:
> > > +def _package_unlink (f):
> > > +    import os
> > > +    try:
> > > +	os.unlink(f)
> > > +	return True
> > > +    except:
> > > +	return False
> > > +
> 
> While it won't hurt in this particular case, a catch-all 'except'
> usually is frowned upon. Better catch specific errors and let other
> errors be handled by the upper layers. I'd recommend catching IOError
> only here.

We need to catch OSError too as this continues to leave behind
directories.

-- 
Tom Rini <tom_rini@mentor.com>
Mentor Graphics Corporation

Re: [PATCH] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[Tom Rini]

On Wed, 2010-03-17 at 10:48 -0700, Tom Rini wrote:
> On Wed, 2010-03-17 at 18:39 +0100, Michael 'Mickey' Lauer wrote:
> > > > +def _package_unlink (f):
> > > > +    import os
> > > > +    try:
> > > > +	os.unlink(f)
> > > > +	return True
> > > > +    except:
> > > > +	return False
> > > > +
> > 
> > While it won't hurt in this particular case, a catch-all 'except'
> > usually is frowned upon. Better catch specific errors and let other
> > errors be handled by the upper layers. I'd recommend catching IOError
> > only here.
> 
> We need to catch OSError too as this continues to leave behind
> directories.

Or, not.  But while we're in here, we should change from bb.note to
bb.debug (and, keep it where it is).  That's probably the best way to
despam output but have something useful if needed.

-- 
Tom Rini <tom_rini@mentor.com>
Mentor Graphics Corporation

[PATCH v2] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[Enrico Scholz]

Doing a '-c clean' operation on a staged package with very much files
(e.g. glibc) took several minutes because

* every removed file was reported
* an 'rm' instance was spawned for every file

This patch uses the native 'os.unlink()' method for removing files and
reports only the removed root directory instead of the single files.

Based upon maillist discussion, reporting happens with 'debug' level
instead of 'note' one, and only error conditions due to non-existing
files will be ignored.  Other (e.g. permission denied) errors will now
abort the build while they were silently ignored previously.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 classes/packaged-staging.bbclass |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/classes/packaged-staging.bbclass b/classes/packaged-staging.bbclass
index 9c72d11..f3648d2 100644
--- a/classes/packaged-staging.bbclass
+++ b/classes/packaged-staging.bbclass
@@ -26,6 +26,16 @@ PSTAGE_NATIVEDEPENDS = "\
 
 BB_STAMP_WHITELIST = "${PSTAGE_NATIVEDEPENDS}"
 
+def _package_unlink (f):
+    import os, errno
+    try:
+	os.unlink(f)
+	return True
+    except OSError, e:
+	if e.errno == errno.ENOENT:
+	    return False
+	raise
+
 python () {
     pstage_allowed = True
 
@@ -87,10 +97,10 @@ def pstage_manualclean(srcname, destvarname, d):
 	dest = bb.data.getVar(destvarname, d, True)
 
 	for walkroot, dirs, files in os.walk(src):
+		bb.debug("rm %s" % walkroot)
 		for file in files:
 			filepath = os.path.join(walkroot, file).replace(src, dest)
-			bb.note("rm %s" % filepath)
-			os.system("rm %s" % filepath)
+			_package_unlink(filepath)
 
 def pstage_set_pkgmanager(d):
     path = bb.data.getVar("PATH", d, 1)
@@ -162,6 +172,7 @@ PSTAGE_TASKS_COVERED = "fetch unpack munge patch configure qa_configure rig_loca
 SCENEFUNCS += "packagestage_scenefunc"
 
 python packagestage_scenefunc () {
+    import glob
     if bb.data.getVar("PSTAGING_ACTIVE", d, 1) == "0":
         return
 
@@ -227,7 +238,9 @@ python packagestage_scenefunc () {
 
         # Remove the stamps and files we added above
         # FIXME - we should really only remove the stamps we added
-        os.system('rm -f ' + stamp + '.*')
+	for fname in glob.glob(stamp + '.*'):
+	    _package_unlink(fname)
+
         os.system(bb.data.expand("rm -rf ${WORKDIR}/tstage", d))
 
         if stageok:
@@ -260,8 +273,8 @@ python packagedstage_stampfixing_eventhandler() {
                     # so we need to remove the autogenerated stamps.
                     for task in taskscovered:
                         dir = "%s.do_%s" % (e.stampPrefix[fn], task)
-                        os.system('rm -f ' + dir)
-                    os.system('rm -f ' + stamp)
+			_package_unlink(dir)
+		    _package_unlink(stamp)
 }
 
 populate_staging_preamble () {
-- 
1.7.0.1

Re: [PATCH v2] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[Sebastian Spaeth]

On 2010-04-14, Enrico Scholz wrote:

> @@ -227,7 +238,9 @@ python packagestage_scenefunc () {
> -        os.system('rm -f ' + stamp + '.*')
> +	for fname in glob.glob(stamp + '.*'):
> +	    _package_unlink(fname)
> +

> @@ -260,8 +273,8 @@ python packagedstage_stampfixing_eventhandler() {
> -                        os.system('rm -f ' + dir)
> -                    os.system('rm -f ' + stamp)
> +			_package_unlink(dir)
> +		    _package_unlink(stamp)

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>

Given you have made sure that those changed indentations are actually
intended (I have not looked at the context of that patch):

Acked-by: Sebastian Spaeth <Sebastian@SSpaeth.de>

Re: [PATCH v2] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[Enrico Scholz]

"Sebastian Spaeth" <Sebastian@SSpaeth.de> writes:

>> @@ -227,7 +238,9 @@ python packagestage_scenefunc () {
>> -        os.system('rm -f ' + stamp + '.*')
>> +	for fname in glob.glob(stamp + '.*'):
>> +	    _package_unlink(fname)
>> +
>
>> @@ -260,8 +273,8 @@ python packagedstage_stampfixing_eventhandler() {
>> -                        os.system('rm -f ' + dir)
>> -                    os.system('rm -f ' + stamp)
>> +			_package_unlink(dir)
>> +		    _package_unlink(stamp)
>
> Given you have made sure that those changed indentations are actually
> intended (I have not looked at the context of that patch):

File had already a mixed tab/whitespace indentation.  I used the style
which I found at the top of the file (tabulators) and did not noticed
that it changed below.



Enrico

Re: [PATCH v2] packaged-staging.bbclass: use 'os.unlink()' instead of spawning 'rm'

[Tom Rini]

On Wed, 2010-04-14 at 11:55 +0200, Enrico Scholz wrote:
> Doing a '-c clean' operation on a staged package with very much files
> (e.g. glibc) took several minutes because
> 
> * every removed file was reported
> * an 'rm' instance was spawned for every file
> 
> This patch uses the native 'os.unlink()' method for removing files and
> reports only the removed root directory instead of the single files.
> 
> Based upon maillist discussion, reporting happens with 'debug' level
> instead of 'note' one, and only error conditions due to non-existing
> files will be ignored.  Other (e.g. permission denied) errors will now
> abort the build while they were silently ignored previously.
> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>

Acked-by: Tom Rini <tom_rini@mentor.com>

(Aside: Do you have write access?  If not, shoot me a msg off list and
I'll put this in).

> ---
>  classes/packaged-staging.bbclass |   23 ++++++++++++++++++-----
>  1 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/classes/packaged-staging.bbclass b/classes/packaged-staging.bbclass
> index 9c72d11..f3648d2 100644
> --- a/classes/packaged-staging.bbclass
> +++ b/classes/packaged-staging.bbclass
> @@ -26,6 +26,16 @@ PSTAGE_NATIVEDEPENDS = "\
>  
>  BB_STAMP_WHITELIST = "${PSTAGE_NATIVEDEPENDS}"
>  
> +def _package_unlink (f):
> +    import os, errno
> +    try:
> +	os.unlink(f)
> +	return True
> +    except OSError, e:
> +	if e.errno == errno.ENOENT:
> +	    return False
> +	raise
> +
>  python () {
>      pstage_allowed = True
>  
> @@ -87,10 +97,10 @@ def pstage_manualclean(srcname, destvarname, d):
>  	dest = bb.data.getVar(destvarname, d, True)
>  
>  	for walkroot, dirs, files in os.walk(src):
> +		bb.debug("rm %s" % walkroot)
>  		for file in files:
>  			filepath = os.path.join(walkroot, file).replace(src, dest)
> -			bb.note("rm %s" % filepath)
> -			os.system("rm %s" % filepath)
> +			_package_unlink(filepath)
>  
>  def pstage_set_pkgmanager(d):
>      path = bb.data.getVar("PATH", d, 1)
> @@ -162,6 +172,7 @@ PSTAGE_TASKS_COVERED = "fetch unpack munge patch configure qa_configure rig_loca
>  SCENEFUNCS += "packagestage_scenefunc"
>  
>  python packagestage_scenefunc () {
> +    import glob
>      if bb.data.getVar("PSTAGING_ACTIVE", d, 1) == "0":
>          return
>  
> @@ -227,7 +238,9 @@ python packagestage_scenefunc () {
>  
>          # Remove the stamps and files we added above
>          # FIXME - we should really only remove the stamps we added
> -        os.system('rm -f ' + stamp + '.*')
> +	for fname in glob.glob(stamp + '.*'):
> +	    _package_unlink(fname)
> +
>          os.system(bb.data.expand("rm -rf ${WORKDIR}/tstage", d))
>  
>          if stageok:
> @@ -260,8 +273,8 @@ python packagedstage_stampfixing_eventhandler() {
>                      # so we need to remove the autogenerated stamps.
>                      for task in taskscovered:
>                          dir = "%s.do_%s" % (e.stampPrefix[fn], task)
> -                        os.system('rm -f ' + dir)
> -                    os.system('rm -f ' + stamp)
> +			_package_unlink(dir)
> +		    _package_unlink(stamp)
>  }
>  
>  populate_staging_preamble () {


-- 
Tom Rini <tom_rini@mentor.com>
Mentor Graphics Corporation