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