Re: [gentoo-portage-dev] [PATCH v2 gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps
On Wed, Mar 11, 2020 at 10:23 PM Zac Medico 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.
Re: [gentoo-portage-dev] [PATCH v2 gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps
On 3/11/20 9:43 PM, Matt Turner wrote: > On Wed, Mar 11, 2020 at 9:36 PM Matt Turner wrote: >> >> On Wed, Mar 11, 2020 at 9:31 PM Zac Medico 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 signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH v2 gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps
On Wed, Mar 11, 2020 at 9:36 PM Matt Turner wrote: > > On Wed, Mar 11, 2020 at 9:31 PM Zac Medico wrote: > > > > On 3/6/20 10:11 PM, Matt Turner wrote: > > > Signed-off-by: Matt Turner > > > --- > > > 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
Re: [gentoo-portage-dev] [PATCH v2 gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps
On 3/11/20 9:36 PM, Matt Turner wrote: > On Wed, Mar 11, 2020 at 9:31 PM Zac Medico 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 signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH v2 gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps
On Wed, Mar 11, 2020 at 9:31 PM Zac Medico wrote: > > On 3/6/20 10:11 PM, Matt Turner wrote: > > Signed-off-by: Matt Turner > > --- > > 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
On 3/11/20 4:32 PM, Matt Turner wrote: > On Tue, Mar 10, 2020 at 8:30 PM Zac Medico 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 signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH v2 gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps
On 3/6/20 10:11 PM, Matt Turner wrote: > Signed-off-by: Matt Turner > --- > 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 signature.asc Description: OpenPGP digital signature
Re: [gentoo-portage-dev] [PATCH v2 gentoolkit 2/2] eclean: Add option to delete binpkgs with changed deps
On Tue, Mar 10, 2020 at 8:30 PM Zac Medico 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
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 signature.asc Description: OpenPGP digital signature