1 |
On 4/6/06, Marius Mauch <genone@g.o> wrote: |
2 |
> On Thu, 06 Apr 2006 19:11:49 -0700 |
3 |
> Zac Medico <zmedico@g.o> wrote: |
4 |
> > The manifest code doesn't have very many use cases so I'd expect that |
5 |
> > we would have hit most major problems by now (even with a small |
6 |
> > sample). Any necessary changes are likely to be small patches. As |
7 |
> > an alternative, we can cut the 2.1 branch at the point before |
8 |
> > manifest2 was merged (2.1_pre7, essentially). |
9 |
> |
10 |
> Releasing 2.1 without manifest2 is a no go, it would significantly |
11 |
> delay the deployment and transition. I'm not requesting to delay 2.1 |
12 |
> for another few months, just one more pre release so people get a |
13 |
> chance to test it for one or two weeks. |
14 |
|
15 |
The manifest2 code is still pretty raw from where I'm sitting and has |
16 |
a few design problems. |
17 |
|
18 |
Offhand, |
19 |
1) usage of settings sucks (awareness of PORTAGE_ACTUAL_DISTDIR is an |
20 |
indication right there that distdir should be passed in rather then |
21 |
manifest code knowing of settings). Throwing around a big ole dict of |
22 |
settings while easy, isn't exactly encapsulated/seperated all that |
23 |
well (thus making any changes to config class that much more of a |
24 |
mess). |
25 |
2) triggering __init__ within create is pretty dodgy and definitely a |
26 |
"wtf" trick |
27 |
3) usual loading everything into memory rather then iterating over |
28 |
objects (file objects in particular) |
29 |
4) incomplete class; notably the sign chunks. |
30 |
5) lack of protection in digest parsing; flipping through it, any |
31 |
old/deviate from the norm digest's generated (say files//blah) the |
32 |
code doesn't protect itself against. |
33 |
6) impenetrable code. |
34 |
|
35 |
This should be simple/easily grokable code- as is, I don't think it is. |
36 |
fex. |
37 |
cpvlist = [os.path.join(self.pkgdir.rstrip(os.sep).split(os.sep)[-2], |
38 |
x[:-7]) for x in \ |
39 |
portage.listdir(self.pkgdir) if x.endswith(".ebuild")] |
40 |
|
41 |
I'm not in front of a gentoo box right now, and the best I can figure |
42 |
that code is doing is pulling the ebuild name and joining the ebuild |
43 |
name + version to it- and I'm pretty sure that's not a correct |
44 |
interpretation of it. |
45 |
|
46 |
The code should be _readable_ by folks, fhashdict (fex) should have an |
47 |
explanation in __init__ explaining it's structure. Else folks are |
48 |
stuck popping open the interpretter, and inspecting the object to |
49 |
figure out what the code sets it to- which sucks because if you have |
50 |
to inspect the implementation to determine it's intentions, finding |
51 |
bugs in the implementation is that much harder. |
52 |
|
53 |
Realistially, tend to think the code would be cleaner/more readable if |
54 |
split into manifest1 and manifest2 classes. Combine then via a |
55 |
derivative class instead of making manifest2 code (which will live on) |
56 |
nastier to read. |
57 |
|
58 |
Aside from that, my usual rant about creating uneccessary lists, using |
59 |
.keys() when should just be iterating over the dict all apply for this |
60 |
code, in general efficient code (usually resulting in less lines) |
61 |
applies. |
62 |
|
63 |
Honestly? Main issue I have with the code is that while the previous |
64 |
digest/manifest code was icky, it was at least known to be stable via |
65 |
live testing/usage by users/devs (for better or worse). The new code, |
66 |
while containing most functionality in one spot is roughly just as |
67 |
dense/opaque in trying to eyeball, but it lacks a year+ of being in |
68 |
actual testing. That combination right there makes it harder to view |
69 |
as "good to go". |
70 |
|
71 |
/me notes also that he applies same criteria to what he has in the |
72 |
past tried to push into portage; beyond features/bug fixes, whats the |
73 |
cost in terms of maintenance? Will someone else be able to actually |
74 |
grok this code without having to use pdb or liter it with print |
75 |
statements? |
76 |
|
77 |
Not intending to be an ass about the code, just think it needs to be |
78 |
simpler then it is. Effectively it's just marshalling code (yes it |
79 |
triggers chksum calls, but that's minor); it's not that complex of a |
80 |
thing and the implementation should reflect that (or at least be |
81 |
heavily documented if not). |
82 |
|
83 |
> > > The remaining feature I'd like to get into 2.1 is the |
84 |
> > > tree-format-check issue, but that could probably be slipped in in |
85 |
> > > the rc phase (don't really like that idea, but it's an option). |
86 |
> > |
87 |
> > I don't want to rush the development of new features such as |
88 |
> > manifest2 or the tree-format-check. We have a 2.1 branch that, in |
89 |
> > it's current state (2.1_pre7-r4, for example), provides significant |
90 |
> > benefits over the 2.0.x branch. By delaying 2.1's release for the |
91 |
> > addition of _new_ features, we run the risk of the release being |
92 |
> > delayed indefinitely by "just one more feature" syndrome. |
93 |
> > Personally, I'd rather have shorter release periods so that "just one |
94 |
> > more feature" syndrome becomes less of an issue. |
95 |
> |
96 |
> Ehm, this is not "just one more feature", both manifest2 and |
97 |
> the tree-format-check are things to improve forward compability (or for |
98 |
> the latter even enable forward compability at all), so delaying them |
99 |
> will hinder future development, not only for us. |
100 |
> Also this isn't exactly news to you all as I sent my intentions already |
101 |
> a while ago, and last I asked you all agreed with them, so is there any |
102 |
> reason to rush this now? |
103 |
|
104 |
We didn't all agree on tree-formal- last it was left at, there were |
105 |
non-trivial criticisms of the implementation/design, and questions of |
106 |
it's actual protection/gain- questions which need to be answered |
107 |
before it's pushed in. Lack of compatibility checks sucks, |
108 |
potentially broken compatibility checks suck far more. :) |
109 |
|
110 |
Re: delaying support, tree-format isn't delaying anything tangible |
111 |
yet, as such the push for it isn't quite as strong as manifest2. |
112 |
|
113 |
~harring |
114 |
|
115 |
-- |
116 |
gentoo-portage-dev@g.o mailing list |