Dnia 2013-08-04, o godz. 15:15:12
Marien Zwart <marienz@gentoo.org> napisał(a):

> On Sun, Aug 4, 2013 at 1:13 AM, Michał Górny <mgorny@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 (today I
> learned "rm -f" (with no further arguments) successfully does nothing,
> while "rm" (with no arguments) fails because of a missing operand).

Yes, the code would use more checks. The 'empty array' was rather
a last-minute invention since originally the eclass wasn't intended to
be used by packages not installing twisted plugins. But then, at least
CamelCase name generator and possibly trial testing would still benefit
those packages.

-- 
Best regards,
Michał Górny