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

[gentoo-portage-dev] [PATCH v2 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>
---
 pym/gentoolkit/eclean/search.py | 113 ++++++++++++++++----------------
 1 file changed, 55 insertions(+), 58 deletions(-)

diff --git a/pym/gentoolkit/eclean/search.py b/pym/gentoolkit/eclean/search.py
index 58bd97e..0efefdb 100644
--- a/pym/gentoolkit/eclean/search.py
+++ b/pym/gentoolkit/eclean/search.py
@@ -498,89 +498,86 @@ def findPackages(
 		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.
+	"""Find obsolete binary packages.
 
 	@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
+	@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: exclude binpkg if newer 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.
+					   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
-	@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"
+	# 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)
+		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
+	# 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 = {}
+
 	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)
+		cp = portage.cpv_getkey(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]
+		# Exclude per --exclude-file=...
+		if exclDictMatchCP(exclude, cp):
 			continue
+
+		# Exclude if binpkg is newer than --time-limit=...
+		if time_limit:
+			mtime = int(bin_dbapi.aux_get(cpv, ['_mtime_'])[0])
+			if mtime >= time_limit:
+				continue
+
+		# Exclude if binpkg exists in the porttree and not --deep
 		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):
+			# 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]
-			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]
+			if buildtime == bin_dbapi.aux_get(cpv, ['BUILD_TIME'])[0]:
 				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]
+		binpkg_path = bin_dbapi.bintree.getname(cpv)
+		dead_binpkgs.setdefault(cpv, []).append(binpkg_path)
+
+	return dead_binpkgs
 
-	return clean_me
+# vim: set ts=4 sw=4 tw=79:
-- 
2.24.1

[gentoo-portage-dev] [PATCH v2 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 | 24 +++++++++++++++++++++++-
 2 files changed, 29 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 0efefdb..17655cb 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,
@@ -562,7 +575,16 @@ 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
+
+			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
 
 		if destructive and var_dbapi.cpv_exists(cpv):
 			# Exclude if an instance of the package is installed due to
-- 
2.24.1

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

[Zac Medico]

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

On 3/6/20 10:11 PM, Matt Turner wrote:
> +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)

It's pure luck that passing a list of depstrings to use_reduce works
here, so it will be more future-proof to use ' '.join(depstr) instead.
The relevant code in use_reduce looks like this:

if isinstance(depstr, list):
	if portage._internal_caller:
		warnings.warn(_("Passing paren_reduced dep arrays to %s is deprecated. " + \
			"Pass the original dep string instead.") % \
			('portage.dep.use_reduce',), DeprecationWarning, stacklevel=2)
	depstr = paren_enclose(depstr)
-- 
Thanks,
Zac




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

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

[Matt Turner]

On Tue, Mar 10, 2020 at 8:30 PM Zac Medico <zmedico@gentoo.org> wrote:
>
> On 3/6/20 10:11 PM, Matt Turner wrote:
> > +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)
>
> It's pure luck that passing a list of depstrings to use_reduce works
> here, so it will be more future-proof to use ' '.join(depstr) instead.
> The relevant code in use_reduce looks like this:
>
> if isinstance(depstr, list):
>         if portage._internal_caller:
>                 warnings.warn(_("Passing paren_reduced dep arrays to %s is deprecated. " + \
>                         "Pass the original dep string instead.") % \
>                         ('portage.dep.use_reduce',), DeprecationWarning, stacklevel=2)
>         depstr = paren_enclose(depstr)

Okay, thank you. I've fixed this with:

- binpkg_deps = bin_dbapi.aux_get(cpv, keys)
- ebuild_deps = port_dbapi.aux_get(cpv, keys)
+ binpkg_deps = ' '.join(bin_dbapi.aux_get(cpv, keys))
+ ebuild_deps = ' '.join(port_dbapi.aux_get(cpv, keys))

With that fixed, do the patches look good to you?

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

[Zac Medico]

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

On 3/6/20 10:11 PM, Matt Turner wrote:
> Signed-off-by: Matt Turner <mattst88@gentoo.org>
> ---
>  pym/gentoolkit/eclean/cli.py    |  7 ++++++-
>  pym/gentoolkit/eclean/search.py | 24 +++++++++++++++++++++++-
>  2 files changed, 29 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 0efefdb..17655cb 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,
> @@ -562,7 +575,16 @@ 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

We can't can't continue above, since that will skip all of the filters
that occur later in the loop. So, we have to nest the below changed-deps
code under if options['changed-deps']:

> +
> +			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
>  
>  		if destructive and var_dbapi.cpv_exists(cpv):
>  			# Exclude if an instance of the package is installed due to
> 


-- 
Thanks,
Zac


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

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

[Zac Medico]

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

On 3/11/20 4:32 PM, Matt Turner wrote:
> On Tue, Mar 10, 2020 at 8:30 PM Zac Medico <zmedico@gentoo.org> wrote:
>>
>> On 3/6/20 10:11 PM, Matt Turner wrote:
>>> +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)
>>
>> It's pure luck that passing a list of depstrings to use_reduce works
>> here, so it will be more future-proof to use ' '.join(depstr) instead.
>> The relevant code in use_reduce looks like this:
>>
>> if isinstance(depstr, list):
>>         if portage._internal_caller:
>>                 warnings.warn(_("Passing paren_reduced dep arrays to %s is deprecated. " + \
>>                         "Pass the original dep string instead.") % \
>>                         ('portage.dep.use_reduce',), DeprecationWarning, stacklevel=2)
>>         depstr = paren_enclose(depstr)
> 
> Okay, thank you. I've fixed this with:
> 
> - binpkg_deps = bin_dbapi.aux_get(cpv, keys)
> - ebuild_deps = port_dbapi.aux_get(cpv, keys)
> + binpkg_deps = ' '.join(bin_dbapi.aux_get(cpv, keys))
> + ebuild_deps = ' '.join(port_dbapi.aux_get(cpv, keys))
> 
> With that fixed, do the patches look good to you?

Yeah, looks good (with options['changed-deps'] logic fix from my other
email).
-- 
Thanks,
Zac


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

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

[Matt Turner]

On Wed, Mar 11, 2020 at 9:31 PM Zac Medico <zmedico@gentoo.org> wrote:
>
> On 3/6/20 10:11 PM, Matt Turner wrote:
> > Signed-off-by: Matt Turner <mattst88@gentoo.org>
> > ---
> >  pym/gentoolkit/eclean/cli.py    |  7 ++++++-
> >  pym/gentoolkit/eclean/search.py | 24 +++++++++++++++++++++++-
> >  2 files changed, 29 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 0efefdb..17655cb 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,
> > @@ -562,7 +575,16 @@ 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
>
> We can't can't continue above, since that will skip all of the filters
> that occur later in the loop. So, we have to nest the below changed-deps
> code under if options['changed-deps']:

I'm happy to make that change, but I don't think it's necessary,
strictly speaking, since this is inside an 'if not destructive'
conditional and the only filter afterwards is 'if destructive'.

In case we add more filters in the future, I'll make the change you suggested.

Thanks a bunch for your reviews!

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

[Zac Medico]

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

On 3/11/20 9:36 PM, Matt Turner wrote:
> On Wed, Mar 11, 2020 at 9:31 PM Zac Medico <zmedico@gentoo.org> wrote:
>>> @@ -562,7 +575,16 @@ 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
>>
>> We can't can't continue above, since that will skip all of the filters
>> that occur later in the loop. So, we have to nest the below changed-deps
>> code under if options['changed-deps']:
> 
> I'm happy to make that change, but I don't think it's necessary,
> strictly speaking, since this is inside an 'if not destructive'
> conditional and the only filter afterwards is 'if destructive'.
> 
> In case we add more filters in the future, I'll make the change you suggested.

Yeah, I think it significantly reduces the cognitive burden to the
reader as well. Thanks!

> Thanks a bunch for your reviews!

You're welcome!
-- 
Thanks,
Zac


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

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

[Matt Turner]

On Wed, Mar 11, 2020 at 9:36 PM Matt Turner <mattst88@gentoo.org> wrote:
>
> On Wed, Mar 11, 2020 at 9:31 PM Zac Medico <zmedico@gentoo.org> wrote:
> >
> > On 3/6/20 10:11 PM, Matt Turner wrote:
> > > Signed-off-by: Matt Turner <mattst88@gentoo.org>
> > > ---
> > >  pym/gentoolkit/eclean/cli.py    |  7 ++++++-
> > >  pym/gentoolkit/eclean/search.py | 24 +++++++++++++++++++++++-
> > >  2 files changed, 29 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 0efefdb..17655cb 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,
> > > @@ -562,7 +575,16 @@ 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
> >
> > We can't can't continue above, since that will skip all of the filters
> > that occur later in the loop. So, we have to nest the below changed-deps
> > code under if options['changed-deps']:
>
> I'm happy to make that change, but I don't think it's necessary,
> strictly speaking, since this is inside an 'if not destructive'
> conditional and the only filter afterwards is 'if destructive'.

Wait... the logic was if not destructive and
package-exists-in-porttree -> continue and do not add it to the dead
package list.

I've just changed it so it does that if changed-deps is not set... so
keep the current behavior without --changed-deps.

And if --changed-deps, check the porttree vs binpkg dependencies, and
if they still match, skip.

What is wrong with that logic?

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

[Zac Medico]

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

On 3/11/20 9:43 PM, Matt Turner wrote:
> On Wed, Mar 11, 2020 at 9:36 PM Matt Turner <mattst88@gentoo.org> wrote:
>>
>> On Wed, Mar 11, 2020 at 9:31 PM Zac Medico <zmedico@gentoo.org> wrote:
>>> We can't can't continue above, since that will skip all of the filters
>>> that occur later in the loop. So, we have to nest the below changed-deps
>>> code under if options['changed-deps']:
>>
>> I'm happy to make that change, but I don't think it's necessary,
>> strictly speaking, since this is inside an 'if not destructive'
>> conditional and the only filter afterwards is 'if destructive'.
> 
> Wait... the logic was if not destructive and
> package-exists-in-porttree -> continue and do not add it to the dead
> package list.
> 
> I've just changed it so it does that if changed-deps is not set... so
> keep the current behavior without --changed-deps.
> 
> And if --changed-deps, check the porttree vs binpkg dependencies, and
> if they still match, skip.

Yeah, you're right.

> What is wrong with that logic?

The coupling with --destructive logic complicates matters. It raises the
question, why isn't --time-limit logic also coupled with --destructive
logic? I think "in order to preserve the status quo" is a reasonable
answer to this question.
-- 
Thanks,
Zac


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

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

[Matt Turner]

On Wed, Mar 11, 2020 at 10:23 PM Zac Medico <zmedico@gentoo.org> wrote:
> The coupling with --destructive logic complicates matters. It raises the
> question, why isn't --time-limit logic also coupled with --destructive
> logic? I think "in order to preserve the status quo" is a reasonable
> answer to this question.

Yeah :(

It's clear to me that these options were added in a very ad hoc
manner. And... named badly if I do say so. E.g., destructive and
package_names do not correspond, at least in my mind, to the
operations they do.

I'd like to clean those up as a follow on.