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

[Matt Turner <mattst88@gentoo.org>]

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.