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

[Zac Medico <zmedico@gentoo.org>]

On 03/14/2016 05:47 PM, Brian Dolbec wrote:
> On Mon, 14 Mar 2016 17:18:27 -0700
> Zac Medico <zmedico@gentoo.org> wrote:
> 
>> On 03/14/2016 10:52 AM, Brian Dolbec wrote:
>>> On Mon, 14 Mar 2016 10:22:11 -0700
>>> Zac Medico <zmedico@gentoo.org> 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.

Makes sense, thanks for the explanation.

> 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.

Yeah, let's do it. It's a great opportunity to add clarity to the code,
and prevent future goofs.

> 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.
> 

It's not a dupe. The Package.invalid property is used to implement
various checks that are not duplicated elsewhere.
-- 
Thanks,
Zac