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 |