[gentoo-portage-dev] [PATCH gentoolkit 1/2] eclean: Rewrite findPackages()

[gentoo-portage-dev] [PATCH gentoolkit 1/2] eclean: Rewrite findPackages()

[Matt Turner]

I found the original code to be nearly incomprehensible. Instead of
populating a dict of potential binpkgs to remove and then removing from
the to-be-removed list, just selectively add to-be-removed packages.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
I switched from tabs to spaces in the process. I can revert back if
desired.

 pym/gentoolkit/eclean/search.py | 189 ++++++++++++++++----------------
 1 file changed, 94 insertions(+), 95 deletions(-)

diff --git a/pym/gentoolkit/eclean/search.py b/pym/gentoolkit/eclean/search.py
index 58bd97e..831ba39 100644
--- a/pym/gentoolkit/eclean/search.py
+++ b/pym/gentoolkit/eclean/search.py
@@ -489,98 +489,97 @@ class DistfilesSearch(object):
 
 
 def findPackages(
-		options,
-		exclude=None,
-		destructive=False,
-		time_limit=0,
-		package_names=False,
-		pkgdir=None,
-		port_dbapi=portage.db[portage.root]["porttree"].dbapi,
-		var_dbapi=portage.db[portage.root]["vartree"].dbapi
-	):
-	"""Find all obsolete binary packages.
-
-	XXX: packages are found only by symlinks.
-	Maybe i should also return .tbz2 files from All/ that have
-	no corresponding symlinks.
-
-	@param options: dict of options determined at runtime
-	@param exclude: an exclusion dict as defined in
-			exclude.parseExcludeFile class.
-	@param destructive: boolean, defaults to False
-	@param time_limit: integer time value as returned by parseTime()
-	@param package_names: boolean, defaults to False.
-			used only if destructive=True
-	@param pkgdir: path to the binary package dir being checked
-	@param port_dbapi: defaults to portage.db[portage.root]["porttree"].dbapi
-					can be overridden for tests.
-	@param var_dbapi: defaults to portage.db[portage.root]["vartree"].dbapi
-					can be overridden for tests.
-
-	@rtype: dict
-	@return clean_me i.e. {'cat/pkg-ver.tbz2': [filepath],}
-	"""
-	if exclude is None:
-		exclude = {}
-	clean_me = {}
-	# create a full package dictionary
-
-	# now do an access test, os.walk does not error for "no read permission"
-	try:
-		test = os.listdir(pkgdir)
-		del test
-	except EnvironmentError as er:
-		if options['ignore-failure']:
-			exit(0)
-		print( pp.error("Error accessing PKGDIR." ), file=sys.stderr)
-		print( pp.error("(Check your make.conf file and environment)."), file=sys.stderr)
-		print( pp.error("Error: %s" %str(er)), file=sys.stderr)
-		exit(1)
-
-	# if portage supports FEATURES=binpkg-multi-instance, then
-	# cpv_all can return multiple instances per cpv, where
-	# instances are distinguishable by some extra attributes
-	# provided by portage's _pkg_str class
-	bin_dbapi = portage.binarytree(pkgdir=pkgdir, settings=var_dbapi.settings).dbapi
-	for cpv in bin_dbapi.cpv_all():
-		mtime = int(bin_dbapi.aux_get(cpv, ['_mtime_'])[0])
-		if time_limit and mtime >= time_limit:
-			# time-limit exclusion
-			continue
-		# dict is cpv->[pkgs] (supports binpkg-multi-instance)
-		clean_me.setdefault(cpv, []).append(cpv)
-
-	# keep only obsolete ones
-	if destructive and package_names:
-		cp_all = dict.fromkeys(var_dbapi.cp_all())
-	else:
-		cp_all = {}
-	for cpv in list(clean_me):
-		if exclDictMatchCP(exclude,portage.cpv_getkey(cpv)):
-			# exclusion because of the exclude file
-			del clean_me[cpv]
-			continue
-		if not destructive and port_dbapi.cpv_exists(cpv):
-			# exclusion because pkg still exists (in porttree)
-			del clean_me[cpv]
-			continue
-		if destructive and var_dbapi.cpv_exists(cpv):
-			buildtime = var_dbapi.aux_get(cpv, ['BUILD_TIME'])[0]
-			clean_me[cpv] = [pkg for pkg in clean_me[cpv]
-				# only keep path if BUILD_TIME is identical with vartree
-				if bin_dbapi.aux_get(pkg, ['BUILD_TIME'])[0] != buildtime]
-			if not clean_me[cpv]:
-				# nothing we can clean for this package
-				del clean_me[cpv]
-				continue
-		if portage.cpv_getkey(cpv) in cp_all and port_dbapi.cpv_exists(cpv):
-			# exclusion because of --package-names
-			del clean_me[cpv]
-
-	# the getname method correctly supports FEATURES=binpkg-multi-instance,
-	# allowing for multiple paths per cpv (the API used here is also compatible
-	# with older portage which does not support binpkg-multi-instance)
-	for cpv, pkgs in clean_me.items():
-		clean_me[cpv] = [bin_dbapi.bintree.getname(pkg) for pkg in pkgs]
-
-	return clean_me
+        options,
+        exclude=None,
+        destructive=False,
+        time_limit=0,
+        package_names=False,
+        pkgdir=None,
+        port_dbapi=portage.db[portage.root]["porttree"].dbapi,
+        var_dbapi=portage.db[portage.root]["vartree"].dbapi
+    ):
+    """Find obsolete binary packages.
+
+    @param options: dict of options determined at runtime
+    @type  options: dict
+    @param exclude: exclusion dict (as defined in the exclude.parseExcludeFile class)
+    @type  exclude: dict, optional
+    @param destructive: binpkg is obsolete if not installed (default: `False`)
+    @type  destructive: bool, optional
+    @param time_limit: binpkg is obsolete if older than time value as returned by parseTime()
+    @type  time_limit: int, optional
+    @param package_names: exclude all binpkg versions if package is installed
+                          (used with `destructive=True`) (default: `False`)
+    @type  package_names: bool, optional
+    @param pkgdir: path to the binpkg cache (PKGDIR)
+    @type  pkgdir: str
+    @param port_dbapi: defaults to portage.db[portage.root]["porttree"].dbapi
+                       Can be overridden for tests.
+    @param  var_dbapi: defaults to portage.db[portage.root]["vartree"].dbapi
+                       Can be overridden for tests.
+
+    @return binary packages to remove. e.g. {'cat/pkg-ver': [filepath]}
+    @rtype: dict
+    """
+    if exclude is None:
+        exclude = {}
+
+    # Access test, os.walk does not error for "no read permission"
+    try:
+        test = os.listdir(pkgdir)
+        del test
+    except EnvironmentError as er:
+        if options['ignore-failure']:
+            exit(0)
+        print(pp.error("Error accessing PKGDIR."), file=sys.stderr)
+        print(pp.error("(Check your make.conf file and environment)."), file=sys.stderr)
+        print(pp.error("Error: %s" % str(er)), file=sys.stderr)
+        exit(1)
+
+    # Create a dictionary of all installed packages
+    if destructive and package_names:
+        installed = dict.fromkeys(var_dbapi.cp_all())
+    else:
+        installed = {}
+
+    # Dictionary of binary packages to clean. Organized as cpv->[pkgs] in order
+    # to support FEATURES=binpkg-multi-instance.
+    dead_binpkgs = {}
+
+    # Create a dictionary of all binary packages whose mtime is older than
+    # time_limit, if set. These packages will be considered for removal.
+    bin_dbapi = portage.binarytree(pkgdir=pkgdir, settings=var_dbapi.settings).dbapi
+    for cpv in bin_dbapi.cpv_all():
+        cp = portage.cpv_getkey(cpv)
+
+        # Exclude per --exclude-file=...
+        if exclDictMatchCP(exclude, cp):
+            continue
+
+        # Exclude if binpkg is obsolete per --time-limit=...
+        if time_limit:
+            mtime = int(bin_dbapi.aux_get(cpv, ['_mtime_']))
+            if mtime >= time_limit:
+                continue
+
+        # Exclude if binpkg exists in the porttree and not --deep
+        if not destructive and port_dbapi.cpv_exists(cpv):
+            continue
+
+        if destructive and var_dbapi.cpv_exists(cpv):
+            # Exclude if an instance of the package is installed due to
+            # the --package-names option.
+            if cp in installed and port_dbapi.cpv_exists(cpv):
+                continue
+
+            # Exclude if BUILD_TIME of binpkg is same as vartree
+            buildtime = var_dbapi.aux_get(cpv, ['BUILD_TIME'])[0]
+            if buildtime == bin_dbapi.aux_get(cpv, ['BUILD_TIME'])[0]:
+                continue
+
+        binpkg_path = bin_dbapi.bintree.getname(cpv)
+        dead_binpkgs.setdefault(cpv, []).append(binpkg_path)
+
+    return dead_binpkgs
+
+# vim: set ts=4 sw=4 tw=79:
-- 
2.24.1

