1 |
Dnia 2013-08-04, o godz. 15:15:12 |
2 |
Marien Zwart <marienz@g.o> napisał(a): |
3 |
|
4 |
> On Sun, Aug 4, 2013 at 1:13 AM, Michał Górny <mgorny@g.o> wrote: |
5 |
> > 2. The eclass comes with a pure bash-3.2 CamelCase converter |
6 |
> > for changing PNs like 'twisted-foo' into 'TwistedFoo'. The relevant |
7 |
> > code can be moved to eutils as portable replacements for bash-4 ${foo^} |
8 |
> > and friends. |
9 |
> |
10 |
> That was considered when the original was committed but rejected as |
11 |
> getting too messy. Two questions: have you tested this contraption |
12 |
> with the oldest version of bash we still care about, and have you |
13 |
> considered making it take the input as an argument and echoing the |
14 |
> result (making it work the way versionator.eclass functions do)? If |
15 |
> you want this to be usable as a portable utility function you'd have |
16 |
> to write it that way, might as well do that from the start. |
17 |
|
18 |
Yes, it's compliant with bash-3.2. That's why it has conditional |
19 |
on bash-4. And the original version worked that way but subshells |
20 |
impact performance, and that's not something we'd like to do |
21 |
in the global scope. However, if it was to be reused, then it will |
22 |
probably work that way. |
23 |
|
24 |
> I'm only ok with this code because we'll eventually end up requiring |
25 |
> bash 4 at which point this can be written sanely. |
26 |
> |
27 |
> > 3. The eclass provides a reusable twisted-r1_python_test and sets it as |
28 |
> > default python_test. If someone needs to override it, he can still call |
29 |
> > it using the former name. |
30 |
> |
31 |
> Kind of a shame EXPORT_FUNCTIONS only works on actual phase functions. |
32 |
|
33 |
I had an eclass creating framework for custom phase functions but it |
34 |
was rejected by the ml as introducing too much abstraction. |
35 |
|
36 |
> The rm -rf in there is slightly hacky: its target will legitimately |
37 |
> not exist if this is the initial install or if we're not in a |
38 |
> twisted-* package (in which case the package should probably not be |
39 |
> hitting this function, but it will if not overridden). |
40 |
|
41 |
Hmm, considering the specifics of the function, I think we could make |
42 |
the cp/rm hack conditional to package name. Non-twisted packages could |
43 |
still reuse plain 'trial' call. |
44 |
|
45 |
> There's potential for confusion there if an upgrade drops or renames a |
46 |
> twisted/plugins/twisted_blah.py file, but that seems like enough of an |
47 |
> edge case it's not worth worrying about. |
48 |
|
49 |
True. |
50 |
|
51 |
> I was going to recommend adding variables that control what gets |
52 |
> copied and removed, but I can't think of any current users of such |
53 |
> variables. So it's probably not worth the hassle. |
54 |
|
55 |
During the tests? Sounds like overkill. |
56 |
|
57 |
> If I read this right it'll break if distutils_install_for_testing ever |
58 |
> changes its mind on where to install (and its docstring says to use |
59 |
> TEST_DIR, not which path that'll be). So I'd add a check for |
60 |
> ${TEST_DIR}/lib being equal to $libdir after the call to |
61 |
> distutils_install_for_testing, and print a noisy QA message about |
62 |
> updating the eclass if they're different. |
63 |
|
64 |
Sounds reasonable. I was wondering about moving TEST_DIR earlier |
65 |
in distutils-r1 but this is probably cleaner. |
66 |
|
67 |
> > 4. Cache updating hack is based off twisted.eclass. Sadly, it's not |
68 |
> > something we can do without postrm/postinst. Similarly to the old |
69 |
> > eclass, TWISTED_PLUGINS needs to list the plugin locations. Since most |
70 |
> > ebuilds install to twisted.plugins, it defaults to that. If an ebuild |
71 |
> > does not install plugins at all, it needs to set empty TWISTED_PLUGINS. |
72 |
> |
73 |
> If I read that right you can just __import__(module), you don't need |
74 |
> __import__(module, globals()). Also, maybe do the loop over plugin |
75 |
> locations in bash. I think if you do that you don't need __import__ |
76 |
> and sys.modules anymore, you can just generate "import $module" and |
77 |
> "list(getPlugins(IPlugin, $module))". |
78 |
|
79 |
But you'll call Python a few times. Looping in Python is less |
80 |
expensive. Plus inlining variables into Python code is really ugly. |
81 |
|
82 |
> I wouldn't worry too much about using pkg_post* for this. It's what |
83 |
> they're there for (twisted's isn't the only plugin system with a file |
84 |
> like this, see for example gdk-pixbuf). |
85 |
> |
86 |
> It seems that if TWISTED_PLUGINS is set to the empty array, "[[ |
87 |
> ${TWISTED_PLUGINS[@]} ]] || TWISTED_PLUGINS=( twisted.plugins )" will |
88 |
> set it to twisted.plugins. Is that intentional? It's not necessarily a |
89 |
> problem (you can just set TWISTED_PLUGINS back to the empty array |
90 |
> after the inherit) but it's a bit confusing and I don't think it's |
91 |
> what you intended (why bother with that conditional at all?) |
92 |
|
93 |
It's all yac's fault :P. I didn't even think about that line. |
94 |
|
95 |
Well, in fact TWISTED_PLUGINS will usually be set after the inherit. |
96 |
But indeed we should use some kind of 'declare' to check if it was |
97 |
declared instead. |
98 |
|
99 |
> I was going to claim importing from twisted.plugin should never fail, |
100 |
> but then I realized dev-python/twisted might want to use these |
101 |
> functions too. |
102 |
|
103 |
And yes, it does. It's responsible for dropping the plugin cache too. |
104 |
|
105 |
> I might add a comment to the functions operating on that array to |
106 |
> remind people it might be the empty array. It wasn't immediately |
107 |
> obvious to me upon reading the code it'd safely do nothing (today I |
108 |
> learned "rm -f" (with no further arguments) successfully does nothing, |
109 |
> while "rm" (with no arguments) fails because of a missing operand). |
110 |
|
111 |
Yes, the code would use more checks. The 'empty array' was rather |
112 |
a last-minute invention since originally the eclass wasn't intended to |
113 |
be used by packages not installing twisted plugins. But then, at least |
114 |
CamelCase name generator and possibly trial testing would still benefit |
115 |
those packages. |
116 |
|
117 |
-- |
118 |
Best regards, |
119 |
Michał Górny |