Hello, On E, 2007-09-24 at 21:14 -0600, Ryan Hill wrote: > since everyone is getting into the reviewing mood, i thought it > would be a good time to get some opinions on the wxwidgets > eclass rewrite i've done.
I figured I'd reply to the old review request on the list instead of doing this privately, so here we go. The notes are based on the latest eclass located at http://overlays.gentoo.org/dev/dirtyepic/browser/eclass/wxwidgets.eclass at current revision 6: * I just assume the documentation markup for the new way of doing eclass documenting is good. The documentation text itself seems mostly good, if maybe not so professional. Some nitpicking: ** Ebuilds are also required set the global -> s/required/required to/? ** The description gives the impression we support installation of release/debug side-by-side - don't think this is true. Maybe we should think about a way for users to be able to set their own wx-config to a debug build, but have the system USE flag dependent. Hmm... Maybe I should revise my stance on debug + release co-existance in some form ** Once 2.8 is in, we should update the simple usage example to that ** With EAPI=1 SLOT depends can be used now, but as wx is always with a version and SLOT matching a way that * wildcard can be used, it's fine to not require things to use EAPI=1 indeed I'd say. Just noting. ** In simple usage example I would rather refer to RDEPEND, not DEPEND, or both * Perhaps we should take care of setting the wxGTK DEPEND and RDEPEND ourselves based on a WX_GTK_VER in ebuilds global scope? Might cause problems for things using wx conditionally for some optional GUI. Talking of which, it seems it already relies on global scope - does that work fine with conditional wx usage? need-wxwidgets for that, I guess? What's the view on eclasses touching DEPEND/RDEPEND these days, anyway? * Taking care of DEPEND and RDEPEND in the eclass itself could allow us to migrate things over to wxBase package in the future without any ebuild migration work, once we fix upstream configure.in to work with a system provided wxBase libraries and not have file collisions. However this probably requires a global variable for base-ansi/base-unicode passing instead of a need-wxwidgets function for that to be possible to work out * I hate any unconditional compiler flags workarounds, such as "append-flags -fno-strict-aliasing". I have some long pending upstream work to do to simply fix it in that one header where it makes noise and once that finally gets resolved (or maybe it already is in 2.8), we have no reason to keep passing this, as to my knowledge it also means less optimization chances for gcc. Do we have a plan on how to get rid of that CFLAGS append once the user is dealing with a wx library that has this fixed? At least this isn't the configure argument mess we used to have in wxlib.eclass where you'd get some important class configured out just because it was broken in an old version :) * Are you sure we want to ewarn unconditionally on all need-wxwidgets function calls? * What's with the TODO on _wxerror() and the function? The previous points encompass mostly only things noted while going over the code - I have yet to look at it with a logic and interaction sense. I figured I'll get this current list off my hands before I get sunk into work. Many many thanks for pushing wxGTK forwards while I'm occupied with work, gnome stuff... and procrastinating -- Mart Raudsepp Gentoo Developer Mail: [EMAIL PROTECTED] Weblog: http://planet.gentoo.org/developers/leio
signature.asc
Description: This is a digitally signed message part