[gentoo-portage-dev] [PATCH gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps

[Matt Turner]

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 pym/gentoolkit/eclean/cli.py    |  7 ++++++-
 pym/gentoolkit/eclean/search.py | 30 +++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/pym/gentoolkit/eclean/cli.py b/pym/gentoolkit/eclean/cli.py
index 1a99b3e..39aafd3 100644
--- a/pym/gentoolkit/eclean/cli.py
+++ b/pym/gentoolkit/eclean/cli.py
@@ -147,6 +147,8 @@ def printUsage(_error=None, help=None):
 	or help in ('all','packages'):
 		print( "Available", yellow("options"),"for the",
 				green("packages"),"action:", file=out)
+		print( yellow("     --changed-deps")+
+			"               - delete packages for which ebuild dependencies have changed", file=out)
 		print( yellow(" -i, --ignore-failure")+
 			"             - ignore failure to locate PKGDIR", file=out)
 		print( file=out)
@@ -263,6 +265,8 @@ def parseArgs(options={}):
 				options['size-limit'] = parseSize(a)
 			elif o in ("-v", "--verbose") and not options['quiet']:
 					options['verbose'] = True
+			elif o in ("--changed-deps"):
+				options['changed-deps'] = True
 			elif o in ("-i", "--ignore-failure"):
 				options['ignore-failure'] = True
 			else:
@@ -290,7 +294,7 @@ def parseArgs(options={}):
 	getopt_options['short']['distfiles'] = "fs:"
 	getopt_options['long']['distfiles'] = ["fetch-restricted", "size-limit="]
 	getopt_options['short']['packages'] = "i"
-	getopt_options['long']['packages'] = ["ignore-failure"]
+	getopt_options['long']['packages'] = ["ignore-failure", "changed-deps"]
 	# set default options, except 'nocolor', which is set in main()
 	options['interactive'] = False
 	options['pretend'] = False
@@ -303,6 +307,7 @@ def parseArgs(options={}):
 	options['fetch-restricted'] = False
 	options['size-limit'] = 0
 	options['verbose'] = False
+	options['changed-deps'] = False
 	options['ignore-failure'] = False
 	# if called by a well-named symlink, set the action accordingly:
 	action = None
diff --git a/pym/gentoolkit/eclean/search.py b/pym/gentoolkit/eclean/search.py
index 831ba39..da8c286 100644
--- a/pym/gentoolkit/eclean/search.py
+++ b/pym/gentoolkit/eclean/search.py
@@ -13,6 +13,8 @@ import sys
 from functools import partial
 
 import portage
+from portage.dep import Atom, use_reduce
+from portage.dep._slot_operator import strip_slots
 
 import gentoolkit.pprinter as pp
 from gentoolkit.eclean.exclude import (exclDictMatchCP, exclDictExpand,
@@ -488,6 +490,17 @@ class DistfilesSearch(object):
 		return clean_me, saved
 
 
+def _deps_equal(deps_a, deps_b, eapi, uselist=None):
+    """Compare two dependency lists given a set of USE flags"""
+    if deps_a == deps_b: return True
+
+    deps_a = use_reduce(deps_a, uselist=uselist, eapi=eapi, token_class=Atom)
+    deps_b = use_reduce(deps_b, uselist=uselist, eapi=eapi, token_class=Atom)
+    strip_slots(deps_a)
+    strip_slots(deps_b)
+    return deps_a == deps_b
+
+
 def findPackages(
         options,
         exclude=None,
@@ -564,7 +577,22 @@ def findPackages(
 
         # Exclude if binpkg exists in the porttree and not --deep
         if not destructive and port_dbapi.cpv_exists(cpv):
-            continue
+            if not options['changed-deps']:
+                continue
+
+            uselist = bin_dbapi.aux_get(cpv, ['USE'])[0].split()
+            all_equal = True
+
+            for k in ('RDEPEND', 'PDEPEND'):
+                binpkg_deps = bin_dbapi.aux_get(cpv, [k])
+                ebuild_deps = port_dbapi.aux_get(cpv, [k])
+
+                if not _deps_equal(binpkg_deps, ebuild_deps, cpv.eapi, uselist):
+                    all_equal = False
+                    break
+
+            if all_equal:
+                continue
 
         if destructive and var_dbapi.cpv_exists(cpv):
             # Exclude if an instance of the package is installed due to
-- 
2.24.1

[gentoo-portage-dev] Re: [PATCH gentoolkit 1/2] eclean: Rewrite findPackages()

[Matt Turner]

On Thu, Feb 20, 2020 at 9:29 PM Matt Turner <mattst88@gentoo.org> wrote:
>
> I found the original code to be nearly incomprehensible. Instead of
> populating a dict of potential binpkgs to remove and then removing from
> the to-be-removed list, just selectively add to-be-removed packages.
>
> Signed-off-by: Matt Turner <mattst88@gentoo.org>
> ---
> I switched from tabs to spaces in the process. I can revert back if
> desired.
>
>  pym/gentoolkit/eclean/search.py | 189 ++++++++++++++++----------------
>  1 file changed, 94 insertions(+), 95 deletions(-)
>
> diff --git a/pym/gentoolkit/eclean/search.py b/pym/gentoolkit/eclean/search.py
> index 58bd97e..831ba39 100644
> --- a/pym/gentoolkit/eclean/search.py
> +++ b/pym/gentoolkit/eclean/search.py
> @@ -489,98 +489,97 @@ class DistfilesSearch(object):
>
>
>  def findPackages(
> -               options,
> -               exclude=None,
> -               destructive=False,
> -               time_limit=0,
> -               package_names=False,
> -               pkgdir=None,
> -               port_dbapi=portage.db[portage.root]["porttree"].dbapi,
> -               var_dbapi=portage.db[portage.root]["vartree"].dbapi
> -       ):
> -       """Find all obsolete binary packages.
> -
> -       XXX: packages are found only by symlinks.
> -       Maybe i should also return .tbz2 files from All/ that have
> -       no corresponding symlinks.
> -
> -       @param options: dict of options determined at runtime
> -       @param exclude: an exclusion dict as defined in
> -                       exclude.parseExcludeFile class.
> -       @param destructive: boolean, defaults to False
> -       @param time_limit: integer time value as returned by parseTime()
> -       @param package_names: boolean, defaults to False.
> -                       used only if destructive=True
> -       @param pkgdir: path to the binary package dir being checked
> -       @param port_dbapi: defaults to portage.db[portage.root]["porttree"].dbapi
> -                                       can be overridden for tests.
> -       @param var_dbapi: defaults to portage.db[portage.root]["vartree"].dbapi
> -                                       can be overridden for tests.
> -
> -       @rtype: dict
> -       @return clean_me i.e. {'cat/pkg-ver.tbz2': [filepath],}
> -       """
> -       if exclude is None:
> -               exclude = {}
> -       clean_me = {}
> -       # create a full package dictionary
> -
> -       # now do an access test, os.walk does not error for "no read permission"
> -       try:
> -               test = os.listdir(pkgdir)
> -               del test
> -       except EnvironmentError as er:
> -               if options['ignore-failure']:
> -                       exit(0)
> -               print( pp.error("Error accessing PKGDIR." ), file=sys.stderr)
> -               print( pp.error("(Check your make.conf file and environment)."), file=sys.stderr)
> -               print( pp.error("Error: %s" %str(er)), file=sys.stderr)
> -               exit(1)
> -
> -       # if portage supports FEATURES=binpkg-multi-instance, then
> -       # cpv_all can return multiple instances per cpv, where
> -       # instances are distinguishable by some extra attributes
> -       # provided by portage's _pkg_str class
> -       bin_dbapi = portage.binarytree(pkgdir=pkgdir, settings=var_dbapi.settings).dbapi
> -       for cpv in bin_dbapi.cpv_all():
> -               mtime = int(bin_dbapi.aux_get(cpv, ['_mtime_'])[0])
> -               if time_limit and mtime >= time_limit:
> -                       # time-limit exclusion
> -                       continue
> -               # dict is cpv->[pkgs] (supports binpkg-multi-instance)
> -               clean_me.setdefault(cpv, []).append(cpv)
> -
> -       # keep only obsolete ones
> -       if destructive and package_names:
> -               cp_all = dict.fromkeys(var_dbapi.cp_all())
> -       else:
> -               cp_all = {}
> -       for cpv in list(clean_me):
> -               if exclDictMatchCP(exclude,portage.cpv_getkey(cpv)):
> -                       # exclusion because of the exclude file
> -                       del clean_me[cpv]
> -                       continue
> -               if not destructive and port_dbapi.cpv_exists(cpv):
> -                       # exclusion because pkg still exists (in porttree)
> -                       del clean_me[cpv]
> -                       continue
> -               if destructive and var_dbapi.cpv_exists(cpv):
> -                       buildtime = var_dbapi.aux_get(cpv, ['BUILD_TIME'])[0]
> -                       clean_me[cpv] = [pkg for pkg in clean_me[cpv]
> -                               # only keep path if BUILD_TIME is identical with vartree
> -                               if bin_dbapi.aux_get(pkg, ['BUILD_TIME'])[0] != buildtime]
> -                       if not clean_me[cpv]:
> -                               # nothing we can clean for this package
> -                               del clean_me[cpv]
> -                               continue
> -               if portage.cpv_getkey(cpv) in cp_all and port_dbapi.cpv_exists(cpv):
> -                       # exclusion because of --package-names
> -                       del clean_me[cpv]
> -
> -       # the getname method correctly supports FEATURES=binpkg-multi-instance,
> -       # allowing for multiple paths per cpv (the API used here is also compatible
> -       # with older portage which does not support binpkg-multi-instance)
> -       for cpv, pkgs in clean_me.items():
> -               clean_me[cpv] = [bin_dbapi.bintree.getname(pkg) for pkg in pkgs]
> -
> -       return clean_me
> +        options,
> +        exclude=None,
> +        destructive=False,
> +        time_limit=0,
> +        package_names=False,
> +        pkgdir=None,
> +        port_dbapi=portage.db[portage.root]["porttree"].dbapi,
> +        var_dbapi=portage.db[portage.root]["vartree"].dbapi
> +    ):
> +    """Find obsolete binary packages.
> +
> +    @param options: dict of options determined at runtime
> +    @type  options: dict
> +    @param exclude: exclusion dict (as defined in the exclude.parseExcludeFile class)
> +    @type  exclude: dict, optional
> +    @param destructive: binpkg is obsolete if not installed (default: `False`)
> +    @type  destructive: bool, optional
> +    @param time_limit: binpkg is obsolete if older than time value as returned by parseTime()
> +    @type  time_limit: int, optional
> +    @param package_names: exclude all binpkg versions if package is installed
> +                          (used with `destructive=True`) (default: `False`)
> +    @type  package_names: bool, optional
> +    @param pkgdir: path to the binpkg cache (PKGDIR)
> +    @type  pkgdir: str
> +    @param port_dbapi: defaults to portage.db[portage.root]["porttree"].dbapi
> +                       Can be overridden for tests.
> +    @param  var_dbapi: defaults to portage.db[portage.root]["vartree"].dbapi
> +                       Can be overridden for tests.
> +
> +    @return binary packages to remove. e.g. {'cat/pkg-ver': [filepath]}
> +    @rtype: dict
> +    """
> +    if exclude is None:
> +        exclude = {}
> +
> +    # Access test, os.walk does not error for "no read permission"
> +    try:
> +        test = os.listdir(pkgdir)
> +        del test
> +    except EnvironmentError as er:
> +        if options['ignore-failure']:
> +            exit(0)
> +        print(pp.error("Error accessing PKGDIR."), file=sys.stderr)
> +        print(pp.error("(Check your make.conf file and environment)."), file=sys.stderr)
> +        print(pp.error("Error: %s" % str(er)), file=sys.stderr)
> +        exit(1)
> +
> +    # Create a dictionary of all installed packages
> +    if destructive and package_names:
> +        installed = dict.fromkeys(var_dbapi.cp_all())
> +    else:
> +        installed = {}
> +
> +    # Dictionary of binary packages to clean. Organized as cpv->[pkgs] in order
> +    # to support FEATURES=binpkg-multi-instance.
> +    dead_binpkgs = {}
> +
> +    # Create a dictionary of all binary packages whose mtime is older than
> +    # time_limit, if set. These packages will be considered for removal.

Naturally immediately after I press send I recognize that this comment
should be removed.

Fixed locally.

> +    bin_dbapi = portage.binarytree(pkgdir=pkgdir, settings=var_dbapi.settings).dbapi
> +    for cpv in bin_dbapi.cpv_all():
> +        cp = portage.cpv_getkey(cpv)
> +
> +        # Exclude per --exclude-file=...
> +        if exclDictMatchCP(exclude, cp):
> +            continue
> +
> +        # Exclude if binpkg is obsolete per --time-limit=...
> +        if time_limit:
> +            mtime = int(bin_dbapi.aux_get(cpv, ['_mtime_']))
> +            if mtime >= time_limit:
> +                continue
> +
> +        # Exclude if binpkg exists in the porttree and not --deep
> +        if not destructive and port_dbapi.cpv_exists(cpv):
> +            continue
> +
> +        if destructive and var_dbapi.cpv_exists(cpv):
> +            # Exclude if an instance of the package is installed due to
> +            # the --package-names option.
> +            if cp in installed and port_dbapi.cpv_exists(cpv):
> +                continue
> +
> +            # Exclude if BUILD_TIME of binpkg is same as vartree
> +            buildtime = var_dbapi.aux_get(cpv, ['BUILD_TIME'])[0]
> +            if buildtime == bin_dbapi.aux_get(cpv, ['BUILD_TIME'])[0]:
> +                continue
> +
> +        binpkg_path = bin_dbapi.bintree.getname(cpv)
> +        dead_binpkgs.setdefault(cpv, []).append(binpkg_path)
> +
> +    return dead_binpkgs
> +
> +# vim: set ts=4 sw=4 tw=79:
> --
> 2.24.1
>

Re: [gentoo-portage-dev] [PATCH gentoolkit 1/2] eclean: Rewrite findPackages()

[Michael 'veremitz' Everitt]

[-- Attachment #1.1: Type: text/plain, Size: 591 bytes --]

On 21/02/20 05:29, Matt Turner wrote:
> I found the original code to be nearly incomprehensible. Instead of
> populating a dict of potential binpkgs to remove and then removing from
> the to-be-removed list, just selectively add to-be-removed packages.
>
> Signed-off-by: Matt Turner <mattst88@gentoo.org>
> ---
> I switched from tabs to spaces in the process. I can revert back if
> desired.
>
Probably best to stick to tabs for consistency with the other portage code,
although naturally Zac probably better to ACK/NACK that.

Otherwise I think this is a good refresh. +1.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [gentoo-portage-dev] [PATCH gentoolkit 1/2] eclean: Rewrite findPackages()

[Zac Medico]

On 2/20/20 9:36 PM, Michael 'veremitz' Everitt wrote:
> On 21/02/20 05:29, Matt Turner wrote:
>> I found the original code to be nearly incomprehensible. Instead of
>> populating a dict of potential binpkgs to remove and then removing from
>> the to-be-removed list, just selectively add to-be-removed packages.
>>
>> Signed-off-by: Matt Turner <mattst88@gentoo.org>
>> ---
>> I switched from tabs to spaces in the process. I can revert back if
>> desired.
>>
> Probably best to stick to tabs for consistency with the other portage code,
> although naturally Zac probably better to ACK/NACK that.

Yeah lets stick with tabs unless we're converting everything to spaces.

> Otherwise I think this is a good refresh. +1.

Yes, looks good.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps

[Zac Medico]

[-- Attachment #1.1: Type: text/plain, Size: 1357 bytes --]

On 2/20/20 9:29 PM, Matt Turner wrote:
> +
>  def findPackages(
>          options,
>          exclude=None,
> @@ -564,7 +577,22 @@ def findPackages(
>  
>          # Exclude if binpkg exists in the porttree and not --deep
>          if not destructive and port_dbapi.cpv_exists(cpv):
> -            continue
> +            if not options['changed-deps']:
> +                continue
> +
> +            uselist = bin_dbapi.aux_get(cpv, ['USE'])[0].split()
> +            all_equal = True
> +
> +            for k in ('RDEPEND', 'PDEPEND'):
> +                binpkg_deps = bin_dbapi.aux_get(cpv, [k])
> +                ebuild_deps = port_dbapi.aux_get(cpv, [k])
> +
> +                if not _deps_equal(binpkg_deps, ebuild_deps, cpv.eapi, uselist):
> +                    all_equal = False
> +                    break
> +
> +            if all_equal:
> +                continue
>  
>          if destructive and var_dbapi.cpv_exists(cpv):
>              # Exclude if an instance of the package is installed due to
> 

The aux_get calls are expensive, so it's more efficient to get multiple
values with each call like:
	keys = ('RDEPEND', 'PDEPEND')
	binpkg_deps = dict(zip(keys, bin_dbapi.aux_get(cpv, keys))
	ebuild_deps = dict(zip(keys, port_dbapi.aux_get(cpv, keys))

Otherwise, looks good.
-- 
Thanks,
Zac


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 981 bytes --]

Re: [gentoo-portage-dev] [PATCH gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps

[Zac Medico]

[-- Attachment #1.1: Type: text/plain, Size: 1000 bytes --]

On 2/20/20 9:29 PM, Matt Turner wrote:
> @@ -564,7 +577,22 @@ def findPackages(
>  
>          # Exclude if binpkg exists in the porttree and not --deep
>          if not destructive and port_dbapi.cpv_exists(cpv):
> -            continue
> +            if not options['changed-deps']:
> +                continue
> +
> +            uselist = bin_dbapi.aux_get(cpv, ['USE'])[0].split()
> +            all_equal = True
> +
> +            for k in ('RDEPEND', 'PDEPEND'):
> +                binpkg_deps = bin_dbapi.aux_get(cpv, [k])
> +                ebuild_deps = port_dbapi.aux_get(cpv, [k])
> +
> +                if not _deps_equal(binpkg_deps, ebuild_deps, cpv.eapi, uselist):
> +                    all_equal = False
> +                    break
> +
> +            if all_equal:
> +                continue

If all_equal is True, then none of the other filters have an opportunity
to add this package to the dead_binpkgs set. That's not good is it?
-- 
Thanks,
Zac


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 981 bytes --]

Re: [gentoo-portage-dev] [PATCH gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps

[Matt Turner]

On Sun, Mar 1, 2020 at 10:32 PM Zac Medico <zmedico@gentoo.org> wrote:
>
> On 2/20/20 9:29 PM, Matt Turner wrote:
> > +
> >  def findPackages(
> >          options,
> >          exclude=None,
> > @@ -564,7 +577,22 @@ def findPackages(
> >
> >          # Exclude if binpkg exists in the porttree and not --deep
> >          if not destructive and port_dbapi.cpv_exists(cpv):
> > -            continue
> > +            if not options['changed-deps']:
> > +                continue
> > +
> > +            uselist = bin_dbapi.aux_get(cpv, ['USE'])[0].split()
> > +            all_equal = True
> > +
> > +            for k in ('RDEPEND', 'PDEPEND'):
> > +                binpkg_deps = bin_dbapi.aux_get(cpv, [k])
> > +                ebuild_deps = port_dbapi.aux_get(cpv, [k])
> > +
> > +                if not _deps_equal(binpkg_deps, ebuild_deps, cpv.eapi, uselist):
> > +                    all_equal = False
> > +                    break
> > +
> > +            if all_equal:
> > +                continue
> >
> >          if destructive and var_dbapi.cpv_exists(cpv):
> >              # Exclude if an instance of the package is installed due to
> >
>
> The aux_get calls are expensive, so it's more efficient to get multiple
> values with each call like:
>         keys = ('RDEPEND', 'PDEPEND')
>         binpkg_deps = dict(zip(keys, bin_dbapi.aux_get(cpv, keys))
>         ebuild_deps = dict(zip(keys, port_dbapi.aux_get(cpv, keys))
>
> Otherwise, looks good.

Thanks, that makes the code a lot nicer too.

Re: [gentoo-portage-dev] [PATCH gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps

[Matt Turner]

On Mon, Mar 2, 2020 at 12:40 PM Matt Turner <mattst88@gentoo.org> wrote:
>
> On Sun, Mar 1, 2020 at 10:32 PM Zac Medico <zmedico@gentoo.org> wrote:
> >
> > On 2/20/20 9:29 PM, Matt Turner wrote:
> > > +
> > >  def findPackages(
> > >          options,
> > >          exclude=None,
> > > @@ -564,7 +577,22 @@ def findPackages(
> > >
> > >          # Exclude if binpkg exists in the porttree and not --deep
> > >          if not destructive and port_dbapi.cpv_exists(cpv):
> > > -            continue
> > > +            if not options['changed-deps']:
> > > +                continue
> > > +
> > > +            uselist = bin_dbapi.aux_get(cpv, ['USE'])[0].split()
> > > +            all_equal = True
> > > +
> > > +            for k in ('RDEPEND', 'PDEPEND'):
> > > +                binpkg_deps = bin_dbapi.aux_get(cpv, [k])
> > > +                ebuild_deps = port_dbapi.aux_get(cpv, [k])
> > > +
> > > +                if not _deps_equal(binpkg_deps, ebuild_deps, cpv.eapi, uselist):
> > > +                    all_equal = False
> > > +                    break
> > > +
> > > +            if all_equal:
> > > +                continue
> > >
> > >          if destructive and var_dbapi.cpv_exists(cpv):
> > >              # Exclude if an instance of the package is installed due to
> > >
> >
> > The aux_get calls are expensive, so it's more efficient to get multiple
> > values with each call like:
> >         keys = ('RDEPEND', 'PDEPEND')
> >         binpkg_deps = dict(zip(keys, bin_dbapi.aux_get(cpv, keys))
> >         ebuild_deps = dict(zip(keys, port_dbapi.aux_get(cpv, keys))
> >
> > Otherwise, looks good.
>
> Thanks, that makes the code a lot nicer too.

Actually, use_reduce wants a list (it calls .split). Wrapping those in
list() looks like it works, but I suspect that's not as you intended.
What does the zip add over just doing this?

  binpkg_deps = bin_dbapi.aux_get(cpv, keys)
  ebuild_deps = port_dbapi.aux_get(cpv, keys)

Re: [gentoo-portage-dev] [PATCH gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps

[Matt Turner]

On Sun, Mar 1, 2020 at 10:39 PM Zac Medico <zmedico@gentoo.org> wrote:
>
> On 2/20/20 9:29 PM, Matt Turner wrote:
> > @@ -564,7 +577,22 @@ def findPackages(
> >
> >          # Exclude if binpkg exists in the porttree and not --deep
> >          if not destructive and port_dbapi.cpv_exists(cpv):
> > -            continue
> > +            if not options['changed-deps']:
> > +                continue
> > +
> > +            uselist = bin_dbapi.aux_get(cpv, ['USE'])[0].split()
> > +            all_equal = True
> > +
> > +            for k in ('RDEPEND', 'PDEPEND'):
> > +                binpkg_deps = bin_dbapi.aux_get(cpv, [k])
> > +                ebuild_deps = port_dbapi.aux_get(cpv, [k])
> > +
> > +                if not _deps_equal(binpkg_deps, ebuild_deps, cpv.eapi, uselist):
> > +                    all_equal = False
> > +                    break
> > +
> > +            if all_equal:
> > +                continue
>
> If all_equal is True, then none of the other filters have an opportunity
> to add this package to the dead_binpkgs set. That's not good is it?

There are four cases we skip including packages: 1) exclude list, 2)
time limit, 3) non-destructive and package still exists (and now
optionally runtime deps haven't changed), 4) destructive and package
is installed. Cases (3) and (4) are non-overlapping.

If none of those cases are true, then we add the package to the
dead_binpkgs set. The logic looks right to me.

Maybe I'm not understanding.

With your other suggestion in place, the code looks like this, which
is hopefully clearer.

    # Exclude if binpkg exists in the porttree and not --deep
    if not destructive and port_dbapi.cpv_exists(cpv):
        if not options['changed-deps']:
            continue

        keys = ('RDEPEND', 'PDEPEND')
        binpkg_deps = bin_dbapi.aux_get(cpv, keys)
        ebuild_deps = port_dbapi.aux_get(cpv, keys)
        uselist = bin_dbapi.aux_get(cpv, ['USE'])[0].split()

        if _deps_equal(binpkg_deps, ebuild_deps, cpv.eapi, uselist):
            continue

Unfortunately I don't have any packages with changed-deps at the
moment for testing :(

Re: [gentoo-portage-dev] [PATCH gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps

[Zac Medico]

[-- Attachment #1.1: Type: text/plain, Size: 2355 bytes --]

On 3/2/20 1:02 PM, Matt Turner wrote:
> On Mon, Mar 2, 2020 at 12:40 PM Matt Turner <mattst88@gentoo.org> wrote:
>>
>> On Sun, Mar 1, 2020 at 10:32 PM Zac Medico <zmedico@gentoo.org> wrote:
>>>
>>> On 2/20/20 9:29 PM, Matt Turner wrote:
>>>> +
>>>>  def findPackages(
>>>>          options,
>>>>          exclude=None,
>>>> @@ -564,7 +577,22 @@ def findPackages(
>>>>
>>>>          # Exclude if binpkg exists in the porttree and not --deep
>>>>          if not destructive and port_dbapi.cpv_exists(cpv):
>>>> -            continue
>>>> +            if not options['changed-deps']:
>>>> +                continue
>>>> +
>>>> +            uselist = bin_dbapi.aux_get(cpv, ['USE'])[0].split()
>>>> +            all_equal = True
>>>> +
>>>> +            for k in ('RDEPEND', 'PDEPEND'):
>>>> +                binpkg_deps = bin_dbapi.aux_get(cpv, [k])
>>>> +                ebuild_deps = port_dbapi.aux_get(cpv, [k])
>>>> +
>>>> +                if not _deps_equal(binpkg_deps, ebuild_deps, cpv.eapi, uselist):
>>>> +                    all_equal = False
>>>> +                    break
>>>> +
>>>> +            if all_equal:
>>>> +                continue
>>>>
>>>>          if destructive and var_dbapi.cpv_exists(cpv):
>>>>              # Exclude if an instance of the package is installed due to
>>>>
>>>
>>> The aux_get calls are expensive, so it's more efficient to get multiple
>>> values with each call like:
>>>         keys = ('RDEPEND', 'PDEPEND')
>>>         binpkg_deps = dict(zip(keys, bin_dbapi.aux_get(cpv, keys))
>>>         ebuild_deps = dict(zip(keys, port_dbapi.aux_get(cpv, keys))
>>>
>>> Otherwise, looks good.
>>
>> Thanks, that makes the code a lot nicer too.
> 
> Actually, use_reduce wants a list (it calls .split). Wrapping those in
> list() looks like it works, but I suspect that's not as you intended.
> What does the zip add over just doing this?
> 
>   binpkg_deps = bin_dbapi.aux_get(cpv, keys)
>   ebuild_deps = port_dbapi.aux_get(cpv, keys)

Using dict(zip(keys, port_dbapi.aux_get(cpv, keys)) is only useful if
you want to use a dictionary to access the values. However, if lists are
good enough then you might just use those instead. You could even join
the values together like this:

  ebuild_deps = ' '.join(port_dbapi.aux_get(cpv, keys))
-- 
Thanks,
Zac


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 981 bytes --]

Re: [gentoo-portage-dev] [PATCH gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps

[Zac Medico]

[-- Attachment #1.1: Type: text/plain, Size: 2289 bytes --]

On 3/2/20 1:11 PM, Matt Turner wrote:
> On Sun, Mar 1, 2020 at 10:39 PM Zac Medico <zmedico@gentoo.org> wrote:
>>
>> On 2/20/20 9:29 PM, Matt Turner wrote:
>>> @@ -564,7 +577,22 @@ def findPackages(
>>>
>>>          # Exclude if binpkg exists in the porttree and not --deep
>>>          if not destructive and port_dbapi.cpv_exists(cpv):
>>> -            continue
>>> +            if not options['changed-deps']:
>>> +                continue
>>> +
>>> +            uselist = bin_dbapi.aux_get(cpv, ['USE'])[0].split()
>>> +            all_equal = True
>>> +
>>> +            for k in ('RDEPEND', 'PDEPEND'):
>>> +                binpkg_deps = bin_dbapi.aux_get(cpv, [k])
>>> +                ebuild_deps = port_dbapi.aux_get(cpv, [k])
>>> +
>>> +                if not _deps_equal(binpkg_deps, ebuild_deps, cpv.eapi, uselist):
>>> +                    all_equal = False
>>> +                    break
>>> +
>>> +            if all_equal:
>>> +                continue
>>
>> If all_equal is True, then none of the other filters have an opportunity
>> to add this package to the dead_binpkgs set. That's not good is it?
> 
> There are four cases we skip including packages: 1) exclude list, 2)
> time limit, 3) non-destructive and package still exists (and now
> optionally runtime deps haven't changed), 4) destructive and package
> is installed. Cases (3) and (4) are non-overlapping.
> 
> If none of those cases are true, then we add the package to the
> dead_binpkgs set. The logic looks right to me.
> 
> Maybe I'm not understanding.

What I imagine is that you could have some old packages that you
probably want to delete because they're so old, even though their deps
have not changed. Meanwhile you have some packages that are
relatively recent and you'd like to delete them if they have changed deps.

Given the current logic, I guess you'd have to do separate passes, one
to delete packages based on age and another to delete packages based on
changed deps. Maybe it's fine to require separate passes for this kind
of thing. I supposed the alternative would be to add an --or flag that
would allow you to say that you want to delete packages if they are at
least a certain age or they have changed deps.
-- 
Thanks,
Zac




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 981 bytes --]

Re: [gentoo-portage-dev] [PATCH gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps

[Matt Turner]

On Mon, Mar 2, 2020 at 9:15 PM Zac Medico <zmedico@gentoo.org> wrote:
>
> On 3/2/20 1:11 PM, Matt Turner wrote:
> > On Sun, Mar 1, 2020 at 10:39 PM Zac Medico <zmedico@gentoo.org> wrote:
> >>
> >> On 2/20/20 9:29 PM, Matt Turner wrote:
> >>> @@ -564,7 +577,22 @@ def findPackages(
> >>>
> >>>          # Exclude if binpkg exists in the porttree and not --deep
> >>>          if not destructive and port_dbapi.cpv_exists(cpv):
> >>> -            continue
> >>> +            if not options['changed-deps']:
> >>> +                continue
> >>> +
> >>> +            uselist = bin_dbapi.aux_get(cpv, ['USE'])[0].split()
> >>> +            all_equal = True
> >>> +
> >>> +            for k in ('RDEPEND', 'PDEPEND'):
> >>> +                binpkg_deps = bin_dbapi.aux_get(cpv, [k])
> >>> +                ebuild_deps = port_dbapi.aux_get(cpv, [k])
> >>> +
> >>> +                if not _deps_equal(binpkg_deps, ebuild_deps, cpv.eapi, uselist):
> >>> +                    all_equal = False
> >>> +                    break
> >>> +
> >>> +            if all_equal:
> >>> +                continue
> >>
> >> If all_equal is True, then none of the other filters have an opportunity
> >> to add this package to the dead_binpkgs set. That's not good is it?
> >
> > There are four cases we skip including packages: 1) exclude list, 2)
> > time limit, 3) non-destructive and package still exists (and now
> > optionally runtime deps haven't changed), 4) destructive and package
> > is installed. Cases (3) and (4) are non-overlapping.
> >
> > If none of those cases are true, then we add the package to the
> > dead_binpkgs set. The logic looks right to me.
> >
> > Maybe I'm not understanding.
>
> What I imagine is that you could have some old packages that you
> probably want to delete because they're so old, even though their deps
> have not changed. Meanwhile you have some packages that are
> relatively recent and you'd like to delete them if they have changed deps.
>
> Given the current logic, I guess you'd have to do separate passes, one
> to delete packages based on age and another to delete packages based on
> changed deps. Maybe it's fine to require separate passes for this kind
> of thing. I supposed the alternative would be to add an --or flag that
> would allow you to say that you want to delete packages if they are at
> least a certain age or they have changed deps.

Oh, I think I understand now.

I was confused about this. I expected that --time-limit=2w to mean
that eclean should delete everything older than two weeks, but it's
actually the opposite, more or less. It actually means to *keep*
everything with less than two weeks old. Surprised me...

>  -t, --time-limit=<time>   - don't delete files modified since <time>

So, with that surprising behavior I think my patch is doing the right
thing, but with the wrong comment. I'll fix those and send v2 patches
with the tabs restored, etc.

Thanks a bunch for the review.