1 |
On Sun, Aug 4, 2013 at 1:13 AM, Michał Górny <mgorny@g.o> wrote: |
2 |
> We've been working with yac for a while to get the old twisted.eclass |
3 |
> converted to be compliant with distutils-r1 both in design |
4 |
> and in spirit. This is the first version we'd like to submit for review. |
5 |
|
6 |
Thanks for doing this. If memory serves (and cvs log says it does) I'm |
7 |
to blame for the first version of this eclass so perhaps I should |
8 |
review this one :) twisted.eclass changed quite a bit since I last |
9 |
looked at it, though. |
10 |
|
11 |
I've reviewed the "One more bit of optimization" version. |
12 |
|
13 |
> 1. The eclass aims to be less conditional than the old one. Especially |
14 |
> we've dropped all the ${CATEGORY}/${PN} checks. The code still sets all |
15 |
> the funny defaults for Twisted suite but those aren't incremental |
16 |
> and can easily be overrode in ebuilds. And in most cases, they simple |
17 |
> are (SRC_URI, LICENSE). |
18 |
|
19 |
The original version just set those unconditionally, the conditionals |
20 |
I think you're referring to were added by Arfrever in revision 1.8. |
21 |
It's not clear to me why. Does someone on this list remember? |
22 |
|
23 |
> 2. The eclass comes with a pure bash-3.2 CamelCase converter |
24 |
> for changing PNs like 'twisted-foo' into 'TwistedFoo'. The relevant |
25 |
> code can be moved to eutils as portable replacements for bash-4 ${foo^} |
26 |
> and friends. |
27 |
|
28 |
That was considered when the original was committed but rejected as |
29 |
getting too messy. Two questions: have you tested this contraption |
30 |
with the oldest version of bash we still care about, and have you |
31 |
considered making it take the input as an argument and echoing the |
32 |
result (making it work the way versionator.eclass functions do)? If |
33 |
you want this to be usable as a portable utility function you'd have |
34 |
to write it that way, might as well do that from the start. |
35 |
|
36 |
I'm only ok with this code because we'll eventually end up requiring |
37 |
bash 4 at which point this can be written sanely. |
38 |
|
39 |
> 3. The eclass provides a reusable twisted-r1_python_test and sets it as |
40 |
> default python_test. If someone needs to override it, he can still call |
41 |
> it using the former name. |
42 |
|
43 |
Kind of a shame EXPORT_FUNCTIONS only works on actual phase functions. |
44 |
The rm -rf in there is slightly hacky: its target will legitimately |
45 |
not exist if this is the initial install or if we're not in a |
46 |
twisted-* package (in which case the package should probably not be |
47 |
hitting this function, but it will if not overridden). There's |
48 |
potential for confusion there if an upgrade drops or renames a |
49 |
twisted/plugins/twisted_blah.py file, but that seems like enough of an |
50 |
edge case it's not worth worrying about. |
51 |
|
52 |
I was going to recommend adding variables that control what gets |
53 |
copied and removed, but I can't think of any current users of such |
54 |
variables. So it's probably not worth the hassle. |
55 |
|
56 |
If I read this right it'll break if distutils_install_for_testing ever |
57 |
changes its mind on where to install (and its docstring says to use |
58 |
TEST_DIR, not which path that'll be). So I'd add a check for |
59 |
${TEST_DIR}/lib being equal to $libdir after the call to |
60 |
distutils_install_for_testing, and print a noisy QA message about |
61 |
updating the eclass if they're different. |
62 |
|
63 |
> 4. Cache updating hack is based off twisted.eclass. Sadly, it's not |
64 |
> something we can do without postrm/postinst. Similarly to the old |
65 |
> eclass, TWISTED_PLUGINS needs to list the plugin locations. Since most |
66 |
> ebuilds install to twisted.plugins, it defaults to that. If an ebuild |
67 |
> does not install plugins at all, it needs to set empty TWISTED_PLUGINS. |
68 |
|
69 |
If I read that right you can just __import__(module), you don't need |
70 |
__import__(module, globals()). Also, maybe do the loop over plugin |
71 |
locations in bash. I think if you do that you don't need __import__ |
72 |
and sys.modules anymore, you can just generate "import $module" and |
73 |
"list(getPlugins(IPlugin, $module))". |
74 |
|
75 |
I wouldn't worry too much about using pkg_post* for this. It's what |
76 |
they're there for (twisted's isn't the only plugin system with a file |
77 |
like this, see for example gdk-pixbuf). |
78 |
|
79 |
It seems that if TWISTED_PLUGINS is set to the empty array, "[[ |
80 |
${TWISTED_PLUGINS[@]} ]] || TWISTED_PLUGINS=( twisted.plugins )" will |
81 |
set it to twisted.plugins. Is that intentional? It's not necessarily a |
82 |
problem (you can just set TWISTED_PLUGINS back to the empty array |
83 |
after the inherit) but it's a bit confusing and I don't think it's |
84 |
what you intended (why bother with that conditional at all?) |
85 |
|
86 |
I was going to claim importing from twisted.plugin should never fail, |
87 |
but then I realized dev-python/twisted might want to use these |
88 |
functions too. |
89 |
|
90 |
I might add a comment to the functions operating on that array to |
91 |
remind people it might be the empty array. It wasn't immediately |
92 |
obvious to me upon reading the code it'd safely do nothing (today I |
93 |
learned "rm -f" (with no further arguments) successfully does nothing, |
94 |
while "rm" (with no arguments) fails because of a missing operand). |
95 |
-- |
96 |
|
97 |
Marien Zwart (marienz) |