Re: [gentoo-portage-dev] [Patch] Repoman rewrite stage2 modularization conversion complete

[Brian Dolbec]

On Mon, 14 Mar 2016 17:18:27 -0700
Zac Medico <zmedico@g.o> wrote:

> On 03/14/2016 10:52 AM, Brian Dolbec wrote:
> > On Mon, 14 Mar 2016 10:22:11 -0700
> > Zac Medico <zmedico@g.o> wrote:
> >
> >> On 03/14/2016 10:14 AM, Zac Medico wrote:
> >>> On 03/05/2016 01:37 PM, Brian Dolbec wrote:
> >>>
> >>>> Zac, I'm done with code changes in the rewrite.  Ready for a last
> >>>> look before a merge.  Can you have a look again?  I did some
> >>>> changes/fixes and rebased them in.  Floppym hasn't reported any
> >>>> more bugs, so I think it's ready for broader testing in a
> >>>> release.  Then we can work on moving all the test data to a
> >>>> separate file in the tree or downloaded...
> >>>
> >>> The dynamic_data stuff in Scanner is a little hard to follow. Then
> >>> it calls dynamic_data.update(rdata), is there any chance that the
> >>> update operation might clobber something that shouldn't have been
> >>> clobbered?
> >>
> >> To clarify my question, suppose that one function returns {'foo':
> >> True} and another one returns {'foo', False}, so now there first
> >> {'foo': True} setting is forgotten. Is that going to be a
> >> problem?
> >
> > No, as stated in my other reply.  There are only a few things that
> > are modified.  Mostly as I made a new module, following the original
> > order the checks were run.  As data was discovered missing it was
> > added to dynamic_data from the previous check that supplied it to
> > the Scanner class.  So, only data needed later was passed back to
> > update the dynamic_data.
> >
> > Also all those checks originally ran in one huge 1k LOC loop with
> > another slightly smaller ebuild loop nested inside it.  So all those
> > variables were subject to change already by previous code run.  In
> > the stage1 rewrite, I/we did the same thing in creating the
> > separated checks classes.  After the check was done, only the data
> > required was brought back into the primary loop.
> >
>
> I've found what may be a real instance of the kind of problem I was
> worried about. The 'allvalid' key is set in both
> pym/repoman/modules/scan/ebuild/isebuild.py and
> pym/repoman/modules/scan/ebuild/ebuild.py, and its non-trivial for me
> to determine whether this is a real problem or not.

It is not a problem.

allvalid is initialized in the isEbuild class at the start of the pkg
level checks to True, then there are several things that if found reset
it to False...

Then when it gets to the ebuild level checks the ebuild check there
just sets it False if the pkg.invalid variable is True.  In some cases
it might be a double False, but never will it trounce the previous
setting.

The only consumer for that allvalid variable is the metadata
UnusedCheck class.  

So the allvalid variable is True until found False
by whichever checks along the way find it to be False.  Like a fuse,
it's good until it's blown, then it can never be good again.  I don't
think this particular variable justifies a special class that more
fully mimics a fuse.  Impossible to reset it like a breaker.

To be honest I did not look into the pkg.invalid variable's need to be
setting it False in the Ebuild class.  It may in fact be a dupe of a
setting in the isEbuild class or it might be moved there. That original
spaghetti code could in fact have had duplicates since it was so hard
to figure out where to embed something.
-- 
Brian Dolbec <dolsen>