From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) by finch.gentoo.org (Postfix) with ESMTP id 342CD1381F3 for ; Sun, 4 Aug 2013 07:34:40 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id D51ECE0AAE; Sun, 4 Aug 2013 07:34:35 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [140.211.166.183]) (using TLSv1 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id F2EE4E0A97 for ; Sun, 4 Aug 2013 07:34:32 +0000 (UTC) Received: from localhost (87-205-64-135.adsl.inetia.pl [87.205.64.135]) (using SSLv3 with cipher ECDHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) (Authenticated sender: mgorny) by smtp.gentoo.org (Postfix) with ESMTPSA id 8C20E33ED88; Sun, 4 Aug 2013 07:34:30 +0000 (UTC) Date: Sun, 4 Aug 2013 09:34:47 +0200 From: =?UTF-8?B?TWljaGHFgiBHw7Nybnk=?= To: gentoo-dev@lists.gentoo.org Cc: marienz@gentoo.org, python@gentoo.org Subject: Re: [gentoo-dev] Re: [New eclass] twisted-r1.eclass Message-ID: <20130804093447.2776bca4@gentoo.org> In-Reply-To: References: <20130803171303.18911eba@gentoo.org> Organization: Gentoo X-Mailer: Claws Mail 3.9.2-dirty (GTK+ 2.24.20; x86_64-pc-linux-gnu) Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-dev@lists.gentoo.org Reply-to: gentoo-dev@lists.gentoo.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=PGP-SHA512; boundary="Sig_/oq4dW/lsbc7x36TQJDIA6AD"; protocol="application/pgp-signature" X-Archives-Salt: 7c0dc57e-081b-49c5-bff6-1027e352ef38 X-Archives-Hash: 39c952129de7a6c80c874bd9f1bda99b --Sig_/oq4dW/lsbc7x36TQJDIA6AD Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Dnia 2013-08-04, o godz. 15:15:12 Marien Zwart napisa=C5=82(a): > On Sun, Aug 4, 2013 at 1:13 AM, Micha=C5=82 G=C3=B3rny 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. >=20 > 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. >=20 > > 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. >=20 > 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. >=20 > 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). >=20 > It seems that if TWISTED_PLUGINS is set to the empty array, "[[ > ${TWISTED_PLUGINS[@]} ]] || TWISTED_PLUGINS=3D( 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. --=20 Best regards, Micha=C5=82 G=C3=B3rny --Sig_/oq4dW/lsbc7x36TQJDIA6AD Content-Type: application/pgp-signature; name=signature.asc Content-Disposition: attachment; filename=signature.asc -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (GNU/Linux) iQJ8BAEBCgBmBQJR/gQbXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXQ1RUJGMjBGOTk2RkIzQzIyQ0M2RkNBNDBC QUJGMUQ1RkY4QzgxMTBBAAoJELq/HV/4yBEK+ssP+wV+03jIrWupvHmm0qv2UBZf z8mmELWLKAZ6JjXubSoZzXS3SvyOJQMGlsM6Fmi7YeEjrFuBkf01OuJKiwlmi2tH tkrdQOe5cIqZTRh/av4lhVij/Obr8eJgSjX3HwPRgzuSLigcLto+e8fDHFxw7iF5 1RtNx5EOwnITOHPzSFdzx/JmEjk8lXV68Hhvb7B6CHInIxl9jwjn3bVrOK+ha85H 8ScP8bPPRzAkVHoQxbAxutOTivnQljYTOqnAfk+OYw5qI851mMrFWmnNxSV+CtFQ /6Vhcgjp1nBkePyMlkmM3RFSv598U/k5oqvrmzWAr+1pEGMGhc+Pmu5xT9fIyas+ 46WJMB3goApSByXDZQevCd/5GcnEwN2wcA5eDfcFdv4+NnixvvkxNwpA29sWt1QV 7OxU1DBKxzPq0n19YjGevf1ipWxYaBxFGRy4lKIE1Df3zlP7cMLlrPHrGGKmoKIs 0rsTzf4Z2STi1w04A9a+vIevlAmcdcYRg/IlsMEHq052JqjqaOa67pa0S9psy80e wZv8MF1Xj42R91erfGjSVGilKrmXT04+y/GGjLpgU6q4G/ujbsnOQEg7Ebr29vTz Qk9WO9GabiY4OQgT1605r03ky17iF8rJIC8Eu1PleQTFjCFpE4D8PedJg3cXHVw4 y49Ps9adshBJnvHdqu38 =cpKK -----END PGP SIGNATURE----- --Sig_/oq4dW/lsbc7x36TQJDIA6AD--