[gentoo-portage-dev] Re: r5993 - main/trunk/pym/portage/dbapi

[Brian Harring]

round two of the patch, still is missing...


On Sun, Feb 18, 2007 at 06:27:59PM +0000, Marius Mauch wrote:
> Author: genone
> Date: 2007-02-18 18:27:59 +0000 (Sun, 18 Feb 2007)
> New Revision: 5993
>
> Modified:
>    main/trunk/pym/portage/dbapi/vartree.py
> Log:
> extend check for internal references, should remove all libs that are only used internally now
>
> Modified: main/trunk/pym/portage/dbapi/vartree.py
> ===================================================================
> --- main/trunk/pym/portage/dbapi/vartree.py	2007-02-18 15:38:56 UTC (rev 5992)
> +++ main/trunk/pym/portage/dbapi/vartree.py	2007-02-18 18:27:59 UTC (rev 5993)
> @@ -1266,14 +1266,33 @@
>  			preserve_libs = old_libs.difference(mylibs)
>
>  			# ignore any libs that are only internally used by the package
> -			for lib in preserve_libs.copy():
> -				old_contents_without_libs = [x for x in old_contents if os.path.basename(x) not in preserve_libs]
> -				if len(set(libmap[lib]).intersection(old_contents_without_libs)) == len(libmap[lib]):
> -					preserve_libs.remove(lib)
> +			def has_external_consumers(lib, contents, otherlibs):
> +				consumers = set(libmap[lib])

# this should be done *once*.
# further, if libmap where k->set, you'd avoid repeated
# creation of sets all over the damn place.

> +				contents_without_libs = [x for x in contents if not os.path.basename(x) in otherlibs]

# os.path.basename invocations aren't horribly expensive, but the 
> +
> +				# just used by objects that will be autocleaned
> +				if len(consumers.difference(contents_without_libs)) == 0:

if not consumers.difference(contents_without_libs)):...
# no point in doing the len, thus don't.
# standard behaviour in python code to rely on bool protocol, thus do 
# so.

> +					return False
> +				# used by objects that are referenced as well, need to check those
> +				# recursively to break any reference cycles
> +				elif len(consumers.difference(contents)) == 0:

# same.  len isn't needed, thus don't use it.
# majority of builtins *do* support boolean, thus use it.

> +					otherlibs = set(otherlibs)

# this will explode. you're passing None in below.

> +					for ol in otherlibs.intersection(consumers):
> +						if has_external_consumers(ol, contents, otherlibs.copy().remove(lib)):

no way that code is right.
1) you're copying a set, then removing an item from it.
2) remove is a Py_RETURN_NONE, meaning it returns None.  you're 
  literally cloning a set, poping an item from it, and then
  throwing away the set, passing None into has_external_consumers
3) algo in general could be replaced by just doing walks over the 
  target preserve libs, pruning until a walk doesn't prune anything.
  Runtime ought to be better, less daft copys, saner to read also.

> +							return True
> +					return False
> +				# used by external objects directly
> +				else:
> +					return True
> +
> +			for lib in list(preserve_libs):
> +				if not has_external_consumers(lib, old_contents, preserve_libs):
> +					preserve_libs.remove(lib)

# this is a bit retarded... for handling libs referring to each other, 
# code above continually recalculates.
# reverse the mapping, and work from that angle; should avoid having 
# to do this insane little dance.

>
>  			# get the real paths for the libs
>  			preserve_paths = [x for x in old_contents if os.path.basename(x) in preserve_libs]
> -
> +			del old_contents, old_libs, mylibs, preserve_libs
> +
>  			# inject files that should be preserved into our image dir
>  			import shutil
>  			for x in preserve_paths:
> @@ -1293,7 +1312,7 @@
>  					preserve_paths.append(linktarget)
>  				else:
>  					shutil.copy2(x, os.path.join(srcroot, x.lstrip(os.sep)))
> -
> +			del preserve_paths
>
>  		# check for package collisions
>  		if "collision-protect" in self.settings.features:
>

Aside from my earlier suggestion that you should pop this crap out of 
svn and work it out outside of mainline, breaking it out into a func 
and doing tests for the func might be wise- very least easier for you 
to iron it out so it behaves.  Upshot, makes it easier for someone to 
refactor it down the line.

Still have the crappy forced double walk of ${D} regression also.

~harring