Re: [gentoo-dev] Re: [New eclass] twisted-r1.eclass
Dnia 2013-08-04, o godz. 15:15:12
Marien Zwart mari...@gentoo.org napisał(a):
On Sun, Aug 4, 2013 at 1:13 AM, Michał Górny mgo...@gentoo.org wrote:
2. The eclass comes with a pure bash-3.2 CamelCase converter
for changing PNs like 'twisted-foo' into 'TwistedFoo'. The relevant
code can be moved to eutils as portable replacements for bash-4 ${foo^}
and friends.
That was considered when the original was committed but rejected as
getting too messy. Two questions: have you tested this contraption
with the oldest version of bash we still care about, and have you
considered making it take the input as an argument and echoing the
result (making it work the way versionator.eclass functions do)? If
you want this to be usable as a portable utility function you'd have
to write it that way, might as well do that from the start.
Yes, it's compliant with bash-3.2. That's why it has conditional
on bash-4. And the original version worked that way but subshells
impact performance, and that's not something we'd like to do
in the global scope. However, if it was to be reused, then it will
probably work that way.
I'm only ok with this code because we'll eventually end up requiring
bash 4 at which point this can be written sanely.
3. The eclass provides a reusable twisted-r1_python_test and sets it as
default python_test. If someone needs to override it, he can still call
it using the former name.
Kind of a shame EXPORT_FUNCTIONS only works on actual phase functions.
I had an eclass creating framework for custom phase functions but it
was rejected by the ml as introducing too much abstraction.
The rm -rf in there is slightly hacky: its target will legitimately
not exist if this is the initial install or if we're not in a
twisted-* package (in which case the package should probably not be
hitting this function, but it will if not overridden).
Hmm, considering the specifics of the function, I think we could make
the cp/rm hack conditional to package name. Non-twisted packages could
still reuse plain 'trial' call.
There's potential for confusion there if an upgrade drops or renames a
twisted/plugins/twisted_blah.py file, but that seems like enough of an
edge case it's not worth worrying about.
True.
I was going to recommend adding variables that control what gets
copied and removed, but I can't think of any current users of such
variables. So it's probably not worth the hassle.
During the tests? Sounds like overkill.
If I read this right it'll break if distutils_install_for_testing ever
changes its mind on where to install (and its docstring says to use
TEST_DIR, not which path that'll be). So I'd add a check for
${TEST_DIR}/lib being equal to $libdir after the call to
distutils_install_for_testing, and print a noisy QA message about
updating the eclass if they're different.
Sounds reasonable. I was wondering about moving TEST_DIR earlier
in distutils-r1 but this is probably cleaner.
4. Cache updating hack is based off twisted.eclass. Sadly, it's not
something we can do without postrm/postinst. Similarly to the old
eclass, TWISTED_PLUGINS needs to list the plugin locations. Since most
ebuilds install to twisted.plugins, it defaults to that. If an ebuild
does not install plugins at all, it needs to set empty TWISTED_PLUGINS.
If I read that right you can just __import__(module), you don't need
__import__(module, globals()). Also, maybe do the loop over plugin
locations in bash. I think if you do that you don't need __import__
and sys.modules anymore, you can just generate import $module and
list(getPlugins(IPlugin, $module)).
But you'll call Python a few times. Looping in Python is less
expensive. Plus inlining variables into Python code is really ugly.
I wouldn't worry too much about using pkg_post* for this. It's what
they're there for (twisted's isn't the only plugin system with a file
like this, see for example gdk-pixbuf).
It seems that if TWISTED_PLUGINS is set to the empty array, [[
${TWISTED_PLUGINS[@]} ]] || TWISTED_PLUGINS=( twisted.plugins ) will
set it to twisted.plugins. Is that intentional? It's not necessarily a
problem (you can just set TWISTED_PLUGINS back to the empty array
after the inherit) but it's a bit confusing and I don't think it's
what you intended (why bother with that conditional at all?)
It's all yac's fault :P. I didn't even think about that line.
Well, in fact TWISTED_PLUGINS will usually be set after the inherit.
But indeed we should use some kind of 'declare' to check if it was
declared instead.
I was going to claim importing from twisted.plugin should never fail,
but then I realized dev-python/twisted might want to use these
functions too.
And yes, it does. It's responsible for dropping the plugin cache too.
I might add a comment to the functions operating on that array to
remind people it might be the empty array. It wasn't immediately
obvious to me upon reading the code it'd safely do nothing
[gentoo-dev] Re: [New eclass] twisted-r1.eclass
On Sun, Aug 4, 2013 at 1:13 AM, Michał Górny mgo...@gentoo.org wrote:
We've been working with yac for a while to get the old twisted.eclass
converted to be compliant with distutils-r1 both in design
and in spirit. This is the first version we'd like to submit for review.
Thanks for doing this. If memory serves (and cvs log says it does) I'm
to blame for the first version of this eclass so perhaps I should
review this one :) twisted.eclass changed quite a bit since I last
looked at it, though.
I've reviewed the One more bit of optimization version.
1. The eclass aims to be less conditional than the old one. Especially
we've dropped all the ${CATEGORY}/${PN} checks. The code still sets all
the funny defaults for Twisted suite but those aren't incremental
and can easily be overrode in ebuilds. And in most cases, they simple
are (SRC_URI, LICENSE).
The original version just set those unconditionally, the conditionals
I think you're referring to were added by Arfrever in revision 1.8.
It's not clear to me why. Does someone on this list remember?
2. The eclass comes with a pure bash-3.2 CamelCase converter
for changing PNs like 'twisted-foo' into 'TwistedFoo'. The relevant
code can be moved to eutils as portable replacements for bash-4 ${foo^}
and friends.
That was considered when the original was committed but rejected as
getting too messy. Two questions: have you tested this contraption
with the oldest version of bash we still care about, and have you
considered making it take the input as an argument and echoing the
result (making it work the way versionator.eclass functions do)? If
you want this to be usable as a portable utility function you'd have
to write it that way, might as well do that from the start.
I'm only ok with this code because we'll eventually end up requiring
bash 4 at which point this can be written sanely.
3. The eclass provides a reusable twisted-r1_python_test and sets it as
default python_test. If someone needs to override it, he can still call
it using the former name.
Kind of a shame EXPORT_FUNCTIONS only works on actual phase functions.
The rm -rf in there is slightly hacky: its target will legitimately
not exist if this is the initial install or if we're not in a
twisted-* package (in which case the package should probably not be
hitting this function, but it will if not overridden). There's
potential for confusion there if an upgrade drops or renames a
twisted/plugins/twisted_blah.py file, but that seems like enough of an
edge case it's not worth worrying about.
I was going to recommend adding variables that control what gets
copied and removed, but I can't think of any current users of such
variables. So it's probably not worth the hassle.
If I read this right it'll break if distutils_install_for_testing ever
changes its mind on where to install (and its docstring says to use
TEST_DIR, not which path that'll be). So I'd add a check for
${TEST_DIR}/lib being equal to $libdir after the call to
distutils_install_for_testing, and print a noisy QA message about
updating the eclass if they're different.
4. Cache updating hack is based off twisted.eclass. Sadly, it's not
something we can do without postrm/postinst. Similarly to the old
eclass, TWISTED_PLUGINS needs to list the plugin locations. Since most
ebuilds install to twisted.plugins, it defaults to that. If an ebuild
does not install plugins at all, it needs to set empty TWISTED_PLUGINS.
If I read that right you can just __import__(module), you don't need
__import__(module, globals()). Also, maybe do the loop over plugin
locations in bash. I think if you do that you don't need __import__
and sys.modules anymore, you can just generate import $module and
list(getPlugins(IPlugin, $module)).
I wouldn't worry too much about using pkg_post* for this. It's what
they're there for (twisted's isn't the only plugin system with a file
like this, see for example gdk-pixbuf).
It seems that if TWISTED_PLUGINS is set to the empty array, [[
${TWISTED_PLUGINS[@]} ]] || TWISTED_PLUGINS=( twisted.plugins ) will
set it to twisted.plugins. Is that intentional? It's not necessarily a
problem (you can just set TWISTED_PLUGINS back to the empty array
after the inherit) but it's a bit confusing and I don't think it's
what you intended (why bother with that conditional at all?)
I was going to claim importing from twisted.plugin should never fail,
but then I realized dev-python/twisted might want to use these
functions too.
I might add a comment to the functions operating on that array to
remind people it might be the empty array. It wasn't immediately
obvious to me upon reading the code it'd safely do nothing (today I
learned rm -f (with no further arguments) successfully does nothing,
while rm (with no arguments) fails because of a missing operand).
--
Marien Zwart (marienz)
