Gentoo Archives: gentoo-portage-dev

From: Zac Medico <zmedico@g.o>
To: gentoo-portage-dev@l.g.o
Subject: Re: [gentoo-portage-dev] [Patch] Repoman rewrite stage2 modularization conversion complete
Date: Tue, 15 Mar 2016 01:05:06
Message-Id: 56E75FB8.9060802@gentoo.org
In Reply to: Re: [gentoo-portage-dev] [Patch] Repoman rewrite stage2 modularization conversion complete by Brian Dolbec
1 On 03/14/2016 05:47 PM, Brian Dolbec wrote:
2 > On Mon, 14 Mar 2016 17:18:27 -0700
3 > Zac Medico <zmedico@g.o> wrote:
4 >
5 >> On 03/14/2016 10:52 AM, Brian Dolbec wrote:
6 >>> On Mon, 14 Mar 2016 10:22:11 -0700
7 >>> Zac Medico <zmedico@g.o> wrote:
8 >>>
9 >>>> On 03/14/2016 10:14 AM, Zac Medico wrote:
10 >>>>> On 03/05/2016 01:37 PM, Brian Dolbec wrote:
11 >>>>>
12 >>>>>> Zac, I'm done with code changes in the rewrite. Ready for a last
13 >>>>>> look before a merge. Can you have a look again? I did some
14 >>>>>> changes/fixes and rebased them in. Floppym hasn't reported any
15 >>>>>> more bugs, so I think it's ready for broader testing in a
16 >>>>>> release. Then we can work on moving all the test data to a
17 >>>>>> separate file in the tree or downloaded...
18 >>>>>
19 >>>>> The dynamic_data stuff in Scanner is a little hard to follow. Then
20 >>>>> it calls dynamic_data.update(rdata), is there any chance that the
21 >>>>> update operation might clobber something that shouldn't have been
22 >>>>> clobbered?
23 >>>>
24 >>>> To clarify my question, suppose that one function returns {'foo':
25 >>>> True} and another one returns {'foo', False}, so now there first
26 >>>> {'foo': True} setting is forgotten. Is that going to be a
27 >>>> problem?
28 >>>
29 >>> No, as stated in my other reply. There are only a few things that
30 >>> are modified. Mostly as I made a new module, following the original
31 >>> order the checks were run. As data was discovered missing it was
32 >>> added to dynamic_data from the previous check that supplied it to
33 >>> the Scanner class. So, only data needed later was passed back to
34 >>> update the dynamic_data.
35 >>>
36 >>> Also all those checks originally ran in one huge 1k LOC loop with
37 >>> another slightly smaller ebuild loop nested inside it. So all those
38 >>> variables were subject to change already by previous code run. In
39 >>> the stage1 rewrite, I/we did the same thing in creating the
40 >>> separated checks classes. After the check was done, only the data
41 >>> required was brought back into the primary loop.
42 >>>
43 >>
44 >> I've found what may be a real instance of the kind of problem I was
45 >> worried about. The 'allvalid' key is set in both
46 >> pym/repoman/modules/scan/ebuild/isebuild.py and
47 >> pym/repoman/modules/scan/ebuild/ebuild.py, and its non-trivial for me
48 >> to determine whether this is a real problem or not.
49 >
50 > It is not a problem.
51 >
52 > allvalid is initialized in the isEbuild class at the start of the pkg
53 > level checks to True, then there are several things that if found reset
54 > it to False...
55 >
56 > Then when it gets to the ebuild level checks the ebuild check there
57 > just sets it False if the pkg.invalid variable is True. In some cases
58 > it might be a double False, but never will it trounce the previous
59 > setting.
60
61 Makes sense, thanks for the explanation.
62
63 > The only consumer for that allvalid variable is the metadata
64 > UnusedCheck class.
65 >
66 > So the allvalid variable is True until found False
67 > by whichever checks along the way find it to be False. Like a fuse,
68 > it's good until it's blown, then it can never be good again. I don't
69 > think this particular variable justifies a special class that more
70 > fully mimics a fuse. Impossible to reset it like a breaker.
71
72 Yeah, let's do it. It's a great opportunity to add clarity to the code,
73 and prevent future goofs.
74
75 > To be honest I did not look into the pkg.invalid variable's need to be
76 > setting it False in the Ebuild class. It may in fact be a dupe of a
77 > setting in the isEbuild class or it might be moved there. That original
78 > spaghetti code could in fact have had duplicates since it was so hard
79 > to figure out where to embed something.
80 >
81
82 It's not a dupe. The Package.invalid property is used to implement
83 various checks that are not duplicated elsewhere.
84 --
85 Thanks,
86 Zac

Replies